This is an archive of the discontinued LLVM Phabricator instance.

[UBSan] Embed UBSan into ASan runtime (compiler-rt part).
ClosedPublic

Authored by samsonov on Mar 26 2015, 2:40 PM.

Details

Summary

Change the way we use ASan and UBSan together. Instead of keeping two
separate runtimes (libclang_rt.asan and libclang_rt.ubsan), embed UBSan
into ASan and get rid of libclang_rt.ubsan. If UBSan is not supported on
a platform, all UBSan sources are just compiled into dummy empty object
files. UBSan initialization code (e.g. flag parsing) is directly called
from ASan initialization, so we are able to enforce correct
initialization order.

This mirrors the approach we already use for ASan+LSan.

This change doesn't modify the way we use standalone UBSan.

Diff Detail

Repository
rL LLVM

Event Timeline

samsonov updated this revision to Diff 22757.Mar 26 2015, 2:40 PM
samsonov retitled this revision from to [UBSan] Embed UBSan into ASan runtime (compiler-rt part)..
samsonov updated this object.
samsonov edited the test plan for this revision. (Show Details)
samsonov added reviewers: kubamracek, zaks.anna, rsmith, kcc.
samsonov added a subscriber: Unknown Object (MLST).
samsonov updated this revision to Diff 22763.Mar 26 2015, 3:08 PM
  • Improvements to functions in ubsan_init.cc.

Kuba, Anna,

Could you check if this change works for you on OS X (modulo probable -lc++abi issue I've mentioned in r233036 review thread)? Thanks!

kubamracek edited edge metadata.Mar 28 2015, 11:27 AM

The CMake build fails due to the -lc++abi issue. After adding -lc++abi into DARWIN_osx_LINKFLAGS, the build works and tests pass. I didn't try the Makefile-based build yet.

lib/ubsan/CMakeLists.txt
43 ↗(On Diff #22763)

Typo "CLFAGS" -> "CFLAGS".

ygribov added inline comments.
lib/ubsan/ubsan_init.cc
52 ↗(On Diff #22763)

Note that this will probably break GCC's dynamic UBSan runtime (e.g. when one part of the app is A-sanitized and another is UB-sanitized).

make/platform/clang_darwin.mk
279 ↗(On Diff #22763)

Looks like you use tabs here and below which is inconsistent with existing code.

kcc added inline comments.Mar 30 2015, 11:16 AM
lib/ubsan/ubsan_init.cc
52 ↗(On Diff #22763)

It may, and we will need to do something with this, most likely forbid some of the combination.
The more combinations we support the more complex the code is, and we can not afford that.

samsonov updated this revision to Diff 22898.Mar 30 2015, 12:32 PM
samsonov edited edge metadata.
  • Improvements to functions in ubsan_init.cc.
  • Address comments by Yuri and Kuba.

Added -lc++abi as Kuba suggests. If you confirm that CMake/Makefile build works and passes tests on Apple now, I would like to submit this. If problems with two ASan tests on Jenkins build will re-appear, we will address this issue separately.

lib/ubsan/CMakeLists.txt
43 ↗(On Diff #22763)

Done

lib/ubsan/ubsan_init.cc
52 ↗(On Diff #22763)

Right. I'd even say that we deliberately forbid linking one .so with ASan, and another .so with UBSan - this setup might not cause visible problems for a while, but is inherently broken - you can't link in two copies of sanitizer_common runtime into a process, and there can be issues with initialization order, flag parsing, etc.

make/platform/clang_darwin.mk
279 ↗(On Diff #22763)

Fixed.

Do you have more comments regarding this patch?

zaks.anna edited edge metadata.Apr 1 2015, 1:51 PM

Sorry for the delay. I am going to test now if you are interested in waiting.

Thanks, Anna!

I've built with both cmake and make and ran ninja check-asan and check-ubsan (against make build). Everything seems to work.

OK, I'm going to submit this and watch the bots.

This revision was automatically updated to reflect the committed changes.