[LLVM-C] Add bindings to `Instruction::getDbgRecordRange()` #107802

vadorovsky · 2024-09-09T03:29:34Z

Since the migration from @llvm.dbg.value intrinsic to #dbg_value records, there is no way to retrieve the debug records for an Instruction in LLVM-C API.

Previously, with debug info intrinsics, retrieving debug info for an Instruction could be done with LLVMGetNextInstructions, because the intrinsic call was also an instruction.

However, to be able to retrieve debug info with the current LLVM, where debug records are used, the getDbgRecordRange() iterator needs to be exposed.

llvmbot · 2024-09-09T03:30:09Z

@llvm/pr-subscribers-debuginfo

@llvm/pr-subscribers-llvm-ir

Author: Michal Rostecki (vadorovsky)

Changes

Since the migration from @llvm.dbg.value intrinsic to #dbg_value records, there is no way to retrieve the debug records for an Instruction in LLVM-C API.

Previously, with debug info intrinsics, retrieving debug info for an Instruction could be done with LLVMGetNextInstructions, because the intrinsic call was also an instruction.

However, to be able to retrieve debug info with the current LLVM, where debug records are used, the getDbgRecordRange() iterator needs to be exposed.

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

3 Files Affected:

(modified) llvm/include/llvm-c/Core.h (+27)
(modified) llvm/lib/IR/Core.cpp (+34)
(modified) llvm/tools/llvm-c-test/debuginfo.c (+28-3)

diff --git a/llvm/include/llvm-c/Core.h b/llvm/include/llvm-c/Core.h
index a0786efb51fdb2..a14c5832396e7b 100644
--- a/llvm/include/llvm-c/Core.h
+++ b/llvm/include/llvm-c/Core.h
@@ -3682,6 +3682,33 @@ LLVMValueRef LLVMInstructionClone(LLVMValueRef Inst);
  */
 LLVMValueRef LLVMIsATerminatorInst(LLVMValueRef Inst);
 
+/**
+ * Obtain the first debug record in a instruction.
+ *
+ * The returned debug record can be used ad an iterator. You will likely
+ * eventually call into LLVMGetNextDbgRecord() with it.
+ *
+ * @see llvm::Instruction::getDbgRecordRange()
+ */
+LLVMDbgRecordRef LLVMGetFirstDbgRecord(LLVMValueRef Inst);
+
+/**
+ * Obtain the last debug record in a instruction.
+ *
+ * @see llvm::Instruction::getDbgRecordRange()
+ */
+LLVMDbgRecordRef LLVMGetLastDbgRecord(LLVMValueRef Inst);
+
+/**
+ * Advance a debug record iterator.
+ */
+LLVMDbgRecordRef LLVMGetNextDbgRecord(LLVMDbgRecordRef DbgRecord);
+
+/**
+ * Go backwards in a debug record iterator.
+ */
+LLVMDbgRecordRef LLVMGetPreviousDbgRecord(LLVMDbgRecordRef DbgRecord);
+
 /**
  * @defgroup LLVMCCoreValueInstructionCall Call Sites and Invocations
  *
diff --git a/llvm/lib/IR/Core.cpp b/llvm/lib/IR/Core.cpp
index 28f603b2c38c8a..32a04c59d4d260 100644
--- a/llvm/lib/IR/Core.cpp
+++ b/llvm/lib/IR/Core.cpp
@@ -12,11 +12,13 @@
 //===----------------------------------------------------------------------===//
 
 #include "llvm-c/Core.h"
+#include "llvm-c/Types.h"
 #include "llvm/IR/Attributes.h"
 #include "llvm/IR/BasicBlock.h"
 #include "llvm/IR/ConstantRange.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/DebugInfoMetadata.h"
+#include "llvm/IR/DebugProgramInstruction.h"
 #include "llvm/IR/DerivedTypes.h"
 #include "llvm/IR/DiagnosticInfo.h"
 #include "llvm/IR/DiagnosticPrinter.h"
@@ -2975,6 +2977,38 @@ LLVMValueRef LLVMIsATerminatorInst(LLVMValueRef Inst) {
   return (I && I->isTerminator()) ? wrap(I) : nullptr;
 }
 
+LLVMDbgRecordRef LLVMGetFirstDbgRecord(LLVMValueRef Inst) {
+  Instruction *Instr = unwrap<Instruction>(Inst);
+  auto I = Instr->DebugMarker->StoredDbgRecords.begin();
+  if (I == Instr->DebugMarker->StoredDbgRecords.end())
+    return nullptr;
+  return wrap(&*I);
+}
+
+LLVMDbgRecordRef LLVMGetLastDbgRecord(LLVMValueRef Inst) {
+  Instruction *Instr = unwrap<Instruction>(Inst);
+  auto I = Instr->DebugMarker->StoredDbgRecords.rbegin();
+  if (I == Instr->DebugMarker->StoredDbgRecords.rend())
+    return nullptr;
+  return wrap(&*I);
+}
+
+LLVMDbgRecordRef LLVMGetNextDbgRecord(LLVMDbgRecordRef Rec) {
+  DbgRecord *Record = unwrap<DbgRecord>(Rec);
+  simple_ilist<DbgRecord>::iterator I(Record);
+  if (++I == Record->getInstruction()->DebugMarker->StoredDbgRecords.end())
+    return nullptr;
+  return wrap(&*I);
+}
+
+LLVMDbgRecordRef LLVMGetPreviousDbgRecord(LLVMDbgRecordRef Rec) {
+  DbgRecord *Record = unwrap<DbgRecord>(Rec);
+  simple_ilist<DbgRecord>::iterator I(Record);
+  if (I == Record->getInstruction()->DebugMarker->StoredDbgRecords.begin())
+    return nullptr;
+  return wrap(&*--I);
+}
+
 unsigned LLVMGetNumArgOperands(LLVMValueRef Instr) {
   if (FuncletPadInst *FPI = dyn_cast<FuncletPadInst>(unwrap(Instr))) {
     return FPI->arg_size();
diff --git a/llvm/tools/llvm-c-test/debuginfo.c b/llvm/tools/llvm-c-test/debuginfo.c
index 49c90f5b87b83a..f068da1d0a63d9 100644
--- a/llvm/tools/llvm-c-test/debuginfo.c
+++ b/llvm/tools/llvm-c-test/debuginfo.c
@@ -12,6 +12,8 @@
 \*===----------------------------------------------------------------------===*/
 
 #include "llvm-c-test.h"
+#include "llvm-c/Core.h"
+#include "llvm-c/Types.h"
 #include "llvm-c/DebugInfo.h"
 
 #include <assert.h>
@@ -172,10 +174,20 @@ int llvm_test_dibuilder(void) {
     LLVMDIBuilderCreateAutoVariable(DIB, FooLexicalBlock, "d", 1, File,
                                     43, Int64Ty, true, 0, 0);
   LLVMValueRef FooVal1 = LLVMConstInt(LLVMInt64Type(), 0, false);
-  LLVMMetadataRef FooVarValueExpr =
+  LLVMMetadataRef FooVarValueExpr1 =
     LLVMDIBuilderCreateConstantValueExpression(DIB, 0);
 
-  LLVMDIBuilderInsertDbgValueRecordAtEnd(DIB, FooVal1, FooVar1, FooVarValueExpr,
+  LLVMDIBuilderInsertDbgValueRecordAtEnd(DIB, FooVal1, FooVar1, FooVarValueExpr1,
+                                         FooVarsLocation, FooVarBlock);
+
+  LLVMMetadataRef FooVar2 =
+    LLVMDIBuilderCreateAutoVariable(DIB, FooLexicalBlock, "e", 1, File,
+                                    44, Int64Ty, true, 0, 0);
+  LLVMValueRef FooVal2 = LLVMConstInt(LLVMInt64Type(), 1, false);
+  LLVMMetadataRef FooVarValueExpr2 =
+    LLVMDIBuilderCreateConstantValueExpression(DIB, 1);
+
+  LLVMDIBuilderInsertDbgValueRecordAtEnd(DIB, FooVal2, FooVar2, FooVarValueExpr2,
                                          FooVarsLocation, FooVarBlock);
 
   LLVMMetadataRef MacroFile =
@@ -224,14 +236,27 @@ int llvm_test_dibuilder(void) {
   LLVMPositionBuilderBeforeInstrAndDbgRecords(Builder, InsertPos);
   LLVMValueRef Phi1 = LLVMBuildPhi(Builder, I64, "p1");
   LLVMAddIncoming(Phi1, &Zero, &FooEntryBlock, 1);
+
   // Do the same again using the other position-setting function.
   LLVMPositionBuilderBeforeDbgRecords(Builder, FooVarBlock, InsertPos);
   LLVMValueRef Phi2 = LLVMBuildPhi(Builder, I64, "p2");
   LLVMAddIncoming(Phi2, &Zero, &FooEntryBlock, 1);
+
   // Insert a non-phi before the `ret` but not before the debug records to
   // test that works as expected.
   LLVMPositionBuilder(Builder, FooVarBlock, Ret);
-  LLVMBuildAdd(Builder, Phi1, Phi2, "a");
+  LLVMValueRef Add = LLVMBuildAdd(Builder, Phi1, Phi2, "a");
+
+  // Iterate over debug records in the add instruction. There should be two.
+  LLVMDbgRecordRef AddDbgRecordFirst = LLVMGetFirstDbgRecord(Add);
+  assert(AddDbgRecordFirst != NULL);
+  LLVMDbgRecordRef AddDbgRecordSecond = LLVMGetNextDbgRecord(AddDbgRecordFirst);
+  assert(AddDbgRecordSecond != NULL);
+  LLVMDbgRecordRef AddDbgRecordLast = LLVMGetLastDbgRecord(Add);
+  assert(AddDbgRecordLast != NULL);
+  assert(AddDbgRecordSecond == AddDbgRecordLast);
+  LLVMDbgRecordRef AddDbgRecordOverTheRange = LLVMGetNextDbgRecord(AddDbgRecordSecond);
+  assert(AddDbgRecordOverTheRange == NULL);
 
   char *MStr = LLVMPrintModuleToString(M);
   puts(MStr);

github-actions · 2024-09-09T03:33:00Z

✅ With the latest revision this PR passed the C/C++ code formatter.

OCHyams

Code changes LGTM, thanks for adding this.

Please could you update the release notes llvm\docs\ReleaseNotes.rst ("Changes to the C API" section) and the migration doc llvm\docs\RemoveDIsDebugInfo.md ("# C-API changes" section)?

weliveindetail

Thanks! LGTM, just 2 minor notes: typo in the comment below and the test doesn't exerciseLLVMGetPreviousDbgRecord()

llvm/include/llvm-c/Core.h

vadorovsky · 2024-09-09T12:34:39Z

@OCHyams updated both doc files.

@weliveindetail Good catch. I also added test cases with for loops now.

Thanks for review!

OCHyams

Thanks for the update - just a couple of extra comments from me.

OCHyams · 2024-09-09T13:05:56Z

llvm/docs/ReleaseNotes.rst

+* The following functions are added to allow iterating over debug records in
+  instructions:


IMO: "in" -> "on" or "attached to".

I like "attached to". Will apply in the docstring in Core.h as well.

OCHyams · 2024-09-09T13:13:46Z

llvm/tools/llvm-c-test/debuginfo.c

+  LLVMDbgRecordRef DbgRec;
+  for (DbgRec = LLVMGetFirstDbgRecord(Add); DbgRec;
+       DbgRec = LLVMGetNextDbgRecord(DbgRec)) {
+    assert(DbgRec != NULL);


hmm given the for-loop's condition is that DbgRec is non-null, this assert feels redundant. I think all the functions used in these loops are used in the "manual iteration" below, so we can probably just cut these two loops from the test?

Not sure if I'm looking at an old diff or something but I still see the for loops

Now should be pushed, sorry

OCHyams · 2024-09-09T13:15:53Z

llvm/docs/RemoveDIsDebugInfo.md

@@ -172,6 +172,24 @@ If you are trying to insert at the start of a block, or purposfully skip debug i

 `LLVMPositionBuilder` and `LLVMPositionBuilderBefore` are unchanged. They insert before the indicated instruction but after any attached debug records.

+`LLVMGetFirstDbgRecord`, `LLVMGetLastDbgRecord`, `LLVMGetNextDbgRecord` and `LLVMGetPreviousDbgRecord` can be used for iterating over debug records in instructions (provided as `LLVMValueRef`).


Sorry this comment got lost somehow.

Same comment regarding "in" applies here.

Please could you add entries in to the New functions (no plans to deprecate) section (15 or so lines above this) in addition to this part.

Otherwise the docs updates look good, thank you

OCHyams

Thanks. Once my final nits (sorry) have been addressed I think this LGTM.

I'll let @weliveindetail Accept when they're happy with the patch too.

OCHyams · 2024-09-09T15:49:47Z

llvm/docs/RemoveDIsDebugInfo.md

+LLVMGetFirstDbgRecord                    # Obtain the first debug record attached to a instruction.
+LLVMGetLastDbgRecord                     # Obtain the last debug record attached to a instruction.


nit: a -> an

OCHyams · 2024-09-09T15:51:51Z

llvm/tools/llvm-c-test/debuginfo.c

+  LLVMDbgRecordRef DbgRec;
+  for (DbgRec = LLVMGetFirstDbgRecord(Add); DbgRec;
+       DbgRec = LLVMGetNextDbgRecord(DbgRec)) {
+    assert(DbgRec != NULL);


Not sure if I'm looking at an old diff or something but I still see the for loops

OCHyams

I think @weliveindetail's comments have been addressed, so I'll accept this after these final nit-picks (sorry that I missed them in the last round). Thanks!

OCHyams · 2024-09-13T08:50:14Z

llvm/tools/llvm-c-test/debuginfo.c

+      LLVMDIBuilderCreateConstantValueExpression(DIB, 1);
+
+  LLVMDIBuilderInsertDbgValueRecordAtEnd(
+      DIB, FooVal2, FooVar2, FooVarValueExpr2, FooVarsLocation, FooVarBlock);


Does llvm\test\Bindings\llvm-c\debug_info.ll not need any updates with this change?

It needed an update indeed, should be fixed now. That was most likely the reason behind red CI builds.

OCHyams · 2024-09-13T08:53:40Z

llvm/include/llvm-c/Core.h

+ * The returned debug record can be used as an iterator. You will likely
+ * eventually call into LLVMGetNextDbgRecord() with it.


We should mention these return null. Something like: "Returns the first DbgRecord attached to Inst or NULL if there are none [...]".

Same thing applies to the other new functions ones below.

Done.

I also changed "Returns" to "Return", to make it consistent with the other sentences, which are imperative.

OCHyams · 2024-09-13T08:55:44Z

llvm/include/llvm-c/Core.h

+LLVMDbgRecordRef LLVMGetNextDbgRecord(LLVMDbgRecordRef DbgRecord);
+
+/**
+ * Go backwards in a debug record iterator.


Since there are already other changes to make here, I'll raise the nit: "Go backwards using" reads better than "Go backwards in" imo. Or rewording it to something like "Get the next DbgRecord or NULL."

Done, I reworded it to "Obtain the next DbgRecord..."

OCHyams

Because these comments are effectively documentation I've been really nit-picky and added some further suggestions (you should be able to merge these with the web UI).

Other than those bits this LGTM (so I'll accept now), thanks for being patient with this slightly protracted review.

llvm/include/llvm-c/Core.h

llvm/docs/RemoveDIsDebugInfo.md

Since the migration from `@llvm.dbg.value` intrinsic to `#dbg_value` records, there is no way to retrieve the debug records for an `Instruction` in LLVM-C API. Previously, with debug info intrinsics, retrieving debug info for an `Instruction` could be done with `LLVMGetNextInstructions`, because the intrinsic call was also an instruction. However, to be able to retrieve debug info with the current LLVM, where debug records are used, the `getDbgRecordRange()` iterator needs to be exposed.

vadorovsky · 2024-09-18T15:39:40Z

@OCHyams Done.

I don't mind these nitpicks, but I just wanted to mention that many of the nitpicked sentences, like:

"Advance a [...] iterator"
"Go backwards in [...] iterator"
"Obtain the next [...] in [...]"

are not my invention and were already used on other docstrings (e.g. LLVMGet*BasicBlock). I think it would be great to adjust these now too, to avoid inconsistency. Not sure if I should submit a (separate) PR or you would prefer to do that? I don't mind either way.

OCHyams · 2024-09-19T08:40:00Z

@OCHyams Done.

I don't mind these nitpicks, but I just wanted to mention that many of the nitpicked sentences, like:

"Advance a [...] iterator"

"Go backwards in [...] iterator"

"Obtain the next [...] in [...]"

are not my invention and were already used on other docstrings (e.g. LLVMGet*BasicBlock). I think it would be great to adjust these now too, to avoid inconsistency. Not sure if I should submit a (separate) PR or you would prefer to do that? I don't mind either way.

Oh right, I hadn't noticed that, thanks for raising this point. If you're happy to submit another PR for that please do (others may have opinions about whether or not its worth changing the wording of those, but fwiw it seems good to me if we can make things consistent and improve clarity)

Do you have commit access? If not I can merge this one for you.

vadorovsky · 2024-09-20T13:26:21Z

Do you have commit access? If not I can merge this one for you.

I don't, so I would be thankful if you can merge it.

- Update `llvm-sys` from `191.0.0-rc1` to `191.0.0`. - Update LLVM to the `rustc/19.1-2024-09-17` branch, which is based on the stable LLVM 19.1.0 release. - The mentioned branch in our LLVM fork contains also a backport of llvm/llvm-project#107802, which is needed for our work on BTF relocations.

- Update `llvm-sys` from `191.0.0-rc1` to `191.0.0`. - Update LLVM to the `rustc/19.1-2024-09-17` branch, which is based on the stable LLVM 19.1.0 release. - The mentioned branch in our LLVM fork contains a backport of llvm/llvm-project#107802, which is needed for our work on BTF relocations.

) Since the migration from `@llvm.dbg.value` intrinsic to `#dbg_value` records, there is no way to retrieve the debug records for an `Instruction` in LLVM-C API. Previously, with debug info intrinsics, retrieving debug info for an `Instruction` could be done with `LLVMGetNextInstructions`, because the intrinsic call was also an instruction. However, to be able to retrieve debug info with the current LLVM, where debug records are used, the `getDbgRecordRange()` iterator needs to be exposed. Add new functions for DbgRecord sequence traversal: LLVMGetFirstDbgRecord LLVMGetLastDbgRecord LLVMGetNextDbgRecord LLVMGetPreviousDbgRecord See llvm/docs/RemoveDIsDebugInfo.md and release notes.

vadorovsky requested a review from nikic as a code owner September 9, 2024 03:29

llvmbot added the llvm:ir label Sep 9, 2024

vadorovsky force-pushed the llvm-c-dbg-record-iter branch 3 times, most recently from 881c490 to c44d0fd Compare September 9, 2024 04:00

nikic added the debuginfo label Sep 9, 2024

nikic requested review from jmorse and OCHyams September 9, 2024 07:00

OCHyams reviewed Sep 9, 2024

View reviewed changes

weliveindetail reviewed Sep 9, 2024

View reviewed changes

llvm/include/llvm-c/Core.h Outdated Show resolved Hide resolved

vadorovsky force-pushed the llvm-c-dbg-record-iter branch from c44d0fd to bde6a1f Compare September 9, 2024 12:13

vadorovsky force-pushed the llvm-c-dbg-record-iter branch from bde6a1f to 5ba7f28 Compare September 9, 2024 12:35

OCHyams reviewed Sep 9, 2024

View reviewed changes

vadorovsky force-pushed the llvm-c-dbg-record-iter branch 2 times, most recently from 8d6bc84 to 6779ad3 Compare September 9, 2024 14:24

OCHyams reviewed Sep 9, 2024

View reviewed changes

vadorovsky force-pushed the llvm-c-dbg-record-iter branch 2 times, most recently from 14f5bd0 to d201603 Compare September 10, 2024 14:58

vadorovsky mentioned this pull request Sep 11, 2024

Add BTF relocation support in eBPF programs aya-rs/aya#349

Open

vadorovsky force-pushed the llvm-c-dbg-record-iter branch from d201603 to 2794bb9 Compare September 13, 2024 06:35

OCHyams reviewed Sep 13, 2024

View reviewed changes

vadorovsky force-pushed the llvm-c-dbg-record-iter branch from 2794bb9 to 9412984 Compare September 15, 2024 16:44

OCHyams approved these changes Sep 17, 2024

View reviewed changes

vadorovsky force-pushed the llvm-c-dbg-record-iter branch from 9412984 to b12c5b3 Compare September 18, 2024 15:29

OCHyams merged commit c320df4 into llvm:main Sep 20, 2024
9 checks passed

vadorovsky deleted the llvm-c-dbg-record-iter branch September 20, 2024 13:37

vadorovsky mentioned this pull request Sep 27, 2024

chore: Update from rc release of LLVM and llvm-sys to the stable one aya-rs/bpf-linker#231

Merged

		* The following functions are added to allow iterating over debug records in
		instructions:

		@@ -172,6 +172,24 @@ If you are trying to insert at the start of a block, or purposfully skip debug i

		`LLVMPositionBuilder` and `LLVMPositionBuilderBefore` are unchanged. They insert before the indicated instruction but after any attached debug records.

		`LLVMGetFirstDbgRecord`, `LLVMGetLastDbgRecord`, `LLVMGetNextDbgRecord` and `LLVMGetPreviousDbgRecord` can be used for iterating over debug records in instructions (provided as `LLVMValueRef`).

		LLVMGetFirstDbgRecord # Obtain the first debug record attached to a instruction.
		LLVMGetLastDbgRecord # Obtain the last debug record attached to a instruction.

		* The returned debug record can be used as an iterator. You will likely
		* eventually call into LLVMGetNextDbgRecord() with it.

[LLVM-C] Add bindings to Instruction::getDbgRecordRange() #107802

[LLVM-C] Add bindings to Instruction::getDbgRecordRange() #107802

Uh oh!

Conversation

vadorovsky commented Sep 9, 2024

Uh oh!

llvmbot commented Sep 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Sep 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

OCHyams left a comment

Choose a reason for hiding this comment

Uh oh!

weliveindetail left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vadorovsky commented Sep 9, 2024

Uh oh!

OCHyams left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vadorovsky Sep 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OCHyams left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OCHyams left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OCHyams left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vadorovsky commented Sep 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

OCHyams commented Sep 19, 2024

Uh oh!

[LLVM-C] Add bindings to `Instruction::getDbgRecordRange()` #107802

[LLVM-C] Add bindings to `Instruction::getDbgRecordRange()` #107802

llvmbot commented Sep 9, 2024 •

edited

Loading

github-actions bot commented Sep 9, 2024 •

edited

Loading

vadorovsky Sep 9, 2024 •

edited

Loading

OCHyams left a comment •

edited

Loading

vadorovsky commented Sep 18, 2024 •

edited

Loading