-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Enable TLS on linux/arm64 only for static resolver #104408
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -784,6 +784,10 @@ void FreeTLSIndicesForLoaderAllocator(LoaderAllocator *pLoaderAllocator) | |||||||||||
|
||||||||||||
static void* GetTlsIndexObjectAddress(); | ||||||||||||
|
||||||||||||
#if !defined(TARGET_OSX) && defined(TARGET_UNIX) && defined(TARGET_ARM64) | ||||||||||||
extern "C" size_t GetTLSResolverAddress(); | ||||||||||||
#endif // !TARGET_OSX && TARGET_UNIX && TARGET_ARM64 | ||||||||||||
|
||||||||||||
bool CanJITOptimizeTLSAccess() | ||||||||||||
{ | ||||||||||||
LIMITED_METHOD_CONTRACT; | ||||||||||||
|
@@ -799,6 +803,23 @@ bool CanJITOptimizeTLSAccess() | |||||||||||
// Optimization is disabled for FreeBSD/arm64 | ||||||||||||
#elif defined(FEATURE_INTERPRETER) | ||||||||||||
// Optimization is disabled when interpreter may be used | ||||||||||||
#elif !defined(TARGET_OSX) && defined(TARGET_UNIX) && defined(TARGET_ARM64) | ||||||||||||
// Optimization is enabled for linux/arm64 only for static resolver. | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I thought about NativeAOT too, but realized that it might be always static resolver.
We can detect static/dynamic resolver for JITting on the fly, but for nativeaot, it might be tricky to know that information ahead of time. Possibly, we might have to embed that check in the generated code itself? But that might add an extra check for code for which we mostly get static resolver. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For native AOT, we should generate the call to the resolver via indirection just like C/C++ compiler does when building dynamic libraries. |
||||||||||||
// For static resolver, the TP offset is same for all threads. | ||||||||||||
// For dynamic resolver, TP offset returned is that of a JIT thread and | ||||||||||||
// will be different for the executing thread. | ||||||||||||
kunalspathak marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||
uint32_t* resolverAddress = reinterpret_cast<uint32_t*>(GetTLSResolverAddress()); | ||||||||||||
if ( | ||||||||||||
// nop or hint 32 | ||||||||||||
((resolverAddress[0] == 0xd503201f) || (resolverAddress[0] == 0xd503241f)) && | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this work correctly for musl glibc? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Currently, we do not do this optimization for musl as you can see few lines above - https://github.com/dotnet/runtime/pull/104408/files/6ecf11fec6c07aefc61f3647a3b3e358e35b6008#diff-838c5a095d88dfabb928a305f3d4ec888cbb0241f0c0d5cf1bb802c706bd1edbR800-R801 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is that because we end up with dynamic resolver on musl? It would be good to find out if this fix covers the musl too (and that ifdef above can be deleted) or if there is something else going on with musl. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
We originally disabled it, probably because of same reason, but do you think that enabling for musl/arm64 can be done as a separate change? EDIT: I am planning to backport this to 8.0, so might be good to just fix it for targets we have this optimization enabled and do a separate change to enable it for musl/arm64 in .net9. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am fine with enabling it for musl in a separate change. We should understand what's going on with musl before backporting to make sure we are not missing anything. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Just to make sure I understand, if it turns out to be same issue for musl as well, we still do not want to enable musl/arm64 for 8.0, right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, the servicing fix should be the minimal change. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Anything else needed for this PR then? |
||||||||||||
// ldr x0, [x0, #8] | ||||||||||||
(resolverAddress[1] == 0xf9400400) && | ||||||||||||
// ret | ||||||||||||
(resolverAddress[2] == 0xd65f03c0) | ||||||||||||
) | ||||||||||||
{ | ||||||||||||
optimizeThreadStaticAccess = true; | ||||||||||||
kunalspathak marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||
} | ||||||||||||
#else | ||||||||||||
optimizeThreadStaticAccess = true; | ||||||||||||
kunalspathak marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||
#if !defined(TARGET_OSX) && defined(TARGET_UNIX) && defined(TARGET_AMD64) | ||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this ok ?