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.

Sign up for GitHub

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

Jump to bottom

[NativeAOT] Respect Align8 flag for thread statics #105905

Merged
merged 2 commits into from
Aug 3, 2024
Merged

[NativeAOT] Respect Align8 flag for thread statics #105905

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fd05a58
Respect Align8 flag for thread statics
filipnavara Aug 3, 2024
dae75e2
Port WASM test
filipnavara Aug 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion src/coreclr/nativeaot/System.Private.CoreLib/src/Internal/Runtime/ThreadStatics.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,15 @@ private static unsafe object AllocateThreadStaticStorageForType(TypeManagerHandl
gcDesc = Internal.Runtime.Augments.RuntimeAugments.TypeLoaderCallbacks.GetThreadStaticGCDescForDynamicType(typeManager, typeTlsIndex);
}

return RuntimeImports.RhNewObject((MethodTable*)gcDesc);
MethodTable *pMethodTable = (MethodTable*)gcDesc;
#if FEATURE_64BIT_ALIGNMENT
if (pMethodTable->RequiresAlign8)
{
return InternalCalls.RhpNewFastAlign8(pMethodTable);
}
#endif

return RuntimeImports.RhNewObject(pMethodTable);
Comment on lines +140 to +148
Copy link
Contributor

@SingleAccretion SingleAccretion Aug 4, 2024 •
edited
Loading

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 •
edited
Loading

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:

runtime/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ILScanner.cs

Line 760 in ffc1174

// 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.

}
}
}
57 changes: 57 additions & 0 deletions src/tests/nativeaot/SmokeTests/UnitTests/BasicThreading.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System;
using System.Collections.Generic;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;
using System.Threading;
using System.Threading.Tasks;

Expand All @@ -18,6 +19,9 @@ internal static int Run()

ThreadStaticsTestWithTasks.Run();

if (ThreadStaticAlignmentTest.Run() != Pass)
return Fail;

if (ThreadTest.Run() != Pass)
return Fail;

Expand Down Expand Up @@ -187,6 +191,59 @@ public static void Run()
}
}

class ThreadStaticAlignmentTest
{
public static int Run()
{
// Check for 8-byte alignment requirement
if (RuntimeInformation.ProcessArchitecture is Architecture.Arm or Architecture.Wasm)
{
// Assume that these are allocated sequentially, use a padding object of size 12 (mod 8 is not 0)
// to move the alignment of the second AddressOfReturnArea in case the first is coincidentally aligned 8.
var ts1Addr = ThreadStaticAlignCheck1.returnArea.AddressOfReturnArea();
var p = new Padder();
var ts2Addr = ThreadStaticAlignCheck2.returnArea.AddressOfReturnArea();

if (((nint)ts1Addr) % 8 != 0)
return BasicThreading.Fail;
if (((nint)ts2Addr) % 8 != 0)
return BasicThreading.Fail;
}

return BasicThreading.Pass;
}

[InlineArray(3)]
private struct ReturnArea
{
private ulong buffer;

internal unsafe nint AddressOfReturnArea()
{
return (nint)Unsafe.AsPointer(ref buffer);
}
}

private class ThreadStaticAlignCheck1
{
[ThreadStatic]
[FixedAddressValueType]
internal static ReturnArea returnArea = default;
}

private class Padder
{
private object o1;
}

private class ThreadStaticAlignCheck2
{
[ThreadStatic]
[FixedAddressValueType]
internal static ReturnArea returnArea = default;
}
}

class ThreadTest
{
private static readonly List<Thread> s_startedThreads = new List<Thread>();
Expand Down
Loading