This is an archive of the discontinued LLVM Phabricator instance.

[sanitizer] Fix suffix-log-path_test.c on arm-linux-gnu
ClosedPublic

Authored by zatrazz on Feb 9 2021, 6:20 AM.

Details

Summary

The recent suffix-log-path_test.c checks for a full stacktrace and
since on some arm-linux-gnu configuration the slow unwinder is used
on default (when the compiler emits thumb code as default), it
requires -funwind-tables on tests.

It also seems to fix the issues disable by d025df3c1de.

Diff Detail

Event Timeline

zatrazz created this revision.Feb 9 2021, 6:20 AM
zatrazz requested review of this revision.Feb 9 2021, 6:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 9 2021, 6:20 AM
Herald added a subscriber: Restricted Project. · View Herald Transcript

Could this also affect regular users of the sanitizers, or is it specific to the way we are running the tests? If it could affect users, then could we fix this in the clang driver, so that unwind tables are turned on by default whenever the sanitizers need them?

compiler-rt/test/sanitizer_common/CMakeLists.txt
82

Testing for Linux using NOT ANDROID AND NOT WINDOWS AND NOT APPLE will break if more operating systems are added in future, and it's not obvious to me that this list is complete now. This is tested with if("${CMAKE_SYSTEM_NAME}" STREQUAL "Linux") elsewhere in compiler-rt, which looks like a better way to do this.

Could this also affect regular users of the sanitizers, or is it specific to the way we are running the tests? If it could affect users, then could we fix this in the clang driver, so that unwind tables are turned on by default whenever the sanitizers need them?

I think it might affect regular uses on arm-linux-gnueabi, specially if they mix thumb generated object built gcc (which uses a slight different scheme to layout the FP than clang). However, I think it is a orthogonal issue, since compiler-rt can be built as a standalone library. I will work on adding this support on clang driver.

zatrazz added inline comments.Feb 11 2021, 4:17 AM
compiler-rt/test/sanitizer_common/CMakeLists.txt
82

Right, although this is not strickly required for Android (since it uses clang as base compiler and fast-unwinder works as expected).

zatrazz updated this revision to Diff 322958.Feb 11 2021, 5:17 AM

Updated patch based on previous comments.

This revision is now accepted and ready to land.Feb 11 2021, 6:31 AM
This revision was automatically updated to reflect the committed changes.