[clang] [llvm] [AArch64] Stack probing for function prologues (PR #66524)
https://github.com/oskarwirga approved this pull request. Testing this patch set on a complex application (including later PRs) yielded no issues :) Thank you for your work on this, I appreciate it! https://github.com/llvm/llvm-project/pull/66524 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[llvm] [clang] [AArch64] Stack probing for function prologues (PR #66524)
@@ -688,6 +689,68 @@ void AArch64FrameLowering::emitCalleeSavedSVERestores(
emitCalleeSavedRestores(MBB, MBBI, true);
}
+void AArch64FrameLowering::allocateSVEStackSpace(
+MachineBasicBlock &MBB, MachineBasicBlock::iterator MBBI,
+StackOffset AllocSize, StackOffset InitialOffset, bool EmitCFI) const {
+ DebugLoc DL;
+ MachineFunction &MF = *MBB.getParent();
+ const AArch64FunctionInfo &MFI = *MF.getInfo();
+ const AArch64Subtarget &Subtarget = MF.getSubtarget();
+ const AArch64RegisterInfo &RegInfo = *Subtarget.getRegisterInfo();
+ const AArch64TargetLowering &TLI = *Subtarget.getTargetLowering();
+ const TargetInstrInfo &TII = *Subtarget.getInstrInfo();
+
+ // If not probing the stack or the (uknown) allocation size is less than the
+ // probe size decrement the stack pointer right away. This avoids having to
+ // emit a probing loop when allocating space for up to 16 SVE registers when
+ // using 4k probes.
+
+ // The bit-length of SVE registers is architecturally limited.
+ const int64_t MAX_BYTES_PER_SCALABLE_BYTE = 16;
+ int64_t ProbeSize = MFI.getStackProbeSize();
+ if (!TLI.hasInlineStackProbe(MF) ||
+ AllocSize.getScalable() * MAX_BYTES_PER_SCALABLE_BYTE +
+ AllocSize.getFixed() <=
+ ProbeSize) {
+emitFrameOffset(MBB, MBBI, DL, AArch64::SP, AArch64::SP, -AllocSize, &TII,
+MachineInstr::FrameSetup, false, false, nullptr, EmitCFI,
+InitialOffset);
+if (TLI.hasInlineStackProbe(MF)) {
+ // Issue a probe at the top of the stack to prepare for subsequent
+ // allocations.
+ // STR XZR, [TargetReg]
oskarwirga wrote:
```suggestion
// STR XZR, [SP]
```
https://github.com/llvm/llvm-project/pull/66524
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [AArch64] Stack probing for function prologues (PR #66524)
https://github.com/oskarwirga edited https://github.com/llvm/llvm-project/pull/66524 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [AArch64] Stack probing for function prologues (PR #66524)
@@ -688,6 +689,68 @@ void AArch64FrameLowering::emitCalleeSavedSVERestores(
emitCalleeSavedRestores(MBB, MBBI, true);
}
+void AArch64FrameLowering::allocateSVEStackSpace(
+MachineBasicBlock &MBB, MachineBasicBlock::iterator MBBI,
+StackOffset AllocSize, StackOffset InitialOffset, bool EmitCFI) const {
+ DebugLoc DL;
+ MachineFunction &MF = *MBB.getParent();
+ const AArch64FunctionInfo &MFI = *MF.getInfo();
+ const AArch64Subtarget &Subtarget = MF.getSubtarget();
+ const AArch64RegisterInfo &RegInfo = *Subtarget.getRegisterInfo();
+ const AArch64TargetLowering &TLI = *Subtarget.getTargetLowering();
+ const TargetInstrInfo &TII = *Subtarget.getInstrInfo();
+
+ // If not probing the stack or the (uknown) allocation size is less than the
oskarwirga wrote:
```suggestion
// If not probing the stack or the (unknown) allocation size is less than the
```
https://github.com/llvm/llvm-project/pull/66524
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [UBSAN] Preserve ubsan code with ubsan-unique-traps (PR #83470)
https://github.com/oskarwirga requested changes to this pull request. If you are going to remove this feature, I would rather you simply revert the old commit. There is no point leaving the flag in at this point. I had explored earlier dealing with the optimization at a later time in the compilation pipeline, but got nowhere and this solution was ideal for my use case, binary size constraints limited any inlining so I never ran into the issue. I appreciate you cleaning this up! :) https://github.com/llvm/llvm-project/pull/83470 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] Modify BoundsSan to improve debuggability (PR #65972)
oskarwirga wrote: > @oskarwirga I'd like to use this feature but without counter, preserving > ubsan IDs > > Also I think in the current the counter has limited use: in optimized code, > after inlining, it will have a lot of same ids, like 0, 1 from different > functions. > > So I propose to undo that part of the patch. WDYT? So long as the debuggability is preserved so that it is possible to attribute crashes to specific lines of code, I am fine! Do you have an alternate solution? https://github.com/llvm/llvm-project/pull/65972 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [UBSAN] Preserve ubsan code with ubsan-unique-traps (PR #83470)
oskarwirga wrote: > It happens later, in LLVM backend, it needs to be fixed. >From https://github.com/llvm/llvm-project/pull/65972#issuecomment-1971855638 Is this something you have planned to fix? If not would replacing the .size() counter with perhaps a seeded random uint8 be acceptable? My use case prevents me from shipping the minimal runtime alongside the instrumentation so my goal was to create an improvement (definitely imperfect!) to the debugability of a production deployment of BoundsSan. This PR as is would revert that behavior entirely. https://github.com/llvm/llvm-project/pull/83470 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [AArch64] Stack probing for function prologues (PR #66524)
oskarwirga wrote: > Upon function entry the caller guarantees that it has probed the stack (e.g. > performed a store) at some address [sp, #N], where0 <= N <= 1024. I haven't been able to produce a minimal, sharable example as of yet, but I'm encountering a runtime error associated with an inlined function where stack probing is active. The error manifests as a null pointer dereference, originating from a stack value that is probed (and set to 0) before being subsequently dereferenced. The IR contributing to this runtime issue is somewhat complex and challenging to interpret, but here's my observations: - A value returned from `malloc(some_struct)` is stored in a stack variable. - This stack variable is passed as an argument to a function. - This function is later inlined, and within the inlined body, it attempts to set a value in the struct. - At runtime, when setting the value we get a null pointer dereference. I'm working to isolate this issue and will share a repro ASAP. In the meantime, any insights or suggestions based on this description would be greatly appreciated. Also is it required to write to the value? Would reading the value be sufficient? https://github.com/llvm/llvm-project/pull/66524 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [AArch64] Stack probing for function prologues (PR #66524)
oskarwirga wrote:
Apologies for still not being able to create a reproducible example I can share
but what I am seeing is the stack probe write overwriting the value at the tip
of the stack when I step debug execution:
```
str xzr, [sp, #-0x10 {var_70}]! {0x0}
...
sturx8, [x29, #-0x10 {var_70}]
...
from the inlined function:
str xzr, [x20] {0x0}
mov sp, x20
...
ldurx8, [x29, #-0x10 {var_70}] << null deref
```
I also was able to isolate the issue to the non-fast register allocators. When
building with optimized code, the greedy register allocator and the basic
register allocator ended up choosing registers that were being clobbered (?
don't know the term) by the stack probe write.
> All the stack probing should have already finished before the call to
> `malloc`.
Only for the containing function, the functions which have their stack probes
inlined will be in the middle of the function which then results in this
null-deref. I think there's some re-arranging happening during optimization and
inlining which causes the registers not to be expired (? don't know the term
here)
> Just to make things simpler, can you try disabling the shrink-wrapping and
> see what happens?
I haven't seen noticeable difference with this, I tried passing it in with
`-Wl,-mllvm,-enable-shrink-wrap=false`
https://github.com/llvm/llvm-project/pull/66524
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [AArch64] Stack probing for function prologues (PR #66524)
@@ -9460,6 +9461,94 @@ bool AArch64InstrInfo::isReallyTriviallyReMaterializable(
return TargetInstrInfo::isReallyTriviallyReMaterializable(MI);
}
+MachineBasicBlock::iterator
+AArch64InstrInfo::insertStackProbingLoop(MachineBasicBlock::iterator MBBI,
+ Register ScratchReg,
+ Register TargetReg) const {
+ MachineBasicBlock &MBB = *MBBI->getParent();
+ MachineFunction &MF = *MBB.getParent();
+ const AArch64InstrInfo *TII =
+ MF.getSubtarget().getInstrInfo();
+ int64_t ProbeSize = MF.getInfo()->getStackProbeSize();
+ DebugLoc DL = MBB.findDebugLoc(MBBI);
+
+ MachineFunction::iterator MBBInsertPoint = std::next(MBB.getIterator());
+ MachineBasicBlock *LoopTestMBB =
+ MF.CreateMachineBasicBlock(MBB.getBasicBlock());
+ MF.insert(MBBInsertPoint, LoopTestMBB);
+ MachineBasicBlock *LoopBodyMBB =
+ MF.CreateMachineBasicBlock(MBB.getBasicBlock());
+ MF.insert(MBBInsertPoint, LoopBodyMBB);
+ MachineBasicBlock *ExitMBB = MF.CreateMachineBasicBlock(MBB.getBasicBlock());
+ MF.insert(MBBInsertPoint, ExitMBB);
+
+ // LoopTest:
+ // SUB ScratchReg, ScratchReg, #ProbeSize
+ emitFrameOffset(*LoopTestMBB, LoopTestMBB->end(), DL, ScratchReg, ScratchReg,
+ StackOffset::getFixed(-ProbeSize), TII,
+ MachineInstr::FrameSetup);
+
+ // CMP ScratchReg, TargetReg
+ AArch64CC::CondCode Cond = AArch64CC::LE;
+ Register Op1 = ScratchReg;
+ Register Op2 = TargetReg;
+ if (Op2 == AArch64::SP) {
+assert(Op1 != AArch64::SP && "At most one of the registers can be SP");
+// CMP TargetReg, ScratchReg
+std::swap(Op1, Op2);
+Cond = AArch64CC::GT;
+ }
+ BuildMI(*LoopTestMBB, LoopTestMBB->end(), DL, TII->get(AArch64::SUBSXrx64),
+ AArch64::XZR)
+ .addReg(Op1)
+ .addReg(Op2)
+ .addImm(AArch64_AM::getArithExtendImm(AArch64_AM::UXTX, 0))
+ .setMIFlags(MachineInstr::FrameSetup);
+
+ // B. LoopExit
+ BuildMI(*LoopTestMBB, LoopTestMBB->end(), DL, TII->get(AArch64::Bcc))
+ .addImm(Cond)
+ .addMBB(ExitMBB)
+ .setMIFlags(MachineInstr::FrameSetup);
+
+ // STR XZR, [ScratchReg]
+ BuildMI(*LoopBodyMBB, LoopBodyMBB->end(), DL, TII->get(AArch64::STRXui))
+ .addReg(AArch64::XZR)
+ .addReg(ScratchReg)
+ .addImm(0)
+ .setMIFlags(MachineInstr::FrameSetup);
+
+ // B loop
+ BuildMI(*LoopBodyMBB, LoopBodyMBB->end(), DL, TII->get(AArch64::B))
+ .addMBB(LoopTestMBB)
+ .setMIFlags(MachineInstr::FrameSetup);
+
+ // LoopExit:
+ // STR XZR, [TargetReg]
+ BuildMI(*ExitMBB, ExitMBB->begin(), DL, TII->get(AArch64::STRXui))
+ .addReg(AArch64::XZR)
+ .addReg(TargetReg)
+ .addImm(0)
+ .setMIFlags(MachineInstr::FrameSetup);
oskarwirga wrote:
> Can you spot a place where the probe instruction is not immediately after a
> decrement of the stack (disregarding some random register-to-register
> arithmetic that may appear)?
This was the thread that led to me understanding what is happening:
```
sub sp, sp, #0x1, lsl #0xc
cmp sp, x1
b.le0x557388
str xzr, [x1] {0x0}
```
We are probing the _old_ stack head! `x1` contains `0x7fee80` but `sp` is
at `7fde80`! This means that the selection of the `x1` register instead of
`sp` is incorrect.
I confirmed this to be the case by fixing this probe here and testing again.
```suggestion
// STR XZR, [ScratchReg]
BuildMI(*ExitMBB, ExitMBB->begin(), DL, TII->get(AArch64::STRXui))
.addReg(AArch64::XZR)
.addReg(ScratchReg)
.addImm(0)
.setMIFlags(MachineInstr::FrameSetup);
```
https://github.com/llvm/llvm-project/pull/66524
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Modify BoundsSan to improve debuggability (PR #65972)
@@ -0,0 +1,83 @@ +; RUN: llc -O3 -mtriple arm64-linux -filetype asm -o - %s | FileCheck %s -check-prefix CHECK-ASM +; What this test does is check that even with nomerge, the functions still get merged in +; compiled code as the ubsantrap call gets lowered to a single instruction: brk. oskarwirga wrote: I understood your comment on the Phabricator diff as a request for this test: > can you please create a test where bounds-checking-single-trap=0 and > setCannotMerge produce invalid result. I will also add a test in `llvm/test/Instrumentation/BoundsChecking/` https://github.com/llvm/llvm-project/pull/65972 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Modify BoundsSan to improve debuggability (PR #65972)
https://github.com/oskarwirga updated
https://github.com/llvm/llvm-project/pull/65972
>From 8dd1d0c534faadd65f546a150bbd2cc5a132aa1e Mon Sep 17 00:00:00 2001
From: Oskar Wirga <10386631+oskarwi...@users.noreply.github.com>
Date: Wed, 27 Sep 2023 10:37:49 -0700
Subject: [PATCH 1/2] Modify array-bounds sanitizer to improve debuggability
---
clang/lib/CodeGen/CGExpr.cpp | 34 ++--
clang/test/CodeGen/bounds-checking.c | 13 +++
2 files changed, 35 insertions(+), 12 deletions(-)
diff --git a/clang/lib/CodeGen/CGExpr.cpp b/clang/lib/CodeGen/CGExpr.cpp
index 3123e278985f210..7b4389c874b3f90 100644
--- a/clang/lib/CodeGen/CGExpr.cpp
+++ b/clang/lib/CodeGen/CGExpr.cpp
@@ -51,6 +51,12 @@
using namespace clang;
using namespace CodeGen;
+// Experiment to make sanitizers easier to debug
+static llvm::cl::opt ClSanitizeDebugDeoptimization(
+"ubsan-unique-traps", llvm::cl::Optional,
+llvm::cl::desc("Deoptimize traps for UBSAN so there is 1 trap per check"),
+llvm::cl::init(false));
+
//======//
//Miscellaneous Helper Methods
//======//
@@ -3553,17 +3559,28 @@ void CodeGenFunction::EmitTrapCheck(llvm::Value
*Checked,
// check-type per function to save on code size.
if (TrapBBs.size() <= CheckHandlerID)
TrapBBs.resize(CheckHandlerID + 1);
+
llvm::BasicBlock *&TrapBB = TrapBBs[CheckHandlerID];
- if (!CGM.getCodeGenOpts().OptimizationLevel || !TrapBB ||
- (CurCodeDecl && CurCodeDecl->hasAttr())) {
+ if (!ClSanitizeDebugDeoptimization &&
+ CGM.getCodeGenOpts().OptimizationLevel && TrapBB &&
+ (!CurCodeDecl || !CurCodeDecl->hasAttr())) {
+auto Call = TrapBB->begin();
+assert(isa(Call) && "Expected call in trap BB");
+
+Call->applyMergedLocation(Call->getDebugLoc(),
+ Builder.getCurrentDebugLocation());
+Builder.CreateCondBr(Checked, Cont, TrapBB);
+ } else {
TrapBB = createBasicBlock("trap");
Builder.CreateCondBr(Checked, Cont, TrapBB);
EmitBlock(TrapBB);
-llvm::CallInst *TrapCall =
-Builder.CreateCall(CGM.getIntrinsic(llvm::Intrinsic::ubsantrap),
- llvm::ConstantInt::get(CGM.Int8Ty, CheckHandlerID));
+llvm::CallInst *TrapCall = Builder.CreateCall(
+CGM.getIntrinsic(llvm::Intrinsic::ubsantrap),
+llvm::ConstantInt::get(CGM.Int8Ty, ClSanitizeDebugDeoptimization
+ ? TrapBB->getParent()->size()
+ : CheckHandlerID));
if (!CGM.getCodeGenOpts().TrapFuncName.empty()) {
auto A = llvm::Attribute::get(getLLVMContext(), "trap-func-name",
@@ -3573,13 +3590,6 @@ void CodeGenFunction::EmitTrapCheck(llvm::Value *Checked,
TrapCall->setDoesNotReturn();
TrapCall->setDoesNotThrow();
Builder.CreateUnreachable();
- } else {
-auto Call = TrapBB->begin();
-assert(isa(Call) && "Expected call in trap BB");
-
-Call->applyMergedLocation(Call->getDebugLoc(),
- Builder.getCurrentDebugLocation());
-Builder.CreateCondBr(Checked, Cont, TrapBB);
}
EmitBlock(Cont);
diff --git a/clang/test/CodeGen/bounds-checking.c
b/clang/test/CodeGen/bounds-checking.c
index 1b9e28915e5d9af..cda208917ea1ba1 100644
--- a/clang/test/CodeGen/bounds-checking.c
+++ b/clang/test/CodeGen/bounds-checking.c
@@ -1,5 +1,6 @@
// RUN: %clang_cc1 -fsanitize=local-bounds -emit-llvm -triple
x86_64-apple-darwin10 %s -o - | FileCheck %s
// RUN: %clang_cc1 -fsanitize=array-bounds -O -fsanitize-trap=array-bounds
-emit-llvm -triple x86_64-apple-darwin10 -DNO_DYNAMIC %s -o - | FileCheck %s
+// RUN: %clang_cc1 -fsanitize=array-bounds -fsanitize-trap=array-bounds -O3
-mllvm -ubsan-unique-traps -emit-llvm -triple x86_64-apple-darwin10 %s -o - |
FileCheck %s --check-prefixes=NOOPTARRAY
//
// REQUIRES: x86-registered-target
@@ -66,3 +67,15 @@ int f7(union U *u, int i) {
// CHECK-NOT: @llvm.ubsantrap
return u->c[i];
}
+
+
+char B[10];
+char B2[10];
+// CHECK-LABEL: @f8
+void f8(int i, int k) {
+ // NOOPTARRAY: call void @llvm.ubsantrap(i8 2)
+ B[i] = '\0';
+
+ // NOOPTARRAY: call void @llvm.ubsantrap(i8 4)
+ B2[k] = '\0';
+}
>From 601856499676b5d5303297ed437f22d40376922e Mon Sep 17 00:00:00 2001
From: Oskar Wirga <10386631+oskarwi...@users.noreply.github.com>
Date: Wed, 27 Sep 2023 10:38:01 -0700
Subject: [PATCH 2/2] Modify local-bounds sanitizer to improve debuggability
---
clang/test/CodeGen/bounds-checking.c | 3 +
.../Instrumentation/BoundsChecking.cpp| 25 --
.../BoundsChecking/ubsan-unique-traps.ll | 45 ++
.../MC/AArch64/local-bounds-single-trap.ll| 83 +++
4 files changed, 149 insertions(+), 7 deletions(-)
create mode 100644
llvm/test/Instrumentation/BoundsChecking/ubsan-u
[clang] Modify BoundsSan to improve debuggability (PR #65972)
https://github.com/oskarwirga resolved https://github.com/llvm/llvm-project/pull/65972 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Modify BoundsSan to improve debuggability (PR #65972)
oskarwirga wrote: @vitalybuka Thank you for reviewing! Can you merge this? I don't have write access (yet!) https://github.com/llvm/llvm-project/pull/65972 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Implement the 'counted_by' attribute (PR #68750)
https://github.com/oskarwirga commented: I've reviewed the admittedly limited sections I'm familiar with and LGTM! This is great work :) https://github.com/llvm/llvm-project/pull/68750 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [AArch64] Stack probing for dynamic allocas in GlobalISel (PR #67123)
https://github.com/oskarwirga approved this pull request. https://github.com/llvm/llvm-project/pull/67123 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [AArch64] Stack probing for dynamic allocas in GlobalISel (PR #67123)
oskarwirga wrote: I was able to confirm this stack works as expected in my local testing. I haven't uncovered any further interactions with other mitigations. Thank you for working on this! https://github.com/llvm/llvm-project/pull/67123 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Modify BoundsSan to improve debuggability (PR #65972)
https://github.com/oskarwirga created
https://github.com/llvm/llvm-project/pull/65972:
Context
BoundsSanitizer is a mitigation that is part of UBSAN. It can be enabled in
"trap" mode to crash on OOB array accesses.
Problem
BoundsSan has zero false positives meaning every crash is a OOB array access,
unfortunately optimizations cause these crashes in production builds to be a
bit useless because we only know which function is crashing but not which line
of code.
Godbolt example of the optimization: https://godbolt.org/z/6qjax9z1b
This Diff
I wanted to provide a way to know exactly which LOC is responsible for the
crash. What we do here is use the size of the basic block as an iterator to an
immediate value for the ubsan trap.
>From f04725992810cf5cec3153eff4e11d2be6f53ba8 Mon Sep 17 00:00:00 2001
From: Oskar Wirga <10386631+oskarwi...@users.noreply.github.com>
Date: Mon, 11 Sep 2023 16:00:36 +0100
Subject: [PATCH 1/2] Modify array-bounds sanitizer to improve debuggability
Context
BoundsSanitizer is a mitigation that is part of UBSAN. It can be enabled in
"trap" mode to crash on OOB array accesses.
Problem
BoundsSan has zero false positives meaning every crash is a OOB array access,
unfortunately optimizations cause these crashes in production builds to be a
bit useless because we only know which function is crashing but not which line
of code.
Godbolt example of the optimization: https://godbolt.org/z/6qjax9z1b
This Diff
I wanted to provide a way to know exactly which LOC is responsible for the
crash. What we do here is use the size of the basic block as an iterator to an
immediate value for the ubsan trap.
Continued from: https://reviews.llvm.org/D148654
---
clang/lib/CodeGen/CGExpr.cpp | 34 ++--
clang/test/CodeGen/bounds-checking.c | 12 ++
2 files changed, 34 insertions(+), 12 deletions(-)
diff --git a/clang/lib/CodeGen/CGExpr.cpp b/clang/lib/CodeGen/CGExpr.cpp
index 76bbeba468db643..9c9a831e6d47ee0 100644
--- a/clang/lib/CodeGen/CGExpr.cpp
+++ b/clang/lib/CodeGen/CGExpr.cpp
@@ -51,6 +51,12 @@
using namespace clang;
using namespace CodeGen;
+// Experiment to make sanitizers easier to debug
+static llvm::cl::opt ClSanitizeDebugDeoptimization(
+"ubsan-unique-traps", llvm::cl::Optional,
+llvm::cl::desc("Deoptimize traps for UBSAN so there is 1 trap per check"),
+llvm::cl::init(false));
+
//======//
//Miscellaneous Helper Methods
//======//
@@ -3553,17 +3559,28 @@ void CodeGenFunction::EmitTrapCheck(llvm::Value
*Checked,
// check-type per function to save on code size.
if (TrapBBs.size() <= CheckHandlerID)
TrapBBs.resize(CheckHandlerID + 1);
+
llvm::BasicBlock *&TrapBB = TrapBBs[CheckHandlerID];
- if (!CGM.getCodeGenOpts().OptimizationLevel || !TrapBB ||
- (CurCodeDecl && CurCodeDecl->hasAttr())) {
+ if (!ClSanitizeDebugDeoptimization &&
+ CGM.getCodeGenOpts().OptimizationLevel && TrapBB &&
+ (!CurCodeDecl || !CurCodeDecl->hasAttr())) {
+auto Call = TrapBB->begin();
+assert(isa(Call) && "Expected call in trap BB");
+
+Call->applyMergedLocation(Call->getDebugLoc(),
+ Builder.getCurrentDebugLocation());
+Builder.CreateCondBr(Checked, Cont, TrapBB);
+ } else {
TrapBB = createBasicBlock("trap");
Builder.CreateCondBr(Checked, Cont, TrapBB);
EmitBlock(TrapBB);
-llvm::CallInst *TrapCall =
-Builder.CreateCall(CGM.getIntrinsic(llvm::Intrinsic::ubsantrap),
- llvm::ConstantInt::get(CGM.Int8Ty, CheckHandlerID));
+llvm::CallInst *TrapCall = Builder.CreateCall(
+CGM.getIntrinsic(llvm::Intrinsic::ubsantrap),
+llvm::ConstantInt::get(CGM.Int8Ty, ClSanitizeDebugDeoptimization
+ ? TrapBB->getParent()->size()
+ : CheckHandlerID));
if (!CGM.getCodeGenOpts().TrapFuncName.empty()) {
auto A = llvm::Attribute::get(getLLVMContext(), "trap-func-name",
@@ -3573,13 +3590,6 @@ void CodeGenFunction::EmitTrapCheck(llvm::Value *Checked,
TrapCall->setDoesNotReturn();
TrapCall->setDoesNotThrow();
Builder.CreateUnreachable();
- } else {
-auto Call = TrapBB->begin();
-assert(isa(Call) && "Expected call in trap BB");
-
-Call->applyMergedLocation(Call->getDebugLoc(),
- Builder.getCurrentDebugLocation());
-Builder.CreateCondBr(Checked, Cont, TrapBB);
}
EmitBlock(Cont);
diff --git a/clang/test/CodeGen/bounds-checking.c
b/clang/test/CodeGen/bounds-checking.c
index 1b9e28915e5d9af..0e355721de976d7 100644
--- a/clang/test/CodeGen/bounds-checking.c
+++ b/clang/test/CodeGen/bounds-checking.c
@@ -1,5 +1,6 @@
// RUN: %clang_cc1 -fsanitize=local-bounds -emit-llvm -triple
x86_64-apple-darw
[clang] Modify BoundsSan to improve debuggability (PR #65972)
https://github.com/oskarwirga review_requested https://github.com/llvm/llvm-project/pull/65972 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Modify BoundsSan to improve debuggability (PR #65972)
https://github.com/oskarwirga review_requested https://github.com/llvm/llvm-project/pull/65972 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Modify BoundsSan to improve debuggability (PR #65972)
https://github.com/oskarwirga review_requested https://github.com/llvm/llvm-project/pull/65972 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Modify BoundsSan to improve debuggability (PR #65972)
oskarwirga wrote: CC: @vitalybuka I addressed some but not all comments pending further clarification :) https://github.com/llvm/llvm-project/pull/65972 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Modify BoundsSan to improve debuggability (PR #65972)
https://github.com/oskarwirga edited https://github.com/llvm/llvm-project/pull/65972 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [UBSAN] Preserve ubsan code with ubsan-unique-traps (PR #83470)
oskarwirga wrote: > I changed my design, so I don't need this patch. Given > https://godbolt.org/z/4KfEKq7zb, I can revert your patch, or just leave it as > is. I have no preference. I would prefer leaving it as is, I will make a note to revisit this pending further testing on my end to see how useful it really is. Thank you :) https://github.com/llvm/llvm-project/pull/83470 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [ubsan] Change ubsan-unique-traps to use nomerge instead of counter (PR #117651)
oskarwirga wrote: Sorry for the lack of review, I was sick but I just wanted to express my gratitude for fixing this hacky approach :) https://github.com/llvm/llvm-project/pull/117651 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [ubsan] Remove -ubsan-unique-traps (replace with -fno-sanitize-merge) (PR #120613)
https://github.com/oskarwirga approved this pull request. https://github.com/llvm/llvm-project/pull/120613 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [BoundsSan] Update BoundsChecking.cpp to use no-merge attribute where applicable (PR #120620)
https://github.com/oskarwirga approved this pull request. https://github.com/llvm/llvm-project/pull/120620 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [analysis] Software Bill of Mitigations (PR #130103)
@@ -0,0 +1,304 @@
+#include "llvm/Analysis/MitigationAnalysis.h"
+#include "llvm/IR/DebugInfo.h"
+#include "llvm/IR/DebugLoc.h"
+#include "llvm/IR/Function.h"
+#include "llvm/IR/InstIterator.h"
+#include "llvm/IR/Metadata.h"
+#include "llvm/Support/Debug.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/JSON.h"
+#include "llvm/Support/raw_ostream.h"
+#include
+#include
+
+using namespace llvm;
+
+AnalysisKey MitigationAnalysis::Key;
+
+// Add a command line flag for the module name
+static cl::opt
+ClOutputModuleName("mitigation-analysis-dso-name", cl::Optional,
+ cl::desc("DSO name for the module"),
+ cl::init("unknown"));
+
+enum class MitigationState { Ineligible, EligibleDisabled, EligibleEnabled };
+
+static const std::unordered_map mapStateToString
=
+{
+{MitigationState::Ineligible, "N/A"},
+{MitigationState::EligibleDisabled, "Disabled"},
+{MitigationState::EligibleEnabled, "Enabled"},
+};
+
+struct MitigationInfo {
+ MitigationState auto_var_init = MitigationState::Ineligible;
+ MitigationState cfi_icall = MitigationState::Ineligible;
+ MitigationState cfi_vcall = MitigationState::Ineligible;
+ MitigationState cfi_nvcall = MitigationState::Ineligible;
+ MitigationState stack_clash_protection = MitigationState::Ineligible;
+ MitigationState stack_protector = MitigationState::Ineligible;
+ MitigationState stack_protector_strong = MitigationState::Ineligible;
+ MitigationState stack_protector_all = MitigationState::Ineligible;
+ MitigationState libcpp_hardening_mode = MitigationState::Ineligible;
+ std::string source_mapping = "(unknown)";
+ std::string type_signature = "??";
+ uint64_t type_id = 0;
+ std::string function;
+ std::string gmodule;
+};
+
+/// Convert an integer value (0 or 1) to the appropriate MitigationState.
+static inline MitigationState valToState(int value) {
+ switch (value) {
+ case 0:
+return MitigationState::EligibleDisabled;
+ case 1:
+return MitigationState::EligibleEnabled;
+ default:
+return MitigationState::Ineligible;
+ }
+}
+
+/// Print out fields in MitigationInfo for debugging/verification purposes.
+#ifndef NDEBUG
+static void printInfo(const MitigationInfo &info) {
+ dbgs() << "module: " << info.gmodule << "\n";
+ dbgs() << "function: " << info.function << "\n";
+ dbgs() << "source_location: " << info.source_mapping << "\n";
+ dbgs() << "auto-var-init: " << mapStateToString.at(info.auto_var_init)
+ << "\n";
+ dbgs() << "cfi-icall: " << mapStateToString.at(info.cfi_icall) << "\n";
+ dbgs() << "cfi-vcall: " << mapStateToString.at(info.cfi_vcall) << "\n";
+ dbgs() << "cfi-nvcall: " << mapStateToString.at(info.cfi_nvcall) << "\n";
+ dbgs() << "stack-clash-protection: "
+ << mapStateToString.at(info.stack_clash_protection) << "\n";
+ dbgs() << "stack-protector: " << mapStateToString.at(info.stack_protector)
+ << "\n";
+ dbgs() << "stack-protector-strong: "
+ << mapStateToString.at(info.stack_protector_strong) << "\n";
+ dbgs() << "stack-protector-all: "
+ << mapStateToString.at(info.stack_protector_all) << "\n";
+ dbgs() << "libcpp-hardening-mode: "
+ << mapStateToString.at(info.libcpp_hardening_mode) << "\n";
+ dbgs() << "type_signature: " << info.type_signature << "\n";
+ dbgs() << "type_id: " << info.type_id << "\n\n";
+}
+#endif
+
+/// Convert a mitigation key + integer value into the appropriate field
+/// of MitigationInfo. This replaces a long chain of if/else statements.
+static void keyAndValueToInfo(MitigationInfo &info, StringRef key, int value) {
+ static constexpr struct {
+const StringRef Key;
+MitigationState MitigationInfo::*Field;
+ } Mappings[] = {
+ {StringRef("auto-var-init"), &MitigationInfo::auto_var_init},
+ {StringRef("cfi-icall"), &MitigationInfo::cfi_icall},
+ {StringRef("cfi-vcall"), &MitigationInfo::cfi_vcall},
+ {StringRef("cfi-nvcall"), &MitigationInfo::cfi_nvcall},
+ {StringRef("stack-clash-protection"),
+ &MitigationInfo::stack_clash_protection},
+ {StringRef("stack-protector"), &MitigationInfo::stack_protector},
+ {StringRef("stack-protector-strong"),
+ &MitigationInfo::stack_protector_strong},
+ {StringRef("stack-protector-all"), &MitigationInfo::stack_protector_all},
+ {StringRef("libcpp-hardening-mode"),
+ &MitigationInfo::libcpp_hardening_mode},
+ };
+
+ for (const auto &Mapping : Mappings) {
+if (key == Mapping.Key) {
+ info.*(Mapping.Field) = valToState(value);
+ break;
+}
+ }
+}
+
+/// Retrieve the first valid source path for the given function.
+static std::string getFunctionSourcePath(const Function &F) {
+ if (const DISubprogram *SP = F.getSubprogram()) {
+std::string Dir = SP->getDirectory().str();
+std::string File = SP->getFilename().str();
+unsigned Line = SP->getLine();
+if (!Dir.empty() && !File.empty())
+ r
[clang] [llvm] [analysis] Software Bill of Mitigations (PR #130103)
@@ -0,0 +1,304 @@
+#include "llvm/Analysis/MitigationAnalysis.h"
+#include "llvm/IR/DebugInfo.h"
+#include "llvm/IR/DebugLoc.h"
+#include "llvm/IR/Function.h"
+#include "llvm/IR/InstIterator.h"
+#include "llvm/IR/Metadata.h"
+#include "llvm/Support/Debug.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/JSON.h"
+#include "llvm/Support/raw_ostream.h"
+#include
+#include
+
+using namespace llvm;
+
+AnalysisKey MitigationAnalysis::Key;
+
+// Add a command line flag for the module name
+static cl::opt
+ClOutputModuleName("mitigation-analysis-dso-name", cl::Optional,
+ cl::desc("DSO name for the module"),
+ cl::init("unknown"));
oskarwirga wrote:
We pass in link unit name information through a flag, we weren't sure if there
was another way available.
https://github.com/llvm/llvm-project/pull/130103
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [analysis] Software Bill of Mitigations (PR #130103)
https://github.com/oskarwirga commented: Added some comments for other reviewers to highlight some of the tradeoffs we had to make + if they could be improved upon. CC: @devincoughlin https://github.com/llvm/llvm-project/pull/130103 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [analysis] Software Bill of Mitigations (PR #130103)
@@ -2512,6 +2516,28 @@ void CodeGenModule::SetLLVMFunctionAttributesForDefinition(const Decl *D, else if (isStackProtectorOn(LangOpts, getTriple(), LangOptions::SSPReq)) B.addAttribute(llvm::Attribute::StackProtectReq); + bool noStackProtectionAttr = D && D->hasAttr(); oskarwirga wrote: We should either refactor our approach to stack protectors or add comment explaining the caveat here that our mitigation analysis pass runs before the stack protectors are lowered so this is more of a heuristic rather than actual hard signal. https://github.com/llvm/llvm-project/pull/130103 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [analysis] Software Bill of Mitigations (PR #130103)
https://github.com/oskarwirga edited https://github.com/llvm/llvm-project/pull/130103 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [analysis] Software Bill of Mitigations (PR #130103)
@@ -2847,6 +2848,8 @@ void CodeGenFunction::EmitTypeMetadataCodeForVCall(const
CXXRecordDecl *RD,
Builder.CreateCall(CGM.getIntrinsic(IID), {VTable, TypeId});
Builder.CreateCall(CGM.getIntrinsic(llvm::Intrinsic::assume), TypeTest);
}
+
+ AttachMitigationMetadataToFunction(*this, MitigationKey::CFI_VCALL, false);
oskarwirga wrote:
This should be `AttachMitigationMetadataToFunction(*this,
MitigationKey::CFI_VCALL, SanOpts.has(SanitizerKind::CFIVCall))`
https://github.com/llvm/llvm-project/pull/130103
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
