[RemoveDIs] Update DIBuilder C API with DbgRecord functions. #95535
Conversation
llvm#84915 llvm#85657 llvm#92417 (comment) As discussed by @nikic and @OCHyams: llvm#92417 (comment) > For the intrinsic-inserting functions, do you think we should: > > a) mark as deprecated but otherwise leave them alone for now. > b) mark as deprecated and change their behaviour so that they > do nothing and return nullptr. > c) outright delete them (it sounds like you're voting this one, > but just wanted to double check). This patch implements option (c).
CarlosAlbertoEnciso
requested review from
nikic,
vaivaswatha,
OCHyams,
alan-j-hu and
pogo59
|
@llvm/pr-subscribers-debuginfo @llvm/pr-subscribers-llvm-ir Author: Carlos Alberto Enciso (CarlosAlbertoEnciso) ChangesThe DIBuilder C API was changed to deal with DbgRecord functions: #84915 As discussed by @nikic and @OCHyams: > For the intrinsic-inserting functions, do you think we should: This patch implements option (c). Patch is 37.60 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/95535.diff 12 Files Affected:
diff --git a/llvm/bindings/ocaml/debuginfo/debuginfo_ocaml.c b/llvm/bindings/ocaml/debuginfo/debuginfo_ocaml.c
index fbe45c0c1e0b0..fd99724ca0559 100644
--- a/llvm/bindings/ocaml/debuginfo/debuginfo_ocaml.c
+++ b/llvm/bindings/ocaml/debuginfo/debuginfo_ocaml.c
@@ -969,45 +969,6 @@ value llvm_dibuild_create_parameter_variable_bytecode(value *argv, int arg) {
);
}
-value llvm_dibuild_insert_declare_before_native(value Builder, value Storage,
- value VarInfo, value Expr,
- value DebugLoc, value Instr) {
- LLVMDbgRecordRef Value = LLVMDIBuilderInsertDeclareBefore(
- DIBuilder_val(Builder), Value_val(Storage), Metadata_val(VarInfo),
- Metadata_val(Expr), Metadata_val(DebugLoc), Value_val(Instr));
- return to_val(Value);
-}
-
-value llvm_dibuild_insert_declare_before_bytecode(value *argv, int arg) {
-
- return llvm_dibuild_insert_declare_before_native(argv[0], // Builder
- argv[1], // Storage
- argv[2], // VarInfo
- argv[3], // Expr
- argv[4], // DebugLoc
- argv[5] // Instr
- );
-}
-
-value llvm_dibuild_insert_declare_at_end_native(value Builder, value Storage,
- value VarInfo, value Expr,
- value DebugLoc, value Block) {
- LLVMDbgRecordRef Value = LLVMDIBuilderInsertDeclareAtEnd(
- DIBuilder_val(Builder), Value_val(Storage), Metadata_val(VarInfo),
- Metadata_val(Expr), Metadata_val(DebugLoc), BasicBlock_val(Block));
- return to_val(Value);
-}
-
-value llvm_dibuild_insert_declare_at_end_bytecode(value *argv, int arg) {
- return llvm_dibuild_insert_declare_at_end_native(argv[0], // Builder
- argv[1], // Storage
- argv[2], // VarInfo
- argv[3], // Expr
- argv[4], // DebugLoc
- argv[5] // Block
- );
-}
-
value llvm_dibuild_expression(value Builder, value Addr) {
return to_val(LLVMDIBuilderCreateExpression(
DIBuilder_val(Builder), (uint64_t *)Op_val(Addr), Wosize_val(Addr)));
diff --git a/llvm/bindings/ocaml/debuginfo/llvm_debuginfo.ml b/llvm/bindings/ocaml/debuginfo/llvm_debuginfo.ml
index 8bb5edb17a2c9..3098b28120cc0 100644
--- a/llvm/bindings/ocaml/debuginfo/llvm_debuginfo.ml
+++ b/llvm/bindings/ocaml/debuginfo/llvm_debuginfo.ml
@@ -592,26 +592,6 @@ external dibuild_create_parameter_variable :
Llvm.llmetadata
= "llvm_dibuild_create_parameter_variable_bytecode" "llvm_dibuild_create_parameter_variable_native"
-external dibuild_insert_declare_before :
- lldibuilder ->
- storage:Llvm.llvalue ->
- var_info:Llvm.llmetadata ->
- expr:Llvm.llmetadata ->
- location:Llvm.llmetadata ->
- instr:Llvm.llvalue ->
- Llvm.lldbgrecord
- = "llvm_dibuild_insert_declare_before_bytecode" "llvm_dibuild_insert_declare_before_native"
-
-external dibuild_insert_declare_at_end :
- lldibuilder ->
- storage:Llvm.llvalue ->
- var_info:Llvm.llmetadata ->
- expr:Llvm.llmetadata ->
- location:Llvm.llmetadata ->
- block:Llvm.llbasicblock ->
- Llvm.lldbgrecord
- = "llvm_dibuild_insert_declare_at_end_bytecode" "llvm_dibuild_insert_declare_at_end_native"
-
external dibuild_expression :
lldibuilder ->
Int64.t array ->
diff --git a/llvm/bindings/ocaml/debuginfo/llvm_debuginfo.mli b/llvm/bindings/ocaml/debuginfo/llvm_debuginfo.mli
index 7c7882ccce855..e3ee209a2ee2b 100644
--- a/llvm/bindings/ocaml/debuginfo/llvm_debuginfo.mli
+++ b/llvm/bindings/ocaml/debuginfo/llvm_debuginfo.mli
@@ -652,30 +652,6 @@ val dibuild_create_parameter_variable :
(** [dibuild_create_parameter_variable] Create a new descriptor for a
function parameter variable. *)
-val dibuild_insert_declare_before :
- lldibuilder ->
- storage:Llvm.llvalue ->
- var_info:Llvm.llmetadata ->
- expr:Llvm.llmetadata ->
- location:Llvm.llmetadata ->
- instr:Llvm.llvalue ->
- Llvm.lldbgrecord
-(** [dibuild_insert_declare_before] Insert a new llvm.dbg.declare
- intrinsic call before the given instruction [instr]. *)
-
-val dibuild_insert_declare_at_end :
- lldibuilder ->
- storage:Llvm.llvalue ->
- var_info:Llvm.llmetadata ->
- expr:Llvm.llmetadata ->
- location:Llvm.llmetadata ->
- block:Llvm.llbasicblock ->
- Llvm.lldbgrecord
-(** [dibuild_insert_declare_at_end] Insert a new llvm.dbg.declare
- intrinsic call at the end of basic block [block]. If [block]
- has a terminator instruction, the intrinsic is inserted
- before that terminator instruction. *)
-
val dibuild_expression : lldibuilder -> Int64.t array -> Llvm.llmetadata
(** [dibuild_expression] Create a new descriptor for the specified variable
which has a complex address expression for its address.
diff --git a/llvm/docs/ReleaseNotes.rst b/llvm/docs/ReleaseNotes.rst
index 5fdbc9f349af4..56417e570fdb7 100644
--- a/llvm/docs/ReleaseNotes.rst
+++ b/llvm/docs/ReleaseNotes.rst
@@ -220,7 +220,43 @@ Changes to the C API
* ``LLVMConstICmp``
* ``LLVMConstFCmp``
-* Added ``LLVMPositionBuilderBeforeDbgRecords`` and ``LLVMPositionBuilderBeforeInstrAndDbgRecords``. Same as ``LLVMPositionBuilder`` and ``LLVMPositionBuilderBefore`` except the insertion position is set to before the debug records that precede the target instruction. See the `debug info migration guide <https://llvm.org/docs/RemoveDIsDebugInfo.html>`_ for more info. ``LLVMPositionBuilder`` and ``LLVMPositionBuilderBefore`` are unchanged; they insert before the indicated instruction but after any attached debug records.
+* Added the following functions to insert before the indicated instruction but
+ after any attached debug records.
+
+ * ``LLVMPositionBuilderBeforeDbgRecords``
+ * ``LLVMPositionBuilderBeforeInstrAndDbgRecords``
+
+ Same as ``LLVMPositionBuilder`` and ``LLVMPositionBuilderBefore`` except the
+ insertion position is set to before the debug records that precede the target
+ instruction. ``LLVMPositionBuilder`` and ``LLVMPositionBuilderBefore`` are
+ unchanged.
+
+ See the `debug info migration guide <https://llvm.org/docs/RemoveDIsDebugInfo.html>`_ for more info.
+
+* Added the following functions to get/set the new non-instruction debug info format.
+
+ * ``LLVMIsNewDbgInfoFormat``
+ * ``LLVMSetIsNewDbgInfoFormat``
+
+* Added the following functions (no plans to deprecate) to insert a debug record
+ (new debug info format).
+
+ * ``LLVMDIBuilderInsertDeclareRecordBefore``
+ * ``LLVMDIBuilderInsertDeclareRecordAtEnd``
+ * ``LLVMDIBuilderInsertDbgValueRecordBefore``
+ * ``LLVMDIBuilderInsertDbgValueRecordAtEnd``
+
+* Deleted the following functions that inserted a debug intrinsic (old debug info format)
+
+ * ``LLVMDIBuilderInsertDeclareBefore``
+ * ``LLVMDIBuilderInsertDeclareAtEnd``
+ * ``LLVMDIBuilderInsertDbgValueBefore``
+ * ``LLVMDIBuilderInsertDbgValueAtEnd``
+
+ * ``LLVMDIBuilderInsertDeclareIntrinsicBefore``
+ * ``LLVMDIBuilderInsertDeclareIntrinsicAtEnd``
+ * ``LLVMDIBuilderInsertDbgValueIntrinsicBefore``
+ * ``LLVMDIBuilderInsertDbgValueIntrinsicAtEnd``
Changes to the CodeGen infrastructure
-------------------------------------
diff --git a/llvm/docs/RemoveDIsDebugInfo.md b/llvm/docs/RemoveDIsDebugInfo.md
index 56634f7ccc6bd..b71143e991282 100644
--- a/llvm/docs/RemoveDIsDebugInfo.md
+++ b/llvm/docs/RemoveDIsDebugInfo.md
@@ -140,34 +140,36 @@ Any tests downstream of the main LLVM repo that test the IR output of LLVM may b
Some new functions that have been added are temporary and will be deprecated in the future. The intention is that they'll help downstream projects adapt during the transition period.
```
-New functions (all to be deprecated)
-------------------------------------
-LLVMIsNewDbgInfoFormat # Returns true if the module is in the new non-instruction mode.
-LLVMSetIsNewDbgInfoFormat # Convert to the requested debug info format.
+Deleted functions
+-----------------
+LLVMDIBuilderInsertDeclareBefore # Insert a debug record (new debug info format) instead of a debug intrinsic (old debug info format).
+LLVMDIBuilderInsertDeclareAtEnd # Same as above.
+LLVMDIBuilderInsertDbgValueBefore # Same as above.
+LLVMDIBuilderInsertDbgValueAtEnd # Same as above.
LLVMDIBuilderInsertDeclareIntrinsicBefore # Insert a debug intrinsic (old debug info format).
LLVMDIBuilderInsertDeclareIntrinsicAtEnd # Same as above.
LLVMDIBuilderInsertDbgValueIntrinsicBefore # Same as above.
LLVMDIBuilderInsertDbgValueIntrinsicAtEnd # Same as above.
-LLVMDIBuilderInsertDeclareRecordBefore # Insert a debug record (new debug info format).
-LLVMDIBuilderInsertDeclareRecordAtEnd # Same as above.
-LLVMDIBuilderInsertDbgValueRecordBefore # Same as above.
-LLVMDIBuilderInsertDbgValueRecordAtEnd # Same as above.
+New functions (to be deprecated)
+--------------------------------
+LLVMIsNewDbgInfoFormat # Returns true if the module is in the new non-instruction mode.
+LLVMSetIsNewDbgInfoFormat # Convert to the requested debug info format.
-Existing functions (behaviour change)
+New functions (no plans to deprecate)
-------------------------------------
-LLVMDIBuilderInsertDeclareBefore # Insert a debug record (new debug info format) instead of a debug intrinsic (old debug info format).
-LLVMDIBuilderInsertDeclareAtEnd # Same as above.
-LLVMDIBuilderInsertDbgValueBefore # Same as above.
-LLVMDIBuilderInsertDbgValueAtEnd # Same as above.
+LLVMDIBuilderInsertDeclareRecordBefore # Insert a debug record (new debug info format).
+LLVMDIBuilderInsertDeclareRecordAtEnd # Same as above. See info below.
+LLVMDIBuilderInsertDbgValueRecordBefore # Same as above. See info below.
+LLVMDIBuilderInsertDbgValueRecordAtEnd # Same as above. See info below.
-New functions (no plans to deprecate)
-----------------------------------
LLVMPositionBuilderBeforeDbgRecords # See info below.
LLVMPositionBuilderBeforeInstrAndDbgRecords # See info below.
```
+`LLVMDIBuilderInsertDeclareRecordBefore`, `LLVMDIBuilderInsertDeclareRecordAtEnd`, `LLVMDIBuilderInsertDbgValueRecordBefore` and `LLVMDIBuilderInsertDbgValueRecordAtEnd` are replacing the deleted `LLVMDIBuilderInsertDeclareBefore-style` functions.
+
`LLVMPositionBuilderBeforeDbgRecords` and `LLVMPositionBuilderBeforeInstrAndDbgRecords` behave the same as `LLVMPositionBuilder` and `LLVMPositionBuilderBefore` except the insertion position is set before the debug records that precede the target instruction. Note that this doesn't mean that debug intrinsics before the chosen instruction are skipped, only debug records (which unlike debug records are not themselves instructions).
If you don't know which function to call then follow this rule:
diff --git a/llvm/include/llvm-c/DebugInfo.h b/llvm/include/llvm-c/DebugInfo.h
index 2c3c75e246c00..81a888c0f4d91 100644
--- a/llvm/include/llvm-c/DebugInfo.h
+++ b/llvm/include/llvm-c/DebugInfo.h
@@ -1261,40 +1261,6 @@ LLVMMetadataRef LLVMDIBuilderCreateTempGlobalVariableFwdDecl(
unsigned LineNo, LLVMMetadataRef Ty, LLVMBool LocalToUnit,
LLVMMetadataRef Decl, uint32_t AlignInBits);
-/*
- * Insert a new Declare DbgRecord before the given instruction.
- *
- * Only use in "new debug mode" (LLVMIsNewDbgInfoFormat() is true).
- * Use LLVMSetIsNewDbgInfoFormat(LLVMBool) to convert between formats.
- * See https://llvm.org/docs/RemoveDIsDebugInfo.html#c-api-changes
- *
- * \param Builder The DIBuilder.
- * \param Storage The storage of the variable to declare.
- * \param VarInfo The variable's debug info descriptor.
- * \param Expr A complex location expression for the variable.
- * \param DebugLoc Debug info location.
- * \param Instr Instruction acting as a location for the new intrinsic.
- */
-LLVMDbgRecordRef
-LLVMDIBuilderInsertDeclareBefore(LLVMDIBuilderRef Builder, LLVMValueRef Storage,
- LLVMMetadataRef VarInfo, LLVMMetadataRef Expr,
- LLVMMetadataRef DebugLoc, LLVMValueRef Instr);
-/**
- * Soon to be deprecated.
- * Only use in "old debug mode" (LLVMIsNewDbgInfoFormat() is false).
- * See https://llvm.org/docs/RemoveDIsDebugInfo.html#c-api-changes
- *
- * Insert a new llvm.dbg.declare intrinsic call before the given instruction.
- * \param Builder The DIBuilder.
- * \param Storage The storage of the variable to declare.
- * \param VarInfo The variable's debug info descriptor.
- * \param Expr A complex location expression for the variable.
- * \param DebugLoc Debug info location.
- * \param Instr Instruction acting as a location for the new intrinsic.
- */
-LLVMValueRef LLVMDIBuilderInsertDeclareIntrinsicBefore(
- LLVMDIBuilderRef Builder, LLVMValueRef Storage, LLVMMetadataRef VarInfo,
- LLVMMetadataRef Expr, LLVMMetadataRef DebugLoc, LLVMValueRef Instr);
/**
* Soon to be deprecated.
* Only use in "new debug mode" (LLVMIsNewDbgInfoFormat() is true).
@@ -1312,43 +1278,6 @@ LLVMDbgRecordRef LLVMDIBuilderInsertDeclareRecordBefore(
LLVMDIBuilderRef Builder, LLVMValueRef Storage, LLVMMetadataRef VarInfo,
LLVMMetadataRef Expr, LLVMMetadataRef DebugLoc, LLVMValueRef Instr);
-/**
- * Insert a new Declare DbgRecord at the end of the given basic block. If the
- * basic block has a terminator instruction, the intrinsic is inserted before
- * that terminator instruction.
- *
- * Only use in "new debug mode" (LLVMIsNewDbgInfoFormat() is true).
- * Use LLVMSetIsNewDbgInfoFormat(LLVMBool) to convert between formats.
- * See https://llvm.org/docs/RemoveDIsDebugInfo.html#c-api-changes
- *
- * \param Builder The DIBuilder.
- * \param Storage The storage of the variable to declare.
- * \param VarInfo The variable's debug info descriptor.
- * \param Expr A complex location expression for the variable.
- * \param DebugLoc Debug info location.
- * \param Block Basic block acting as a location for the new intrinsic.
- */
-LLVMDbgRecordRef LLVMDIBuilderInsertDeclareAtEnd(
- LLVMDIBuilderRef Builder, LLVMValueRef Storage, LLVMMetadataRef VarInfo,
- LLVMMetadataRef Expr, LLVMMetadataRef DebugLoc, LLVMBasicBlockRef Block);
-/**
- * Soon to be deprecated.
- * Only use in "old debug mode" (LLVMIsNewDbgInfoFormat() is false).
- * See https://llvm.org/docs/RemoveDIsDebugInfo.html#c-api-changes
- *
- * Insert a new llvm.dbg.declare intrinsic call at the end of the given basic
- * block. If the basic block has a terminator instruction, the intrinsic is
- * inserted before that terminator instruction.
- * \param Builder The DIBuilder.
- * \param Storage The storage of the variable to declare.
- * \param VarInfo The variable's debug info descriptor.
- * \param Expr A complex location expression for the variable.
- * \param DebugLoc Debug info location.
- * \param Block Basic block acting as a location for the new intrinsic.
- */
-LLVMValueRef LLVMDIBuilderInsertDeclareIntrinsicAtEnd(
- LLVMDIBuilderRef Builder, LLVMValueRef Storage, LLVMMetadataRef VarInfo,
- LLVMMetadataRef Expr, LLVMMetadataRef DebugLoc, LLVMBasicBlockRef Block);
/**
* Soon to be deprecated.
* Only use in "new debug mode" (LLVMIsNewDbgInfoFormat() is true).
@@ -1368,40 +1297,6 @@ LLVMDbgRecordRef LLVMDIBuilderInsertDeclareRecordAtEnd(
LLVMDIBuilderRef Builder, LLVMValueRef Storage, LLVMMetadataRef VarInfo,
LLVMMetadataRef Expr, LLVMMetadataRef DebugLoc, LLVMBasicBlockRef Block);
-/**
- * Insert a new Value DbgRecord before the given instruction.
- *
- * Only use in "new debug mode" (LLVMIsNewDbgInfoFormat() is true).
- * Use LLVMSetIsNewDbgInfoFormat(LLVMBool) to convert between formats.
- * See https://llvm.org/docs/RemoveDIsDebugInfo.html#c-api-changes
- *
- * \param Builder The DIBuilder.
- * \param Val The value of the variable.
- * \param VarInfo The variable's debug info descriptor.
- * \param Expr A complex location expression for the variable.
- * \param DebugLoc Debug info location.
- * \param Instr Instruction acting as a location for the new intrinsic.
- */
-LLVMDbgRecordRef
-LLVMDIBuilderInsertDbgValueBefore(LLVMDIBuilderRef Builder, LLVMValueRef Val,
- LLVMMetadataRef VarInfo, LLVMMetadataRef Expr,
- LLVMMetadataRef DebugLoc, LLVMValueRef Instr);
-/**
- * Soon to be deprecated.
- * Only use in "old debug mode" (LLVMIsNewDbgInfoFormat() is false).
- * See https://llvm.org/docs/RemoveDIsDebugInfo.html#c-api-changes
- *
- * Insert a new llvm.dbg.value intrinsic call before the given instruction.
- * \param Builder The DIBuilder.
- * \param Val The value of the variable.
- * \param VarInfo The variable's debug info descriptor.
- * \param Expr A complex location expression for the variable.
- * \param DebugLoc Debug info location.
- * \param Instr Instruction acting as a location for the new intrinsic.
- */
-LLVMValueRef LLVMDIBuilderInsertDbgValueIntrinsicBefore(
- LLVMDIBuilderRef Builder, LLVMValueRef Val, LLVMMetadataRef VarInfo,
- LLVMMetadataRef Expr, LLVMMetadataRef DebugLoc, LLVMValueRef Instr);
/**
* Soon to be deprecated.
* Only use in "new debug mode" (LLVMIsNewDbgInfoFormat() is true).
@@ -1419,43 +1314,6 @@ LLVMDbgRecordRef LLVMDIBuilderInsertDbgValueRecordBefore(
LLVMDIBuilderRef Builder, LLVMValueRef Val, LLVMMetadataRef VarInfo,
LLVMMetadataRef Expr, LLVMMetadataRef DebugLoc, LLVMValueRef Instr);
-/**
- * Insert a new Value DbgRecord at the end of the given basic block. If the
- * basic block has a terminator instruction, the intrinsic is inserted before
- * that terminator instruction.
- *
- * Only use in "new debug mode" (LLVMIsNewDbgInfoFormat() is true).
- * Use LLVMSetIsNewDbgInfoFormat(LLVMBool) to convert between formats.
- * See https://llvm.org/docs/RemoveDIsDebugInfo.html#c-api-changes
- *
- * \param Builder The DIBuilder.
- * \param Val The value of the variable.
- * \param VarInfo The variable's debug info descriptor.
- * \param Expr A complex location expression for the variable.
- * \param DebugLoc Debug info location.
- * \param Block Basic block acting as a location for the new intrinsic.
- */
-LLVMDbgRecordRef LLVMDIBuilderInsertDbgValueAtEnd(
- LLVMDIBuilderRef Builder, LLVMValueRef Val, LLVMMetadataRef VarInfo,
- LLVMMetadataRef Expr, LLVMMetadataRef DebugLoc, LLVMBasicBlockRef Block);
-/**
- * Soon to be deprecated.
- * Only use in "old debug mode" (LLVMIsNewDbgInfoFormat() is false).
- * See https://llvm.org/docs/RemoveDIsDebugInfo.html#c-api-changes
- *
- * Insert a new llvm.dbg.value intrinsic call at the end of the given basic
- * block. If the basic block has a terminator instruction, the intrinsic is
- * inserted before that terminator instruction.
- * \param Builder The DIBuilder.
- * \param Val The value of the variable.
- * \param VarInfo The variable's debug info descriptor.
- * \param Expr A complex location expression for the variable.
- * \param DebugLoc Debug info location.
- * \param Block Basic block acting as a location for the new intrinsic.
- */
-LLVMValueRef LLVMDIBuilderInsertDbgValueIntrinsicAtEnd(
- LLVMDIBuilderRef Builder, LLVMValueRef Val, LLVMMetadataRef VarInfo,
- LLVMMetadataRef Expr, LLVMMetadataRef DebugLoc, LLVMBasicBlockRef Block);
/**
* Soon to be deprecated.
* Only use in "new debug mode" (LLVMIsNewDbgInfoFormat() is true).
diff --git a/llvm/lib/IR/DebugInfo.cpp b/llvm/lib/IR/DebugInfo.cpp
index 228e17641ffc4..e57c3fe8c825d 100644
--- a/llvm/lib/IR/DebugInfo.cpp
+++ b/llvm/lib/IR/DebugInfo.cpp
@@ -1662,29 +1662,6 @@ LLVMMetadataRef LLVMDIBuilderCreateTempGlobalVariableFwdDecl(
unwrapDI<MDNode>(Decl), nullptr, AlignInBits));
}
-LLVMDbgRecordRef
-LLVMDIBuilderInsertDeclareBefore(LLVMDIBuilderRef Builder, LLVMValueRef Storage,
- LLVMMetadataRef VarInfo, LLVMMetadataRef Expr,
- LLVMMetadataRef DL, LLVMValueRef Instr) {...
[truncated]
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Fix clang-format reported issues.
llvm/docs/ReleaseNotes.rst
Outdated
| * ``LLVMDIBuilderInsertDeclareIntrinsicBefore`` | ||
| * ``LLVMDIBuilderInsertDeclareIntrinsicAtEnd`` | ||
| * ``LLVMDIBuilderInsertDbgValueIntrinsicBefore`` | ||
| * ``LLVMDIBuilderInsertDbgValueIntrinsicAtEnd`` |
There was a problem hiding this comment.
You don't need to mention these as these were ones I added that we're removing, they are not part of the original C API.
llvm/docs/ReleaseNotes.rst
Outdated
| * ``LLVMIsNewDbgInfoFormat`` | ||
| * ``LLVMSetIsNewDbgInfoFormat`` | ||
|
|
||
| * Added the following functions (no plans to deprecate) to insert a debug record |
There was a problem hiding this comment.
"no plans to deprecate" would be the default assumption so I don't think you need to mention it here.
llvm/docs/RemoveDIsDebugInfo.md
Outdated
| LLVMDIBuilderInsertDeclareIntrinsicBefore # Insert a debug intrinsic (old debug info format). | ||
| LLVMDIBuilderInsertDeclareIntrinsicAtEnd # Same as above. | ||
| LLVMDIBuilderInsertDbgValueIntrinsicBefore # Same as above. | ||
| LLVMDIBuilderInsertDbgValueIntrinsicAtEnd # Same as above. |
There was a problem hiding this comment.
Same as before - these ones don't need to be mentioned at all.
| if (LLVMIsNewDbgInfoFormat(M)) | ||
| LLVMDIBuilderInsertDeclareAtEnd( | ||
| DIB, LLVMConstInt(LLVMInt64Type(), 0, false), FooParamVar1, | ||
| FooParamExpression, FooParamLocation, FooEntryBlock); | ||
| else | ||
| LLVMDIBuilderInsertDeclareIntrinsicAtEnd( | ||
| DIB, LLVMConstInt(LLVMInt64Type(), 0, false), FooParamVar1, | ||
| FooParamExpression, FooParamLocation, FooEntryBlock); |
There was a problem hiding this comment.
The test should now call the LLVMDIBuilderInsertDeclareRecord... style functions. e.g. at this site:
delete the if ... else ..., and in its place instead just call LLVMDIBuilderInsertDeclareRecordAtEnd(LLVMInt64Type(), 0, false), FooParamVar1, FooParamExpression, FooParamLocation, FooEntryBlock);
It looks like you've done that for the final block in the test but not the first 3.
Otherwise we're losing test coverage.
There was a problem hiding this comment.
Good point.
Added for all 3: FooParamVar1, FooParamVar2, FooParamVar3
LLVMDIBuilderInsertDeclareRecordAtEnd(
DIB, LLVMConstInt(LLVMInt64Type(), 0, false), FooParamVar1,
FooParamExpression, FooParamLocation, FooEntryBlock);
Updated test case.
llvm/include/llvm-c/DebugInfo.h
Outdated
| */ | ||
| LLVMValueRef LLVMDIBuilderInsertDeclareIntrinsicBefore( | ||
| LLVMDIBuilderRef Builder, LLVMValueRef Storage, LLVMMetadataRef VarInfo, | ||
| LLVMMetadataRef Expr, LLVMMetadataRef DebugLoc, LLVMValueRef Instr); | ||
| /** | ||
| * Soon to be deprecated. |
There was a problem hiding this comment.
Please can you remove the Soon to be deprecated notes from all the Record versions? The plan is now to keep the Record versions so these comments are out of date and misleading.
While you're there please could you add another note to the functions indicating that the debug format can be switched later after inserting the records using LLVMSetIsNewDbgInfoFormat, if needed for legacy or transitionary reasons.
There was a problem hiding this comment.
Delete the Soon to be deprecated note.
Added the following note:
*
* The debug format can be switched later after inserting the records using
* LLVMSetIsNewDbgInfoFormat, if needed for legacy or transitionary reasons.
*
| block:Llvm.llbasicblock -> | ||
| Llvm.lldbgrecord | ||
| = "llvm_dibuild_insert_declare_at_end_bytecode" "llvm_dibuild_insert_declare_at_end_native" | ||
|
|
There was a problem hiding this comment.
See comments above from debuginfo_ocml.c - I don't think these should be deleted
There was a problem hiding this comment.
Restoring the deleted functions.
|
Is this removing any C APIs that existed in the previous release? That's a no-no. |
Yes and no. It's removing some functions that I added during this release (unambiguously OK) but it's also removing some functions that insert debug intrinsics (as opposed to debug records) in accordance with the discussion linked by Carlos in the summary (#92417 (comment) is the important one). It was discussed whether the existing functions should be deprecated first, but @nikic voted in favour of deleting them. |
Address comments from @OCHyams.
|
@OCHyams Thanks for your valuable comments. Uploaded new patch to address them. |
|
@alan-j-hu: looking at the original rework of the C-API, it seems that you run the unit test for the OCaml bindings. I followed https://ocaml.org/docs/installing-ocaml to install OCaml and added Running Is there any more specific instructions that I can follow for the OCaml bindings. Thanks |
There was a problem hiding this comment.
Just one nit and otherwise LGTM, thanks for this @CarlosAlbertoEnciso
We'll want to wait for @pogo59 to give the thumbs-up and @nikic to take a quick look (though I'm pretty confident that these changes line up with what @nikic outlines in the other threads).
llvm/docs/ReleaseNotes.rst
Outdated
| * ``LLVMDIBuilderInsertDbgValueRecordBefore`` | ||
| * ``LLVMDIBuilderInsertDbgValueRecordAtEnd`` | ||
|
|
||
| * Deleted the following functions that inserted a debug intrinsic (old debug info format) |
There was a problem hiding this comment.
nit: missing full stop.
It's a bit repetitive but we probably want See the debug info migration guide https://llvm.org/docs/RemoveDIsDebugInfo.html_ for more info. included in each one of these descriptive bullet points. What do you think?
There was a problem hiding this comment.
What I would suggest is something like this:
Changes to the C API
...
Note: The following changes are due to the removal of the debug info
intrinsics from LLVM and to the introduction of debug records into LLVM.
They are described in detail in the debug info migration guide <https://llvm.org/docs/RemoveDIsDebugInfo.html>_.
- Added the following functions to insert before the indicated instruction but
....
There was a problem hiding this comment.
That moves the info further away from the place someone would be looking, and requires these changes always sit at the bottom of the list. These are very minor concerns though so I'm happy to go with whatever you prefer here (don't forget the full stop).
There was a problem hiding this comment.
These notes are for a group of related changes. The "Note" seems okay to me. Another tactic would be to put this group of changes into a new subsection, which could help other release-note writers realize what's going on. That said, C API changes aren't super common and I'm equally fine with just having the Note.
llvm/include/llvm-c/DebugInfo.h
Outdated
| * Only use in "new debug mode" (LLVMIsNewDbgInfoFormat() is true). | ||
| * See https://llvm.org/docs/RemoveDIsDebugInfo.html#c-api-changes | ||
| * The debug format can be switched later after inserting the records using | ||
| * LLVMSetIsNewDbgInfoFormat, if needed for legacy or transitionary reasons. | ||
| * | ||
| * Insert a new llvm.dbg.value intrinsic call at the end of the given basic |
There was a problem hiding this comment.
The doc comments for the record functions still refer to intrinsics...
There was a problem hiding this comment.
@nikic Thanks for your valuable review. Replaced the 'intrinsic' occurrences.
|
@CarlosAlbertoEnciso IIRC the build files are missing the OCaml debuginfo module in some list, so you have to explicitly build the target I started to build your PR, but it's morning and the build didn't finish before it was time for me to leave for work. |
|
OK I've looked up the C API evolution rules and removing APIs is okay given a release note, which you have. So, no objection from me. I do have to wonder whether |
We needed it in tests while transitioning. There's a chance it might be useful still for downsteam projects still atm (allows a producer flip back to the old format after inserting the records). It won't be useful in the future once intrinsics have been completely removed (hence the planned deprecation). I'm not sure it's doing much harm and could potentially be helpful, but I don't have strong feelings about it. If you think that the clutter isn't warranted we could remove that too (maybe in a separate commit to keep things moving?). |
Just wanted to make sure there was some potential purpose. If you plan to remove it later that's fine. |
|
@alan-j-hu Thanks very much for your valuable input on how to build the Ocaml bindings and for your effort on running the Ocaml tests. |
The DIBuilder C API was changed to deal with DbgRecord functions:
#84915
#85657
#92417 (comment)
As discussed by @nikic and @OCHyams:
#92417 (comment)
This patch implements option (c).