Page MenuHomePhabricator

[LLVM-C] Add C APIs to access DebugLoc info
ClosedPublic

Authored by jberdine on Sep 17 2018, 6:25 PM.

Details

Summary

Add thin shims to C interface to provide access to DebugLoc info for
Instructions, GlobalVariables and Functions.

Diff Detail

Event Timeline

jberdine created this revision.Sep 17 2018, 6:25 PM
whitequark requested changes to this revision.Sep 17 2018, 6:42 PM

Why are there three sets of functions that all operate on LLVMValueRef? Can't we make it a single set of functions?

All new C API functions that return strings must accept an unsigned *Length parameter.

Since this is a rather nontrivial addition to the C API, I think you should split this into a C patch which I'd like to get a second opinion on, and then a separate OCaml patch.

This revision now requires changes to proceed.Sep 17 2018, 6:42 PM
jberdine updated this revision to Diff 165989.Sep 18 2018, 9:20 AM
jberdine retitled this revision from [LLVM-C][OCaml] Add C and OCaml APIs to access DebugLoc info to [LLVM-C] Add C APIs to access DebugLoc info.
jberdine edited the summary of this revision. (Show Details)

split LLVM-C from OCaml changes

I don't know if this is the idiomatic way to do dynamic dispatch in C, feedback very welcome.

I put these functions after the Metadata ones, but there might be a better place.

@whitequark, did you have someone in mind to look at these LLVM-C changes, or should I do some git blaming?

whitequark added a subscriber: echristo.

@deadalnix, @echristo, any opinion on this C API?

abdulras added inline comments.
lib/IR/Core.cpp
1194

I think that unwrap(Val) should be possible to be hoisted rather than done in each case.

1195

Can't you hoist the unwrap here into the conditional?

if (const auto *I = unwrap<Instruction>(Val))
1196

Please mark the Length parameter as _Nonnull, add an assert, or add a check for Length being non-null here.

1208

I think that this is better with an early return:

if (GVEs.size() == 0) {
  *Length = 0;
  return nullptr;
}
StringRef S = GVEs[0]->getVariable()->getDirectory();
*Length = S.size();
return S.data();
1218

Similar with an early return.

1220

This is going to get "optimized" away in NDEBUG builds. If you hoist the S out of the cases, and sink the Length assignment and return, you at least get predictable behaviour.

1251

Similar behavioural changes as above.

1266

Similar behavioural changes.

1269–1272

Why not:

if (const auto *I = unwrap<Instruction>(Val))
  if (const auto &L = I->getDebugLoc())
    return L->getColumn();
return 0;
hiraditya added inline comments.
lib/IR/Core.cpp
1220

This assert will not trigger in a release build. Is there a better way to handle this?

1250

Same as above

hiraditya added inline comments.Sep 24 2018, 8:15 AM
lib/IR/Core.cpp
1202

getVariable can return nullptr

jberdine updated this revision to Diff 166793.Sep 24 2018, 5:44 PM

Suggested changes to LLVMGetDebugLocDirectory and LLVMGetDebugLocColumn, will update LLVMGetDebugLocFilename and LLVMGetDebugLocLine when LLVMGetDebugLocDirectory settles

jberdine marked 6 inline comments as done.Sep 24 2018, 5:53 PM
jberdine added inline comments.
lib/IR/Core.cpp
1194

Can further hoisting be done now?

1196

I added an assert above instead of _Nonnull since this is expected to be called from outside, where an attribute is less likely to be checkable.

1220

Do you expect the current version to be so "optimized"? (I'm not positive I grokked what you had in mind with 'sink the Length assignment and return'.)

jberdine marked an inline comment as done.Sep 24 2018, 5:55 PM
compnerd requested changes to this revision.Sep 25 2018, 10:33 AM
compnerd added a subscriber: compnerd.
compnerd added inline comments.
lib/IR/Core.cpp
1194

Not sure I understand the comment about _Nonnull not being able to check-able.

1201

The use of the goto is unnecessary. Just do:

if (GVEs.Size())
  if (const DIGlobalVariable *DGV = GVEs[0]->getVariable())
    S = DGV->getDirectory();
1208

The goto is unnecesary. Just do:

if (const DISubprogram *DSP = F->getSubprogram())
  S = DSP->getDirectory();
1220

Yes, it isn't unheard of for builds to be done without asserts enabled, so that is a valid concern.

1247

I think that this can be rewritten similar to the function above.

1262

I think that it makes sense to use the same pattern with the conditional assignments.

This revision now requires changes to proceed.Sep 25 2018, 10:33 AM
hiraditya added inline comments.Sep 25 2018, 2:12 PM
lib/IR/Core.cpp
1257

GVEs[0]->getVariable() can return nullptr.

jberdine updated this revision to Diff 168720.Oct 8 2018, 3:47 PM
jberdine marked 9 inline comments as done.

Rely on default initialization of StringRef, and use conditional assignment instead of early return.

lib/IR/Core.cpp
1194

This is an entry point that gets called from other language bindings via their foreign function interfaces, which are unlikely to check _Nonnull, or even be able to "see" it since they are essentially written against the ABI.

compnerd added inline comments.Oct 9 2018, 4:43 PM
lib/IR/Core.cpp
1194

The _Nonnull is a hint to the compiler that the value is non-null. When building that against a header that declares it, the compiler should ensure that the value being passed is non-null. The problem is that the assertion can be compiled out (-DNDEBUG). Maybe change this to a if (!Length) return nullptr?

1208

Add a return nullptr. The assertion can be compiled out.

1215

Same as above, either annotate with _Nonnull or change this to an early return.

1229

Same as above, add a return nullptr.

1236

Please change this to unsigned to match the return type of DebugLoc::getLine.

1249

Please add a return -1 here since the assert can be compiled out.

1255

Please change this to an unsigned to match the return type of DebugLoc::getColumn.

jberdine updated this revision to Diff 168921.Oct 9 2018, 5:37 PM
jberdine marked 6 inline comments as done.

early return instead of assert, unsigned instead of int

jberdine marked an inline comment as done.Oct 9 2018, 5:37 PM
compnerd accepted this revision.Oct 10 2018, 8:53 AM

This seems reasonable to me. @whitequark, are you okay with them too? To @whitequark's original point, I think that the dispatching the type based on a query is pretty cool (you can do an enumeration + union output), but, I think API-wise, this is safer and easier to work with, not to mention also simplifies the implementation.

whitequark accepted this revision.Oct 10 2018, 12:38 PM

Looks good to me now, thanks @compnerd !

This revision is now accepted and ready to land.Oct 10 2018, 12:38 PM

Thanks for your help and patience! This has certainly improved a lot from the first version I submitted.

Would one of you mind committing this?

jberdine updated this revision to Diff 169101.Oct 10 2018, 2:47 PM

rebase and squash

compnerd closed this revision.Oct 10 2018, 4:55 PM

SVN r344202