[Instrumentation] Mark instrumented calls as implicit #106447
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
|
@llvm/pr-subscribers-llvm-transforms @llvm/pr-subscribers-debuginfo Author: None (serge-sans-paille) ChangesMake sure that instrumentation code doesn't interferes with with prologue endpoint computation when generating debug information. Fix #54873 Full diff: https://github.com/llvm/llvm-project/pull/106447.diff 7 Files Affected:
diff --git a/llvm/include/llvm/Transforms/Instrumentation.h b/llvm/include/llvm/Transforms/Instrumentation.h
index 1a4824a806dc6e..7f7e107f41e776 100644
--- a/llvm/include/llvm/Transforms/Instrumentation.h
+++ b/llvm/include/llvm/Transforms/Instrumentation.h
@@ -194,15 +194,29 @@ static inline uint32_t scaleBranchCount(uint64_t Count, uint64_t Scale) {
// if the enclosing function does.
struct InstrumentationIRBuilder : IRBuilder<> {
static void ensureDebugInfo(IRBuilder<> &IRB, const Function &F) {
- if (IRB.getCurrentDebugLocation())
+ if (DebugLoc L = IRB.getCurrentDebugLocation()) {
+ if (!L.isImplicitCode()) {
+ L.setImplicitCode(true);
+ IRB.SetCurrentDebugLocation(L);
+ }
return;
- if (DISubprogram *SP = F.getSubprogram())
- IRB.SetCurrentDebugLocation(DILocation::get(SP->getContext(), 0, 0, SP));
+ }
+ if (DISubprogram *SP = F.getSubprogram()) {
+ DebugLoc L = DILocation::get(SP->getContext(), 0, 0, SP, nullptr,
+ /*ImplicitCode=*/true);
+ IRB.SetCurrentDebugLocation(L);
+ }
}
explicit InstrumentationIRBuilder(Instruction *IP) : IRBuilder<>(IP) {
ensureDebugInfo(*this, *IP->getFunction());
}
+
+ void SetCurrentImplicitDebugLocation(DebugLoc L) {
+ if (!L.isImplicitCode())
+ L.setImplicitCode(true);
+ SetCurrentDebugLocation(L);
+ }
};
} // end namespace llvm
diff --git a/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp b/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
index f111b4aea06f1b..457eab6ed2df33 100644
--- a/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
@@ -2145,14 +2145,16 @@ static std::pair<DebugLoc, bool> findPrologueEndLoc(const MachineFunction *MF) {
for (const auto &MBB : *MF) {
for (const auto &MI : MBB) {
if (!MI.isMetaInstruction()) {
- if (!MI.getFlag(MachineInstr::FrameSetup) && MI.getDebugLoc()) {
+ if (!MI.getFlag(MachineInstr::FrameSetup) &&
+ !MI.getDebugLoc().isImplicitCode()) {
// Scan forward to try to find a non-zero line number. The
// prologue_end marks the first breakpoint in the function after the
// frame setup, and a compiler-generated line 0 location is not a
// meaningful breakpoint. If none is found, return the first
// location after the frame setup.
- if (MI.getDebugLoc().getLine())
+ if (MI.getDebugLoc().getLine()) {
return std::make_pair(MI.getDebugLoc(), IsEmptyPrologue);
+ }
LineZeroLoc = MI.getDebugLoc();
}
diff --git a/llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp b/llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
index 6a89cee9aaf6cc..cfbc21b69faebe 100644
--- a/llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
+++ b/llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
@@ -958,7 +958,7 @@ void ModuleSanitizerCoverage::InjectCoverageAtBlock(Function &F, BasicBlock &BB,
InstrumentationIRBuilder IRB(&*IP);
if (EntryLoc)
- IRB.SetCurrentDebugLocation(EntryLoc);
+ IRB.SetCurrentImplicitDebugLocation(EntryLoc);
if (Options.TracePC) {
IRB.CreateCall(SanCovTracePC)
->setCannotMerge(); // gets the PC using GET_CALLER_PC.
diff --git a/llvm/test/Instrumentation/AddressSanitizer/missing_dbg.ll b/llvm/test/Instrumentation/AddressSanitizer/missing_dbg.ll
index f6553322fd6787..5bb1c01e9d1231 100644
--- a/llvm/test/Instrumentation/AddressSanitizer/missing_dbg.ll
+++ b/llvm/test/Instrumentation/AddressSanitizer/missing_dbg.ll
@@ -34,4 +34,4 @@ entry:
!4 = !DISubroutineType(types: !5)
!5 = !{}
-; CHECK: [[DBG]] = !DILocation(line: 0, scope: !4)
+; CHECK: [[DBG]] = !DILocation(line: 0, scope: !4, isImplicitCode: true)
diff --git a/llvm/test/Instrumentation/SanitizerCoverage/crit-edge-sancov.ll b/llvm/test/Instrumentation/SanitizerCoverage/crit-edge-sancov.ll
index f42fa7139fd585..cbab3ac9fdf302 100644
--- a/llvm/test/Instrumentation/SanitizerCoverage/crit-edge-sancov.ll
+++ b/llvm/test/Instrumentation/SanitizerCoverage/crit-edge-sancov.ll
@@ -21,9 +21,9 @@ define void @update_shadow(i1 %c) !dbg !3 {
; CHECK-NEXT: call void @__sanitizer_cov_trace_pc() #[[ATTR0]], !dbg [[DBG7:![0-9]+]]
; CHECK: if.end22.i:
; CHECK-NEXT: call void @__sanitizer_cov_trace_pc() #[[ATTR0]], !dbg [[DBG8:![0-9]+]]
-; CHECK: [[DBG6]] = !DILocation(line: 192, scope: !3)
-; CHECK: [[DBG7]] = !DILocation(line: 0, scope: !3)
-; CHECK: [[DBG8]] = !DILocation(line: 129, column: 2, scope: !3)
+; CHECK: [[DBG6]] = !DILocation(line: 192, scope: !3, isImplicitCode: true)
+; CHECK: [[DBG7]] = !DILocation(line: 0, scope: !3, isImplicitCode: true)
+; CHECK: [[DBG8]] = !DILocation(line: 129, column: 2, scope: !3, isImplicitCode: true)
entry:
br i1 %c, label %for.inc.i, label %if.end22.i
diff --git a/llvm/test/Instrumentation/SanitizerCoverage/missing_dbg.ll b/llvm/test/Instrumentation/SanitizerCoverage/missing_dbg.ll
index 21c6fcdb3a84b0..67af5663ceff64 100644
--- a/llvm/test/Instrumentation/SanitizerCoverage/missing_dbg.ll
+++ b/llvm/test/Instrumentation/SanitizerCoverage/missing_dbg.ll
@@ -46,5 +46,5 @@ entry:
!6 = !DILocation(line: 192, scope: !3)
!7 = !DILocation(line: 0, scope: !3)
-; CHECK: [[DBG1]] = !DILocation(line: 192, scope: !3)
-; CHECK: [[DBG2]] = !DILocation(line: 0, scope: !3)
+; CHECK: [[DBG1]] = !DILocation(line: 192, scope: !3, isImplicitCode: true)
+; CHECK: [[DBG2]] = !DILocation(line: 0, scope: !3, isImplicitCode: true)
diff --git a/llvm/test/Instrumentation/ThreadSanitizer/missing_dbg.ll b/llvm/test/Instrumentation/ThreadSanitizer/missing_dbg.ll
index a9d6bcd5191487..a5432fa97e2b96 100644
--- a/llvm/test/Instrumentation/ThreadSanitizer/missing_dbg.ll
+++ b/llvm/test/Instrumentation/ThreadSanitizer/missing_dbg.ll
@@ -51,5 +51,5 @@ entry:
!5 = !{}
!7 = distinct !DISubprogram(name: "foo", scope: !1, file: !1, line: 195, type: !4, scopeLine: 199, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagLocalToUnit | DISPFlagDefinition | DISPFlagOptimized, unit: !0)
-; CHECK: [[DBG1]] = !DILocation(line: 0, scope: !3)
-; CHECK: [[DBG2]] = !DILocation(line: 0, scope: !7)
+; CHECK: [[DBG1]] = !DILocation(line: 0, scope: !3, isImplicitCode: true)
+; CHECK: [[DBG2]] = !DILocation(line: 0, scope: !7, isImplicitCode: true)
|
jmorse
reviewed
Sep 4, 2024
There was a problem hiding this comment.
Hola; this looks like a good fix, however I wonder whether existing facilities for implicit code would be sufficient to achieve this. Nowadays, by convention, a DILocation with a line-number of zero is interpreted as being a compiler-generated source location and is ignored by various pieces of LLVM (and DWARF consumers too). Would using that achieve your aims instead?
Specifically, I gave this a quick spin:
diff --git a/llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp b/llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
index 6a89cee9aaf6..48d00cfb9671 100644
--- a/llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
+++ b/llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
@@ -949,7 +949,7 @@ void ModuleSanitizerCoverage::InjectCoverageAtBlock(Function &F, BasicBlock &BB,
DebugLoc EntryLoc;
if (IsEntryBB) {
if (auto SP = F.getSubprogram())
- EntryLoc = DILocation::get(SP->getContext(), SP->getScopeLine(), 0, SP);
+ EntryLoc = DILocation::get(SP->getContext(), 0, 0, SP);
// Keep static allocas and llvm.localescape calls in the entry block. Even
// if we aren't splitting the block, it's nice for allocas to be before
// calls.
and the reproducer from the linked ticket/issue shifted the prologue_end flag back to the desired position. As a side-effect the instrumentation code then can't be stepped on (a feature, in my view).
Disclosure: I'd prefer to avoid more users of isImplicitCode, as it'd be quite nice to repurpose it in the future, and it's little-used right now.
|
The change dates back to 201733b, and I agree with the simplicity of your approach, let me update the diff. |
…uginfo Doing so makes sure instrumentation code doesn't interferes with with prologue endpoint computation when generating debug information, as prologue_endpoint computation skips over such instructions. Fix llvm#54873
serge-sans-paille
force-pushed
the
feature/debuginfo
branch
from
0060c7d
to
de017e6
Compare
|
@jmorse : updated! |
jmorse
approved these changes
Sep 5, 2024
Comment on lines
17
to
18
| ; Test that __sanitizer_cov_trace_pc_guard call has !dbg pointing to the opening { of A::f(). | ||
| ; CHECK: call void @__sanitizer_cov_trace_pc_guard(ptr{{.*}}) #{{.*}}, !dbg [[A:!.*]] |
There was a problem hiding this comment.
This comment will want updating as it's now not true I guess; it's unclear to me whether this indicates there are expectations that the coverage call gets a "real" source location?
There was a problem hiding this comment.
The intent of the original commit (493df13) is unclear :-/
| @@ -949,7 +949,7 @@ void ModuleSanitizerCoverage::InjectCoverageAtBlock(Function &F, BasicBlock &BB, | |||
| DebugLoc EntryLoc; | |||
| if (IsEntryBB) { | |||
| if (auto SP = F.getSubprogram()) | |||
| EntryLoc = DILocation::get(SP->getContext(), SP->getScopeLine(), 0, SP); | |||
| EntryLoc = DILocation::get(SP->getContext(), 0, 0, SP); | |||
There was a problem hiding this comment.
IMHO this warrants a comment along the lines of "Use a compile-generated source-location for all coverage calls", to aid future readers
|
This needs to be tested against actual users of this API before it's merged. The example given in the docs (https://clang.llvm.org/docs/SanitizerCoverage.html) extern "C" void __sanitizer_cov_trace_pc_guard(uint32_t *guard) {
if (!*guard) return; // Duplicate the guard check.
// If you set *guard to 0 this code will not be called again for this edge.
// Now you can get the PC and do whatever you want:
// store it somewhere or symbolize it and print right away.
// The values of `*guard` are as you set them in
// __sanitizer_cov_trace_pc_guard_init and so you can make them consecutive
// and use them to dereference an array or a bit vector.
void *PC = __builtin_return_address(0);
char PcDescr[1024];
// This function is a part of the sanitizer run-time.
// To use it, link with AddressSanitizer or other sanitizer.
__sanitizer_symbolize_pc(PC, "%p %F %L", PcDescr, sizeof(PcDescr));
printf("guard: %p %x PC %s\n", guard, *guard, PcDescr);
}Seems very likely to break (__sanitizer_symbolize_pc will end up finding the line 0 location with the instrumentation sequence in #54873). |
I wondering if https://clang.llvm.org/docs/SanitizerCoverage.html#id16 will still work? |
vitalybuka
requested changes
Oct 8, 2024
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.
Make sure that instrumentation code doesn't interferes with with prologue endpoint computation when generating debug information.
Fix #54873