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
