This is an archive of the discontinued LLVM Phabricator instance.

Differential D84805

[Scudo][CMake] Add -fno-lto to Scudo libraries
ClosedPublic

Authored by aeubanks on Jul 28 2020, 3:01 PM.

Details

Reviewers
cryptoad
pcc
hctim
Commits
rGe7196bdf8125: [Scudo][CMake] Add -fno-lto to Scudo libraries
Summary

-fno-lto is in SANITIZER_COMMON_CFLAGS but not here.
Don't use SANITIZER_COMMON_CFLAGS because of performance issues.
See https://bugs.llvm.org/show_bug.cgi?id=46838.

Fixes
$ ninja TScudoCUnitTest-i386-Test
on an LLVM build with -DLLVM_ENABLE_LTO=Thin.
check-scudo now passes.

Diff Detail

Event Timeline

aeubanks created this revision.Jul 28 2020, 3:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 28 2020, 3:01 PM
Herald added subscribers: Restricted Project, dexonsmith, cryptoad, mgorny. · View Herald Transcript
aeubanks requested review of this revision.Jul 28 2020, 3:01 PM
aeubanks added reviewers: cryptoad, pcc.Jul 28 2020, 3:03 PM
cryptoad added inline comments.Jul 28 2020, 3:10 PM
compiler-rt/lib/scudo/standalone/CMakeLists.txt
8

So there are various things we don't want from SANITIZER_COMMON_CFLAGS, like -fno-builtin etc.
The other sanitizer are not meant to be performance critical while Scudo is.
Just using SANITIZER_COMMON_CFLAGS as is wouldn't work for us.

aeubanks added inline comments.Jul 28 2020, 3:19 PM
compiler-rt/lib/scudo/standalone/CMakeLists.txt
8

Can you suggest any alternatives?
I'm not familiar with Scudo, but it seems like it's pulling in GWPAsan stuff, and that uses SANITIZER_COMMON_CFLAGS.

cryptoad added a subscriber: hctim.Jul 28 2020, 3:34 PM
cryptoad added inline comments.
compiler-rt/lib/scudo/standalone/CMakeLists.txt
8

cc: @hctim
Didn't we have some error in the past with mismatched GWP-ASan/Scudo flags?

hctim added inline comments.Jul 28 2020, 3:51 PM
compiler-rt/lib/scudo/standalone/CMakeLists.txt
8

I vaguely remember something like this - but I believe it was prior to the standalone version of scudo.

We should probably remove ${SANITIZER_COMMON_CFLAGS} from GWP-ASan now that there's an integration into scudo standalone.

aeubanks updated this revision to Diff 281410.Jul 28 2020, 4:28 PM

Manually add -fno-lto to Scudo flags instead
Add FIXME for sharing flags with GWPAsan

aeubanks retitled this revision from [Scudo][CMake] Initialize SCUDO_CFLAGS with SANITIZER_COMMON_CFLAGS to [Scudo][CMake] Add -fno-lto to Scudo libraries.Jul 28 2020, 4:29 PM
aeubanks edited the summary of this revision. (Show Details)
aeubanks added a reviewer: hctim.
Herald added a subscriber: inglorion. · View Herald TranscriptJul 28 2020, 4:29 PM
cryptoad added inline comments.Jul 28 2020, 4:30 PM
compiler-rt/lib/scudo/standalone/CMakeLists.txt
12

Don't you want to add NO_LTO_FLAGS to SCUDO_FLAGS?

Harbormaster completed remote builds in B66101: Diff 281372.Jul 28 2020, 4:35 PM
aeubanks updated this revision to Diff 281424.Jul 28 2020, 5:14 PM

Actually do the right thing

aeubanks updated this revision to Diff 281425.Jul 28 2020, 5:15 PM

Remove extra line

aeubanks added inline comments.Jul 28 2020, 5:17 PM
compiler-rt/lib/scudo/standalone/CMakeLists.txt
12

Sorry, I'm pretty sure I tested it and thought I had fixed it, but I guess not.
I think check-scudo doesn't build TScudoCUnitTest-i386-Test.

Also I realized the lines below do what I want better so I made it similar to those lines.

Harbormaster completed remote builds in B66125: Diff 281410.Jul 28 2020, 7:05 PM
Harbormaster completed remote builds in B66133: Diff 281424.Jul 28 2020, 8:06 PM
Harbormaster completed remote builds in B66134: Diff 281425.Jul 28 2020, 8:17 PM
cryptoad accepted this revision.Jul 29 2020, 10:21 AM
This revision is now accepted and ready to land.Jul 29 2020, 10:21 AM
Closed by commit rGe7196bdf8125: [Scudo][CMake] Add -fno-lto to Scudo libraries (authored by aeubanks). · Explain WhyJul 29 2020, 10:30 AM
This revision was automatically updated to reflect the committed changes.
aeubanks added a commit: rGe7196bdf8125: [Scudo][CMake] Add -fno-lto to Scudo libraries.

Revision Contents

PathSize
compiler-rt/
lib/
scudo/
standalone/
3 lines

Diff 281658

compiler-rt/lib/scudo/standalone/CMakeLists.txt

add_compiler_rt_component(scudo_standalone) add_compiler_rt_component(scudo_standalone)
if (COMPILER_RT_HAS_GWP_ASAN) if (COMPILER_RT_HAS_GWP_ASAN)
add_dependencies(scudo_standalone gwp_asan) add_dependencies(scudo_standalone gwp_asan)
endif() endif()
include_directories(../.. include) include_directories(../.. include)
set(SCUDO_CFLAGS) set(SCUDO_CFLAGS)
cryptoadUnsubmitted
Not Done
ReplyInline Actions

So there are various things we don't want from SANITIZER_COMMON_CFLAGS, like -fno-builtin etc.
The other sanitizer are not meant to be performance critical while Scudo is.
Just using SANITIZER_COMMON_CFLAGS as is wouldn't work for us.

cryptoad: So there are various things we don't want from SANITIZER_COMMON_CFLAGS, like -fno-builtin etc.
aeubanksAuthorUnsubmitted
Done
ReplyInline Actions

Can you suggest any alternatives?
I'm not familiar with Scudo, but it seems like it's pulling in GWPAsan stuff, and that uses SANITIZER_COMMON_CFLAGS.

aeubanks: Can you suggest any alternatives? I'm not familiar with Scudo, but it seems like it's pulling…
cryptoadUnsubmitted
Not Done
ReplyInline Actions

cc: @hctim
Didn't we have some error in the past with mismatched GWP-ASan/Scudo flags?

cryptoad: cc: @hctim Didn't we have some error in the past with mismatched GWP-ASan/Scudo flags?
hctimUnsubmitted
Not Done
ReplyInline Actions

I vaguely remember something like this - but I believe it was prior to the standalone version of scudo.

We should probably remove ${SANITIZER_COMMON_CFLAGS} from GWP-ASan now that there's an integration into scudo standalone.

hctim: I vaguely remember something like this - but I believe it was prior to the standalone version…
list(APPEND SCUDO_CFLAGS list(APPEND SCUDO_CFLAGS
-Werror=conversion -Werror=conversion
-Wall -Wall
cryptoadUnsubmitted
Not Done
ReplyInline Actions

Don't you want to add NO_LTO_FLAGS to SCUDO_FLAGS?

cryptoad: Don't you want to add NO_LTO_FLAGS to SCUDO_FLAGS?
aeubanksAuthorUnsubmitted
Done
ReplyInline Actions

Sorry, I'm pretty sure I tested it and thought I had fixed it, but I guess not.
I think check-scudo doesn't build TScudoCUnitTest-i386-Test.

Also I realized the lines below do what I want better so I made it similar to those lines.

aeubanks: Sorry, I'm pretty sure I tested it and thought I had fixed it, but I guess not. I think check…
-nostdinc++) -nostdinc++)
# Remove -stdlib= which is unused when passing -nostdinc++. # Remove -stdlib= which is unused when passing -nostdinc++.
string(REGEX REPLACE "-stdlib=[a-zA-Z+]*" "" CMAKE_CXX_FLAGS ${CMAKE_CXX_FLAGS}) string(REGEX REPLACE "-stdlib=[a-zA-Z+]*" "" CMAKE_CXX_FLAGS ${CMAKE_CXX_FLAGS})
append_list_if(COMPILER_RT_HAS_FFREESTANDING_FLAG -ffreestanding SCUDO_CFLAGS) append_list_if(COMPILER_RT_HAS_FFREESTANDING_FLAG -ffreestanding SCUDO_CFLAGS)
append_list_if(COMPILER_RT_HAS_FVISIBILITY_HIDDEN_FLAG -fvisibility=hidden SCUDO_CFLAGS) append_list_if(COMPILER_RT_HAS_FVISIBILITY_HIDDEN_FLAG -fvisibility=hidden SCUDO_CFLAGS)
# FIXME: find cleaner way to agree with GWPAsan flags
append_list_if(COMPILER_RT_HAS_FNO_LTO_FLAG -fno-lto SCUDO_CFLAGS)
if (COMPILER_RT_HAS_GWP_ASAN) if (COMPILER_RT_HAS_GWP_ASAN)
append_list_if(COMPILER_RT_HAS_OMIT_FRAME_POINTER_FLAG -fno-omit-frame-pointer append_list_if(COMPILER_RT_HAS_OMIT_FRAME_POINTER_FLAG -fno-omit-frame-pointer
SCUDO_CFLAGS) SCUDO_CFLAGS)
append_list_if(COMPILER_RT_HAS_OMIT_FRAME_POINTER_FLAG append_list_if(COMPILER_RT_HAS_OMIT_FRAME_POINTER_FLAG
-mno-omit-leaf-frame-pointer SCUDO_CFLAGS) -mno-omit-leaf-frame-pointer SCUDO_CFLAGS)
endif() # COMPILER_RT_HAS_GWP_ASAN endif() # COMPILER_RT_HAS_GWP_ASAN
if(COMPILER_RT_DEBUG) if(COMPILER_RT_DEBUG)
▲ Show 20 Lines • Show All 133 Lines • Show Last 20 Lines