Page MenuHomePhabricator

[Sanitizers] Basic Solaris sanitizer support (PR 33274)
ClosedPublic

Authored by ro on Dec 6 2017, 8:28 AM.

Details

Summary

This patch (on top of https://reviews.llvm.org/D35755) provides the clang side necessary
to enable the Solaris port of the sanitizers implemented by https://reviews.llvm.org/D40898,
https://reviews.llvm.org/D40899, and https://reviews.llvm.org/D40900).

A few features of note:

  • While compiler-rt cmake/base-config-ix.cmake (COMPILER_RT_OS_DIR) places the runtime libs in a tolower(CMAKE_SYSTEM_NAME) directory, clang defaults to the OS part of the target triplet (solaris2.11 in the case at hand). The patch makes them agree on compiler-rt's idea.
  • While Solaris ld accepts a considerable number of GNU ld options for compatibility, it only does so for the double-dash forms. clang unfortunately is inconsistent here and sometimes uses the double-dash form, sometimes the single-dash one that confuses the hell out of Solaris ld. I've changed the affected places to use the double-dash form that should always work.
  • As described in https://reviews.llvm.org/D40899, Solaris ld doesn't create the start_sancov_guards/__stop___sancov_guards labels gld/gold/lld do, so I'm including additional runtime libs into the link that provide them.
  • One test uses -fstack-protector, but unlike other systems libssp hasn't been folded into Solaris libc, but needs to be linked with separately.
  • For now, only 32-bit x86 asan is enabled on Solaris. 64-bit x86 should follow, but sparc (which requires additional compiler-rt changes not yet submitted) fails miserably due to a llvmsparc backend limitation:

fatal error: error in backend: Function "_ZN7testing8internal16BoolFromGTestEnvEPKcb": over-aligned dynamic alloca not supported.

However, inside the gcc tree, Solaris/sparc asan works almost as well as x86.

Diff Detail

Repository
rC Clang

Event Timeline

ro created this revision.Dec 6 2017, 8:28 AM
alekseyshl added inline comments.Dec 12 2017, 1:55 PM
lib/Driver/ToolChains/CommonArgs.cpp
699

If it does not exist on Solaris, why change it then? For the consistency sake?

ro added inline comments.Dec 13 2017, 1:53 AM
lib/Driver/ToolChains/CommonArgs.cpp
699

Consistency for one. Besides, I'm talking to the Solaris linker
engineers whether to just accept and ignore that option
for GNU ld compatibility.

alekseyshl added inline comments.Dec 13 2017, 10:05 AM
lib/Driver/ToolChains/CommonArgs.cpp
525

Can you elaborate why Solaris ld does not need dynamic lists? How does it export sanitizer symbols then?

ro added inline comments.Dec 18 2017, 6:43 AM
lib/Driver/ToolChains/CommonArgs.cpp
525

Solaris ld simply defaults to --export-dynamic.

alekseyshl accepted this revision.Dec 18 2017, 7:47 AM
alekseyshl added inline comments.
lib/Driver/ToolChains/CommonArgs.cpp
525

Can we say then that ld "defaults to --export-dynamic behavior" (as --export-dynamic does not exists there) instead of "doesn't need this"?

This revision is now accepted and ready to land.Dec 18 2017, 7:47 AM
ro marked an inline comment as done.Dec 19 2017, 4:41 AM
ro added inline comments.
lib/Driver/ToolChains/CommonArgs.cpp
525

Sure, that's certainly clearer. As in the revised patch?

ro updated this revision to Diff 127499.Dec 19 2017, 4:53 AM
ro marked an inline comment as done.

Reworded comment.

ro added a comment.Dec 19 2017, 4:55 AM

There are two issues before the patch can be commited:

  • I'm currently working with an experimental version of Solaris ld that *does* support generation of start_sancov_guards etc. labels. This will most likely go into Solaris 11.4, but somehow older linkers without that support (from Solaris 11.3 or OpenSolaris derivatives) will have to be supported, probably by a cmake test. Not sure yet how best to do this.
ro added a comment.Feb 5 2018, 2:25 PM

Could anyone please commit this for me?

Thanks.

Rainer
This revision was automatically updated to reflect the committed changes.

Rainer, check rC324302 out, you probably want to add test cases for Solaris to test/Driver/sanitizer-ld.c too.

ro added a comment.Feb 6 2018, 1:12 AM

Rainer, check rC324302 out, you probably want to add test cases for Solaris to test/Driver/sanitizer-ld.c too.

Sorry about that! I certainly will, but it'll be some time because I'm about to leave for an extended vacation, to return only in late march.