[MTE] Apply alignment / size in AsmPrinter rather than IR #111918
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Created using spr 1.3.4
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
fmayer
changed the title
fmayer
changed the title
llvmbot
added
clang
|
@llvm/pr-subscribers-flang-parser @llvm/pr-subscribers-backend-aarch64 Author: Florian Mayer (fmayer) ChangesThis greatly simplifies the code, and makes sure no optimizations are Full diff: https://github.com/llvm/llvm-project/pull/111918.diff 10 Files Affected:
diff --git a/clang/lib/CodeGen/SanitizerMetadata.cpp b/clang/lib/CodeGen/SanitizerMetadata.cpp
index 5b212a163611dc..95e3f8a01f14bc 100644
--- a/clang/lib/CodeGen/SanitizerMetadata.cpp
+++ b/clang/lib/CodeGen/SanitizerMetadata.cpp
@@ -34,6 +34,37 @@ static SanitizerMask expandKernelSanitizerMasks(SanitizerMask Mask) {
return Mask;
}
+static bool shouldTagGlobal(const llvm::GlobalVariable &G) {
+ // For now, don't instrument constant data, as it'll be in .rodata anyway. It
+ // may be worth instrumenting these in future to stop them from being used as
+ // gadgets.
+ if (G.getName().starts_with("llvm.") || G.isThreadLocal() || G.isConstant())
+ return false;
+
+ // Globals can be placed implicitly or explicitly in sections. There's two
+ // different types of globals that meet this criteria that cause problems:
+ // 1. Function pointers that are going into various init arrays (either
+ // explicitly through `__attribute__((section(<foo>)))` or implicitly
+ // through `__attribute__((constructor)))`, such as ".(pre)init(_array)",
+ // ".fini(_array)", ".ctors", and ".dtors". These function pointers end up
+ // overaligned and overpadded, making iterating over them problematic, and
+ // each function pointer is individually tagged (so the iteration over
+ // them causes SIGSEGV/MTE[AS]ERR).
+ // 2. Global variables put into an explicit section, where the section's name
+ // is a valid C-style identifier. The linker emits a `__start_<name>` and
+ // `__stop_<name>` symbol for the section, so that you can iterate over
+ // globals within this section. Unfortunately, again, these globals would
+ // be tagged and so iteration causes SIGSEGV/MTE[AS]ERR.
+ //
+ // To mitigate both these cases, and because specifying a section is rare
+ // outside of these two cases, disable MTE protection for globals in any
+ // section.
+ if (G.hasSection())
+ return false;
+
+ return true;
+}
+
void SanitizerMetadata::reportGlobal(llvm::GlobalVariable *GV,
SourceLocation Loc, StringRef Name,
QualType Ty,
@@ -60,11 +91,15 @@ void SanitizerMetadata::reportGlobal(llvm::GlobalVariable *GV,
Meta.NoHWAddress |= CGM.isInNoSanitizeList(
FsanitizeArgument.Mask & SanitizerKind::HWAddress, GV, Loc, Ty);
- Meta.Memtag |=
- static_cast<bool>(FsanitizeArgument.Mask & SanitizerKind::MemtagGlobals);
- Meta.Memtag &= !NoSanitizeAttrSet.hasOneOf(SanitizerKind::MemTag);
- Meta.Memtag &= !CGM.isInNoSanitizeList(
- FsanitizeArgument.Mask & SanitizerKind::MemTag, GV, Loc, Ty);
+ if (shouldTagGlobal(*GV)) {
+ Meta.Memtag |= static_cast<bool>(FsanitizeArgument.Mask &
+ SanitizerKind::MemtagGlobals);
+ Meta.Memtag &= !NoSanitizeAttrSet.hasOneOf(SanitizerKind::MemTag);
+ Meta.Memtag &= !CGM.isInNoSanitizeList(
+ FsanitizeArgument.Mask & SanitizerKind::MemTag, GV, Loc, Ty);
+ } else {
+ Meta.Memtag = false;
+ }
Meta.IsDynInit = IsDynInit && !Meta.NoAddress &&
FsanitizeArgument.has(SanitizerKind::Address) &&
diff --git a/clang/test/CodeGen/memtag-globals.cpp b/clang/test/CodeGen/memtag-globals.cpp
index b4f5dc0d7dcf04..a721ac6ce3345f 100644
--- a/clang/test/CodeGen/memtag-globals.cpp
+++ b/clang/test/CodeGen/memtag-globals.cpp
@@ -8,6 +8,7 @@
// RUN: FileCheck %s --check-prefix=IGNORELIST
int global;
+int __attribute__((__section__("my_section"))) section_global;
int __attribute__((no_sanitize("memtag"))) attributed_global;
int __attribute__((disable_sanitizer_instrumentation)) disable_instrumentation_global;
int ignorelisted_global;
@@ -22,6 +23,8 @@ void func() {
// CHECK: @{{.*}}extra_global{{.*}} ={{.*}} sanitize_memtag
// CHECK: @{{.*}}global{{.*}} ={{.*}} sanitize_memtag
+// CHECK: @{{.*}}section_global{{.*}} =
+// CHECK-NOT: sanitize_memtag
// CHECK: @{{.*}}attributed_global{{.*}} =
// CHECK-NOT: sanitize_memtag
// CHECK: @{{.*}}disable_instrumentation_global{{.*}} =
@@ -30,7 +33,7 @@ void func() {
// CHECK-NOT: sanitize_memtag
// CHECK: @{{.*}}static_var{{.*}} ={{.*}} sanitize_memtag
-// CHECK: @{{.*}} = {{.*}} c"Hello, world!\00"{{.*}} sanitize_memtag
+// CHECK: @{{.*}} = {{.*}} c"Hello, world!\00"{{.*}}
// CHECK: @{{.*}}external_global{{.*}} ={{.*}} sanitize_memtag
// IGNORELIST: @{{.*}}extra_global{{.*}} ={{.*}} sanitize_memtag
diff --git a/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp b/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
index 3a8cde7330efc0..aade8e1368ee8c 100644
--- a/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
@@ -764,11 +764,17 @@ void AsmPrinter::emitGlobalVariable(const GlobalVariable *GV) {
const DataLayout &DL = GV->getDataLayout();
uint64_t Size = DL.getTypeAllocSize(GV->getValueType());
+ if (GV->isTagged())
+ Size = alignTo(Size, 16);
// If the alignment is specified, we *must* obey it. Overaligning a global
// with a specified alignment is a prompt way to break globals emitted to
// sections and expected to be contiguous (e.g. ObjC metadata).
- const Align Alignment = getGVAlignment(GV, DL);
+ Align Alignment = getGVAlignment(GV, DL);
+ if (GV->isTagged() && Alignment < 16) {
+ assert(!GV->hasSection());
+ Alignment = Align(16);
+ }
for (auto &Handler : DebugHandlers)
Handler->setSymbolSize(GVSym, Size);
diff --git a/llvm/lib/Target/AArch64/AArch64.h b/llvm/lib/Target/AArch64/AArch64.h
index 62fbf94e803f0c..ffa578d412b3c2 100644
--- a/llvm/lib/Target/AArch64/AArch64.h
+++ b/llvm/lib/Target/AArch64/AArch64.h
@@ -72,7 +72,6 @@ FunctionPass *createAArch64PostLegalizerLowering();
FunctionPass *createAArch64PostSelectOptimize();
FunctionPass *createAArch64StackTaggingPass(bool IsOptNone);
FunctionPass *createAArch64StackTaggingPreRAPass();
-ModulePass *createAArch64GlobalsTaggingPass();
ModulePass *createAArch64Arm64ECCallLoweringPass();
void initializeAArch64A53Fix835769Pass(PassRegistry&);
@@ -89,7 +88,6 @@ void initializeAArch64ConditionalComparesPass(PassRegistry &);
void initializeAArch64DAGToDAGISelLegacyPass(PassRegistry &);
void initializeAArch64DeadRegisterDefinitionsPass(PassRegistry&);
void initializeAArch64ExpandPseudoPass(PassRegistry &);
-void initializeAArch64GlobalsTaggingPass(PassRegistry &);
void initializeAArch64LoadStoreOptPass(PassRegistry&);
void initializeAArch64LowerHomogeneousPrologEpilogPass(PassRegistry &);
void initializeAArch64MIPeepholeOptPass(PassRegistry &);
diff --git a/llvm/lib/Target/AArch64/AArch64GlobalsTagging.cpp b/llvm/lib/Target/AArch64/AArch64GlobalsTagging.cpp
deleted file mode 100644
index a49d391d9148c3..00000000000000
--- a/llvm/lib/Target/AArch64/AArch64GlobalsTagging.cpp
+++ /dev/null
@@ -1,155 +0,0 @@
-//===- AArch64GlobalsTagging.cpp - Global tagging in IR -------------------===//
-//
-// The LLVM Compiler Infrastructure
-//
-// This file is distributed under the University of Illinois Open Source
-// License. See LICENSE.TXT for details.
-//
-//===----------------------------------------------------------------------===//
-//===----------------------------------------------------------------------===//
-
-#include "AArch64.h"
-#include "llvm/ADT/SmallVector.h"
-#include "llvm/IR/Constants.h"
-#include "llvm/IR/GlobalValue.h"
-#include "llvm/IR/GlobalVariable.h"
-#include "llvm/IR/Module.h"
-#include "llvm/Pass.h"
-
-#include <algorithm>
-
-using namespace llvm;
-
-static const Align kTagGranuleSize = Align(16);
-
-static bool shouldTagGlobal(const GlobalVariable &G) {
- // For now, don't instrument constant data, as it'll be in .rodata anyway. It
- // may be worth instrumenting these in future to stop them from being used as
- // gadgets.
- if (G.getName().starts_with("llvm.") || G.isThreadLocal() || G.isConstant())
- return false;
-
- // Globals can be placed implicitly or explicitly in sections. There's two
- // different types of globals that meet this criteria that cause problems:
- // 1. Function pointers that are going into various init arrays (either
- // explicitly through `__attribute__((section(<foo>)))` or implicitly
- // through `__attribute__((constructor)))`, such as ".(pre)init(_array)",
- // ".fini(_array)", ".ctors", and ".dtors". These function pointers end up
- // overaligned and overpadded, making iterating over them problematic, and
- // each function pointer is individually tagged (so the iteration over
- // them causes SIGSEGV/MTE[AS]ERR).
- // 2. Global variables put into an explicit section, where the section's name
- // is a valid C-style identifier. The linker emits a `__start_<name>` and
- // `__stop_<name>` symbol for the section, so that you can iterate over
- // globals within this section. Unfortunately, again, these globals would
- // be tagged and so iteration causes SIGSEGV/MTE[AS]ERR.
- //
- // To mitigate both these cases, and because specifying a section is rare
- // outside of these two cases, disable MTE protection for globals in any
- // section.
- if (G.hasSection())
- return false;
-
- return true;
-}
-
-// Technically, due to ELF symbol interposition semantics, we can't change the
-// alignment or size of symbols. If we increase the alignment or size of a
-// symbol, the compiler may make optimisations based on this new alignment or
-// size. If the symbol is interposed, this optimisation could lead to
-// alignment-related or OOB read/write crashes.
-//
-// This is handled in the linker. When the linker sees multiple declarations of
-// a global variable, and some are tagged, and some are untagged, it resolves it
-// to be an untagged definition - but preserves the tag-granule-rounded size and
-// tag-granule-alignment. This should prevent these kind of crashes intra-DSO.
-// For cross-DSO, it's been a reasonable contract that if you're interposing a
-// sanitizer-instrumented global, then the interposer also needs to be
-// sanitizer-instrumented.
-//
-// FIXME: In theory, this can be fixed by splitting the size/alignment of
-// globals into two uses: an "output alignment" that's emitted to the ELF file,
-// and an "optimisation alignment" that's used for optimisation. Thus, we could
-// adjust the output alignment only, and still optimise based on the pessimistic
-// pre-tagging size/alignment.
-static void tagGlobalDefinition(Module &M, GlobalVariable *G) {
- Constant *Initializer = G->getInitializer();
- uint64_t SizeInBytes =
- M.getDataLayout().getTypeAllocSize(Initializer->getType());
-
- uint64_t NewSize = alignTo(SizeInBytes, kTagGranuleSize);
- if (SizeInBytes != NewSize) {
- // Pad the initializer out to the next multiple of 16 bytes.
- llvm::SmallVector<uint8_t> Init(NewSize - SizeInBytes, 0);
- Constant *Padding = ConstantDataArray::get(M.getContext(), Init);
- Initializer = ConstantStruct::getAnon({Initializer, Padding});
- auto *NewGV = new GlobalVariable(
- M, Initializer->getType(), G->isConstant(), G->getLinkage(),
- Initializer, "", G, G->getThreadLocalMode(), G->getAddressSpace());
- NewGV->copyAttributesFrom(G);
- NewGV->setComdat(G->getComdat());
- NewGV->copyMetadata(G, 0);
-
- NewGV->takeName(G);
- G->replaceAllUsesWith(NewGV);
- G->eraseFromParent();
- G = NewGV;
- }
-
- G->setAlignment(std::max(G->getAlign().valueOrOne(), kTagGranuleSize));
-
- // Ensure that tagged globals don't get merged by ICF - as they should have
- // different tags at runtime.
- G->setUnnamedAddr(GlobalValue::UnnamedAddr::None);
-}
-
-namespace {
-class AArch64GlobalsTagging : public ModulePass {
-public:
- static char ID;
-
- explicit AArch64GlobalsTagging() : ModulePass(ID) {
- initializeAArch64GlobalsTaggingPass(*PassRegistry::getPassRegistry());
- }
-
- bool runOnModule(Module &M) override;
-
- StringRef getPassName() const override { return "AArch64 Globals Tagging"; }
-};
-} // anonymous namespace
-
-char AArch64GlobalsTagging::ID = 0;
-
-bool AArch64GlobalsTagging::runOnModule(Module &M) {
- // No mutating the globals in-place, or iterator invalidation occurs.
- SmallVector<GlobalVariable *> GlobalsToTag;
- for (GlobalVariable &G : M.globals()) {
- if (G.isDeclaration() || !G.isTagged())
- continue;
-
- assert(G.hasSanitizerMetadata() &&
- "Missing sanitizer metadata, but symbol is apparently tagged.");
- if (!shouldTagGlobal(G)) {
- GlobalValue::SanitizerMetadata Meta = G.getSanitizerMetadata();
- Meta.Memtag = false;
- G.setSanitizerMetadata(Meta);
- assert(!G.isTagged());
- }
- GlobalsToTag.push_back(&G);
- }
-
- for (GlobalVariable *G : GlobalsToTag) {
- tagGlobalDefinition(M, G);
- }
-
- return true;
-}
-
-INITIALIZE_PASS_BEGIN(AArch64GlobalsTagging, "aarch64-globals-tagging",
- "AArch64 Globals Tagging Pass", false, false)
-INITIALIZE_PASS_END(AArch64GlobalsTagging, "aarch64-globals-tagging",
- "AArch64 Globals Tagging Pass", false, false)
-
-ModulePass *llvm::createAArch64GlobalsTaggingPass() {
- return new AArch64GlobalsTagging();
-}
diff --git a/llvm/lib/Target/AArch64/AArch64TargetMachine.cpp b/llvm/lib/Target/AArch64/AArch64TargetMachine.cpp
index 7b0ae23358673e..b8d737f5beee42 100644
--- a/llvm/lib/Target/AArch64/AArch64TargetMachine.cpp
+++ b/llvm/lib/Target/AArch64/AArch64TargetMachine.cpp
@@ -269,7 +269,6 @@ extern "C" LLVM_EXTERNAL_VISIBILITY void LLVMInitializeAArch64Target() {
initializeAArch64StackTaggingPreRAPass(*PR);
initializeAArch64LowerHomogeneousPrologEpilogPass(*PR);
initializeAArch64DAGToDAGISelLegacyPass(*PR);
- initializeAArch64GlobalsTaggingPass(*PR);
}
//===----------------------------------------------------------------------===//
@@ -632,7 +631,6 @@ void AArch64PassConfig::addIRPasses() {
if (getOptLevel() == CodeGenOptLevel::Aggressive && EnableSelectOpt)
addPass(createSelectOptimizePass());
- addPass(createAArch64GlobalsTaggingPass());
addPass(createAArch64StackTaggingPass(
/*IsOptNone=*/TM->getOptLevel() == CodeGenOptLevel::None));
diff --git a/llvm/lib/Target/AArch64/CMakeLists.txt b/llvm/lib/Target/AArch64/CMakeLists.txt
index da13db8e68b0e6..2300e479bc1106 100644
--- a/llvm/lib/Target/AArch64/CMakeLists.txt
+++ b/llvm/lib/Target/AArch64/CMakeLists.txt
@@ -57,7 +57,6 @@ add_llvm_target(AArch64CodeGen
AArch64FastISel.cpp
AArch64A53Fix835769.cpp
AArch64FrameLowering.cpp
- AArch64GlobalsTagging.cpp
AArch64CompressJumpTables.cpp
AArch64ConditionOptimizer.cpp
AArch64RedundantCopyElimination.cpp
diff --git a/llvm/test/CodeGen/AArch64/O0-pipeline.ll b/llvm/test/CodeGen/AArch64/O0-pipeline.ll
index e01df9ea03e78b..0d079881cb909c 100644
--- a/llvm/test/CodeGen/AArch64/O0-pipeline.ll
+++ b/llvm/test/CodeGen/AArch64/O0-pipeline.ll
@@ -25,8 +25,6 @@
; CHECK-NEXT: Instrument function entry/exit with calls to e.g. mcount() (post inlining)
; CHECK-NEXT: Scalarize Masked Memory Intrinsics
; CHECK-NEXT: Expand reduction intrinsics
-; CHECK-NEXT: AArch64 Globals Tagging
-; CHECK-NEXT: FunctionPass Manager
; CHECK-NEXT: Dominator Tree Construction
; CHECK-NEXT: Natural Loop Information
; CHECK-NEXT: Lazy Branch Probability Analysis
diff --git a/llvm/test/CodeGen/AArch64/O3-pipeline.ll b/llvm/test/CodeGen/AArch64/O3-pipeline.ll
index fb94c040ae341a..84ed0d61fceab4 100644
--- a/llvm/test/CodeGen/AArch64/O3-pipeline.ll
+++ b/llvm/test/CodeGen/AArch64/O3-pipeline.ll
@@ -72,7 +72,6 @@
; CHECK-NEXT: Lazy Block Frequency Analysis
; CHECK-NEXT: Optimization Remark Emitter
; CHECK-NEXT: Optimize selects
-; CHECK-NEXT: AArch64 Globals Tagging
; CHECK-NEXT: Stack Safety Analysis
; CHECK-NEXT: FunctionPass Manager
; CHECK-NEXT: Dominator Tree Construction
diff --git a/llvm/utils/gn/secondary/llvm/lib/Target/AArch64/BUILD.gn b/llvm/utils/gn/secondary/llvm/lib/Target/AArch64/BUILD.gn
index 57570de8813751..6ef0bc7a7d67a1 100644
--- a/llvm/utils/gn/secondary/llvm/lib/Target/AArch64/BUILD.gn
+++ b/llvm/utils/gn/secondary/llvm/lib/Target/AArch64/BUILD.gn
@@ -126,7 +126,6 @@ static_library("LLVMAArch64CodeGen") {
"AArch64FalkorHWPFFix.cpp",
"AArch64FastISel.cpp",
"AArch64FrameLowering.cpp",
- "AArch64GlobalsTagging.cpp",
"AArch64ISelDAGToDAG.cpp",
"AArch64ISelLowering.cpp",
"AArch64InstrInfo.cpp",
|
|
@llvm/pr-subscribers-clang Author: Florian Mayer (fmayer) ChangesThis greatly simplifies the code, and makes sure no optimizations are Full diff: https://github.com/llvm/llvm-project/pull/111918.diff 10 Files Affected:
diff --git a/clang/lib/CodeGen/SanitizerMetadata.cpp b/clang/lib/CodeGen/SanitizerMetadata.cpp
index 5b212a163611dc..95e3f8a01f14bc 100644
--- a/clang/lib/CodeGen/SanitizerMetadata.cpp
+++ b/clang/lib/CodeGen/SanitizerMetadata.cpp
@@ -34,6 +34,37 @@ static SanitizerMask expandKernelSanitizerMasks(SanitizerMask Mask) {
return Mask;
}
+static bool shouldTagGlobal(const llvm::GlobalVariable &G) {
+ // For now, don't instrument constant data, as it'll be in .rodata anyway. It
+ // may be worth instrumenting these in future to stop them from being used as
+ // gadgets.
+ if (G.getName().starts_with("llvm.") || G.isThreadLocal() || G.isConstant())
+ return false;
+
+ // Globals can be placed implicitly or explicitly in sections. There's two
+ // different types of globals that meet this criteria that cause problems:
+ // 1. Function pointers that are going into various init arrays (either
+ // explicitly through `__attribute__((section(<foo>)))` or implicitly
+ // through `__attribute__((constructor)))`, such as ".(pre)init(_array)",
+ // ".fini(_array)", ".ctors", and ".dtors". These function pointers end up
+ // overaligned and overpadded, making iterating over them problematic, and
+ // each function pointer is individually tagged (so the iteration over
+ // them causes SIGSEGV/MTE[AS]ERR).
+ // 2. Global variables put into an explicit section, where the section's name
+ // is a valid C-style identifier. The linker emits a `__start_<name>` and
+ // `__stop_<name>` symbol for the section, so that you can iterate over
+ // globals within this section. Unfortunately, again, these globals would
+ // be tagged and so iteration causes SIGSEGV/MTE[AS]ERR.
+ //
+ // To mitigate both these cases, and because specifying a section is rare
+ // outside of these two cases, disable MTE protection for globals in any
+ // section.
+ if (G.hasSection())
+ return false;
+
+ return true;
+}
+
void SanitizerMetadata::reportGlobal(llvm::GlobalVariable *GV,
SourceLocation Loc, StringRef Name,
QualType Ty,
@@ -60,11 +91,15 @@ void SanitizerMetadata::reportGlobal(llvm::GlobalVariable *GV,
Meta.NoHWAddress |= CGM.isInNoSanitizeList(
FsanitizeArgument.Mask & SanitizerKind::HWAddress, GV, Loc, Ty);
- Meta.Memtag |=
- static_cast<bool>(FsanitizeArgument.Mask & SanitizerKind::MemtagGlobals);
- Meta.Memtag &= !NoSanitizeAttrSet.hasOneOf(SanitizerKind::MemTag);
- Meta.Memtag &= !CGM.isInNoSanitizeList(
- FsanitizeArgument.Mask & SanitizerKind::MemTag, GV, Loc, Ty);
+ if (shouldTagGlobal(*GV)) {
+ Meta.Memtag |= static_cast<bool>(FsanitizeArgument.Mask &
+ SanitizerKind::MemtagGlobals);
+ Meta.Memtag &= !NoSanitizeAttrSet.hasOneOf(SanitizerKind::MemTag);
+ Meta.Memtag &= !CGM.isInNoSanitizeList(
+ FsanitizeArgument.Mask & SanitizerKind::MemTag, GV, Loc, Ty);
+ } else {
+ Meta.Memtag = false;
+ }
Meta.IsDynInit = IsDynInit && !Meta.NoAddress &&
FsanitizeArgument.has(SanitizerKind::Address) &&
diff --git a/clang/test/CodeGen/memtag-globals.cpp b/clang/test/CodeGen/memtag-globals.cpp
index b4f5dc0d7dcf04..a721ac6ce3345f 100644
--- a/clang/test/CodeGen/memtag-globals.cpp
+++ b/clang/test/CodeGen/memtag-globals.cpp
@@ -8,6 +8,7 @@
// RUN: FileCheck %s --check-prefix=IGNORELIST
int global;
+int __attribute__((__section__("my_section"))) section_global;
int __attribute__((no_sanitize("memtag"))) attributed_global;
int __attribute__((disable_sanitizer_instrumentation)) disable_instrumentation_global;
int ignorelisted_global;
@@ -22,6 +23,8 @@ void func() {
// CHECK: @{{.*}}extra_global{{.*}} ={{.*}} sanitize_memtag
// CHECK: @{{.*}}global{{.*}} ={{.*}} sanitize_memtag
+// CHECK: @{{.*}}section_global{{.*}} =
+// CHECK-NOT: sanitize_memtag
// CHECK: @{{.*}}attributed_global{{.*}} =
// CHECK-NOT: sanitize_memtag
// CHECK: @{{.*}}disable_instrumentation_global{{.*}} =
@@ -30,7 +33,7 @@ void func() {
// CHECK-NOT: sanitize_memtag
// CHECK: @{{.*}}static_var{{.*}} ={{.*}} sanitize_memtag
-// CHECK: @{{.*}} = {{.*}} c"Hello, world!\00"{{.*}} sanitize_memtag
+// CHECK: @{{.*}} = {{.*}} c"Hello, world!\00"{{.*}}
// CHECK: @{{.*}}external_global{{.*}} ={{.*}} sanitize_memtag
// IGNORELIST: @{{.*}}extra_global{{.*}} ={{.*}} sanitize_memtag
diff --git a/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp b/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
index 3a8cde7330efc0..aade8e1368ee8c 100644
--- a/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
@@ -764,11 +764,17 @@ void AsmPrinter::emitGlobalVariable(const GlobalVariable *GV) {
const DataLayout &DL = GV->getDataLayout();
uint64_t Size = DL.getTypeAllocSize(GV->getValueType());
+ if (GV->isTagged())
+ Size = alignTo(Size, 16);
// If the alignment is specified, we *must* obey it. Overaligning a global
// with a specified alignment is a prompt way to break globals emitted to
// sections and expected to be contiguous (e.g. ObjC metadata).
- const Align Alignment = getGVAlignment(GV, DL);
+ Align Alignment = getGVAlignment(GV, DL);
+ if (GV->isTagged() && Alignment < 16) {
+ assert(!GV->hasSection());
+ Alignment = Align(16);
+ }
for (auto &Handler : DebugHandlers)
Handler->setSymbolSize(GVSym, Size);
diff --git a/llvm/lib/Target/AArch64/AArch64.h b/llvm/lib/Target/AArch64/AArch64.h
index 62fbf94e803f0c..ffa578d412b3c2 100644
--- a/llvm/lib/Target/AArch64/AArch64.h
+++ b/llvm/lib/Target/AArch64/AArch64.h
@@ -72,7 +72,6 @@ FunctionPass *createAArch64PostLegalizerLowering();
FunctionPass *createAArch64PostSelectOptimize();
FunctionPass *createAArch64StackTaggingPass(bool IsOptNone);
FunctionPass *createAArch64StackTaggingPreRAPass();
-ModulePass *createAArch64GlobalsTaggingPass();
ModulePass *createAArch64Arm64ECCallLoweringPass();
void initializeAArch64A53Fix835769Pass(PassRegistry&);
@@ -89,7 +88,6 @@ void initializeAArch64ConditionalComparesPass(PassRegistry &);
void initializeAArch64DAGToDAGISelLegacyPass(PassRegistry &);
void initializeAArch64DeadRegisterDefinitionsPass(PassRegistry&);
void initializeAArch64ExpandPseudoPass(PassRegistry &);
-void initializeAArch64GlobalsTaggingPass(PassRegistry &);
void initializeAArch64LoadStoreOptPass(PassRegistry&);
void initializeAArch64LowerHomogeneousPrologEpilogPass(PassRegistry &);
void initializeAArch64MIPeepholeOptPass(PassRegistry &);
diff --git a/llvm/lib/Target/AArch64/AArch64GlobalsTagging.cpp b/llvm/lib/Target/AArch64/AArch64GlobalsTagging.cpp
deleted file mode 100644
index a49d391d9148c3..00000000000000
--- a/llvm/lib/Target/AArch64/AArch64GlobalsTagging.cpp
+++ /dev/null
@@ -1,155 +0,0 @@
-//===- AArch64GlobalsTagging.cpp - Global tagging in IR -------------------===//
-//
-// The LLVM Compiler Infrastructure
-//
-// This file is distributed under the University of Illinois Open Source
-// License. See LICENSE.TXT for details.
-//
-//===----------------------------------------------------------------------===//
-//===----------------------------------------------------------------------===//
-
-#include "AArch64.h"
-#include "llvm/ADT/SmallVector.h"
-#include "llvm/IR/Constants.h"
-#include "llvm/IR/GlobalValue.h"
-#include "llvm/IR/GlobalVariable.h"
-#include "llvm/IR/Module.h"
-#include "llvm/Pass.h"
-
-#include <algorithm>
-
-using namespace llvm;
-
-static const Align kTagGranuleSize = Align(16);
-
-static bool shouldTagGlobal(const GlobalVariable &G) {
- // For now, don't instrument constant data, as it'll be in .rodata anyway. It
- // may be worth instrumenting these in future to stop them from being used as
- // gadgets.
- if (G.getName().starts_with("llvm.") || G.isThreadLocal() || G.isConstant())
- return false;
-
- // Globals can be placed implicitly or explicitly in sections. There's two
- // different types of globals that meet this criteria that cause problems:
- // 1. Function pointers that are going into various init arrays (either
- // explicitly through `__attribute__((section(<foo>)))` or implicitly
- // through `__attribute__((constructor)))`, such as ".(pre)init(_array)",
- // ".fini(_array)", ".ctors", and ".dtors". These function pointers end up
- // overaligned and overpadded, making iterating over them problematic, and
- // each function pointer is individually tagged (so the iteration over
- // them causes SIGSEGV/MTE[AS]ERR).
- // 2. Global variables put into an explicit section, where the section's name
- // is a valid C-style identifier. The linker emits a `__start_<name>` and
- // `__stop_<name>` symbol for the section, so that you can iterate over
- // globals within this section. Unfortunately, again, these globals would
- // be tagged and so iteration causes SIGSEGV/MTE[AS]ERR.
- //
- // To mitigate both these cases, and because specifying a section is rare
- // outside of these two cases, disable MTE protection for globals in any
- // section.
- if (G.hasSection())
- return false;
-
- return true;
-}
-
-// Technically, due to ELF symbol interposition semantics, we can't change the
-// alignment or size of symbols. If we increase the alignment or size of a
-// symbol, the compiler may make optimisations based on this new alignment or
-// size. If the symbol is interposed, this optimisation could lead to
-// alignment-related or OOB read/write crashes.
-//
-// This is handled in the linker. When the linker sees multiple declarations of
-// a global variable, and some are tagged, and some are untagged, it resolves it
-// to be an untagged definition - but preserves the tag-granule-rounded size and
-// tag-granule-alignment. This should prevent these kind of crashes intra-DSO.
-// For cross-DSO, it's been a reasonable contract that if you're interposing a
-// sanitizer-instrumented global, then the interposer also needs to be
-// sanitizer-instrumented.
-//
-// FIXME: In theory, this can be fixed by splitting the size/alignment of
-// globals into two uses: an "output alignment" that's emitted to the ELF file,
-// and an "optimisation alignment" that's used for optimisation. Thus, we could
-// adjust the output alignment only, and still optimise based on the pessimistic
-// pre-tagging size/alignment.
-static void tagGlobalDefinition(Module &M, GlobalVariable *G) {
- Constant *Initializer = G->getInitializer();
- uint64_t SizeInBytes =
- M.getDataLayout().getTypeAllocSize(Initializer->getType());
-
- uint64_t NewSize = alignTo(SizeInBytes, kTagGranuleSize);
- if (SizeInBytes != NewSize) {
- // Pad the initializer out to the next multiple of 16 bytes.
- llvm::SmallVector<uint8_t> Init(NewSize - SizeInBytes, 0);
- Constant *Padding = ConstantDataArray::get(M.getContext(), Init);
- Initializer = ConstantStruct::getAnon({Initializer, Padding});
- auto *NewGV = new GlobalVariable(
- M, Initializer->getType(), G->isConstant(), G->getLinkage(),
- Initializer, "", G, G->getThreadLocalMode(), G->getAddressSpace());
- NewGV->copyAttributesFrom(G);
- NewGV->setComdat(G->getComdat());
- NewGV->copyMetadata(G, 0);
-
- NewGV->takeName(G);
- G->replaceAllUsesWith(NewGV);
- G->eraseFromParent();
- G = NewGV;
- }
-
- G->setAlignment(std::max(G->getAlign().valueOrOne(), kTagGranuleSize));
-
- // Ensure that tagged globals don't get merged by ICF - as they should have
- // different tags at runtime.
- G->setUnnamedAddr(GlobalValue::UnnamedAddr::None);
-}
-
-namespace {
-class AArch64GlobalsTagging : public ModulePass {
-public:
- static char ID;
-
- explicit AArch64GlobalsTagging() : ModulePass(ID) {
- initializeAArch64GlobalsTaggingPass(*PassRegistry::getPassRegistry());
- }
-
- bool runOnModule(Module &M) override;
-
- StringRef getPassName() const override { return "AArch64 Globals Tagging"; }
-};
-} // anonymous namespace
-
-char AArch64GlobalsTagging::ID = 0;
-
-bool AArch64GlobalsTagging::runOnModule(Module &M) {
- // No mutating the globals in-place, or iterator invalidation occurs.
- SmallVector<GlobalVariable *> GlobalsToTag;
- for (GlobalVariable &G : M.globals()) {
- if (G.isDeclaration() || !G.isTagged())
- continue;
-
- assert(G.hasSanitizerMetadata() &&
- "Missing sanitizer metadata, but symbol is apparently tagged.");
- if (!shouldTagGlobal(G)) {
- GlobalValue::SanitizerMetadata Meta = G.getSanitizerMetadata();
- Meta.Memtag = false;
- G.setSanitizerMetadata(Meta);
- assert(!G.isTagged());
- }
- GlobalsToTag.push_back(&G);
- }
-
- for (GlobalVariable *G : GlobalsToTag) {
- tagGlobalDefinition(M, G);
- }
-
- return true;
-}
-
-INITIALIZE_PASS_BEGIN(AArch64GlobalsTagging, "aarch64-globals-tagging",
- "AArch64 Globals Tagging Pass", false, false)
-INITIALIZE_PASS_END(AArch64GlobalsTagging, "aarch64-globals-tagging",
- "AArch64 Globals Tagging Pass", false, false)
-
-ModulePass *llvm::createAArch64GlobalsTaggingPass() {
- return new AArch64GlobalsTagging();
-}
diff --git a/llvm/lib/Target/AArch64/AArch64TargetMachine.cpp b/llvm/lib/Target/AArch64/AArch64TargetMachine.cpp
index 7b0ae23358673e..b8d737f5beee42 100644
--- a/llvm/lib/Target/AArch64/AArch64TargetMachine.cpp
+++ b/llvm/lib/Target/AArch64/AArch64TargetMachine.cpp
@@ -269,7 +269,6 @@ extern "C" LLVM_EXTERNAL_VISIBILITY void LLVMInitializeAArch64Target() {
initializeAArch64StackTaggingPreRAPass(*PR);
initializeAArch64LowerHomogeneousPrologEpilogPass(*PR);
initializeAArch64DAGToDAGISelLegacyPass(*PR);
- initializeAArch64GlobalsTaggingPass(*PR);
}
//===----------------------------------------------------------------------===//
@@ -632,7 +631,6 @@ void AArch64PassConfig::addIRPasses() {
if (getOptLevel() == CodeGenOptLevel::Aggressive && EnableSelectOpt)
addPass(createSelectOptimizePass());
- addPass(createAArch64GlobalsTaggingPass());
addPass(createAArch64StackTaggingPass(
/*IsOptNone=*/TM->getOptLevel() == CodeGenOptLevel::None));
diff --git a/llvm/lib/Target/AArch64/CMakeLists.txt b/llvm/lib/Target/AArch64/CMakeLists.txt
index da13db8e68b0e6..2300e479bc1106 100644
--- a/llvm/lib/Target/AArch64/CMakeLists.txt
+++ b/llvm/lib/Target/AArch64/CMakeLists.txt
@@ -57,7 +57,6 @@ add_llvm_target(AArch64CodeGen
AArch64FastISel.cpp
AArch64A53Fix835769.cpp
AArch64FrameLowering.cpp
- AArch64GlobalsTagging.cpp
AArch64CompressJumpTables.cpp
AArch64ConditionOptimizer.cpp
AArch64RedundantCopyElimination.cpp
diff --git a/llvm/test/CodeGen/AArch64/O0-pipeline.ll b/llvm/test/CodeGen/AArch64/O0-pipeline.ll
index e01df9ea03e78b..0d079881cb909c 100644
--- a/llvm/test/CodeGen/AArch64/O0-pipeline.ll
+++ b/llvm/test/CodeGen/AArch64/O0-pipeline.ll
@@ -25,8 +25,6 @@
; CHECK-NEXT: Instrument function entry/exit with calls to e.g. mcount() (post inlining)
; CHECK-NEXT: Scalarize Masked Memory Intrinsics
; CHECK-NEXT: Expand reduction intrinsics
-; CHECK-NEXT: AArch64 Globals Tagging
-; CHECK-NEXT: FunctionPass Manager
; CHECK-NEXT: Dominator Tree Construction
; CHECK-NEXT: Natural Loop Information
; CHECK-NEXT: Lazy Branch Probability Analysis
diff --git a/llvm/test/CodeGen/AArch64/O3-pipeline.ll b/llvm/test/CodeGen/AArch64/O3-pipeline.ll
index fb94c040ae341a..84ed0d61fceab4 100644
--- a/llvm/test/CodeGen/AArch64/O3-pipeline.ll
+++ b/llvm/test/CodeGen/AArch64/O3-pipeline.ll
@@ -72,7 +72,6 @@
; CHECK-NEXT: Lazy Block Frequency Analysis
; CHECK-NEXT: Optimization Remark Emitter
; CHECK-NEXT: Optimize selects
-; CHECK-NEXT: AArch64 Globals Tagging
; CHECK-NEXT: Stack Safety Analysis
; CHECK-NEXT: FunctionPass Manager
; CHECK-NEXT: Dominator Tree Construction
diff --git a/llvm/utils/gn/secondary/llvm/lib/Target/AArch64/BUILD.gn b/llvm/utils/gn/secondary/llvm/lib/Target/AArch64/BUILD.gn
index 57570de8813751..6ef0bc7a7d67a1 100644
--- a/llvm/utils/gn/secondary/llvm/lib/Target/AArch64/BUILD.gn
+++ b/llvm/utils/gn/secondary/llvm/lib/Target/AArch64/BUILD.gn
@@ -126,7 +126,6 @@ static_library("LLVMAArch64CodeGen") {
"AArch64FalkorHWPFFix.cpp",
"AArch64FastISel.cpp",
"AArch64FrameLowering.cpp",
- "AArch64GlobalsTagging.cpp",
"AArch64ISelDAGToDAG.cpp",
"AArch64ISelLowering.cpp",
"AArch64InstrInfo.cpp",
|
eugenis
approved these changes
Oct 21, 2024
pcc
reviewed
Oct 21, 2024
pcc
reviewed
Oct 21, 2024
|
@pcc does this look better now? |
…ker-rather-than-ir
Created using spr 1.3.4
fmayer
removed
coroutines
pcc
reviewed
Dec 11, 2024
arichardson
reviewed
Dec 11, 2024
There was a problem hiding this comment.
I had to do something very similar for CHERI downstream: We have to ensure that all globals are precisely representable (which may require rounding up the size+alignment) so you don't end up creating bounds that include adjacent ones.
The original patch from 2019 is here: CTSRD-CHERI/llvm-project@fd224dd
I wonder if it would be possible have a hook that CHERI can reuse downstream: something like: getRequiredGlobalAlignment() and getRequiredGlobalSize(). For MTE this could return 16 for alignment and round the size up to a multiple of 16 and after the next merge the CHERI downstream could reuse those hooks.
Adding @jrtc27 for visibility.
Created using spr 1.3.4
pcc
approved these changes
Dec 13, 2024
…ker-rather-than-ir
fmayer
deleted the
users/fmayer/spr/apply-alignment-size-in-linker-rather-than-ir
branch
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/181/builds/10481 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/42/builds/2376 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/136/builds/2026 Here is the relevant piece of the build log for the reference |
|
The s390x failures are unrelated. |
As far as I can tell this feedback was completely ignored?.. |
Yes, I will look at that in a follow up after Christmas, forgot to leave the comment. I don't think there's a reason to block this. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/146/builds/1856 Here is the relevant piece of the build log for the reference |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This makes sure no optimizations are applied that assume the
bigger alignment or size, which could be incorrect if we link
together with non-instrumented code.