This is an archive of the discontinued LLVM Phabricator instance.

[NFC][Sanitizer] Pull up GetStackTrace into sanitizer_common
ClosedPublic

Authored by yln on Feb 25 2019, 4:20 PM.

Details

Summary

We already independently declare GetStackTrace in all (except TSan)
sanitizer runtime headers. Lets move it to sanitizer_stacktrace.h to
have one canonical way to fill in a BufferedStackFrame. Also enables us
to use it in sanitizer_common itself.

This patch defines GetStackTrace for TSan and moves the function from
ubsan_diag.cc to ubsan_diag_standalone.cc to avoid duplicate symbols
for the UBSan-ASan runtime.

Other than that this patch just moves the code out of headers and into
the correct namespace.

Diff Detail

Repository
rL LLVM

Event Timeline

yln created this revision.Feb 25 2019, 4:20 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptFeb 25 2019, 4:20 PM
Herald added subscribers: llvm-commits, Restricted Project, kubamracek. · View Herald Transcript

I like it!

compiler-rt/lib/tsan/rtl/tsan_stack_trace.cc
57 ↗(On Diff #188270)

why common_flags()->fast_unwind_on_fatal when you had argument request_fast_unwind?

vitalybuka accepted this revision.Feb 26 2019, 2:17 PM

LGTM assuming fast_unwind_on_fatal about is a typo

This revision is now accepted and ready to land.Feb 26 2019, 2:17 PM
yln marked 2 inline comments as done.Feb 26 2019, 2:34 PM
yln added inline comments.
compiler-rt/lib/tsan/rtl/tsan_stack_trace.cc
57 ↗(On Diff #188270)

Good catch! Thank you for your diligent reviews! :)

yln marked an inline comment as done.Feb 27 2019, 2:12 PM
This revision was automatically updated to reflect the committed changes.
yln added a comment.EditedFeb 27 2019, 3:49 PM

This introduced the following linker error on some architectures (including ARM).
Attempt to fix: https://github.com/llvm/llvm-project/commit/52b751088b11547e0f4ef0589ebbe5e57752c68c

[2155/3703] Linking CXX shared library lib/clang/9.0.0/lib/linux/libclang_rt.scudo-armhf.so
FAILED: lib/clang/9.0.0/lib/linux/libclang_rt.scudo-armhf.so
: && /usr/local/bin/c++ -fPIC -fPIC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -std=c++11 -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wstring-conversion -fdiagnostics-color -ffunction-sections -fdata-sections -Wall -std=c++11 -Wno-unused-parameter -O3 -Wl,-z,defs -Wl,-z,nodelete -march=armv7-a -mfloat-abi=hard -nodefaultlibs -Wl,--gc-sections -Wl,-rpath-link,/home/buildslave/buildslave/clang-cmake-armv8-full/stage1/./lib -shared -Wl,-soname,libclang_rt.scudo-armhf.so -o lib/clang/9.0.0/lib/linux/libclang_rt.scudo-armhf.so projects/compiler-rt/lib/sanitizer_common/CMakeFiles/RTSanitizerCommonNoTermination.armhf.dir/sanitizer_allocator.cc.o projects/compiler-rt/lib/sanitizer_common/CMakeFiles/RTSanitizerCommonNoTermination.armhf.dir/sanitizer_common.cc.o projects/compiler-rt/lib/sanitizer_common/CMakeFiles/ [...] projects/compiler-rt/lib/scudo/CMakeFiles/clang_rt.scudo-dynamic-armhf.dir/scudo_flags.cpp.o projects/compiler-rt/lib/scudo/CMakeFiles/clang_rt.scudo-dynamic-armhf.dir/scudo_malloc.cpp.o projects/compiler-rt/lib/scudo/CMakeFiles/clang_rt.scudo-dynamic-armhf.dir/scudo_termination.cpp.o projects/compiler-rt/lib/scudo/CMakeFiles/clang_rt.scudo-dynamic-armhf.dir/scudo_tsd_exclusive.cpp.o projects/compiler-rt/lib/scudo/CMakeFiles/clang_rt.scudo-dynamic-armhf.dir/scudo_tsd_shared.cpp.o projects/compiler-rt/lib/scudo/CMakeFiles/clang_rt.scudo-dynamic-armhf.dir/scudo_utils.cpp.o projects/compiler-rt/lib/scudo/CMakeFiles/clang_rt.scudo-dynamic-armhf.dir/scudo_new_delete.cpp.o -lgcc_s -lc -ldl -lrt -lpthread -lstdc++ -Wl,-rpath,"\$ORIGIN/../lib" && :
projects/compiler-rt/lib/ubsan/CMakeFiles/RTUbsan.armhf.dir/ubsan_diag.cc.o: In function `MaybePrintStackTrace':
/home/buildslave/buildslave/clang-cmake-armv8-full/llvm/projects/compiler-rt/lib/ubsan/ubsan_diag.cc:36: undefined reference to `sanitizer::GetStackTrace(sanitizer::BufferedStackTrace*, unsigned long, unsigned long, unsigned long, void*, bool)'
clang-6.0: error: linker command failed with exit code 1 (use -v to see invocation)

+ @rnk

It turns out that the fix for Windows in rL355052 didn't turn out to help either: http://lab.llvm.org:8011/builders/sanitizer-windows/builds/42624/steps/stage%201%20build/logs/stdio

FWIW, on MinGW, this didn't break compilation like on MSVC, but linking still fails because SANITIZER_WEAK_ATTRIBUTE (as part of SANITIZER_WEAK_CXX_DEFAULT_IMPL) is empty for windows. (For MinGW, GCC does support __attribute__((weak)), but that isn't implemented in clang/llvm.)

yln added a comment.Feb 28 2019, 9:35 AM

Is there a way to emulate weak linkage on windows?
Can you offer guidance on how to resolve this?

rnk added a comment.Feb 28 2019, 9:39 AM
In D58651#1413884, @yln wrote:

Is there a way to emulate weak linkage on windows?
Can you offer guidance on how to resolve this?

You have activated the sanitizer trap card, attempting to use weak functions in portable code. You can achieve the same affect with weak aliases, #pragma comment(linker, "/alternatename:foo=foo_default"), but it is not directly functionally equivalent to weak functions. If you search around for that pragma you should be able to find the code pattern you need.

I wish the sanitizers would use fewer weak functions so that we didn't experience this pain every time a new one is introduced.

yln added a comment.Feb 28 2019, 9:51 AM

You can achieve the same affect with weak aliases, #pragma comment(linker, "/alternatename:foo=foo_default"), but it is not directly functionally equivalent to weak functions. If you search around for that pragma you should be able to find the code pattern you need.

Thanks!

You have activated the sanitizer trap card, attempting to use weak functions in portable code.
I wish the sanitizers would use fewer weak functions so that we didn't experience this pain every time a new one is introduced.

I totally agree. This is a clear dependency violation (sanitizer_common -> asan, ubsan, ...). In my defense: this was present before (the implementation code is copy & pasted 5 times), but having it declared in sanitizer_common header "exposed" it.

The end goal of my refactoring is to either unify the implementation and pull it up into sanitizer_common or, if that is not possible, at least break the dependency violation (callback, templates?).