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

Wed Nov 25 02:48:50 PST 2020

ro added inline comments.

================
Comment at: clang/tools/driver/CMakeLists.txt:123
+
+check_linker_flag("-Wl,-z,relax=transtls" LINKER_SUPPORTS_Z_RELAX_TRANSTLS)
----------------
MaskRay wrote:
> MaskRay wrote:
> > ro wrote:
> > > MaskRay wrote:
> > > > ro wrote:
> > > > > MaskRay wrote:
> > > > > > 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.
> > > > > > 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.
> > > > > 
> > > > > Tell the binutils maintainers that ;-)  While I'm still unconcerned about this particular case (it's only used on a Solaris host where `clang` hardcodes the use of `/usr/bin/ld`), I continue to be annoyed by GNU `ld`'s nonsensical (or even outright dangerous) behaviour of accepting every `-z` option.
> > > > > 
> > > > > It took me some wrestling with `cmake` , but now `check_linker_flag` correctly rejects `-z ` flags where GNU `ld` produces the warning.
> > > > > 
> > > > > Some caveats about the implementation:
> > > > > - `check_cxx_compiler_flag` doesn't support the `FAIL_REGEX` arg, so I had to switch to `check_cxx_source_compiles` instead.
> > > > > - While it would be more appropriate to add the linker flag under test to `CMAKE_REQUIRED_LINK_OPTIONS`, that is only present since `cmake` 3.14 while LLVM still only requires 3.13.
> > > > > warning: -z.* ignored
> > > > 
> > > > Doesn't this stop working if binutils starts to use `error: -z.* ignored`? Isn't there a way to call `check_linker_flag` only when the target is Solaris? Does `LLVM_LINKER_IS_SOLARISLD` work?
> > > > > warning: -z.* ignored
> > > > 
> > > > Doesn't this stop working if binutils starts to use `error: -z.* ignored`? 
> > > 
> > > No: `FAIL_REGEX` only adds to error detection, every other condition just remains as is.
> > > 
> > > > Isn't there a way to call `check_linker_flag` only when the target is Solaris? Does `LLVM_LINKER_IS_SOLARISLD` work?
> > > 
> > > That would be wrong: this is about working around a GNU `ld` bug.  Imagine adding a new `-z` option to `lld` which either GNU `ld` didn't adopt at all or only in the latest release.  You'd want to reject the use of that option on earlier GNU `ld` just the same, no Solaris in sight.
> > > 
> > > As I said: `LINKER_SUPPORTS_Z_RELAX_TRANSTLS` must always be defined because `Solaris.cpp` is always compiled no matter what the target.
> > > 
> > > I really don't understand what you're trying to guard against by putting up roadblock after roadblock for this patch.
> > > I really don't understand what you're trying to guard against by putting up roadblock after roadblock for this patch.
> > 
> > Because I am concerned the additional Solaris specific complexity (to make systems released 9 years ago work) in generic code (`CheckLinkerFlag.cmake`) may not pull its weight. I am sorry but I hope it is not unfair to say that Solaris is not a first-tier OS. I am fairly worried about more configure-time variables which can fragment testing (testing one specific configuration does not guarantee it working in another; this patch makes the situation worse).
> > 
> > Can't you make the Z_RELAX_TRANSTLS check only running on Solaris?
> `ld.bfd --fatal-warnings -z relax=transtls a.o` is an error.
> > I really don't understand what you're trying to guard against by putting up roadblock after roadblock for this patch.
> 
> Because I am concerned the additional Solaris specific complexity (to make systems released 9 years ago work) in generic code (`CheckLinkerFlag.cmake`) may not pull its weight. I am sorry but I hope it is not unfair to say that Solaris is not a first-tier OS. I am fairly worried about more configure-time variables which can fragment testing (testing one specific configuration does not guarantee it working in another; this patch makes the situation worse).

Then why on earth didn't you say so up front instead of asking the same questions repeatedly, obviously only half reading the answers?  Besides, the check is purely for the benefit of Illumos, an open-source OpenSolaris derivative.  Solaris can use the option unconditionally.
> 
> Can't you make the Z_RELAX_TRANSTLS check only running on Solaris?

I've already answered this twice, obviously you're not listening.

This is the most hostile and toxic review I've ever experienced: instead of rejecting the patch up-front either for policy reasons ("we don't want Solaris support in LLVM") or for technical ones, you're just casting doubt with little technical content, all with a vague undercurrent of dislike.

This is just a total waste of my time: I'm leaving LLVM development for good.  Mission accomplished.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91605