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

[jberdine]

Summary

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

[whitequark]

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.

[jberdine]

split LLVM-C from OCaml changes

[jberdine]

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]

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

[abdulras]

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]

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]

lib/IR/Core.cpp

1202

getVariable can return nullptr

[jberdine]

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

[jberdine]

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'.)

[compnerd]

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.

[hiraditya]

lib/IR/Core.cpp

1257

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

[jberdine]

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]

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]

early return instead of assert, unsigned instead of int

[compnerd]

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]

Looks good to me now, thanks @compnerd !

[jberdine]

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]

rebase and squash

[compnerd]

SVN r344202

include/llvm-c/Core.h

  *
  * @see llvm::Module::getNamedMetadata()
  * @see llvm::MDNode::addOperand()
  */
 void LLVMAddNamedMetadataOperand(LLVMModuleRef M, const char *Name,
                                  LLVMValueRef Val);
 
 /**
+ * Return the directory of the debug location for this value, which must be
+ * an llvm::Instruction, llvm::GlobalVariable, or llvm::Function.
+ *
+ * @see llvm::Instruction::getDebugLoc()
+ * @see llvm::GlobalVariable::getDebugInfo()
+ * @see llvm::Function::getSubprogram()
+ */
+const char *LLVMGetDebugLocDirectory(LLVMValueRef Val, unsigned *Length);
+
+/**
+ * Return the filename of the debug location for this value, which must be
+ * an llvm::Instruction, llvm::GlobalVariable, or llvm::Function.
+ *
+ * @see llvm::Instruction::getDebugLoc()
+ * @see llvm::GlobalVariable::getDebugInfo()
+ * @see llvm::Function::getSubprogram()
+ */
+const char *LLVMGetDebugLocFilename(LLVMValueRef Val, unsigned *Length);
+
+/**
+ * Return the line number of the debug location for this value, which must be
+ * an llvm::Instruction, llvm::GlobalVariable, or llvm::Function.
+ *
+ * @see llvm::Instruction::getDebugLoc()
+ * @see llvm::GlobalVariable::getDebugInfo()
+ * @see llvm::Function::getSubprogram()
+ */
+unsigned LLVMGetDebugLocLine(LLVMValueRef Val);
+
+/**
+ * Return the column number of the debug location for this value, which must be
+ * an llvm::Instruction.
+ *
+ * @see llvm::Instruction::getDebugLoc()
+ */
+unsigned LLVMGetDebugLocColumn(LLVMValueRef Val);
+
+/**
  * Add a function to a module under a specified name.
  *
  * @see llvm::Function::Create()
  */
 LLVMValueRef LLVMAddFunction(LLVMModuleRef M, const char *Name,
                              LLVMTypeRef FunctionTy);
 
 /**

lib/IR/Core.cpp

 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm-c/Core.h"
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/IR/Attributes.h"
 #include "llvm/IR/CallSite.h"
 #include "llvm/IR/Constants.h"
+#include "llvm/IR/DebugInfoMetadata.h"
 #include "llvm/IR/DerivedTypes.h"
 #include "llvm/IR/DiagnosticInfo.h"
 #include "llvm/IR/DiagnosticPrinter.h"
 #include "llvm/IR/GlobalAlias.h"
 #include "llvm/IR/GlobalVariable.h"
 #include "llvm/IR/IRBuilder.h"
 #include "llvm/IR/InlineAsm.h"
 #include "llvm/IR/IntrinsicInst.h"
void LLVMAddNamedMetadataOperand(LLVMModuleRef M, const char *Name,
   NamedMDNode *N = unwrap(M)->getOrInsertNamedMetadata(Name);
   if (!N)
     return;
   if (!Val)
     return;
   N->addOperand(extractMDNode(unwrap<MetadataAsValue>(Val)));
 }
 
+const char *LLVMGetDebugLocDirectory(LLVMValueRef Val, unsigned *Length) {
+  if (!Length) return nullptr;
+  StringRef S;
+  if (const auto *I = unwrap<Instruction>(Val)) {
+    S = I->getDebugLoc()->getDirectory();
+  } else if (const auto *GV = unwrap<GlobalVariable>(Val)) {
+    SmallVector<DIGlobalVariableExpression *, 1> GVEs;
+    GV->getDebugInfo(GVEs);
+    if (GVEs.size())
+      if (const DIGlobalVariable *DGV = GVEs[0]->getVariable())
+        S = DGV->getDirectory();
+  } else if (const auto *F = unwrap<Function>(Val)) {
+    if (const DISubprogram *DSP = F->getSubprogram())
+      S = DSP->getDirectory();
+  } else {
+    assert(0 && "Expected Instruction, GlobalVariable or Function");
+    return nullptr;
+  }
+  *Length = S.size();
+  return S.data();
+}
+
+const char *LLVMGetDebugLocFilename(LLVMValueRef Val, unsigned *Length) {
+  if (!Length) return nullptr;
+  StringRef S;
+  if (const auto *I = unwrap<Instruction>(Val)) {
+    S = I->getDebugLoc()->getFilename();
+  } else if (const auto *GV = unwrap<GlobalVariable>(Val)) {
+    SmallVector<DIGlobalVariableExpression *, 1> GVEs;
+    GV->getDebugInfo(GVEs);
+    if (GVEs.size())
+      if (const DIGlobalVariable *DGV = GVEs[0]->getVariable())
+        S = DGV->getFilename();
+  } else if (const auto *F = unwrap<Function>(Val)) {
+    if (const DISubprogram *DSP = F->getSubprogram())
+      S = DSP->getFilename();
+  } else {
+    assert(0 && "Expected Instruction, GlobalVariable or Function");
+    return nullptr;
+  }
+  *Length = S.size();
+  return S.data();
+}
+
+unsigned LLVMGetDebugLocLine(LLVMValueRef Val) {
+  unsigned L = 0;
+  if (const auto *I = unwrap<Instruction>(Val)) {
+    L = I->getDebugLoc()->getLine();
+  } else if (const auto *GV = unwrap<GlobalVariable>(Val)) {
+    SmallVector<DIGlobalVariableExpression *, 1> GVEs;
+    GV->getDebugInfo(GVEs);
+    if (GVEs.size())
+      if (const DIGlobalVariable *DGV = GVEs[0]->getVariable())
+        L = DGV->getLine();
+  } else if (const auto *F = unwrap<Function>(Val)) {
+    if (const DISubprogram *DSP = F->getSubprogram())
+      L = DSP->getLine();
+  } else {
+    assert(0 && "Expected Instruction, GlobalVariable or Function");
+    return -1;
+  }
+  return L;
+}
+
+unsigned LLVMGetDebugLocColumn(LLVMValueRef Val) {
+  unsigned C = 0;
+  if (const auto *I = unwrap<Instruction>(Val))
+    if (const auto &L = I->getDebugLoc())
+      C = L->getColumn();
+  return C;
+}
+
 /*--.. Operations on scalar constants ......................................--*/
 
 LLVMValueRef LLVMConstInt(LLVMTypeRef IntTy, unsigned long long N,
                           LLVMBool SignExtend) {
   return wrap(ConstantInt::get(unwrap<IntegerType>(IntTy), N, SignExtend != 0));
 }
 
 LLVMValueRef LLVMConstIntOfArbitraryPrecision(LLVMTypeRef IntTy,
                                               unsigned NumWords,
                                               const uint64_t Words[]) {
     IntegerType *Ty = unwrap<IntegerType>(IntTy);
     return wrap(ConstantInt::get(Ty->getContext(),
                                  APInt(Ty->getBitWidth(),
                                        makeArrayRef(Words, NumWords))));
 }