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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
