This is an archive of the discontinued LLVM Phabricator instance.

[Draft] Possible mapping of DIBuilder in the C API
Needs ReviewPublic

Authored by deadalnix on Apr 13 2016, 6:16 PM.

Details

Summary

This will help discuss D19037

Diff Detail

Event Timeline

deadalnix updated this revision to Diff 53648.Apr 13 2016, 6:16 PM
deadalnix retitled this revision from to [Draft] Possible mapping of DIBuilder in the C API.
deadalnix updated this object.
deadalnix added a subscriber: llvm-commits.

I'm not sure that this is worth the effort and code churn.

In the example that prompted you to post this you said that the overhead from creating all of the locations would be to big. But you do not need to create any DILocation yourself as you can Builder::SetCurrentDebugLocation and will do that for you. I will admit there will be a overhead tho since there is still quite a few bits of Metadata that needs to be wrapped.

Also you can not change the semantics of the functions, you will need to add duplicate functions for all of the metadata functions. What if llvm-c-test.c was written in D and the bindings hadn't been updated, it would have crashed in weird and wonderful ways. Types are part of the ABI.

Cheers, Jakob.

deadalnix added a comment.EditedApr 22 2016, 3:36 PM

Hey, I don't think this is ready to be merged, as I put it there in relation to D19037 .

Now on your points. Metadata used to be part of the value hierarchy but recently moved to their own hierarchy in LLVM. This allow for great resource reduction when emitting debug metadata. It came with the introduction of MetadataAsValue and ValueAsMetadata in order to bridge the 2 hierarchy. That means, in order to manipulate metadata as value, the metadata need to be wrapped in a value node and unwrapped with every method calls. I think it is worthwhile to get away from the MetadataAsValue all over the place as the resource consumption of all these extra nodes is huge, and kind of defeat the whole point of the new metadata API.

As it turns out, methods that generate metadata now return a value of MetadataAsValue kind. Method that expect metadata unwrap that node and get the metadata out of it. As it turns out, this is transparent as passing other kind of values won't work, except for LLVMMDNode which special case constants. I think this one specifically can be updated, but I do not think it is necessary for other functions as using them in a way that would be incompatible with the current scheme would be a runtime error already.

In fact, I used that patch in SDC as a drop in, and it works even with outdated header for that very reason.

I propose to move forward with that the following way:

  • Introduce LLVMMetadataRef and update function to use it. Introduce new functions for these that would accept values other than metadata (namely the LLVMMDNode family of functions).
  • Start mapping DIBuilder, maybe in another header with backward compatibility disclaimer, like for Orc (the metadata hierarchy is quite new and will be subject to frequent changes for a few release I'm sure).

Would that makes sense ?

Let's try to get started with D19448, which should be an easy first step.

whitequark resigned from this revision.May 23 2018, 2:39 PM