MaskRay 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:
> 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.
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
