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

[Rainer Orth via Phabricator via cfe-commits]

ro added inline comments.


================
Comment at: clang/lib/Driver/ToolChains/Solaris.cpp:150
+      const SanitizerArgs &SA = getToolChain().getSanitizerArgs();
+      if (LINKER_SUPPORTS_Z_RELAX_TRANSTLS &&
+          getToolChain().getTriple().getArch() == llvm::Triple::x86_64 &&
----------------
MaskRay wrote:
> MaskRay wrote:
> > Can the configure-time variable `LINKER_SUPPORTS_Z_RELAX_TRANSTLS` be replaced by a version check on the triple suffix?
> > 
> > You can see some tests where `x86_64-unknown-freebsd11` or `x86_64-unknown-freebsd12` is used. The idea is that the driver continuously bumps the default version.
> .. and the code base should not be littered with -D macros from configure-time variables here and there.
> Can the configure-time variable `LINKER_SUPPORTS_Z_RELAX_TRANSTLS` be replaced by a version check on the triple suffix?
> 
> You can see some tests where `x86_64-unknown-freebsd11` or `x86_64-unknown-freebsd12` is used. The idea is that the driver continuously bumps the default version.

This cannot work: both Solaris 11 and Illumos share the same configure triple, `x86_64-pc-solaris2.11`.

Please keep in mind that Solaris 11 was released almost exactly 9 years ago, is in its 5th micro version since, each of which getting monthly updates (at least for a while).  Even if it were possible to extract the micro version and update number from the triple, hardcoding which introduced which feature is effectively impossible.  Some `ld` and `libc` features are also backported to an update of the previous micro version; hardcoding all this knowledge is simply a nightmare.

TBH, I feel put back to the bad old times 20+ years ago when software tried to hardcode all knowledge about the environment, and usually failed even at the time of writing.  This only got better once feature-based approaches got traction.


================
Comment at: clang/lib/Driver/ToolChains/Solaris.cpp:150
+      const SanitizerArgs &SA = getToolChain().getSanitizerArgs();
+      if (LINKER_SUPPORTS_Z_RELAX_TRANSTLS &&
+          getToolChain().getTriple().getArch() == llvm::Triple::x86_64 &&
----------------
ro wrote:
> MaskRay wrote:
> > MaskRay wrote:
> > > Can the configure-time variable `LINKER_SUPPORTS_Z_RELAX_TRANSTLS` be replaced by a version check on the triple suffix?
> > > 
> > > You can see some tests where `x86_64-unknown-freebsd11` or `x86_64-unknown-freebsd12` is used. The idea is that the driver continuously bumps the default version.
> > .. and the code base should not be littered with -D macros from configure-time variables here and there.
> > Can the configure-time variable `LINKER_SUPPORTS_Z_RELAX_TRANSTLS` be replaced by a version check on the triple suffix?
> > 
> > You can see some tests where `x86_64-unknown-freebsd11` or `x86_64-unknown-freebsd12` is used. The idea is that the driver continuously bumps the default version.
> 
> This cannot work: both Solaris 11 and Illumos share the same configure triple, `x86_64-pc-solaris2.11`.
> 
> Please keep in mind that Solaris 11 was released almost exactly 9 years ago, is in its 5th micro version since, each of which getting monthly updates (at least for a while).  Even if it were possible to extract the micro version and update number from the triple, hardcoding which introduced which feature is effectively impossible.  Some `ld` and `libc` features are also backported to an update of the previous micro version; hardcoding all this knowledge is simply a nightmare.
> 
> TBH, I feel put back to the bad old times 20+ years ago when software tried to hardcode all knowledge about the environment, and usually failed even at the time of writing.  This only got better once feature-based approaches got traction.
> .. and the code base should not be littered with -D macros from configure-time variables here and there.

What's so bad about that?  Why is `LINKER_SUPPORTS_Z_RELAX_TRANSTLS` detected at build time better than a runtime check for Solaris/x86 11.3 or higher, but not 11.3 update < x and not Illumos?

With the configure-based approach, I know at least which features are tested by the code, rather than finding months or even years later that some check is hardcoded in some obscure part of the code that I missed?  And for new targets, many things just fall into place automatically.  Consider `compiler-rt`'s maze in `sanitizer_platform_interceptors.h`. With features introduced in micro versions and updates, one cannot even express that some feature got introduced in one of those.


================
Comment at: clang/tools/driver/CMakeLists.txt:123
+
+check_linker_flag("-Wl,-z,relax=transtls" LINKER_SUPPORTS_Z_RELAX_TRANSTLS)
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91605