Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[NativeAOT] Respect Align8 flag for thread statics #105905

Merged
merged 2 commits into from
Aug 3, 2024

Conversation

filipnavara
Copy link
Member

Contributes to #103234, #97729

filipnavara and others added 2 commits August 3, 2024 07:30
Co-authored-by: yowl <scott.waye@hubse.com>
Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@jkotas jkotas merged commit 1cc0186 into dotnet:main Aug 3, 2024
94 checks passed
Comment on lines +140 to +148
MethodTable *pMethodTable = (MethodTable*)gcDesc;
#if FEATURE_64BIT_ALIGNMENT
if (pMethodTable->RequiresAlign8)
{
return InternalCalls.RhpNewFastAlign8(pMethodTable);
}
#endif

return RuntimeImports.RhNewObject(pMethodTable);
Copy link
Contributor

@SingleAccretion SingleAccretion Aug 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this work? AFAICT, RhNewObject already takes RequiresAlign8 into account.

When I looked at the root cause of this a while back, the problem was that RequiresAlign8 isn't set on the special gcDesc method tables (by the compiler, at least - I didn't check dynamic statics).

Copy link
Member Author

@filipnavara filipnavara Aug 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point.

I will double check when I get to office with my ARM device. I assumed that if the test passes it should be all right but I could have overlooked something in the process.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The relevant place to fix would be this one:

// N.B. for ARM32, we would need to deal with > PointerSize alignments.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SingleAccretion Good catch!

the problem was that RequiresAlign8 isn't set on the special gcDesc method tables

dotnet/runtimelab#2609 worked around this problem by allocating all thread statics with 8-byte alignment.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree we should revert it... I wanted to do a clean build on my RPi to figure out if I had some stale build artifacts... but unfortunately that turned out to be more painful than I expected. The combination of build tool updates and zlib-ng seems to hit a compiler bug now and I need to figure out first how to get past it:

  [ 30%] Building C object _deps/fetchzlibng-build/CMakeFiles/zlib.dir/arch/arm/slide_hash_armv6.c.o
  fatal error: error in backend: Cannot select: intrinsic %llvm.arm.uqsub16

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out, it was indeed failing even on my machine. I suspect that I accidentally made the final change with adding the if (pMethodTable->RequiresAlign8) in a wrong VS Code window and I changed it on my local machine instead of the remote SSH session on the Raspberry Pi. I waited for the CI to be green but I also forgot that the inner loop doesn't run the smoke tests on ARM32.

I started working on a proper fix. Some early working attempt is here:
filipnavara@0ccb0d4. It's not exactly pretty. I'll sleep on it for a day or two before submitting it.

jkotas added a commit that referenced this pull request Aug 4, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Sep 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-NativeAOT-coreclr community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants