Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DebugInfo] Correctly track metadata slots for DPValues #76941

Merged
merged 1 commit into from
Jan 5, 2024

Conversation

SLTozer
Copy link
Contributor

@SLTozer SLTozer commented Jan 4, 2024

Currently, the AsmWriter can print DPValues, but does not consider them when creating slots for metadata, which can result in erroneous output where metadata is numbered incorrectly. This patch modifies the ModuleSlotTracker to correctly track slots for metadata that appears in DPValues.

Currently, the AsmWriter can print DPValues, but does not consider them when
creating slots for metadata, which can result in erroneous output where
metadata is numbered incorrectly. This patch modifies the ModuleSlotTracker
to correctly track slots for metadata that appears in DPValues.
@llvmbot
Copy link
Collaborator

llvmbot commented Jan 4, 2024

@llvm/pr-subscribers-llvm-ir

@llvm/pr-subscribers-debuginfo

Author: Stephen Tozer (SLTozer)

Changes

Currently, the AsmWriter can print DPValues, but does not consider them when creating slots for metadata, which can result in erroneous output where metadata is numbered incorrectly. This patch modifies the ModuleSlotTracker to correctly track slots for metadata that appears in DPValues.


Full diff: https://github.com/llvm/llvm-project/pull/76941.diff

1 Files Affected:

  • (modified) llvm/lib/IR/AsmWriter.cpp (+12-1)
diff --git a/llvm/lib/IR/AsmWriter.cpp b/llvm/lib/IR/AsmWriter.cpp
index 95cdec722062e3..278cdfce411050 100644
--- a/llvm/lib/IR/AsmWriter.cpp
+++ b/llvm/lib/IR/AsmWriter.cpp
@@ -859,6 +859,9 @@ class SlotTracker : public AbstractSlotTrackerStorage {
 
   /// Add all of the metadata from an instruction.
   void processInstructionMetadata(const Instruction &I);
+
+  /// Add all of the metadata from an instruction.
+  void processDPValueMetadata(const DPValue &DPV);
 };
 
 } // end namespace llvm
@@ -1126,11 +1129,19 @@ void SlotTracker::processGlobalObjectMetadata(const GlobalObject &GO) {
 void SlotTracker::processFunctionMetadata(const Function &F) {
   processGlobalObjectMetadata(F);
   for (auto &BB : F) {
-    for (auto &I : BB)
+    for (auto &I : BB) {
+      for (const DPValue &DPV : I.getDbgValueRange())
+        processDPValueMetadata(DPV);
       processInstructionMetadata(I);
+    }
   }
 }
 
+void SlotTracker::processDPValueMetadata(const DPValue &DPV) {
+  CreateMetadataSlot(DPV.getVariable());
+  CreateMetadataSlot(DPV.getDebugLoc());
+}
+
 void SlotTracker::processInstructionMetadata(const Instruction &I) {
   // Process metadata used directly by intrinsics.
   if (const CallInst *CI = dyn_cast<CallInst>(&I))

@jmorse
Copy link
Member

jmorse commented Jan 5, 2024

IIRC there's a reproducer somewhere that stimulates this, would we be able to add that as a regression test? Otherwise LGTM.

Copy link
Member

@jmorse jmorse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, right, #76952 stimulates the change, I see. LGTM.

@SLTozer SLTozer merged commit a776740 into llvm:main Jan 5, 2024
5 of 6 checks passed
OCHyams added a commit that referenced this pull request Jan 5, 2024
The change is fairly mechanical:
1. Factor code from `FastISel::selectIntrinsicCall`, which converts
debug intrinsics into debug instructions, into functions (NFC).
2. Call those functions for DPValues attached to instructions too.

The test updates look the same as other RemoveDIs changes: re-run the
tests with `--try-experimental-debuginfo-iterators`, which checks the
output is identical using the new debug info format (if it has been
enabled in the cmake configuration).

Depends on #76941 (otherwise some modified tests spuriously fail).
OCHyams added a commit that referenced this pull request Jan 8, 2024
In line with other RemoveDIs test updates. This test fails without #76941.
justinfargnoli pushed a commit to justinfargnoli/llvm-project that referenced this pull request Jan 28, 2024
In line with other RemoveDIs test updates. This test fails without llvm#76941.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants