[clang] eecb22d - [SanitizerBinaryMetadata] Use weak __start_/__stop_ instead of dummy empty section

[Fangrui Song via cfe-commits]

Author: Fangrui Song
Date: 2022-12-04T15:06:34-08:00
New Revision: eecb22d8e1fe5ac6cc35ace63ae517c33ff40d85

URL: https://github.com/llvm/llvm-project/commit/eecb22d8e1fe5ac6cc35ace63ae517c33ff40d85
DIFF: https://github.com/llvm/llvm-project/commit/eecb22d8e1fe5ac6cc35ace63ae517c33ff40d85.diff

LOG: [SanitizerBinaryMetadata] Use weak __start_/__stop_ instead of dummy empty section

D130887 uses a dummy empty section `sanmd_covered` (with the SHF_GNU_RETAIN flag on
ELF) to prevent `undefined symbol: __start_sanmd_covered` if all `sanmd_covered`
are discarded by `ld --gc-sections` (in `-z start-stop-gc` mode).

The dummy `sanmd_covered` does not have the SHF_LINK_ORDER flag, so mixing it
with SHF_LINK_ORDER `sanmd_covered` causes an issue to GNU ld<2.36
(https://sourceware.org/bugzilla/show_bug.cgi?id=26256).

Similar to D98903 for SanitizerCoverage, let's make encapsulation symbols
undefined weak[1]. This additionally avoids size cost due to the dummy section and
symbol.

[1]: https://maskray.me/blog/2021-01-31-metadata-sections-comdat-and-shf-link-order

Reviewed By: melver

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

Added: 
    

Modified: 
    clang/test/CodeGen/sanitize-metadata.c
    llvm/lib/Transforms/Instrumentation/SanitizerBinaryMetadata.cpp
    llvm/test/Instrumentation/SanitizerBinaryMetadata/atomics.ll

Removed: 
    


################################################################################
diff  --git a/clang/test/CodeGen/sanitize-metadata.c b/clang/test/CodeGen/sanitize-metadata.c
index 58d47ff84f517..3ab0fbc06fe09 100644
--- a/clang/test/CodeGen/sanitize-metadata.c
+++ b/clang/test/CodeGen/sanitize-metadata.c
@@ -1,6 +1,11 @@
 // RUN: %clang_cc1 -O -fexperimental-sanitize-metadata=atomics -triple x86_64-gnu-linux -x c -S -emit-llvm %s -o - | FileCheck %s --check-prefixes=CHECK,ATOMICS
 // RUN: %clang_cc1 -O -fexperimental-sanitize-metadata=atomics -triple aarch64-gnu-linux -x c -S -emit-llvm %s -o - | FileCheck %s --check-prefixes=CHECK,ATOMICS
 
+// CHECK: @__start_sanmd_atomics = extern_weak hidden global ptr
+// CHECK: @__stop_sanmd_atomics = extern_weak hidden global ptr
+// CHECK: @__start_sanmd_covered = extern_weak hidden global ptr
+// CHECK: @__stop_sanmd_covered = extern_weak hidden global ptr
+
 int x, y;
 
 void empty() {

diff  --git a/llvm/lib/Transforms/Instrumentation/SanitizerBinaryMetadata.cpp b/llvm/lib/Transforms/Instrumentation/SanitizerBinaryMetadata.cpp
index 58c29d8b211e2..169e87625c254 100644
--- a/llvm/lib/Transforms/Instrumentation/SanitizerBinaryMetadata.cpp
+++ b/llvm/lib/Transforms/Instrumentation/SanitizerBinaryMetadata.cpp
@@ -147,10 +147,6 @@ class SanitizerBinaryMetadata {
   // Get start/end section marker pointer.
   GlobalVariable *getSectionMarker(const Twine &MarkerName, Type *Ty);
 
-  // Create a 0-sized object in a section, so that the section is not discarded
-  // if all inputs have been discarded.
-  void createZeroSizedObjectInSection(Type *Ty, StringRef SectionSuffix);
-
   // Returns the target-dependent section name.
   StringRef getSectionName(StringRef SectionSuffix);
 
@@ -213,7 +209,6 @@ bool SanitizerBinaryMetadata::run() {
     }
     appendToGlobalCtors(Mod, Ctor, kCtorDtorPriority, CtorData);
     appendToGlobalDtors(Mod, Dtor, kCtorDtorPriority, DtorData);
-    createZeroSizedObjectInSection(Int8PtrTy, MI->SectionSuffix);
   }
 
   return true;
@@ -285,27 +280,15 @@ bool SanitizerBinaryMetadata::runOn(Instruction &I, MetadataInfoSet &MIS,
 
 GlobalVariable *
 SanitizerBinaryMetadata::getSectionMarker(const Twine &MarkerName, Type *Ty) {
+  // Use ExternalWeak so that if all sections are discarded due to section
+  // garbage collection, the linker will not report undefined symbol errors.
   auto *Marker = new GlobalVariable(Mod, Ty, /*isConstant=*/false,
-                                    GlobalVariable::ExternalLinkage,
+                                    GlobalVariable::ExternalWeakLinkage,
                                     /*Initializer=*/nullptr, MarkerName);
   Marker->setVisibility(GlobalValue::HiddenVisibility);
   return Marker;
 }
 
-void SanitizerBinaryMetadata::createZeroSizedObjectInSection(
-    Type *Ty, StringRef SectionSuffix) {
-  auto *DummyInit = ConstantAggregateZero::get(ArrayType::get(Ty, 0));
-  auto *DummyEntry = new GlobalVariable(Mod, DummyInit->getType(), true,
-                                        GlobalVariable::ExternalLinkage,
-                                        DummyInit, "__dummy_" + SectionSuffix);
-  DummyEntry->setSection(getSectionName(SectionSuffix));
-  DummyEntry->setVisibility(GlobalValue::HiddenVisibility);
-  if (TargetTriple.supportsCOMDAT())
-    DummyEntry->setComdat(Mod.getOrInsertComdat(DummyEntry->getName()));
-  // Make sure the section isn't discarded by gc-sections.
-  appendToUsed(Mod, DummyEntry);
-}
-
 StringRef SanitizerBinaryMetadata::getSectionName(StringRef SectionSuffix) {
   // FIXME: Other TargetTriple (req. string pool)
   return SectionSuffix;

diff  --git a/llvm/test/Instrumentation/SanitizerBinaryMetadata/atomics.ll b/llvm/test/Instrumentation/SanitizerBinaryMetadata/atomics.ll
index 06893b21aa6e1..01e504d3c6c2d 100644
--- a/llvm/test/Instrumentation/SanitizerBinaryMetadata/atomics.ll
+++ b/llvm/test/Instrumentation/SanitizerBinaryMetadata/atomics.ll
@@ -1,6 +1,12 @@
 ; RUN: opt < %s -passes='module(sanmd-module)' -sanitizer-metadata-atomics -S | FileCheck %s
 
 ; Check that atomic memory operations receive PC sections metadata.
+
+; CHECK: @__start_sanmd_atomics = extern_weak hidden global ptr
+; CHECK: @__stop_sanmd_atomics = extern_weak hidden global ptr
+; CHECK: @__start_sanmd_covered = extern_weak hidden global ptr
+; CHECK: @__stop_sanmd_covered = extern_weak hidden global ptr
+
 target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"
 
 define i8 @atomic8_load_unordered(ptr %a) nounwind uwtable {