[C-API] Preserve backwards compatability for LLVMDIBuilderCreateObjectPointerType

[junlarsen]

#122928 Introduces a change to the C DebugInfo API which breaks compatability with older versions. This patch proposes the same functionality through a new function instead. Another idea is maybe to expose more of the flags here instead of taking a single boolean?

I'm not that familiar with Clang development, but from how things are described in the original patch (and its parent) it seems like it should be an avoidable breakage.

I suppose the same could be said for the OCaml API.

[llvmbot]

@llvm/pr-subscribers-debuginfo

@llvm/pr-subscribers-llvm-ir

Author: Mats Jun Larsen (junlarsen)

Changes

#122928 Introduces a change to the C DebugInfo API which breaks compatability with older versions. This patch proposes the same functionality through a new function instead. Another idea is maybe to expose more of the flags here instead of taking a single boolean?

I'm not that familiar with Clang development, but from how things are described in the original patch (and its parent) it seems like it should be an avoidable breakage.

I suppose the same could be said for the OCaml API.

---

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

2 Files Affected:

• (modified) llvm/include/llvm-c/DebugInfo.h (+11-3)
• (modified) llvm/lib/IR/DebugInfo.cpp (+9-3)

diff --git a/llvm/include/llvm-c/DebugInfo.h b/llvm/include/llvm-c/DebugInfo.h
index ac7ee5a7cc9a19..6e02313c3d2983 100644
--- a/llvm/include/llvm-c/DebugInfo.h
+++ b/llvm/include/llvm-c/DebugInfo.h
@@ -869,6 +869,14 @@ LLVMDIBuilderCreateObjCProperty(LLVMDIBuilderRef Builder,
                                 unsigned PropertyAttributes,
                                 LLVMMetadataRef Ty);
 
+/**
+ * Create a uniqued DIType* clone with FlagObjectPointer.
+ * \param Builder   The DIBuilder.
+ * \param Type      The underlying type to which this pointer points.
+ */
+LLVMMetadataRef LLVMDIBuilderCreateObjectPointerType(LLVMDIBuilderRef Builder,
+                                                     LLVMMetadataRef Type);
+
 /**
  * Create a uniqued DIType* clone with FlagObjectPointer. If \c Implicit
  * is true, then also set FlagArtificial.
@@ -877,9 +885,9 @@ LLVMDIBuilderCreateObjCProperty(LLVMDIBuilderRef Builder,
  * \param Implicit  Indicates whether this pointer was implicitly generated
  *                  (i.e., not spelled out in source).
  */
-LLVMMetadataRef LLVMDIBuilderCreateObjectPointerType(LLVMDIBuilderRef Builder,
-                                                     LLVMMetadataRef Type,
-                                                     LLVMBool Implicit);
+LLVMMetadataRef
+LLVMDIBuilderCreateImplicitObjectPointerType(LLVMDIBuilderRef Builder,
+                                             LLVMMetadataRef Type);
 
 /**
  * Create debugging information entry for a qualified
diff --git a/llvm/lib/IR/DebugInfo.cpp b/llvm/lib/IR/DebugInfo.cpp
index 4ce518009bd3ea..6ff0d280081fdd 100644
--- a/llvm/lib/IR/DebugInfo.cpp
+++ b/llvm/lib/IR/DebugInfo.cpp
@@ -1433,10 +1433,16 @@ LLVMDIBuilderCreateObjCProperty(LLVMDIBuilderRef Builder,
 }
 
 LLVMMetadataRef LLVMDIBuilderCreateObjectPointerType(LLVMDIBuilderRef Builder,
-                                                     LLVMMetadataRef Type,
-                                                     LLVMBool Implicit) {
+                                                     LLVMMetadataRef Type) {
   return wrap(unwrap(Builder)->createObjectPointerType(unwrapDI<DIType>(Type),
-                                                       Implicit));
+                                                       /*Implicit=*/false));
+}
+
+LLVMMetadataRef
+LLVMDIBuilderCreateImplicitObjectPointerType(LLVMDIBuilderRef Builder,
+                                             LLVMMetadataRef Type) {
+  return wrap(unwrap(Builder)->createObjectPointerType(unwrapDI<DIType>(Type),
+                                                       /*Implicit=*/true));
 }
 
 LLVMMetadataRef

[junlarsen]

cc @Michael137

[Michael137]

The APIs aren't stable and we break them every now and then without worrying about backward compatibility.

@pogo59 thoughts?

[pogo59]

The APIs aren't stable and we break them every now and then without worrying about backward compatibility.

The C++ APIs aren't stable. The C APIs very much have to stay stable.

[Michael137]

The APIs aren't stable and we break them every now and then without worrying about backward compatibility.

The C++ APIs aren't stable. The C APIs very much have to stay stable.

Lets update the documentation in that case:

llvm-project/llvm/include/llvm-c/DebugInfo.h

Lines 7 to 14 in 1c28b92

	//===----------------------------------------------------------------------===//
	///
	/// This file declares the C API endpoints for generating DWARF Debug Info
	///
	/// Note: This interface is experimental. It is *NOT* stable, and may be
	/// changed without warning.
	///
	//===----------------------------------------------------------------------===//

Unless I'm misunderstanding

[efriedma-quic]

Even if it's not technically "stable", messing with the signature of an existing function without changing the name is extremely unfriendly to anything consuming the API through FFI bindings.

[Michael137]

We should update the doxygen comments

[Michael137]

Given how trivial this change is i don't mind landing this if it helps consumers. But please update the documentation of the APIs. Particularly the Implicit version should say that it sets the artificial flag and the non-Implicit version does not set it

[Michael137]

Oh and we'd have to adjust the OCaml bindings again...And given the new API has been sitting on top-of-tree, users of this have probably been updated. So changing this all back might be even more disruptive. So I'm not sure if we do want to land this after all actually

I should've just made the API always pass the Implicit flag, instead of adding that parameter. Then if someone needed it they could've updated the APIs.

I'll defer to the others for approval.

not sure if changing this again is less disruptive then just keeping the new API as-is

[junlarsen]

Fair point with the OCaml bindings. I think we'll leave this as-is then.