[PATCH] D91605: [sanitizers] Implement GetTls on Solaris

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.


================
Comment at: clang/tools/driver/CMakeLists.txt:123
+
+check_linker_flag("-Wl,-z,relax=transtls" LINKER_SUPPORTS_Z_RELAX_TRANSTLS)
----------------
ro wrote:
> MaskRay wrote:
> > ro wrote:
> > > MaskRay wrote:
> > > > GNU ld reports a warning instead of an error when an unknown `-z` is seen. The warning remains a warning even with `--fatal-warnings`.
> > > > GNU ld reports a warning instead of an error when an unknown `-z` is seen. The warning remains a warning even with `--fatal-warnings`.
> > > 
> > > Thanks for reminding me about that misfeature of GNU `ld`.  I guess `check_linker_flags` needs to be updated to handle that.
> > > In the case at hand, it won't matter either way: the flag is only passed to `ld`, which on Solaris is guaranteed to be the native linker.  Once (if at all) I get around to completing D85309, I can deal with that.  For now, other targets won't see linker warnings about this flag, other than when the flag is used at build time.
> > OK. Then I guess you can condition this when the OS is Solaris?
> > OK. Then I guess you can condition this when the OS is Solaris?
> 
> I fear not: `LINKER_SUPPORTS_Z_RELAX_TRANSTLS` is tested inside an `if` in `Solaris.cpp`: this code is also compiled on non-Solaris hosts.  Why are you worried about the definition being always present?
It is not suitable if LINKER_SUPPORTS_Z_RELAX_TRANSTLS returns a wrong result for GNU ld, even if it is not used for non-Solaris. We should make the value correct in other configurations.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91605/new/

https://reviews.llvm.org/D91605