This is an archive of the discontinued LLVM Phabricator instance.

[scudo] Fuchsia minimal shared runtime
ClosedPublic

Authored by cryptoad on Mar 22 2018, 10:24 AM.

Details

Summary

Fuchsia requires its Scudo shared runtime to not be C++ dependant. Since they
don't use UBSan in conjunction with Scudo, we can just remove the runtime,
and add the extra nostdinc++ and nostdlib++ flags. No need for Coverage
either. This allows to keep things going while working on additional splits
of sanitizer_commong and a more minimal runtime.

Event Timeline

cryptoad created this revision.Mar 22 2018, 10:24 AM
Herald added subscribers: Restricted Project, delcypher, mgorny. · View Herald TranscriptMar 22 2018, 10:24 AM

I am actually unsure about the FUCHSIA ifs, it is be used on the builtins but I can't see it defined in compiler-rt.

alekseyshl accepted this revision.Mar 22 2018, 5:09 PM
This revision is now accepted and ready to land.Mar 22 2018, 5:09 PM

Finally have a working Fuchsia toolchain, so I don't have to ask @flowerhack to test my stuff all the time.
I am going to make sure this works as intended and comment back here.

I am actually unsure about the FUCHSIA ifs, it is be used on the builtins but I can't see it defined in compiler-rt.

FUCHSIA is defined by CMake when the CMAKE_SYSTEM_NAME is set to Fuchsia.

cryptoad updated this revision to Diff 139745.Mar 25 2018, 1:11 PM

Finally got the toolchain working.

A few changes: -nostdlib++ is a clang flag, and as of now Fuchsia requires
libunwind to be explicitely linked (it was in SANITIZER_CXX_ABI_LIBRARY
before). It will go away when I am done splitting the stacktraces from
RTSanitizerCommon.

Resulting binary for Fuchsia is no longer C++ dependent (UBSan is gone - for
now), but still has ~512kB of unneeded BSS usage due to the internal allocator
used by the symbolizer. This will also go away when the split it done.

cryptoad requested review of this revision.Mar 25 2018, 1:11 PM
phosek added inline comments.Mar 25 2018, 5:04 PM
lib/scudo/CMakeLists.txt
24

You could add RTSanitizerCommonCoverage, RTUbsan, RTUbsan_cxx to a variable (e.g. ${SCUDO_EXTRA_OBJECT_LIBS}) and ${SANITIZER_CXX_ABI_LIBRARY} to ${SCUDO_DYNAMIC_LIBS} in the else branch and then you don't need the two different branches for add_compiler_rt_runtime(clang_rt.scudo ...) below.

63–64

Is this needed? I see it being included in STATIC library for all platforms and SHARED library for everything but Fuchsia. Is that correct?

cryptoad added inline comments.Mar 25 2018, 5:51 PM
lib/scudo/CMakeLists.txt
24

Will do.

63–64

That is correct.
ubsan_init.cc makes use of __sanitizer::InitializeCoverage so everything that uses UBsan has to include coverage.
Scudo doesn't need coverage or symbolizer, which are intertwined within RTSanitizerCommon, so I am in the process of splitting them both in order to have a thin runtime that isn't pulling in extraneous dependencies from sanitizer_common.

cryptoad updated this revision to Diff 139752.Mar 25 2018, 6:05 PM

Addressing review comments.

phosek added inline comments.Mar 25 2018, 11:31 PM
lib/scudo/CMakeLists.txt
26

I think -nostdlib++ should be in SCUDO_DYNAMIC_LINK_FLAGS as -Wl,-nostdlib++ (unless add_compiler_rt_runtime already adds all CFLAGS to LDFLAGS automatically).

cryptoad added inline comments.Mar 26 2018, 7:55 AM
lib/scudo/CMakeLists.txt
26

I tried that initially but lld (unlike ld) doesn't seem to support it:
ld.lld: error: unknown argument: -nostdlib++
Documentation I could find seems to indicate it as a clang flag (https://clang.llvm.org/docs/ClangCommandLineReference.html)

phosek added inline comments.Mar 26 2018, 12:21 PM
lib/scudo/CMakeLists.txt
26

Oh my bad, it should be -nostdlib++ since that flag is actually processed by Clang driver, not by LLD.

26

However, it should still be in SCUDO_DYNAMIC_LINK_FLAGS rather than SCUDO_FLAGS.

cryptoad updated this revision to Diff 139835.Mar 26 2018, 12:26 PM
cryptoad marked 8 inline comments as done.

Moving -nostdlib++ into the dynamic link flags.

phosek accepted this revision.Mar 26 2018, 3:03 PM

LGTM

This revision is now accepted and ready to land.Mar 26 2018, 3:03 PM
This revision was automatically updated to reflect the committed changes.