Add DIBuilder functions to build RAUWable DIVariables and DIFunctions.

[friss]

Summary

These will be used to implement support for useful forward declarartions.

[dblaikie]

lib/IR/DIBuilder.cpp

1062 ↗

(On Diff #13619)

Should this be hardcoded as a definition? Seems like the "createTempStaticVariableFwdDecl" would want a declaration, not a definition.

1196 ↗

(On Diff #13619)

Should this take "isDefinition"? The function name indicates that it's only a declaration.

1240 ↗

(On Diff #13619)

Wonder if there's a nicer way to do this - templated & pass in a lambda?

But it's nothing drastic either way.

[friss]

lib/IR/DIBuilder.cpp

1062 ↗

(On Diff #13619)

Doh! you're right. I had it and then reworked the patch and lost that change...

1240 ↗

(On Diff #13619)

I had a /simpler/ way, but I'm not sure you'll find it nicer (In fact I changed it because I thought you'd not like it). In the first incarnation of the patch, I just checked the isDefinition value and decided upon temporary or not based on that. I audited all users and if I didn't miss anything, none passes false for isDefinition today. Thus only the ones I will introduce would need to care about the temporary aspect.

And similarly for the variables (where I introduced a new argument for isDefinition that I lost in this version of the patch).

[dblaikie]

lib/IR/DIBuilder.cpp

1072 ↗

(On Diff #13619)

How's this work? What will we do at the finalize step?

1240 ↗

(On Diff #13619)

Yeah - the only function declarations we create currently are member functions for which we use "createMethod" - though that's a subtle distinction that's probably not ideal to rely on (one day we might decide that createMethod can go away & we can just create declarations - declarations that would never be replaced with definitions).

What about passing in a function pointer? (&MDNode::getTemporary or &MDNode::get) Avoids a boolean argument (which is always a bit unclear at a call site without a comment) except we need to do other work (AllSubprograms and AllGVs)...

I guess we could rely on the fact that we only emit declarations when there's something that refers to them (so they don't necessarily need to be in retention lists), but that's still pretty subtle.

[friss]

lib/IR/DIBuilder.cpp

1072 ↗

(On Diff #13619)

I realise I hadn't replied to this. In followup patches, in CGDebugInfo::finalize(), forward declarations which have seen definitions are RAUWed and dropped, the other ones are placed into the corresponding retention list. They need to be in retention lists, not because thy would be lost otherwise, but simply because these lists are emitted first in DwarfUnit. And the imported_declaration stuff expects that all the referenced DIEs are already emitted at the time they are processed.

1240 ↗

(On Diff #13619)

I agree regarding the general meaning of a boolean at the call site. The function pointer would be nice if we didn't do any special processing depending on the value, but I thing testing for a function pointer of a certain value would look odd.

And regarding not putting the declarations in the retention lists, I replied above.

What about your first suggestion of a lambda? It would be pretty explicit.

[friss]

Addressed review comments:

• add isDefinition parameter to createStaticVariable
• use a std::function and lambdas to convey the intent of the caller into createFunctionHelper and createStaticVariableHelper

[dblaikie]

lib/IR/DIBuilder.cpp

1173 ↗

(On Diff #13709)

Since this is a static function, you can make it a template and avoid the std::function dynamic overhead - but I suppose that's a binary size V execution time tradeoff... I don't feel too strongly about it either way, though I'd probably have gone with a template rather than std::function, personally.

1248 ↗

(On Diff #13709)

Not sure what the style guide says, but I usually just capture by "[&]" rather than listing the me explicitly (goes for here and above)

1072 ↗

(On Diff #13619)

Cna we just fix the imported_declaration stuff so it doesn't expect all the referenced DIEs to be already emitted (it can use the usual "getOrCreate" routines that lazily construct stuff if it's not already been constructed).

Also you said: " forward declarations which have seen definitions are RAUWed and dropped, the other ones are placed into the corresponding retention list." - they'll still need to be RAUW'd with themselves to promote them from temporary to non-temporary. I imagine this should look much the same as the code we already have for handling temporary type forward declarations, no? I'd expect to see it pretty much exactly the same/symmetric - is that not going to be the case?

[friss]

lib/IR/DIBuilder.cpp

1173 ↗

(On Diff #13709)

Can do. I find the std::function more self documenting, but it has a sometimes noticeable overhead. I didn't think it would show up on profiles here, but if you prefer I'll happily change that to a template parameter.

1248 ↗

(On Diff #13709)

I haven't tried it, but I think [&] wouldn't capture 'this', as it can't be captured by reference. And wouldn't capturing everything by value create a lot of useless copies (in other words, does the compiler only copy what is needed?).

1072 ↗

(On Diff #13619)

I'll try to look into the imported DIE emission code to see if we can remove constraints.

It's not exactly symmetric, but it looks pretty much the same. For example, in CGDebugInfo::finalise(), the loop handling the type expects that every type in the ReplaceMap has a counterpart in the TypeCache (I'm not totally clear how this is always true). For functions and variables, the handling loop allows definitions not to exist, but it's basically the same.

[dblaikie]

lib/IR/DIBuilder.cpp

1173 ↗

(On Diff #13709)

Nah, that's fine.

1248 ↗

(On Diff #13709)

I believe [&] does the right thing for 'this'

& yes, even when using a default capture, only things that are ODR-used within the lambda are captured. Things you don't reference won't be unnecessarily captured.

[friss]

Addressed review comments: Do not capture explicitely in lambdas

[dblaikie]

Looks good

[friss]

Closed by commit rL217949 (authored by @friss).

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

class DIBuilder {
     /// @param Val         llvm::Value of the variable.
     /// @param Decl        Reference to the corresponding declaration.
     DIGlobalVariable
     createStaticVariable(DIDescriptor Context, StringRef Name,
                          StringRef LinkageName, DIFile File, unsigned LineNo,
                          DITypeRef Ty, bool isLocalToUnit, llvm::Value *Val,
                          MDNode *Decl = nullptr);
 
+    /// createTempStaticVariableFwdDecl - Identical to createStaticVariable
+    /// except that the resulting DbgNode is temporary and meant to be RAUWed.
+    DIGlobalVariable
+    createTempStaticVariableFwdDecl(DIDescriptor Context, StringRef Name,
+                                    StringRef LinkageName, DIFile File,
+                                    unsigned LineNo, DITypeRef Ty,
+                                    bool isLocalToUnit, llvm::Value *Val,
+                                    MDNode *Decl = nullptr);
+
 
     /// createLocalVariable - Create a new descriptor for the specified
     /// local variable.
     /// @param Tag         Dwarf TAG. Usually DW_TAG_auto_variable or
     ///                    DW_TAG_arg_variable.
     /// @param Scope       Variable scope.
     /// @param Name        Variable name.
     /// @param File        File where this variable is defined.
DISubprogram createFunction(DIDescriptor Scope, StringRef Name,
                                 bool isDefinition,
                                 unsigned ScopeLine,
                                 unsigned Flags = 0,
                                 bool isOptimized = false,
                                 Function *Fn = nullptr,
                                 MDNode *TParam = nullptr,
                                 MDNode *Decl = nullptr);
 
+    /// createTempFunctionFwdDecl - Identical to createFunction,
+    /// except that the resulting DbgNode is meant to be RAUWed.
+    DISubprogram createTempFunctionFwdDecl(DIDescriptor Scope, StringRef Name,
+                                           StringRef LinkageName,
+                                           DIFile File, unsigned LineNo,
+                                           DICompositeType Ty, bool isLocalToUnit,
+                                           bool isDefinition,
+                                           unsigned ScopeLine,
+                                           unsigned Flags = 0,
+                                           bool isOptimized = false,
+                                           Function *Fn = nullptr,
+                                           MDNode *TParam = nullptr,
+                                           MDNode *Decl = nullptr);
+
+
     /// FIXME: this is added for dragonegg. Once we update dragonegg
     /// to call resolve function, this will be removed.
     DISubprogram createFunction(DIScopeRef Scope, StringRef Name,
                                 StringRef LinkageName,
                                 DIFile File, unsigned LineNo,
                                 DICompositeType Ty, bool isLocalToUnit,
                                 bool isDefinition,
                                 unsigned ScopeLine,
class DIBuilder {
     /// insertDbgValueIntrinsic - Insert a new llvm.dbg.value intrinsic call.
     /// @param Val          llvm::Value of the variable
     /// @param Offset       Offset
     /// @param VarInfo      Variable's debug info descriptor.
     /// @param InsertBefore Location for the new intrinsic.
     Instruction *insertDbgValueIntrinsic(llvm::Value *Val, uint64_t Offset,
                                          DIVariable VarInfo,
                                          Instruction *InsertBefore);
-
   };
 } // end namespace llvm
 
 #endif

llvm/trunk/lib/IR/DIBuilder.cpp

DIGlobalVariable DIBuilder::createGlobalVariable(StringRef Name, DIFile F,
                                                  unsigned LineNumber,
                                                  DITypeRef Ty,
                                                  bool isLocalToUnit,
                                                  Value *Val) {
   return createGlobalVariable(Name, Name, F, LineNumber, Ty, isLocalToUnit,
                               Val);
 }
 
-/// createStaticVariable - Create a new descriptor for the specified static
-/// variable.
-DIGlobalVariable DIBuilder::createStaticVariable(DIDescriptor Context,
-                                                 StringRef Name,
-                                                 StringRef LinkageName,
-                                                 DIFile F, unsigned LineNumber,
-                                                 DITypeRef Ty,
-                                                 bool isLocalToUnit,
-                                                 Value *Val, MDNode *Decl) {
+static DIGlobalVariable
+createStaticVariableHelper(LLVMContext &VMContext, DIDescriptor Context,
+                           StringRef Name, StringRef LinkageName, DIFile F,
+                           unsigned LineNumber, DITypeRef Ty, bool isLocalToUnit,
+                           Value *Val, MDNode *Decl, bool isDefinition,
+                           std::function<MDNode *(ArrayRef<Value *>)> CreateFunc) {
   Value *Elts[] = {
     GetTagConstant(VMContext, dwarf::DW_TAG_variable),
     Constant::getNullValue(Type::getInt32Ty(VMContext)),
     getNonCompileUnitScope(Context),
     MDString::get(VMContext, Name),
     MDString::get(VMContext, Name),
     MDString::get(VMContext, LinkageName),
     F,
     ConstantInt::get(Type::getInt32Ty(VMContext), LineNumber),
     Ty,
     ConstantInt::get(Type::getInt32Ty(VMContext), isLocalToUnit),
-    ConstantInt::get(Type::getInt32Ty(VMContext), 1), /* isDefinition*/
+    ConstantInt::get(Type::getInt32Ty(VMContext), isDefinition),
     Val,
     DIDescriptor(Decl)
   };
+
+  return DIGlobalVariable(CreateFunc(Elts));
+}
+
+/// createStaticVariable - Create a new descriptor for the specified
+/// variable.
+DIGlobalVariable DIBuilder::createStaticVariable(DIDescriptor Context,
+                                                 StringRef Name,
+                                                 StringRef LinkageName,
+                                                 DIFile F, unsigned LineNumber,
+                                                 DITypeRef Ty,
+                                                 bool isLocalToUnit,
+                                                 Value *Val, MDNode *Decl) {
+  return createStaticVariableHelper(VMContext, Context, Name, LinkageName, F,
+                                    LineNumber, Ty, isLocalToUnit, Val, Decl, true,
+                                    [&] (ArrayRef<Value *> Elts) -> MDNode * {
                                       MDNode *Node = MDNode::get(VMContext, Elts);
                                       AllGVs.push_back(Node);
-  return DIGlobalVariable(Node);
+                                      return Node;
+                                    });
+}
+
+/// createTempStaticVariableFwdDecl - Create a new temporary descriptor for the
+/// specified variable declarartion.
+DIGlobalVariable
+DIBuilder::createTempStaticVariableFwdDecl(DIDescriptor Context,
+                                           StringRef Name,
+                                           StringRef LinkageName,
+                                           DIFile F, unsigned LineNumber,
+                                           DITypeRef Ty,
+                                           bool isLocalToUnit,
+                                           Value *Val, MDNode *Decl) {
+  return createStaticVariableHelper(VMContext, Context, Name, LinkageName, F,
+                                    LineNumber, Ty, isLocalToUnit, Val, Decl, false,
+                                    [&] (ArrayRef<Value *> Elts) {
+                                      return MDNode::getTemporary(VMContext, Elts);
+                                    });
 }
 
 /// createVariable - Create a new descriptor for the specified variable.
 DIVariable DIBuilder::createLocalVariable(unsigned Tag, DIDescriptor Scope,
                                           StringRef Name, DIFile File,
                                           unsigned LineNo, DITypeRef Ty,
                                           bool AlwaysPreserve, unsigned Flags,
                                           unsigned ArgNo) {
DISubprogram DIBuilder::createFunction(DIScopeRef Context, StringRef Name,
   // dragonegg does not generate identifier for types, so using an empty map
   // to resolve the context should be fine.
   DITypeIdentifierMap EmptyMap;
   return createFunction(Context.resolve(EmptyMap), Name, LinkageName, File,
                         LineNo, Ty, isLocalToUnit, isDefinition, ScopeLine,
                         Flags, isOptimized, Fn, TParams, Decl);
 }
 
-/// createFunction - Create a new descriptor for the specified function.
-DISubprogram DIBuilder::createFunction(DIDescriptor Context, StringRef Name,
-                                       StringRef LinkageName, DIFile File,
-                                       unsigned LineNo, DICompositeType Ty,
-                                       bool isLocalToUnit, bool isDefinition,
-                                       unsigned ScopeLine, unsigned Flags,
-                                       bool isOptimized, Function *Fn,
-                                       MDNode *TParams, MDNode *Decl) {
+static DISubprogram
+createFunctionHelper(LLVMContext &VMContext, DIDescriptor Context, StringRef Name,
+                     StringRef LinkageName, DIFile File, unsigned LineNo,
+                     DICompositeType Ty, bool isLocalToUnit, bool isDefinition,
+                     unsigned ScopeLine, unsigned Flags, bool isOptimized,
+                     Function *Fn, MDNode *TParams, MDNode *Decl,
+                     std::function<MDNode *(ArrayRef<Value *>)> CreateFunc) {
   assert(Ty.getTag() == dwarf::DW_TAG_subroutine_type &&
          "function types should be subroutines");
   Value *TElts[] = { GetTagConstant(VMContext, DW_TAG_base_type) };
   Value *Elts[] = {
     GetTagConstant(VMContext, dwarf::DW_TAG_subprogram),
     File.getFileNode(),
     DIScope(getNonCompileUnitScope(Context)).getRef(),
     MDString::get(VMContext, Name),
Value *Elts[] = {
     ConstantInt::get(Type::getInt32Ty(VMContext), Flags),
     ConstantInt::get(Type::getInt1Ty(VMContext), isOptimized),
     Fn,
     TParams,
     Decl,
     MDNode::getTemporary(VMContext, TElts),
     ConstantInt::get(Type::getInt32Ty(VMContext), ScopeLine)
   };
-  MDNode *Node = MDNode::get(VMContext, Elts);
 
-  // Create a named metadata so that we do not lose this mdnode.
-  if (isDefinition)
-    AllSubprograms.push_back(Node);
-  DISubprogram S(Node);
+  DISubprogram S(CreateFunc(Elts));
   assert(S.isSubprogram() &&
          "createFunction should return a valid DISubprogram");
   return S;
 }
 
+
+/// createFunction - Create a new descriptor for the specified function.
+DISubprogram DIBuilder::createFunction(DIDescriptor Context, StringRef Name,
+                                       StringRef LinkageName, DIFile File,
+                                       unsigned LineNo, DICompositeType Ty,
+                                       bool isLocalToUnit, bool isDefinition,
+                                       unsigned ScopeLine, unsigned Flags,
+                                       bool isOptimized, Function *Fn,
+                                       MDNode *TParams, MDNode *Decl) {
+  return createFunctionHelper(VMContext, Context, Name, LinkageName, File,
+                              LineNo, Ty, isLocalToUnit, isDefinition, ScopeLine,
+                              Flags, isOptimized, Fn, TParams, Decl,
+                              [&] (ArrayRef<Value *> Elts) -> MDNode *{
+                                MDNode *Node = MDNode::get(VMContext, Elts);
+                                // Create a named metadata so that we
+                                // do not lose this mdnode.
+                                if (isDefinition)
+                                  AllSubprograms.push_back(Node);
+                                return Node;
+                              });
+}
+
+/// createTempFunctionFwdDecl - Create a new temporary descriptor for
+/// the specified function declaration.
+DISubprogram
+DIBuilder::createTempFunctionFwdDecl(DIDescriptor Context, StringRef Name,
+                                     StringRef LinkageName, DIFile File,
+                                     unsigned LineNo, DICompositeType Ty,
+                                     bool isLocalToUnit, bool isDefinition,
+                                     unsigned ScopeLine, unsigned Flags,
+                                     bool isOptimized, Function *Fn,
+                                     MDNode *TParams, MDNode *Decl) {
+  return createFunctionHelper(VMContext, Context, Name, LinkageName, File,
+                              LineNo, Ty, isLocalToUnit, isDefinition, ScopeLine,
+                              Flags, isOptimized, Fn, TParams, Decl,
+                              [&] (ArrayRef<Value *> Elts) {
+                                return MDNode::getTemporary(VMContext, Elts);
+                              });
+}
+
 /// createMethod - Create a new descriptor for the specified C++ method.
 DISubprogram DIBuilder::createMethod(DIDescriptor Context, StringRef Name,
                                      StringRef LinkageName, DIFile F,
                                      unsigned LineNo, DICompositeType Ty,
                                      bool isLocalToUnit, bool isDefinition,
                                      unsigned VK, unsigned VIndex,
                                      DIType VTableHolder, unsigned Flags,
                                      bool isOptimized, Function *Fn,