Introduce LLVMDIBuilderRef

[harlanhaskins]

Summary

This patch adds a C API wrapper for the DIBuilder API and exposes most of the functionality. This is not entirely complete, as I am not very familiar with the DI APIs and likely missed some important API coverage.

This builds off the work done by the Rust team, but removes Rust-specific code, generalizes the API, and implements the remaining features from DIBuilder.

This patch adds a definition of LLVMDIBuilderRef that represents an llvm::DIBuilder.

[deadalnix]

That's awesome. A few things.

• Is the API of the DIBuilder stable ? The C API is much harder to change than the C++ one, so maybe we want to reduce the exposed surface to a subset that we know will age well. I worked on thsi in the past and it wasn't very stable. If that's still the case, we should aim to support a few core function first and then extend as needed.
• I committed D19448 which was around for a long time and accepted, but I did not move forward with it because the next steps were missing. Can you make sure you expose the builder type the same way the metadata type is exposed ? That can be a diff of its own and should be simple enough that it can be accepted very quickly. Getting the type in will also help you get less to recompile when switching from one branch to another.
• It doesn't looks like there is a way to use the metadata yet. I would suggest you map just one or two function and then use it to create a basic test case. Once we get something working end to end, it'll be easier to both review and make sure this is going in the right direction.

Finally, there is no test. As mentioned above, it'd be great to get a small end to end test case and include it as an echo test. Features from the builder can then be added one by one with tests and all.

[aprantl]

I think this is generally very nice. My question is, how do we want to deal with future changes to the API? Is it acceptable for future releases of LLVM to break the C-API? If the answer is yes, then I have absolutely no problem with adding this. Otherwise this may turn into a maintenance nightmare as we continue to evolve the debug info interface.

[deadalnix]

Is it acceptable for future releases of LLVM to break the C-API?

The C API has much stronger constraint than the C++ API. This is because a lot of people are using the C API to bind to other languages.

It can be broken via the following process:
1/ Deprecate a function1 in version n and introduce function2.
2/ Remove function1 in version n+1 .

Changing a function prototype is not possible.

[echristo]

One comment:

This sort of thing is why we started an unstable C API. As long as we mark this as unstable I'm generally happy with a simple wrapper around it. So if we're happy with a comment like the one in the OrcBindings:

* Note: This interface is experimental. It is *NOT* stable, and may be *

* changed without warning. *

then I'm happy.

That said, would random changes to this API be upsetting Harlan? :)

-eric

[harlanhaskins]

In D32122#728233, @echristo wrote:

That said, would random changes to this API be upsetting Harlan? :)

It wouldn't upset me -- LLVMSwift is tracking LLVM latest and will continue to make breaking changes as the C API evolves. The real question is established bindings like the OCaml and python bindings -- would they be willing to use this API with breaking changes?

I'm also willing to continue maintaining these bindings as the DI API evolves.

[deadalnix]

It wouldn't upset me -- LLVMSwift is tracking LLVM latest and will continue to make breaking changes as the C API evolves. The real question is established bindings like the OCaml and python bindings -- would they be willing to use this API with breaking changes?

There are a lot of binding out there that we just aren't aware of. And getting failure at runtime because the expected arguments to a function do not match anymore is seriously not funny to debug.

[harlanhaskins]

Update the definitions for LLVMDIBuilderRef and LLVMMetadataRef to be more in line with how the rest of the opaque types are declared.

[harlanhaskins]

In D32122#728263, @deadalnix wrote:

There are a lot of binding out there that we just aren't aware of. And getting failure at runtime because the expected arguments to a function do not match anymore is seriously not funny to debug.

I agree there. So I'm wondering if we can settle on a small subset of these changes that are going to be relatively stable? I think there's a middleground; we can probably remove the Objective-C-specific functions and some of the C++-specific functions.

Alternatively, I'm okay splitting this into smaller patches that are easier to decide about and test independently.

[whitequark]

The real question is established bindings like the OCaml and python bindings -- would they be willing to use this API with breaking changes?

I maintain the OCaml bindings, do a lot of work keeping llvmlite up to date and still do some work on ruby-llvm. The answer is, emphatically, yes. Right now the only way (with llvmlite, which goes through the string representation) is to generate the metadata as strings, and this breaks *every* version.

I would actually suggest, specifically for the DI bindings, an even weaker compatibility guarantee. Functions can be added or removed at any time, provided that a function name is never reused with a different ABI. There is really no need for a stronger guarantee so long as the underlying *semantics* actually changes.

As a side note, the OCaml bindings do not need *any* compatibility guarantee as they are in-tree. For all I know they could be using the C++ interface directly with no harm done.

[harlanhaskins]

I talked with @CodaFi about this API; he's been modeling the wrapper for LLVMSwift based on this initial review just to get a feel for the API.

He's expressed some concerns regarding the DIExpression APIs, essentially that their scope is huge and the only place they're absolutely needed is inside LLVMDIBuilderCreateGlobalVariableExpression and LLVMDIBuilderInsert{Declare|DbgValue}{AtEnd|Before}; the rest of the DIExpression-based APIs cover a tiny portion of the full API and are difficult to use correctly. We can add it in a later PR, provided we can replace the functionality.

How should I go about providing a call for LLVMDIBuilderCreateGlobalVariable independent of DIExpressions, if that's the route we want to take? Is that possible?

[deadalnix]

Alternatively, I'm okay splitting this into smaller patches that are easier to decide about and test independently.

I do think this is the best approach. We can come up with a set of core function that we know won't change that much, then decide for the rest on a case by case basis. You can put me as reviewer on these diff, i'm very interested by this and I'm willing to put time in for review.

I think the current state of the patch is pretty good, and beside a few details as per comments, can be merged.

Could you please use arc in the future to submit patches ? This will make review easier (this will also make your job easier once you get the initial setup nailed).

include/llvm-c/Types.h

83 ↗

(On Diff #95450)

This is already defined.

[harlanhaskins]

include/llvm-c/Types.h

83 ↗

(On Diff #95450)

Hmm...I can't find its definition anywhere else. Where is it defined?

[harlanhaskins]

I seem to have messed up this patch.

include/llvm-c/Types.h

83 ↗

(On Diff #95450)

Ah! Didn't see that D19448 was merged in. Great!

[deadalnix]

include/llvm-c/Types.h

83 ↗

(On Diff #95450)

You make the question and answers, perfect :)

[harlanhaskins]

Simplifies the patch to just define LLVMDIBuilderRef

[harlanhaskins]

@echristo, @whitequark, @aprantl Are we good to merge this?

[aprantl]

No, there's a typo :-)

include/llvm-c/Types.h

102 ↗

(On Diff #95618)

models

[harlanhaskins]

• Fix typo

include/llvm-c/Types.h

102 ↗

(On Diff #95618)

Thank you! Fixed.

[deadalnix]

I think you have two green light, it's fine to proceed. Do you have commit rights ? If not I can commit this for you.

[harlanhaskins]

I do not have commit rights. If you could merge, that'd be great. Then I'll submit the first subset of DIBuilder APIs!

llvm/trunk/include/llvm-c/Types.h

 /**
  * Represents an LLVM basic block builder.
  *
  * This models llvm::IRBuilder.
  */
 typedef struct LLVMOpaqueBuilder *LLVMBuilderRef;
 
 /**
+ * Represents an LLVM debug info builder.
+ *
+ * This models llvm::DIBuilder.
+ */
+typedef struct LLVMOpaqueDIBuilder *LLVMDIBuilderRef;
+
+/**
  * Interface used to provide a module to JIT or interpreter.
  * This is now just a synonym for llvm::Module, but we have to keep using the
  * different type to keep binary compatibility.
  */
 typedef struct LLVMOpaqueModuleProvider *LLVMModuleProviderRef;
 
 /** @see llvm::PassManagerBase */
 typedef struct LLVMOpaquePassManager *LLVMPassManagerRef;

llvm/trunk/include/llvm/IR/DIBuilder.h

NodeTy *replaceTemporary(TempMDNode &&N, NodeTy *Replacement) {
       if (N.get() == Replacement)
         return cast<NodeTy>(MDNode::replaceWithUniqued(std::move(N)));
 
       N->replaceAllUsesWith(Replacement);
       return Replacement;
     }
   };
 
+  // Create wrappers for C Binding types (see CBindingWrapping.h).
+  DEFINE_ISA_CONVERSION_FUNCTIONS(DIBuilder, LLVMDIBuilderRef)
+
 } // end namespace llvm
 
 #endif // LLVM_IR_DIBUILDER_H