This is an archive of the discontinued LLVM Phabricator instance.

[clang] Explicitly set the EmulatedTLS codegen option
ClosedPublic

Authored by mstorsjo on Aug 30 2022, 1:57 AM.

Details

Summary

Set the EmulatedTLS option based on Triple::hasDefaultEmulatedTLS()
if the user didn't specify it; set ExplicitEmulatedTLS to true
in llvm::TargetOptions and set EmulatedTLS to Clang's
opinion of what the default or preference is.

This avoids any risk of deviance between the two.

This affects one check of getCodeGenOpts().EmulatedTLS in
shouldAssumeDSOLocal in CodeGenModule, but as that check only
is done for TT.isWindowsGNUEnvironment(), and
hasDefaultEmulatedTLS() returns false for such environments
it doesn't make any current testable difference.

Some mingw distributions carry a downstream patch, that enables
emulated TLS by default for mingw targets in hasDefaultEmulatedTLS()

  • and for such cases, this patch does make a difference and fixes the

detection of emulated TLS, if it is implicitly enabled.

This is an alternative to D132848.

Diff Detail

Event Timeline

mstorsjo created this revision.Aug 30 2022, 1:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 30 2022, 1:57 AM
Herald added a subscriber: ormris. · View Herald Transcript
mstorsjo requested review of this revision.Aug 30 2022, 1:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 30 2022, 1:57 AM

@efriedma Does this look like what you had in mind; does it seem reasonable to @aaron.ballman?

@efriedma Does this look like what you had in mind; does it seem reasonable to @aaron.ballman?

This seems reasonable to me, but I leave it to @efriedma and @rjmccall for the final sign-off. This looks to be NFC, or have I missed something? (If it is NFC, you should add that to the title, and if it's not NFC, is there test coverage that can be added?)

@efriedma Does this look like what you had in mind; does it seem reasonable to @aaron.ballman?

This seems reasonable to me, but I leave it to @efriedma and @rjmccall for the final sign-off. This looks to be NFC, or have I missed something? (If it is NFC, you should add that to the title, and if it's not NFC, is there test coverage that can be added?)

There's no observable change in upstream llvm-project/clang, but it is a functional change for downstreams with other vendor patches applied. So I'm a bit undecided whether it should be marked that or not; you're certainly right about the point that a patch either is NFC or should have test changes though...

efriedma accepted this revision.Sep 9 2022, 3:05 PM

LGTM

(I think you can call it "NFC" if there's no visible change in community; you don't have to try to account for downstream changes that might interact with it.)

This revision is now accepted and ready to land.Sep 9 2022, 3:05 PM
This revision was landed with ongoing or failed builds.Sep 13 2022, 12:56 AM
This revision was automatically updated to reflect the committed changes.