[SanitizerBinaryMetadata] Optimize used space for features and UAR stack args

Optimize the encoding of "covered" metadata by:

 1. Reducing feature mask from 4 bytes to 1 byte (needs increase once we
    reach more than 8 features).

 2. Only emitting UAR stack args size if it is non-zero, saving 4 bytes
    in the common case.

One caveat is that the emitted metadata for function PC (offset), size,
and UAR size (if enabled) are no longer aligned to 4 bytes.

SanitizerBinaryMetadata version base is increased to 2, since the change
is backwards incompatible.

Reviewed By: dvyukov

Differential Revision: https://reviews.llvm.org/D143482

diff --git a/clang/test/CodeGen/sanitize-metadata.c b/clang/test/CodeGen/sanitize-metadata.c
index 3ab0fbc..881d148 100644 (file)
--- a/clang/test/CodeGen/sanitize-metadata.c
+++ b/clang/test/CodeGen/sanitize-metadata.c
@@ -22,15 +22,15 @@ int atomics() {
   return y;
 }
 // ATOMICS-LABEL: __sanitizer_metadata_atomics.module_ctor
-// ATOMICS: call void @__sanitizer_metadata_atomics_add(i32 1, ptr @__start_sanmd_atomics, ptr @__stop_sanmd_atomics)
+// ATOMICS: call void @__sanitizer_metadata_atomics_add(i32 2, ptr @__start_sanmd_atomics, ptr @__stop_sanmd_atomics)
 // ATOMICS-LABEL: __sanitizer_metadata_atomics.module_dtor
-// ATOMICS: call void @__sanitizer_metadata_atomics_del(i32 1, ptr @__start_sanmd_atomics, ptr @__stop_sanmd_atomics)
+// ATOMICS: call void @__sanitizer_metadata_atomics_del(i32 2, ptr @__start_sanmd_atomics, ptr @__stop_sanmd_atomics)
 
 // CHECK-LABEL: __sanitizer_metadata_covered.module_ctor
-// CHECK: call void @__sanitizer_metadata_covered_add(i32 1, ptr @__start_sanmd_covered, ptr @__stop_sanmd_covered)
+// CHECK: call void @__sanitizer_metadata_covered_add(i32 2, ptr @__start_sanmd_covered, ptr @__stop_sanmd_covered)
 // CHECK-LABEL: __sanitizer_metadata_covered.module_dtor
-// CHECK: call void @__sanitizer_metadata_covered_del(i32 1, ptr @__start_sanmd_covered, ptr @__stop_sanmd_covered)
+// CHECK: call void @__sanitizer_metadata_covered_del(i32 2, ptr @__start_sanmd_covered, ptr @__stop_sanmd_covered)
 
 // ATOMICS: ![[ATOMICS_COVERED]] = !{!"sanmd_covered", ![[ATOMICS_COVERED_AUX:[0-9]+]]}
-// ATOMICS: ![[ATOMICS_COVERED_AUX]] = !{i32 1}
+// ATOMICS: ![[ATOMICS_COVERED_AUX]] = !{i8 1}
 // ATOMICS: ![[ATOMIC_OP]] = !{!"sanmd_atomics"}

diff --git a/compiler-rt/test/metadata/common.h b/compiler-rt/test/metadata/common.h
index b121e87..9a7774f 100644 (file)
--- a/compiler-rt/test/metadata/common.h
+++ b/compiler-rt/test/metadata/common.h
@@ -1,43 +1,54 @@
 #include <assert.h>
 #include <stdint.h>
 #include <stdio.h>
+#include <string.h>
 
 int main() { printf("main\n"); }
 
-typedef unsigned long uptr;
-
+namespace {
 #define FN(X)                                                                  \
-  if (pc == reinterpret_cast<uptr>(X))                                         \
+  if (pc == reinterpret_cast<uintptr_t>(X))                                    \
   return #X
 
-const char *symbolize(uptr pc) {
+const char *symbolize(uintptr_t pc) {
   FUNCTIONS;
   return nullptr;
 }
 
 template <typename T> T consume(const char *&pos, const char *end) {
-  T v = *reinterpret_cast<const T *>(pos);
+  T v;
+  // We need to memcpy from pos, because it's not guaranteed that every entry
+  // has the required alignment of T.
+  memcpy(&v, pos, sizeof(T));
   pos += sizeof(T);
   assert(pos <= end);
   return v;
 }
 
+constexpr uint32_t kSanitizerBinaryMetadataUARHasSize = 1 << 2;
+
 uint32_t meta_version;
 const char *meta_start;
 const char *meta_end;
+const char *atomics_start;
+const char *atomics_end;
+}; // namespace
 
 extern "C" {
 void __sanitizer_metadata_covered_add(uint32_t version, const char *start,
                                       const char *end) {
-  printf("metadata add version %u\n", version);
+  const uint32_t version_base = version & 0xffff;
+  const bool offset_ptr_sized = version & (1 << 16);
+  assert(version_base == 2);
+  printf("metadata add version %u\n", version_base);
   for (const char *pos = start; pos < end;) {
-    const uptr base = reinterpret_cast<uptr>(pos);
-    const long offset = (version & (1 << 16)) ? consume<long>(pos, end)
-                                              : consume<int>(pos, end);
-    const uint32_t size = consume<uint32_t>(pos, end);
-    const uint32_t features = consume<uint32_t>(pos, end);
+    const auto base = reinterpret_cast<uintptr_t>(pos);
+    const intptr_t offset = offset_ptr_sized ? consume<intptr_t>(pos, end)
+                                             : consume<int32_t>(pos, end);
+    [[maybe_unused]] const uint32_t size = consume<uint32_t>(pos, end);
+    const uint32_t features = consume<uint8_t>(pos, end);
     uint32_t stack_args = 0;
-    if (features & (1 << 1))
+    if (features & kSanitizerBinaryMetadataUARHasSize)
       stack_args = consume<uint32_t>(pos, end);
     if (const char *name = symbolize(base + offset))
       printf("%s: features=%x stack_args=%u\n", name, features, stack_args);
@@ -54,9 +65,6 @@ void __sanitizer_metadata_covered_del(uint32_t version, const char *start,
   assert(end == meta_end);
 }
 
-const char *atomics_start;
-const char *atomics_end;
-
 void __sanitizer_metadata_atomics_add(uint32_t version, const char *start,
                                       const char *end) {
   assert(version == meta_version);

diff --git a/compiler-rt/test/metadata/covered.cpp b/compiler-rt/test/metadata/covered.cpp
index 609a8c0..92abe4f 100644 (file)
--- a/compiler-rt/test/metadata/covered.cpp
+++ b/compiler-rt/test/metadata/covered.cpp
@@ -6,11 +6,12 @@
 // RUN: %clangxx %s -o %t -fexperimental-sanitize-metadata=covered,uar && %t | FileCheck -check-prefix=CHECK-CU %s
 // RUN: %clangxx %s -o %t -fexperimental-sanitize-metadata=atomics,uar && %t | FileCheck -check-prefix=CHECK-AU %s
 // RUN: %clangxx %s -o %t -fexperimental-sanitize-metadata=covered,atomics,uar && %t | FileCheck -check-prefix=CHECK-CAU %s
+// RUN: %clangxx %s -o %t -mcmodel=large -fexperimental-sanitize-metadata=covered,atomics,uar && %t | FileCheck -check-prefix=CHECK-CAU %s
 
 const int const_global = 42;
 
 __attribute__((noinline, not_tail_called)) void escape(const volatile void *p) {
-  static const volatile void *sink;
+  [[maybe_unused]] static const volatile void *sink;
   sink = p;
 }
 
@@ -69,6 +70,20 @@ int with_atomic_escape(int *p) {
   return __atomic_load_n(p, __ATOMIC_RELAXED);
 }
 
+// CHECK-C:   with_atomic_escape_lots_of_args: features=0
+// CHECK-A:   with_atomic_escape_lots_of_args: features=1
+// CHECK-U:   with_atomic_escape_lots_of_args: features=6
+// CHECK-CA:  with_atomic_escape_lots_of_args: features=1
+// CHECK-CU:  with_atomic_escape_lots_of_args: features=6
+// CHECK-AU:  with_atomic_escape_lots_of_args: features=7
+// CHECK-CAU: with_atomic_escape_lots_of_args: features=7
+long with_atomic_escape_lots_of_args(int *p, long a0, long a1, long a2, long a3,
+                                     long a4, long a5, long a6) {
+  escape(&p);
+  return a0 + a1 + a2 + a3 + a4 + a5 + a6 +
+         __atomic_load_n(p, __ATOMIC_RELAXED);
+}
+
 // CHECK-C:     ellipsis: features=0
 // CHECK-A:     ellipsis: features=1
 // CHECK-U-NOT: ellipsis:
@@ -78,7 +93,7 @@ int with_atomic_escape(int *p) {
 // CHECK-CAU:   ellipsis: features=1
 void ellipsis(int *p, ...) {
   escape(&p);
-  volatile int x;
+  [[maybe_unused]] volatile int x;
   x = 0;
 }
 
@@ -100,6 +115,7 @@ int ellipsis_with_atomic(int *p, ...) {
   FN(with_const_global);                                                       \
   FN(with_atomic);                                                             \
   FN(with_atomic_escape);                                                      \
+  FN(with_atomic_escape_lots_of_args);                                         \
   FN(ellipsis);                                                                \
   FN(ellipsis_with_atomic);                                                    \
   /**/

diff --git a/compiler-rt/test/metadata/lit.site.cfg.py.in b/compiler-rt/test/metadata/lit.site.cfg.py.in
index 7b95b4b..8993b31 100644 (file)
--- a/compiler-rt/test/metadata/lit.site.cfg.py.in
+++ b/compiler-rt/test/metadata/lit.site.cfg.py.in
@@ -11,4 +11,5 @@ lit_config.load_config(config, "@COMPILER_RT_BINARY_DIR@/test/lit.common.configu
 # Load tool-specific config that would do the real work.
 lit_config.load_config(config, "@METADATA_LIT_SOURCE_DIR@/lit.cfg.py")
 
-config.substitutions.append(("%clangxx ", " " + config.clang + " " + config.target_cflags + " "))
+clangxx = [config.clang] + config.cxx_mode_flags + ["-Wall", config.target_cflags]
+config.substitutions.append(("%clangxx ", " ".join([""] + clangxx + [""])))

diff --git a/compiler-rt/test/metadata/uar.cpp b/compiler-rt/test/metadata/uar.cpp
index ed7f7dd..cbafe46 100644 (file)
--- a/compiler-rt/test/metadata/uar.cpp
+++ b/compiler-rt/test/metadata/uar.cpp
@@ -1,10 +1,11 @@
 // RUN: %clangxx %s -O1 -o %t -fexperimental-sanitize-metadata=covered,uar && %t | FileCheck %s
-// RUN: %clangxx %s -O1 -o %t -fexperimental-sanitize-metadata=covered,uar -fsanitize=address,signed-integer-overflow && %t | FileCheck %s
+// RUN: %clangxx %s -O1 -o %t -fexperimental-sanitize-metadata=covered,uar -fsanitize=address,signed-integer-overflow,alignment && %t | FileCheck %s
+// RUN: %clangxx %s -O1 -o %t -mcmodel=large -fexperimental-sanitize-metadata=covered,uar -fsanitize=address,signed-integer-overflow,alignment && %t | FileCheck %s
 
-// CHECK: metadata add version 1
+// CHECK: metadata add version 2
 
 __attribute__((noinline, not_tail_called)) void escape(const volatile void *p) {
-  static const volatile void *sink;
+  [[maybe_unused]] static const volatile void *sink;
   sink = p;
 }
 
@@ -44,20 +45,20 @@ void no_stack_args(long a0, long a1, long a2, long a3, long a4, long a5) {
   escape(&x);
 }
 
-// CHECK: stack_args: features=2 stack_args=16
+// CHECK: stack_args: features=6 stack_args=16
 void stack_args(long a0, long a1, long a2, long a3, long a4, long a5, long a6) {
   int x;
   escape(&x);
 }
 
-// CHECK: more_stack_args: features=2 stack_args=32
+// CHECK: more_stack_args: features=6 stack_args=32
 void more_stack_args(long a0, long a1, long a2, long a3, long a4, long a5,
                      long a6, long a7, long a8) {
   int x;
   escape(&x);
 }
 
-// CHECK: struct_stack_args: features=2 stack_args=144
+// CHECK: struct_stack_args: features=6 stack_args=144
 struct large {
   char x[131];
 };

diff --git a/llvm/include/llvm/Transforms/Instrumentation/SanitizerBinaryMetadata.h b/llvm/include/llvm/Transforms/Instrumentation/SanitizerBinaryMetadata.h
index beb67d5..c41d5e3 100644 (file)
--- a/llvm/include/llvm/Transforms/Instrumentation/SanitizerBinaryMetadata.h
+++ b/llvm/include/llvm/Transforms/Instrumentation/SanitizerBinaryMetadata.h
@@ -28,11 +28,14 @@ struct SanitizerBinaryMetadataOptions {
 
 inline constexpr int kSanitizerBinaryMetadataAtomicsBit = 0;
 inline constexpr int kSanitizerBinaryMetadataUARBit = 1;
+inline constexpr int kSanitizerBinaryMetadataUARHasSizeBit = 2;
 
 inline constexpr uint32_t kSanitizerBinaryMetadataAtomics =
     1 << kSanitizerBinaryMetadataAtomicsBit;
 inline constexpr uint32_t kSanitizerBinaryMetadataUAR =
     1 << kSanitizerBinaryMetadataUARBit;
+inline constexpr uint32_t kSanitizerBinaryMetadataUARHasSize =
+    1 << kSanitizerBinaryMetadataUARHasSizeBit;
 
 inline constexpr char kSanitizerBinaryMetadataCoveredSection[] =
     "sanmd_covered";

diff --git a/llvm/lib/CodeGen/SanitizerBinaryMetadata.cpp b/llvm/lib/CodeGen/SanitizerBinaryMetadata.cpp
index dd70a2f..2107e00 100644 (file)
--- a/llvm/lib/CodeGen/SanitizerBinaryMetadata.cpp
+++ b/llvm/lib/CodeGen/SanitizerBinaryMetadata.cpp
@@ -57,7 +57,8 @@ bool MachineSanitizerBinaryMetadata::runOnMachineFunction(MachineFunction &MF) {
   auto &AuxMDs = *cast<MDTuple>(MD->getOperand(1));
   // Assume it currently only has features.
   assert(AuxMDs.getNumOperands() == 1);
-  auto *Features = cast<ConstantAsMetadata>(AuxMDs.getOperand(0))->getValue();
+  Constant *Features =
+      cast<ConstantAsMetadata>(AuxMDs.getOperand(0))->getValue();
   if (!Features->getUniqueInteger()[kSanitizerBinaryMetadataUARBit])
     return false;
   // Calculate size of stack args for the function.
@@ -69,12 +70,18 @@ bool MachineSanitizerBinaryMetadata::runOnMachineFunction(MachineFunction &MF) {
     Align = std::max(Align, MFI.getObjectAlign(i).value());
   }
   Size = (Size + Align - 1) & ~(Align - 1);
+  if (!Size)
+    return false;
+  // Non-zero size, update metadata.
   auto &F = MF.getFunction();
   IRBuilder<> IRB(F.getContext());
   MDBuilder MDB(F.getContext());
   // Keep the features and append size of stack args to the metadata.
-  F.setMetadata(LLVMContext::MD_pcsections,
-                MDB.createPCSections(
-                    {{Section.getString(), {Features, IRB.getInt32(Size)}}}));
+  APInt NewFeatures = Features->getUniqueInteger();
+  NewFeatures.setBit(kSanitizerBinaryMetadataUARHasSizeBit);
+  F.setMetadata(
+      LLVMContext::MD_pcsections,
+      MDB.createPCSections({{Section.getString(),
+                             {IRB.getInt(NewFeatures), IRB.getInt32(Size)}}}));
   return false;
 }

diff --git a/llvm/lib/Transforms/Instrumentation/SanitizerBinaryMetadata.cpp b/llvm/lib/Transforms/Instrumentation/SanitizerBinaryMetadata.cpp
index e990ea2..0af5bce 100644 (file)
--- a/llvm/lib/Transforms/Instrumentation/SanitizerBinaryMetadata.cpp
+++ b/llvm/lib/Transforms/Instrumentation/SanitizerBinaryMetadata.cpp
@@ -43,6 +43,7 @@
 
 #include <array>
 #include <cstdint>
+#include <limits>
 
 using namespace llvm;
 
@@ -52,7 +53,7 @@ namespace {
 
 //===--- Constants --------------------------------------------------------===//
 
-constexpr uint32_t kVersionBase = 1;                // occupies lower 16 bits
+constexpr uint32_t kVersionBase = 2;                // occupies lower 16 bits
 constexpr uint32_t kVersionPtrSizeRel = (1u << 16); // offsets are pointer-sized
 constexpr int kCtorDtorPriority = 2;
 
@@ -269,9 +270,10 @@ void SanitizerBinaryMetadata::runOn(Function &F, MetadataInfoSet &MIS) {
     const auto *MI = &MetadataInfo::Covered;
     MIS.insert(MI);
     const StringRef Section = getSectionName(MI->SectionSuffix);
-    // The feature mask will be placed after the size (32 bit) of the function,
-    // so in total one covered entry will use `sizeof(void*) + 4 + 4`.
-    Constant *CFM = IRB.getInt32(FeatureMask);
+    // The feature mask will be placed after the size of the function.
+    assert(FeatureMask <= std::numeric_limits<uint8_t>::max() &&
+           "Increase feature mask bytes and bump version");
+    Constant *CFM = IRB.getInt8(FeatureMask);
     F.setMetadata(LLVMContext::MD_pcsections,
                   MDB.createPCSections({{Section, {CFM}}}));
   }

diff --git a/llvm/test/Instrumentation/SanitizerBinaryMetadata/atomics.ll b/llvm/test/Instrumentation/SanitizerBinaryMetadata/atomics.ll
index 984950e..cef890f 100644 (file)
--- a/llvm/test/Instrumentation/SanitizerBinaryMetadata/atomics.ll
+++ b/llvm/test/Instrumentation/SanitizerBinaryMetadata/atomics.ll
@@ -2039,7 +2039,7 @@ entry:
 ; CHECK-DAG: entry:
 ; CHECK-NEXT:  br i1 icmp ne (ptr @__sanitizer_metadata_atomics_add, ptr null), label %callfunc, label %ret
 ; CHECK-DAG: callfunc:
-; CHECK-NEXT:  call void @__sanitizer_metadata_atomics_add(i32 1, ptr @__start_sanmd_atomics, ptr @__stop_sanmd_atomics)
+; CHECK-NEXT:  call void @__sanitizer_metadata_atomics_add(i32 2, ptr @__start_sanmd_atomics, ptr @__stop_sanmd_atomics)
 ; CHECK-NEXT:  br label %ret
 ; CHECK-DAG: ret:
 ; CHECK-NEXT:  ret void
@@ -2048,7 +2048,7 @@ entry:
 ; CHECK-DAG: entry:
 ; CHECK-NEXT:  br i1 icmp ne (ptr @__sanitizer_metadata_atomics_del, ptr null), label %callfunc, label %ret
 ; CHECK-DAG: callfunc:
-; CHECK-NEXT:  call void @__sanitizer_metadata_atomics_del(i32 1, ptr @__start_sanmd_atomics, ptr @__stop_sanmd_atomics)
+; CHECK-NEXT:  call void @__sanitizer_metadata_atomics_del(i32 2, ptr @__start_sanmd_atomics, ptr @__stop_sanmd_atomics)
 ; CHECK-NEXT:  br label %ret
 ; CHECK-DAG: ret:
 ; CHECK-NEXT:  ret void
@@ -2057,7 +2057,7 @@ entry:
 ; CHECK-DAG: entry:
 ; CHECK-NEXT:  br i1 icmp ne (ptr @__sanitizer_metadata_covered_add, ptr null), label %callfunc, label %ret
 ; CHECK-DAG: callfunc:
-; CHECK-NEXT:  call void @__sanitizer_metadata_covered_add(i32 1, ptr @__start_sanmd_covered, ptr @__stop_sanmd_covered)
+; CHECK-NEXT:  call void @__sanitizer_metadata_covered_add(i32 2, ptr @__start_sanmd_covered, ptr @__stop_sanmd_covered)
 ; CHECK-NEXT:  br label %ret
 ; CHECK-DAG: ret:
 ; CHECK-NEXT:  ret void
@@ -2066,11 +2066,11 @@ entry:
 ; CHECK-DAG: entry:
 ; CHECK-NEXT:  br i1 icmp ne (ptr @__sanitizer_metadata_covered_del, ptr null), label %callfunc, label %ret
 ; CHECK-DAG: callfunc:
-; CHECK-NEXT:  call void @__sanitizer_metadata_covered_del(i32 1, ptr @__start_sanmd_covered, ptr @__stop_sanmd_covered)
+; CHECK-NEXT:  call void @__sanitizer_metadata_covered_del(i32 2, ptr @__start_sanmd_covered, ptr @__stop_sanmd_covered)
 ; CHECK-NEXT:  br label %ret
 ; CHECK-DAG: ret:
 ; CHECK-NEXT:  ret void
 
 ; CHECK: !0 = !{!"sanmd_covered", !1}
-; CHECK: !1 = !{i32 1}
+; CHECK: !1 = !{i8 1}
 ; CHECK: !2 = !{!"sanmd_atomics"}