Work around a Linux arm32 unaligned data issue in SuperPMI #56517
Conversation
impImportStaticReadOnlyField reads the data at the address returned by getFieldAddress. SuperPMI saves and replays this data, but it doesn't store it at a naturally aligned address. For int64/double (at least), this is a problem on Linux arm32, which raises a SIGBUS exception on such unaligned access. This works around the problem by copying the potentially unaligned data to a known aligned spot before reading it. This is only done under DEBUG and when we know we are replaying under SuperPMI. A proper fix would be to teach SuperPMI about alignment, but that would be a large and risky change, compared to this small and isolated workaround. The fixes the non-determinism of dotnet#56156.
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
|
@kunalspathak @dotnet/jit-contrib PTAL |
| static_assert_no_msg(sizeof(unsigned __int8) == sizeof(signed char)); | ||
| static_assert_no_msg(sizeof(unsigned __int8) == sizeof(unsigned char)); | ||
| *(unsigned __int8*)p_aligned_data = *((unsigned __int8*)fldAddr); | ||
| break; |
There was a problem hiding this comment.
Nothing should need to be copied for the Byte case.
This looks like it makes an unnecessary copy
p_aligned_data = fldAddr; should work.
src/coreclr/jit/importer.cpp
Outdated
| case TYP_USHORT: | ||
| static_assert_no_msg(sizeof(unsigned __int16) == sizeof(short)); | ||
| static_assert_no_msg(sizeof(unsigned __int16) == sizeof(unsigned short)); | ||
| *(unsigned __int16*)p_aligned_data = GET_UNALIGNED_16(fldAddr); |
There was a problem hiding this comment.
In these cases, you could check the current alignment of fldAddr and just use it if it meets the alignment requirement of 2,4 or 8
There was a problem hiding this comment.
@BruceForstall - Help me understand, but don't we want the workaround only in cases when fldAddr is unaligned?
There was a problem hiding this comment.
We only need the workaround for unaligned fldAddr, but it doesn't hurt (except some miniscule perf -- in SuperPMI replay only) to do it. I went ahead and did it only when necessary.
There was a problem hiding this comment.
You mean you will move about chance only if flAddr is unaligned?
There was a problem hiding this comment.
@kunalspathak I don't understand your question, but check the updated code to see if it answers it.
There was a problem hiding this comment.
Perfect. That's what I was expecting.
There was a problem hiding this comment.
FWIW I prefer what it was before. Actually, can't the entire thing just be:
alignas(8) unsigned char aligned_data[8] = {};
if (info.compMethodSuperPMIIndex != -1)
{
// Work around arm32 alignment bug in SuperPMI
memcpy(aligned_data, fldAddr, min(sizeof(aligned_data), genTypeSize(lclTyp)));
fldAddr = aligned_data;
}?
Anyway, I guess this workaround will disappear soon anyway, so no real need to do anything more.
There was a problem hiding this comment.
Yes, you're right that it could be smaller. Would I trust that alignas works for all compilers? Even so, could use double as I'm currently doing. And could do conditional, only-if-needed, as well. I think I'll leave it as is; there's not a lot of reason to optimize it.
There was a problem hiding this comment.
Would I trust that alignas works for all compilers?
I believe alignas is part of C++11.
I think I'll leave it as is; there's not a lot of reason to optimize it.
Agree, sounds fine to me.
ghost
locked as resolved and limited conversation to collaborators
impImportStaticReadOnlyField reads the data at the address returned by
getFieldAddress. SuperPMI saves and replays this data, but it doesn't
store it at a naturally aligned address. For int64/double (at least),
this is a problem on Linux arm32, which raises a SIGBUS exception on
such unaligned access.
This works around the problem by copying the potentially unaligned data
to a known aligned spot before reading it. This is only done under DEBUG
and when we know we are replaying under SuperPMI.
A proper fix would be to teach SuperPMI about alignment, but that would be
a large and risky change, compared to this small and isolated workaround.
The fixes the non-determinism of #56156.