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

[Rainer Orth via Phabricator via cfe-commits]

ro abandoned this revision.
ro added a comment.

Superceded by D119829  and D120048 
.  If absolutely necessary, D120059 
 could be revived if Illumos cannot implement 
`dlpi_tls_modid`.


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

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

[Rainer Orth via Phabricator via cfe-commits]

ro added a comment.

In D91605#3329375 , @ro wrote:

> 



> As an experiment, I've tried to use `GetStaticTlsBoundary` instead.  It does 
> indeed work on recent Solaris 11.4 and the resulting patch is at D120048 
> .  I'm pretty certain that support for 
> Solaris 11.3/Illumos which lack `dlpi_tls_modid` using `dlinfo(RTLD_SELF, 
> RTLD_DI_LINKMAP)` can be added on top of that one, unbreaking the Illumos 
> build.  This would avoid considerable duplication of the `dl_iterate_phdr` 
> code, which is certainly a nice benefit.  I'll experiment with that route 
> later.

Now posted as D120059 .  It this approach is 
acceptable, the series of D119829 , D120048 
, and D120059 
 will supercede this one.


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

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

[Rainer Orth via Phabricator via cfe-commits]

ro added a comment.

In D91605#3322312 , @ro wrote:

> In D91605#3321554 , @MaskRay wrote:
>
>> `GetTls` is about the static TLS block size. It consists of the main 
>> executable's TLS, and initially loaded shared objects' TLS, `struct 
>> pthread`, and padding.
>> Solaris should be able to just use the code path like FreeBSD, Linux musl, 
>> and various unpopular architectures of Linux glibc:
>>
>>   #elif SANITIZER_FREEBSD || SANITIZER_LINUX
>> uptr align;
>> GetStaticTlsBoundary(addr, size, );
>
> Not unconditionally, unfortunally: as the comment above `GetSizeFromHdr` 
> explains, `dlpi_tls_modid` was only introduced in an update to Solaris 11.4 
> FCS (which is sort of a problem), but isn't present in 11.3 (don't reallly 
> care) and Illumos (this would break compilation for them).  OTOH my solution 
> is successfully being used in GCC's `libphobos` on Solaris 11.3, 11.4, and 
> most likely Illumos, too.  I'd rather not burn the Illumos bridge if it can 
> be avoided.

As an experiment, I've tried to use `GetStaticTlsBoundary` instead.  It does 
indeed work on recent Solaris 11.4 and the resulting patch is at D120048 
.  I'm pretty certain that support for 
Solaris 11.3/Illumos which lack `dlpi_tls_modid` using `dlinfo(RTLD_SELF, 
RTLD_DI_LINKMAP)` can be added on top of that one, unbreaking the Illumos 
build.  This would avoid considerable duplication of the `dl_iterate_phdr` 
code, which is certainly a nice benefit.  I'll experiment with that route later.

I know now why I did the present patch the way it is: `GetStaticTlsBoundary` 
was only introduced months after I submitted this one.


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

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

[Rainer Orth via Phabricator via cfe-commits]

ro added a comment.

In D91605#3321556 , @MaskRay wrote:

> The Clang driver change should be in a separate patch with clang/test/Driver 
> tests.

Done now: D119829 .


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

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

[Rainer Orth via Phabricator via cfe-commits]

ro added a comment.

In D91605#3321554 , @MaskRay wrote:

> `GetTls` is about the static TLS block size. It consists of the main 
> executable's TLS, and initially loaded shared objects' TLS, `struct pthread`, 
> and padding.
> Solaris should be able to just use the code path like FreeBSD, Linux musl, 
> and various unpopular architectures of Linux glibc:
>
>   #elif SANITIZER_FREEBSD || SANITIZER_LINUX
> uptr align;
> GetStaticTlsBoundary(addr, size, );

Not unconditionally, unfortunally: as the comment above `GetSizeFromHdr` 
explains, `dlpi_tls_modid` was only introduced in an update to Solaris 11.4 FCS 
(which is sort of a problem), but isn't present in 11.3 (don't reallly care) 
and Illumos (this would break compilation for them).  OTOH my solution is 
successfully being used in GCC's `libphobos` on Solaris 11.3, 11.4, and most 
likely Illumos, too.  I'd rather not burn the Illumos bridge if it can be 
avoided.


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

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

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

The Clang driver change should be in a separate patch with clang/test/Driver 
tests.


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

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

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

`GetTls` is about the static TLS block size. It consists of the main 
executable's TLS, and initially loaded shared objects' TLS, `struct pthread`, 
and padding.
Solaris should be able to just use the code path like Linux musl:

  #elif SANITIZER_FREEBSD || SANITIZER_LINUX
uptr align;
GetStaticTlsBoundary(addr, size, );

I think NetBSD should use this code path as well, but I don't have NetBSD for 
testing.


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

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

[Rainer Orth via Phabricator via cfe-commits]

ro added a comment.

@vitalybuka , @MaskRay could I persuade you to review the revision version 
which should be much less controversial and completely Solaris-specific?  
Thanks.


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

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

[Rainer Orth via Phabricator via cfe-commits]

ro updated this revision to Diff 408389.
ro added a subscriber: nikic.
ro added a comment.

Since the build-time check for the `ld -z relax=transtls` option was met with 
massive resistance and was only necessary for Illumos anyway, this revision 
just uses it unconditionally.

Tested on `amd64-pc-solaris2.11` and `sparcv9-sun-solaris2.11`.

The Illumos community will have to provide a way to somehow distinguish the two 
OSes.  The configure triple is still the same, and the `uname -o` output 
(`Solaris` vs. `illumos` seems to be hardcoded in the `uname` command and not 
be available programmatically.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91605

Files:
  clang/lib/Driver/ToolChains/Solaris.cpp
  compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp
  compiler-rt/lib/sanitizer_common/sanitizer_solaris.h

Index: compiler-rt/lib/sanitizer_common/sanitizer_solaris.h
===
--- /dev/null
+++ compiler-rt/lib/sanitizer_common/sanitizer_solaris.h
@@ -0,0 +1,63 @@
+//===-- sanitizer_solaris.h -*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// This file is a part of Sanitizer runtime. It contains Solaris-specific
+// definitions.
+//
+//===--===//
+
+#ifndef SANITIZER_SOLARIS_H
+#define SANITIZER_SOLARIS_H
+
+#include "sanitizer_internal_defs.h"
+
+#if SANITIZER_SOLARIS
+
+#include 
+
+namespace __sanitizer {
+
+// Beginning of declaration from OpenSolaris/Illumos
+// $SRC/cmd/sgs/include/rtld.h.
+struct Rt_map {
+  Link_map rt_public;
+  const char *rt_pathname;
+  ulong_t rt_padstart;
+  ulong_t rt_padimlen;
+  ulong_t rt_msize;
+  uint_t rt_flags;
+  uint_t rt_flags1;
+  ulong_t rt_tlsmodid;
+};
+
+// Structure matching the Solaris 11.4 struct dl_phdr_info used to determine
+// presence of dlpi_tls_modid field at runtime.  Cf. Solaris 11.4
+// dl_iterate_phdr(3C), Example 2.
+struct dl_phdr_info_test {
+  ElfW(Addr) dlpi_addr;
+  const char *dlpi_name;
+  const ElfW(Phdr) * dlpi_phdr;
+  ElfW(Half) dlpi_phnum;
+  u_longlong_t dlpi_adds;
+  u_longlong_t dlpi_subs;
+  size_t dlpi_tls_modid;
+  void *dlpi_tls_data;
+};
+
+struct TLS_index {
+  unsigned long ti_moduleid;
+  unsigned long ti_tlsoffset;
+};
+
+extern "C" void *__tls_get_addr(TLS_index *);
+
+}  // namespace __sanitizer
+
+#endif  // SANITIZER_SOLARIS
+
+#endif  // SANITIZER_SOLARIS_H
Index: compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp
===
--- compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp
+++ compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp
@@ -27,6 +27,7 @@
 #include "sanitizer_linux.h"
 #include "sanitizer_placement_new.h"
 #include "sanitizer_procmaps.h"
+#include "sanitizer_solaris.h"
 
 #if SANITIZER_NETBSD
 #define _RTLD_SOURCE  // for __lwp_gettcb_fast() / __lwp_getprivate_fast()
@@ -62,6 +63,7 @@
 #endif
 
 #if SANITIZER_SOLARIS
+#include 
 #include 
 #include 
 #endif
@@ -444,6 +446,39 @@
   void **);
 #endif
 
+#if SANITIZER_SOLARIS
+// dlpi_tls_modid is only available since Solaris 11.4 SRU 10.  Use
+// dlinfo(RTLD_DI_LINKMAP) instead which works on both Solaris 11.3 and Illumos.
+
+static size_t main_tls_modid;
+
+int GetSizeFromHdr(struct dl_phdr_info *info, size_t size, void *data) {
+  const ElfW(Phdr) *hdr = info->dlpi_phdr;
+  const ElfW(Phdr) *last_hdr = hdr + info->dlpi_phnum;
+
+  // With the introduction of dlpi_tls_modid, the tlsmodid of the executable
+  // was changed to 1 to match other implementations.
+  if (size >= offsetof(dl_phdr_info_test, dlpi_tls_modid))
+main_tls_modid = 1;
+  else
+main_tls_modid = 0;
+
+  for (; hdr != last_hdr; ++hdr) {
+if (hdr->p_type == PT_TLS) {
+  Rt_map *map;
+
+  dlinfo(RTLD_SELF, RTLD_DI_LINKMAP, );
+
+  if (map->rt_tlsmodid == main_tls_modid) {
+*(uptr *)data = hdr->p_memsz;
+return -1;
+  }
+}
+  }
+  return 0;
+}
+#endif  // SANITIZER_SOLARIS
+
 #if !SANITIZER_GO
 static void GetTls(uptr *addr, uptr *size) {
 #if SANITIZER_ANDROID
@@ -538,9 +573,15 @@
 }
   }
 #elif SANITIZER_SOLARIS
-  // FIXME
   *addr = 0;
   *size = 0;
+  // Find size (p_memsz) of TLS block of the main program.
+  dl_iterate_phdr(GetSizeFromHdr, size);
+
+  if (*size != 0) {
+TLS_index ti = {(unsigned long)main_tls_modid, 0};
+*addr = (uptr)__tls_get_addr();
+  }
 #else
 #error "Unknown OS"
 #endif
Index: clang/lib/Driver/ToolChains/Solaris.cpp

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

[Rainer Orth via Phabricator via cfe-commits]

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 

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

[Fangrui Song via Phabricator via cfe-commits]

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 

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

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.



Comment at: clang/tools/driver/CMakeLists.txt:123
+
+check_linker_flag("-Wl,-z,relax=transtls" LINKER_SUPPORTS_Z_RELAX_TRANSTLS)

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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91605

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

[Rainer Orth via Phabricator via cfe-commits]

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


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

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

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.



Comment at: clang/tools/driver/CMakeLists.txt:123
+
+check_linker_flag("-Wl,-z,relax=transtls" LINKER_SUPPORTS_Z_RELAX_TRANSTLS)

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?


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

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

[Rainer Orth via Phabricator via cfe-commits]

ro updated this revision to Diff 307067.
ro added a comment.
Herald added a project: LLVM.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91605

Files:
  clang/include/clang/Config/config.h.cmake
  clang/lib/Driver/ToolChains/Solaris.cpp
  clang/tools/driver/CMakeLists.txt
  compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp
  compiler-rt/lib/sanitizer_common/sanitizer_solaris.h
  llvm/cmake/modules/CheckLinkerFlag.cmake

Index: llvm/cmake/modules/CheckLinkerFlag.cmake
===
--- llvm/cmake/modules/CheckLinkerFlag.cmake
+++ llvm/cmake/modules/CheckLinkerFlag.cmake
@@ -1,6 +1,10 @@
-include(CheckCXXCompilerFlag)
+include(CheckCXXSourceCompiles)
 
 function(check_linker_flag flag out_var)
-  set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} ${flag}")
-  check_cxx_compiler_flag("" ${out_var})
+  set(OLD_CMAKE_REQUIRED_FLAGS ${CMAKE_REQUIRED_FLAGS})
+  set(CMAKE_REQUIRED_FLAGS "${CMAKE_REQUIRED_FLAGS} ${flag}")
+  # GNU ld accepts every -z option, warning if it isn't supported, e.g.
+  # /usr/bin/ld: warning: -z relax=transtls ignored
+  check_cxx_source_compiles("int main() { return 0; }" ${out_var} FAIL_REGEX "warning: -z.* ignored")
+  set(CMAKE_REQUIRED_FLAGS ${OLD_CMAKE_REQUIRED_FLAGS})
 endfunction()
Index: compiler-rt/lib/sanitizer_common/sanitizer_solaris.h
===
--- /dev/null
+++ compiler-rt/lib/sanitizer_common/sanitizer_solaris.h
@@ -0,0 +1,63 @@
+//===-- sanitizer_solaris.h -*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// This file is a part of Sanitizer runtime. It contains Solaris-specific
+// definitions.
+//
+//===--===//
+
+#ifndef SANITIZER_SOLARIS_H
+#define SANITIZER_SOLARIS_H
+
+#include "sanitizer_internal_defs.h"
+
+#if SANITIZER_SOLARIS
+
+#include 
+
+namespace __sanitizer {
+
+// Beginning of declaration from OpenSolaris/Illumos
+// $SRC/cmd/sgs/include/rtld.h.
+struct Rt_map {
+  Link_map rt_public;
+  const char *rt_pathname;
+  ulong_t rt_padstart;
+  ulong_t rt_padimlen;
+  ulong_t rt_msize;
+  uint_t rt_flags;
+  uint_t rt_flags1;
+  ulong_t rt_tlsmodid;
+};
+
+// Structure matching the Solaris 11.4 struct dl_phdr_info used to determine
+// presence of dlpi_tls_modid field at runtime.  Cf. Solaris 11.4
+// dl_iterate_phdr(3C), Example 2.
+struct dl_phdr_info_test {
+  ElfW(Addr) dlpi_addr;
+  const char *dlpi_name;
+  const ElfW(Phdr) * dlpi_phdr;
+  ElfW(Half) dlpi_phnum;
+  u_longlong_t dlpi_adds;
+  u_longlong_t dlpi_subs;
+  size_t dlpi_tls_modid;
+  void *dlpi_tls_data;
+};
+
+struct TLS_index {
+  unsigned long ti_moduleid;
+  unsigned long ti_tlsoffset;
+};
+
+extern "C" void *__tls_get_addr(TLS_index *);
+
+}  // namespace __sanitizer
+
+#endif  // SANITIZER_SOLARIS
+
+#endif  // SANITIZER_SOLARIS_H
Index: compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp
===
--- compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp
+++ compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp
@@ -27,6 +27,7 @@
 #include "sanitizer_linux.h"
 #include "sanitizer_placement_new.h"
 #include "sanitizer_procmaps.h"
+#include "sanitizer_solaris.h"
 
 #if SANITIZER_NETBSD
 #define _RTLD_SOURCE  // for __lwp_gettcb_fast() / __lwp_getprivate_fast()
@@ -57,6 +58,7 @@
 #endif
 
 #if SANITIZER_SOLARIS
+#include 
 #include 
 #include 
 #endif
@@ -451,6 +453,39 @@
   void **);
 #endif
 
+#if SANITIZER_SOLARIS
+// dlpi_tls_modid is only available since Solaris 11.4 SRU 10.  Use
+// dlinfo(RTLD_DI_LINKMAP) instead which works on both Solaris 11.3 and Illumos.
+
+static size_t main_tls_modid;
+
+int GetSizeFromHdr(struct dl_phdr_info *info, size_t size, void *data) {
+  const ElfW(Phdr) *hdr = info->dlpi_phdr;
+  const ElfW(Phdr) *last_hdr = hdr + info->dlpi_phnum;
+
+  // With the introduction of dlpi_tls_modid, the tlsmodid of the executable
+  // was changed to 1 to match other implementations.
+  if (size >= offsetof(dl_phdr_info_test, dlpi_tls_modid))
+main_tls_modid = 1;
+  else
+main_tls_modid = 

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

[Rainer Orth via Phabricator via cfe-commits]

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


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

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

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.



Comment at: clang/tools/driver/CMakeLists.txt:123
+
+check_linker_flag("-Wl,-z,relax=transtls" LINKER_SUPPORTS_Z_RELAX_TRANSTLS)

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.


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

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

[Rainer Orth via Phabricator via cfe-commits]

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:
> 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?



Comment at: compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp:455-520
+#if SANITIZER_SOLARIS
+// dlpi_tls_modid is only available since Solaris 11.4 SRU 10.  Use
+// dlinfo(RTLD_DI_LINKMAP) instead which works on both Solaris 11.3 and 
Illumos.
+
+// Beginning of declaration from OpenSolaris/Illumos
+// $SRC/cmd/sgs/include/rtld.h.
+typedef struct {

vitalybuka wrote:
> ro wrote:
> > vitalybuka wrote:
> > > can this go into sanitizer_platform_limits_solaris.h ?
> > > 
> > 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.
> Chech xdl_iterate_phdr and sanitizer_freebsd.h
> I think it's better to move this into some sanitizer_solaris.h/cpp as well
> 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.



Comment at: compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp:461
+// $SRC/cmd/sgs/include/rtld.h.
+typedef struct {
+  Link_map rt_public;

MaskRay wrote:
> In C++ you can just write `struct  { ... }`, not need for typedef
> 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.


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

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

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.



Comment at: clang/tools/driver/CMakeLists.txt:123
+
+check_linker_flag("-Wl,-z,relax=transtls" LINKER_SUPPORTS_Z_RELAX_TRANSTLS)

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?



Comment at: compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp:461
+// $SRC/cmd/sgs/include/rtld.h.
+typedef struct {
+  Link_map rt_public;

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


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

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

[Vitaly Buka via Phabricator via cfe-commits]

vitalybuka added a comment.

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




Comment at: compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp:455-520
+#if SANITIZER_SOLARIS
+// dlpi_tls_modid is only available since Solaris 11.4 SRU 10.  Use
+// dlinfo(RTLD_DI_LINKMAP) instead which works on both Solaris 11.3 and 
Illumos.
+
+// Beginning of declaration from OpenSolaris/Illumos
+// $SRC/cmd/sgs/include/rtld.h.
+typedef struct {

ro wrote:
> vitalybuka wrote:
> > can this go into sanitizer_platform_limits_solaris.h ?
> > 
> 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.
Chech xdl_iterate_phdr and sanitizer_freebsd.h
I think it's better to move this into some sanitizer_solaris.h/cpp as well


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

[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  = 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  = 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 

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

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.



Comment at: clang/lib/Driver/ToolChains/Solaris.cpp:150
+  const SanitizerArgs  = getToolChain().getSanitizerArgs();
+  if (LINKER_SUPPORTS_Z_RELAX_TRANSTLS &&
+  getToolChain().getTriple().getArch() == llvm::Triple::x86_64 &&

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.


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

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

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.



Comment at: clang/lib/Driver/ToolChains/Solaris.cpp:150
+  const SanitizerArgs  = getToolChain().getSanitizerArgs();
+  if (LINKER_SUPPORTS_Z_RELAX_TRANSTLS &&
+  getToolChain().getTriple().getArch() == llvm::Triple::x86_64 &&

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.



Comment at: clang/tools/driver/CMakeLists.txt:123
+
+check_linker_flag("-Wl,-z,relax=transtls" LINKER_SUPPORTS_Z_RELAX_TRANSTLS)

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


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

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

[Rainer Orth via Phabricator via cfe-commits]

ro added inline comments.



Comment at: compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp:455-520
+#if SANITIZER_SOLARIS
+// dlpi_tls_modid is only available since Solaris 11.4 SRU 10.  Use
+// dlinfo(RTLD_DI_LINKMAP) instead which works on both Solaris 11.3 and 
Illumos.
+
+// Beginning of declaration from OpenSolaris/Illumos
+// $SRC/cmd/sgs/include/rtld.h.
+typedef struct {

vitalybuka wrote:
> can this go into sanitizer_platform_limits_solaris.h ?
> 
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.


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

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

[Vitaly Buka via Phabricator via cfe-commits]

vitalybuka added inline comments.



Comment at: compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp:455-520
+#if SANITIZER_SOLARIS
+// dlpi_tls_modid is only available since Solaris 11.4 SRU 10.  Use
+// dlinfo(RTLD_DI_LINKMAP) instead which works on both Solaris 11.3 and 
Illumos.
+
+// Beginning of declaration from OpenSolaris/Illumos
+// $SRC/cmd/sgs/include/rtld.h.
+typedef struct {

can this go into sanitizer_platform_limits_solaris.h ?



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

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

[Rainer Orth via Phabricator via cfe-commits]

ro created this revision.
ro added reviewers: vitalybuka, MaskRay.
ro added a project: Sanitizers.
Herald added subscribers: Sanitizers, pengfei, fedor.sergeev, mgorny, jyknight.
Herald added a project: clang.
ro requested review of this revision.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D91605

Files:
  clang/include/clang/Config/config.h.cmake
  clang/lib/Driver/ToolChains/Solaris.cpp
  clang/tools/driver/CMakeLists.txt
  compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp

Index: compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp
===
--- compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp
+++ compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp
@@ -57,6 +57,7 @@
 #endif
 
 #if SANITIZER_SOLARIS
+#include 
 #include 
 #include 
 #endif
@@ -451,6 +452,73 @@
   void **);
 #endif
 
+#if SANITIZER_SOLARIS
+// dlpi_tls_modid is only available since Solaris 11.4 SRU 10.  Use
+// dlinfo(RTLD_DI_LINKMAP) instead which works on both Solaris 11.3 and Illumos.
+
+// Beginning of declaration from OpenSolaris/Illumos
+// $SRC/cmd/sgs/include/rtld.h.
+typedef struct {
+  Link_map rt_public;
+  const char *rt_pathname;
+  ulong_t rt_padstart;
+  ulong_t rt_padimlen;
+  ulong_t rt_msize;
+  uint_t rt_flags;
+  uint_t rt_flags1;
+  ulong_t rt_tlsmodid;
+} Rt_map;
+
+// Structure matching the Solaris 11.4 struct dl_phdr_info used to determine
+// presence of dlpi_tls_modid field at runtime.  Cf. Solaris 11.4
+// dl_iterate_phdr(3C), Example 2.
+typedef struct dl_phdr_info_test {
+  ElfW(Addr) dlpi_addr;
+  const char *dlpi_name;
+  const ElfW(Phdr) * dlpi_phdr;
+  ElfW(Half) dlpi_phnum;
+  u_longlong_t dlpi_adds;
+  u_longlong_t dlpi_subs;
+  size_t dlpi_tls_modid;
+  void *dlpi_tls_data;
+} dl_phdr_info_test;
+
+typedef struct {
+  unsigned long ti_moduleid;
+  unsigned long ti_tlsoffset;
+} TLS_index;
+
+extern "C" void *__tls_get_addr(TLS_index *);
+
+static size_t main_tls_modid;
+
+int GetSizeFromHdr(struct dl_phdr_info *info, size_t size, void *data) {
+  const ElfW(Phdr) *hdr = info->dlpi_phdr;
+  const ElfW(Phdr) *last_hdr = hdr + info->dlpi_phnum;
+
+  // With the introduction of dlpi_tls_modid, the tlsmodid of the executable
+  // was changed to 1 to match other implementations.
+  if (size >= offsetof(dl_phdr_info_test, dlpi_tls_modid))
+main_tls_modid = 1;
+  else
+main_tls_modid = 0;
+
+  for (; hdr != last_hdr; ++hdr) {
+if (hdr->p_type == PT_TLS) {
+  Rt_map *map;
+
+  dlinfo(RTLD_SELF, RTLD_DI_LINKMAP, );
+
+  if (map->rt_tlsmodid == main_tls_modid) {
+*(uptr *)data = hdr->p_memsz;
+return -1;
+  }
+}
+  }
+  return 0;
+}
+#endif  // SANITIZER_SOLARIS
+
 #if !SANITIZER_GO
 static void GetTls(uptr *addr, uptr *size) {
 #if SANITIZER_ANDROID
@@ -507,9 +575,15 @@
 }
   }
 #elif SANITIZER_SOLARIS
-  // FIXME
   *addr = 0;
   *size = 0;
+  // Find size (p_memsz) of TLS block of the main program.
+  dl_iterate_phdr(GetSizeFromHdr, size);
+
+  if (*size != 0) {
+TLS_index ti = {(unsigned long)main_tls_modid, 0};
+*addr = (uptr)__tls_get_addr();
+  }
 #else
 #error "Unknown OS"
 #endif
Index: clang/tools/driver/CMakeLists.txt
===
--- clang/tools/driver/CMakeLists.txt
+++ clang/tools/driver/CMakeLists.txt
@@ -91,9 +91,10 @@
   set(TOOL_INFO_BUILD_VERSION)
 endif()
 
+include(CheckLinkerFlag)
+
 if(CLANG_ORDER_FILE AND
-(LLVM_LINKER_IS_LD64 OR LLVM_LINKER_IS_GOLD OR LLVM_LINKER_IS_LLD))
-  include(CheckLinkerFlag)
+   (LLVM_LINKER_IS_LD64 OR LLVM_LINKER_IS_GOLD OR LLVM_LINKER_IS_LLD))
 
   if (LLVM_LINKER_IS_LD64)
 set(LINKER_ORDER_FILE_OPTION "-Wl,-order_file,${CLANG_ORDER_FILE}")
@@ -118,3 +119,5 @@
 set_target_properties(clang PROPERTIES LINK_DEPENDS ${CLANG_ORDER_FILE})
   endif()
 endif()
+
+check_linker_flag("-Wl,-z,relax=transtls"