This is an archive of the discontinued LLVM Phabricator instance.

[scudo] Add memtag_test
ClosedPublic

Authored by vitalybuka on May 24 2021, 11:12 PM.

Diff Detail

Event Timeline

vitalybuka created this revision.May 24 2021, 11:12 PM
vitalybuka requested review of this revision.May 24 2021, 11:12 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 24 2021, 11:12 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
hctim added inline comments.May 25 2021, 10:08 AM
compiler-rt/lib/scudo/standalone/tests/memtag_test.cpp
50

Should we restore to the previous MTE mode (SYNC/ASYNC) instead of just turning it to SYNC?

100

It wasn't obvious to me on first glance why *P was a deterministic fault. Might be worth adding a comment to selectRandomTag that tag zero is excluded from when the prctl was issued.

105

Wouldn't we want to die if the prctl failed?

152

Untag after test?

173

Implicit invariant that 0x100 >= 4 * archMemoryTagGranuleSize(). Maybe make it explicit?

uptr WorkingRegionSize = 4 * archMemoryTagGranuleSize();
for (uptr Size = 0; Size < WorkingRegionSize; ++Size, P += WorkingRegionSize) {
pcc added inline comments.May 25 2021, 10:19 AM
compiler-rt/lib/scudo/standalone/memtag.h
160

This isn't necessary, the caller should do that themselves.

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

We normally handle this by skipping the tests if the system isn't detecting tag faults. Can we set the tagging mode in the main function? That way, both these tests and the combined allocator tests should run.

103

We have ScopedDisableMemoryTagChecks for this purpose.

vitalybuka marked 6 inline comments as done.

updates

vitalybuka marked an inline comment as done.May 28 2021, 3:20 AM
vitalybuka added inline comments.
compiler-rt/lib/scudo/standalone/memtag.h
160

DCHECK?

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

going with main() function, so nothing to restore

100

And removing 0 from prctl does not look nice.
I don't need selectRandomTag here anyway.

103

That one uses TSO, here I tests prctl.
I may use ScopedDisableMemoryTagChecks in the next test,
however ScopedDisableMemoryTagChecks is unused in the code, so maybe we should remove it at all?

152

::TearDown munpap entire buffer

hctim accepted this revision.Jun 3 2021, 2:33 PM

LGTM, but WANT_LGTM=pcc.

pcc requested changes to this revision.Jun 3 2021, 8:14 PM
pcc added inline comments.
compiler-rt/lib/scudo/standalone/tests/memtag_test.cpp
103

The {disable,enable}MemoryTagChecksTestOnly functions use TCO, same as ScopedDisableMemoryTagChecks. There's a use of ScopedDisableMemoryTagChecks in combined.h and we could probably start using ScopedDisableMemoryTagChecks in combined_test.cpp which would let us remove {disable,enable}MemoryTagChecksTestOnly.

This revision now requires changes to proceed.Jun 3 2021, 8:14 PM

Removed MemoryTagChecksTestOnly test

pcc accepted this revision.Jun 4 2021, 11:32 AM

LGTM

This revision is now accepted and ready to land.Jun 4 2021, 11:32 AM
This revision was landed with ongoing or failed builds.Jun 4 2021, 12:38 PM
This revision was automatically updated to reflect the committed changes.

This is causing failures https://lab.llvm.org/buildbot/#/builders/57/builds/7002/steps/6/logs/stdio:

/home/buildbots/docker-RHEL-buildbot/SetupBot/worker_env/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/llvm/utils/unittest/googletest/include/gtest/gtest.h:1527:11: error: comparison of integers of different signs: 'const int' and 'const unsigned long' [-Werror,-Wsign-compare]
  if (lhs == rhs) {
      ~~~ ^  ~~~
/home/buildbots/docker-RHEL-buildbot/SetupBot/worker_env/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/llvm/utils/unittest/googletest/include/gtest/gtest.h:1554:12: note: in instantiation of function template specialization 'testing::internal::CmpHelperEQ<int, unsigned long>' requested here
    return CmpHelperEQ(lhs_expression, rhs_expression, lhs, rhs);
           ^
/home/buildbots/docker-RHEL-buildbot/SetupBot/worker_env/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/compiler-rt/lib/scudo/standalone/tests/memtag_test.cpp:77:3: note: in instantiation of function template specialization 'testing::internal::EqHelper::Compare<int, unsigned long, nullptr>' requested here
  EXPECT_EQ(0xffff, Tags);
  ^
/home/buildbots/docker-RHEL-buildbot/SetupBot/worker_env/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/llvm/utils/unittest/googletest/include/gtest/gtest.h:2028:54: note: expanded from macro 'EXPECT_EQ'
  EXPECT_PRED_FORMAT2(::testing::internal::EqHelper::Compare, val1, val2)
                                                     ^
1 error generated.
pcc added inline comments.Sep 30 2021, 3:58 PM
compiler-rt/lib/scudo/standalone/tests/memtag_test.cpp
14

Why do we need this? The tests should all fail to detect MTE on non-Linux platforms, shouldn't they?

vitalybuka added inline comments.Oct 1 2021, 12:11 AM
compiler-rt/lib/scudo/standalone/tests/memtag_test.cpp
14

it uses GTEST stuff which is likely not available on zxtest of Fucsia