This is an archive of the discontinued LLVM Phabricator instance.

[scudo] Add SCUDO_ENABLE_HOOKS to enable hooks at compilation time
ClosedPublic

Authored by Chia-hungDuan on Aug 24 2023, 3:08 PM.

Details

Summary

Accessing the PLT entries of hooks can lead a certain amount of
performance overhead. This is observed on certain tasks which will do a
bunch of malloc/free and their throughputs are impacted by the null
check of hooks.

Also add SCUDO_ENABLE_HOOKS_TESTS to select if we want to run the hook
tests. On some platforms they may have different ways to run the
wrappers tests (end-to-end tests) and test the hooks along with the
wrappers tests may not be feasible. Provide an option to turn it ON/OFF.

By default, we only verify the hook behavior in the scudo standalone
tests if SCUDO_ENABLE_HOOKS is defined or COMPILER_RT_DEBUG is true.

Diff Detail

Event Timeline

Chia-hungDuan created this revision.Aug 24 2023, 3:08 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 24 2023, 3:08 PM
Chia-hungDuan requested review of this revision.Aug 24 2023, 3:08 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 24 2023, 3:08 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
cferris requested changes to this revision.Aug 25 2023, 3:19 PM
cferris added inline comments.
compiler-rt/lib/scudo/standalone/tests/CMakeLists.txt
22–25

Would it be better to make this a 0/1 define? Then you could just do a:

if (SCUDO_HOOKS_ENABLED) {

.
.
.

}

I think that would remove a lot of the macro definitions. You could even add it to the report functions so that none of the calls have to be removed, the calls themselves just don't do anything.

This revision now requires changes to proceed.Aug 25 2023, 3:19 PM
  1. Address review comment
  2. Add SCUDO_ENABLE_HOOKS_TESTS to bypass the hooks tests

By default, Fuchsia doesn't support running hooks tests with wrapper tests

Chia-hungDuan marked an inline comment as done.Aug 25 2023, 7:15 PM
Chia-hungDuan edited the summary of this revision. (Show Details)Aug 25 2023, 7:17 PM

Thanks for adding the possibility to disable the hooks tests

compiler-rt/lib/scudo/standalone/platform.h
77

I can add SCUDO_ENABLE_HOOKS=1 to our build files instead, to avoid introducing special cases here.

fabio-d accepted this revision.Aug 28 2023, 10:31 AM
cferris requested changes to this revision.Aug 28 2023, 12:34 PM
cferris added inline comments.
compiler-rt/lib/scudo/standalone/tests/wrappers_c_test.cpp
67

I do prefer this method, but the rest of the code just uses #if XXX.

It's up to you since this is a small nit.

83

Would it be better to use GTEST_SKIP() << "Hooks ..."?

I think if you do this, you can remove the if checks in all of the tests.

This revision now requires changes to proceed.Aug 28 2023, 12:34 PM
Chia-hungDuan marked an inline comment as done.Aug 28 2023, 12:55 PM
Chia-hungDuan added inline comments.
compiler-rt/lib/scudo/standalone/tests/wrappers_c_test.cpp
67

Agree. Let me change the style to (XXX == 1) in another CL

83

The hooks tests are embedded in end-to-end tests. Using GTEST_SKIP() will skip all the tests. Or am I misunderstanding it?

Chia-hungDuan marked an inline comment as done.Aug 28 2023, 12:55 PM

Remove SCUDO_ENABLE_HOOKS=1 on Fuchsia by default

cferris added inline comments.Aug 28 2023, 2:03 PM
compiler-rt/lib/scudo/standalone/tests/wrappers_c_test.cpp
83

Oops, I somehow thought all of the tests were hooks. You are correct that would cause all tests to be skipped.

Should this be some kind of error? This seems to indicate a configuration error?

Also, should you test the opposite (!SCUDO_ENABLE_HOOKS && SCUDO_ENABLE_HOOKS_TESTS)?

Chia-hungDuan marked an inline comment as done.Aug 28 2023, 2:24 PM
Chia-hungDuan added inline comments.
compiler-rt/lib/scudo/standalone/tests/wrappers_c_test.cpp
83

For (SCUDO_ENABLE_HOOKS && !SCUDO_ENABLE_HOOKS_TESTS), it's supposed to allow the platforms don't support this kind of hooks test still be able to run the remaining end-to-end stuff

For (!SCUDO_ENABLE_HOOKS && SCUDO_ENABLE_HOOKS_TESTS), this will issue an error at the beginning of these two end-to-end test files

cferris accepted this revision.Aug 28 2023, 2:30 PM

LGTM

This revision is now accepted and ready to land.Aug 28 2023, 2:30 PM
This revision was automatically updated to reflect the committed changes.
Chia-hungDuan marked an inline comment as done.