This is an archive of the discontinued LLVM Phabricator instance.

Reland D146570 "[scudo] Switch to use MemMap in tests"
ClosedPublic

Authored by Chia-hungDuan on Apr 5 2023, 5:09 PM.

Diff Detail

Event Timeline

Chia-hungDuan created this revision.Apr 5 2023, 5:09 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 5 2023, 5:09 PM
Chia-hungDuan requested review of this revision.Apr 5 2023, 5:09 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 5 2023, 5:09 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript

Fix variable shadowing

Verified on MTE device

cferris requested changes to this revision.Apr 6 2023, 4:13 PM

Mostly small nits.

compiler-rt/lib/scudo/standalone/tests/common_test.cpp
40

You lost the ASSERT_NE(nullptr, P) line. Even though getBase should not even return 0, it's probably worth keeping it in just in case.

61

Same as above, missing P is not nullptr check.

compiler-rt/lib/scudo/standalone/tests/memtag_test.cpp
51

Should there be a check that Addr is non-zero?

66

Technically, you don't do this any other place because MemMap is already initialized properly. Doesn't matter if you leave this or not though.

This revision now requires changes to proceed.Apr 6 2023, 4:13 PM
Chia-hungDuan marked 4 inline comments as done.

Address review comment

compiler-rt/lib/scudo/standalone/tests/common_test.cpp
40

Good catch!

compiler-rt/lib/scudo/standalone/tests/memtag_test.cpp
66

Right, just added because of some weird bug only happens on Android (Which I need to add {} for MemMap). Just in case it complains again and in order to make the style consistent.

cferris accepted this revision.Apr 6 2023, 4:47 PM

LGTM.

This revision is now accepted and ready to land.Apr 6 2023, 4:47 PM