[sanitizers] Implement GetTls on Solaris

[ro]

Summary

In the initial Solaris ASan port, GetTls was left unimplemented. This patch corrects that. There are a couple of caveats, unfortunately:

While current Solaris 11.4 supports the dlpi_tls_modid field of struct dl_phdr_info, this was only added in SRU 10 and isn't present in either Solaris 11.3 or Illumos. Instead, this uses a method used in GCC's D runtime library libphobos dlpi_tls_modid workaround which works even on Solaris 10.

However, the direct call to __tls_get_address triggers a Solaris ld bug on amd64, which needs to be worked around the same way as in libphobos: ld workaround.

Together, they allow the sanitizer_common TLS tests to PASS on both sparc and x86.
I've also verified that the patch doesn't break the Illumos build; however compiler-rt test results continue to be horrible there.

[vitalybuka]

compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp

456–521

can this go into sanitizer_platform_limits_solaris.h ?

[ro]

compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp

456–521

I don't think it belongs there: AFAICS that header is for types used by the interceptors.

I've been following what other targets do here, like declaring internal types and functions, and adding helpers like GetSizeFromHdr. It would only be confusing if Solaris were treated differently. It certainly helped me a lot being able to see what other targets do in once place.

[MaskRay]

clang/lib/Driver/ToolChains/Solaris.cpp

150

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.

150

.. and the code base should not be littered with -D macros from configure-time variables here and there.

clang/tools/driver/CMakeLists.txt

123

GNU ld reports a warning instead of an error when an unknown -z is seen. The warning remains a warning even with --fatal-warnings.

[ro]

clang/lib/Driver/ToolChains/Solaris.cpp

150

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.

150

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

clang/tools/driver/CMakeLists.txt

123

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.

[vitalybuka]

Other than file issue, compiler-rt part is LGTM
leaving the rest to @MaskRay

compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp

456–521

Chech xdl_iterate_phdr and sanitizer_freebsd.h
I think it's better to move this into some sanitizer_solaris.h/cpp as well

[MaskRay]

clang/tools/driver/CMakeLists.txt

123

OK. Then I guess you can condition this when the OS is Solaris?

compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp

462

In C++ you can just write struct .... { ... }, not need for typedef

[ro]

clang/tools/driver/CMakeLists.txt

123

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?

compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp

456–521

Chech xdl_iterate_phdr and sanitizer_freebsd.h
I think it's better to move this into some sanitizer_solaris.h/cpp as well

Nice: this removes the clutter in common code.

What about GetSizeFromHdr, though? It still lives in sanitizer_linux_libcdep.cpp on FreeBSD. I feel like it's better to keep the two ports similar in this respect.

462

In C++ you can just write struct .... { ... }, not need for typedef

Thanks for the reminder: I keep forgetting about this, being primarily a C guy.

[MaskRay]

clang/tools/driver/CMakeLists.txt

123

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.

[ro]

clang/tools/driver/CMakeLists.txt

123

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.

[ro]

• Move declarations to new sanitizer_solaris.h.
• Augment check_linker_flag do reject unknown -z options that GNU ld noisily accepts.

Tested on amd64-pc-solaris2.11, sparcv9-sun-solaris2.11, and x86_64-pc-linux-gnu. CMakeCache.txt was unchanged on Solaris, while LINKER_SUPPORTS_Z_RELAX_TRANSTLS is now false on Linux, both for stage 1 and 2.

[MaskRay]

clang/tools/driver/CMakeLists.txt

123

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?

[ro]

clang/tools/driver/CMakeLists.txt

123

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.

[MaskRay]

clang/tools/driver/CMakeLists.txt

123

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?

[MaskRay]

clang/tools/driver/CMakeLists.txt

123

ld.bfd --fatal-warnings -z relax=transtls a.o is an error.

[ro]

clang/tools/driver/CMakeLists.txt

123

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.