[PATCH] Add DIBuilder functions to build RAUWable DIVariables and DIFunctions.

[Frederic Riss]

================
Comment at: lib/IR/DIBuilder.cpp:1173
@@ -1141,9 +1172,3 @@
 
-/// 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,
----------------
dblaikie wrote:
> 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.
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.

================
Comment at: lib/IR/DIBuilder.cpp:1248
@@ +1247,3 @@
+                              Flags, isOptimized, Fn, TParams, Decl,
+                              [this] (ArrayRef<Value *> Elts) {
+                                return MDNode::getTemporary(VMContext, Elts);
----------------
dblaikie wrote:
> Not sure what the style guide says, but I usually just capture by "[&]" rather than listing the me explicitly (goes for here and above)
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?).

================
Comment at: lib/IR/DIBuilder.cpp:1072
@@ +1071,3 @@
+    Node = MDNode::get(VMContext, Elts);
+    // Do not put temporary nodes in the GV list. We'll add the
+    // necessary ones at the finalize step.
----------------
dblaikie wrote:
> friss wrote:
> > dblaikie wrote:
> > > How's this work? What will we do at the finalize step?
> > 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.
> 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?
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.

http://reviews.llvm.org/D5328