[scudo] Small cleanup of memory tagging code part 2.

[cferris1000]

Make the systemSupportsMemoryTagging() function return even on system that don't support memory tagging. This avoids the need to always check if memory tagging is supported before calling the function.

Modify iterateOverChunks() to call useMemoryTagging<>(Options) to determine if mte is supported. This already uses the cached check of systemSupportsMemoryTagging() rather than directly calling that function.

Updated the code that calls systemSupportsMemoryTagging().

[cferris1000]

This is a reland without the added caching, but using the caching that already exists.

[llvmbot]

@llvm/pr-subscribers-compiler-rt-sanitizer

Author: Christopher Ferris (cferris1000)

Changes

Make the systemSupportsMemoryTagging() function return even on system that don't support memory tagging. This avoids the need to always check if memory tagging is supported before calling the function.

Modify iterateOverChunks() to call useMemoryTagging<>(Options) to determine if mte is supported. This already uses the cached check of systemSupportsMemoryTagging() rather than directly calling that function.

Updated the code that calls systemSupportsMemoryTagging().

---

Full diff: https://github.com/llvm/llvm-project/pull/168807.diff

4 Files Affected:

• (modified) compiler-rt/lib/scudo/standalone/combined.h (+4-6)
• (modified) compiler-rt/lib/scudo/standalone/memtag.h (+2-2)
• (modified) compiler-rt/lib/scudo/standalone/tests/memtag_test.cpp (-1)
• (modified) compiler-rt/lib/scudo/standalone/tests/secondary_test.cpp (+1-3)

diff --git a/compiler-rt/lib/scudo/standalone/combined.h b/compiler-rt/lib/scudo/standalone/combined.h
index ffe9554203241..5108f02f2dcbb 100644
--- a/compiler-rt/lib/scudo/standalone/combined.h
+++ b/compiler-rt/lib/scudo/standalone/combined.h
@@ -171,8 +171,7 @@ class Allocator {
       Primary.Options.set(OptionBit::DeallocTypeMismatch);
     if (getFlags()->delete_size_mismatch)
       Primary.Options.set(OptionBit::DeleteSizeMismatch);
-    if (allocatorSupportsMemoryTagging<AllocatorConfig>() &&
-        systemSupportsMemoryTagging())
+    if (systemSupportsMemoryTagging())
       Primary.Options.set(OptionBit::UseMemoryTagging);
 
     QuarantineMaxChunkSize =
@@ -689,16 +688,15 @@ class Allocator {
       Base = untagPointer(Base);
     const uptr From = Base;
     const uptr To = Base + Size;
-    bool MayHaveTaggedPrimary =
-        allocatorSupportsMemoryTagging<AllocatorConfig>() &&
-        systemSupportsMemoryTagging();
+    const Options Options = Primary.Options.load();
+    bool MayHaveTaggedPrimary = useMemoryTagging<AllocatorConfig>(Options);
     auto Lambda = [this, From, To, MayHaveTaggedPrimary, Callback,
                    Arg](uptr Block) {
       if (Block < From || Block >= To)
         return;
       uptr Chunk;
       Chunk::UnpackedHeader Header;
-      if (MayHaveTaggedPrimary) {
+      if (UNLIKELY(MayHaveTaggedPrimary)) {
         // A chunk header can either have a zero tag (tagged primary) or the
         // header tag (secondary, or untagged primary). We don't know which so
         // try both.
diff --git a/compiler-rt/lib/scudo/standalone/memtag.h b/compiler-rt/lib/scudo/standalone/memtag.h
index 83ebe676433eb..a9d04e554767c 100644
--- a/compiler-rt/lib/scudo/standalone/memtag.h
+++ b/compiler-rt/lib/scudo/standalone/memtag.h
@@ -261,8 +261,8 @@ inline uptr loadTag(uptr Ptr) {
 
 #else
 
-inline NORETURN bool systemSupportsMemoryTagging() {
-  UNREACHABLE("memory tagging not supported");
+inline bool systemSupportsMemoryTagging() {
+  return false;
 }
 
 inline NORETURN bool systemDetectsMemoryTagFaultsTestOnly() {
diff --git a/compiler-rt/lib/scudo/standalone/tests/memtag_test.cpp b/compiler-rt/lib/scudo/standalone/tests/memtag_test.cpp
index 09093e11452dd..d0d93316f212e 100644
--- a/compiler-rt/lib/scudo/standalone/tests/memtag_test.cpp
+++ b/compiler-rt/lib/scudo/standalone/tests/memtag_test.cpp
@@ -28,7 +28,6 @@ TEST(MemtagBasicDeathTest, Unsupported) {
   EXPECT_DEATH(untagPointer((uptr)0), "not supported");
   EXPECT_DEATH(extractTag((uptr)0), "not supported");
 
-  EXPECT_DEATH(systemSupportsMemoryTagging(), "not supported");
   EXPECT_DEATH(systemDetectsMemoryTagFaultsTestOnly(), "not supported");
   EXPECT_DEATH(enableSystemMemoryTaggingTestOnly(), "not supported");
 
diff --git a/compiler-rt/lib/scudo/standalone/tests/secondary_test.cpp b/compiler-rt/lib/scudo/standalone/tests/secondary_test.cpp
index 855a3e6e6109f..8741c8299b57c 100644
--- a/compiler-rt/lib/scudo/standalone/tests/secondary_test.cpp
+++ b/compiler-rt/lib/scudo/standalone/tests/secondary_test.cpp
@@ -27,9 +27,7 @@
 const scudo::uptr PageSize = scudo::getPageSizeCached();
 
 template <typename Config> static scudo::Options getOptionsForConfig() {
-  if (!Config::getMaySupportMemoryTagging() ||
-      !scudo::archSupportsMemoryTagging() ||
-      !scudo::systemSupportsMemoryTagging())
+  if (!scudo::systemSupportsMemoryTagging())
     return {};
   scudo::AtomicOptions AO;
   AO.set(scudo::OptionBit::UseMemoryTagging);

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[github-actions]

🐧 Linux x64 Test Results

• 5820 tests passed
• 1323 tests skipped

[vitalybuka]

could useMemoryTagging be a constexpr ?

[cferris1000]

It can and it does now.

[pcc]

It seems a bit confusing for this to be constexpr because it reads a runtime value (options bit).

[cferris1000]

That's true. I don't really have a very strong opinion on this, so I'm happy to remove it, or leave it.

It sounds like like it could be confusing, so I'll remove it unless there is a strong reason to re-add it.

[vitalybuka]

constexpr?

[cferris1000]

Done.