Page MenuHomePhabricator

[sanitizer] Split Symbolizer/StackTraces from core RTSanitizerCommon
ClosedPublic

Authored by cryptoad on Apr 9 2018, 2:26 PM.

Details

Summary

Host symbolizer & stacktraces related code in their own RT:
RTSanitizerCommonSymbolizer, which is "libcdep" by nature. Symbolizer &
stacktraces specific code that used to live in common files is moved to a new
file sanitizer_symbolizer_report.cc as is.

The purpose of this is the enforce a separation between code that relies on
symbolization and code that doesn't. This saves the inclusion of spurious code
due to the interface functions with default visibility, and the extra data
associated.

The following sanitizers makefiles were modified & tested locally:

  • dfsan: doesn't require the new symbolizer RT
  • esan: requires it
  • hwasan: requires it
  • lsan: requires it
  • msan: requires it
  • safestack: doesn't require it
  • xray: doesn't require it
  • tsan: requires it
  • ubsan: requires it
  • ubsan_minimal: doesn't require it
  • scudo: requires it (but not for Fuchsia that has a minimal runtime)

This was tested locally on Linux, Android, Fuchsia.

Diff Detail

Event Timeline

cryptoad created this revision.Apr 9 2018, 2:26 PM
Herald added subscribers: Restricted Project, llvm-commits, delcypher and 4 others. · View Herald TranscriptApr 9 2018, 2:26 PM
cryptoad updated this revision to Diff 141850.Apr 10 2018, 7:48 AM

Fuchsia related changes.

cryptoad edited the summary of this revision. (Show Details)Apr 11 2018, 8:24 AM
cryptoad added a reviewer: mcgrathr.
cryptoad updated this revision to Diff 142032.Apr 11 2018, 10:01 AM

Removing sanitizer_symbolizer.h from sanitizer_coverage_libcdep_new.cc.

cryptoad updated this revision to Diff 142039.Apr 11 2018, 10:20 AM

Adding the forgotten LLVM comment header to the new file.

alekseyshl accepted this revision.Apr 11 2018, 10:28 AM
alekseyshl added inline comments.
lib/sanitizer_common/sanitizer_symbolizer_report.cc
9

You're one dash short here and in the line below :)

11

Remove extra /

This revision is now accepted and ready to land.Apr 11 2018, 10:28 AM
cryptoad updated this revision to Diff 142044.Apr 11 2018, 10:38 AM
cryptoad marked an inline comment as done.

Aleksey pointed out I was a dash short in the comment. It turns out I copied it
from sanitizer_fuchsia.cc which was also 1 short on each line. So update that
as well, and sanitizer_coverage_fuchsia.cc.

cryptoad added inline comments.Apr 11 2018, 10:39 AM
lib/sanitizer_common/sanitizer_symbolizer_report.cc
11

So the triple / comes from https://llvm.org/docs/CodingStandards.html#file-headers:
The main body is a doxygen comment (identified by the /// comment marker instead of the usual //) describing the purpose of the file.

cryptoad updated this revision to Diff 142046.Apr 11 2018, 10:48 AM
cryptoad marked 2 inline comments as done.

Adding /// to all lines of the second section of the header to be LLVM standard
compliant.

cryptoad updated this revision to Diff 142416.Apr 13 2018, 8:08 AM

Rebasing.

Anyone else for comments?

vitalybuka added inline comments.Apr 13 2018, 2:59 PM
lib/sanitizer_common/sanitizer_symbolizer_report.cc
21

most of this code was in _libcdep.cc
now its in the file without prefix, so in common file
could this be a problem?

vitalybuka accepted this revision.Apr 13 2018, 3:00 PM
cryptoad added inline comments.Apr 13 2018, 3:16 PM
lib/sanitizer_common/sanitizer_symbolizer_report.cc
21

As far as I can tell, this didn't matter anywhere.
I think the _libcdep suffixes could go away for RTSanitizerCommonCoverage & RTSanitizerCommonSymbolizer files if the contract is that they are libc-dependent by nature. The only nolibc consummer that I can see is SafeStack, which depends on neither, so the distinction between libc & nolibc could be reduced to only the core RTSanitizerCommon files.

Once this lands, I can see some added value to consolidating headers & removing dead code as well to see if the distinction can be further reduced.

cryptoad updated this revision to Diff 142634.Apr 16 2018, 8:01 AM

Rebasing.

This revision was automatically updated to reflect the committed changes.