[CGVTables] Finalize SP before attempting to clone it

[loladiro]

Summary

LLVM commit D33655 was reverted, because it exposed an assertion failure,
in GenerateVarArgsThunk. That function attempts to clone a function
before clang is entirely done generating the debug info for the current
compliation unit. That is not a valid use of the API, because it expects
that there are no temporary MD nodes in the nodes that it needs to clone.
However, until now, this did not cause any problems (though could have
caused assertions in the backend due to mismatched debug info metadata).

Attempt to rectify this by finalizing the SP before attempting to clone it.
The requisite LLVM API is being introduced in D33704.

[loladiro]

Finalize all subprograms when we're done emitting them.
The one we're interested in is a side effect, but doing
this uniformly might be cleaner and help avoid similar errors in the future.

[loladiro]

@aprantl @dblaikie See if you like this better.

[dblaikie]

I guess this would need a cross-project test case (ie: it'd have to run LLVM optimizations to fail/pass/demonstrate the fix). I think it'd be OK to add one if there's a neat/clean/obvious optimization that can be reliably triggered to do the cloning that would assert/crash - please add one if you think that's practical/reasonable.

[loladiro]

There's already such a test case, but the cloning currently doesn't assert properly (but it can generate incorrect code). D33655 fixes that up, so I think the testing is covered once that LLVM commit goes in. I'll hold off a little while to give @aprantl a chance to look at this as well, but I do want to get this in in short order, so I can recommit D33655.

[aprantl]

Looks good.. Are you also planning to change DIBuilder to not finalize subprograms automatically any more (and not insert them into AllSubprograms)? (That will be the more impactful change as it will force all non-clang frontends to make a similar change).

[loladiro]

I don't think that change is entirely necessary. I don't have any strong objections to it, but I also don't see a good reason to require it. In any case, let me get this in to be able to re-land D33655 and we can revisit the more disruptive change later.

cfe/trunk/lib/CodeGen/CGDebugInfo.h

public:
   void EmitInlineFunctionStart(CGBuilderTy &Builder, GlobalDecl GD);
   /// End an inlined function scope.
   void EmitInlineFunctionEnd(CGBuilderTy &Builder);
 
   /// Emit debug info for a function declaration.
   void EmitFunctionDecl(GlobalDecl GD, SourceLocation Loc, QualType FnType);
 
   /// Constructs the debug code for exiting a function.
-  void EmitFunctionEnd(CGBuilderTy &Builder);
+  void EmitFunctionEnd(CGBuilderTy &Builder, llvm::Function *Fn);
 
   /// Emit metadata to indicate the beginning of a new lexical block
   /// and push the block onto the stack.
   void EmitLexicalBlockStart(CGBuilderTy &Builder, SourceLocation Loc);
 
   /// Emit metadata to indicate the end of a new lexical block and pop
   /// the current block.
   void EmitLexicalBlockEnd(CGBuilderTy &Builder, SourceLocation Loc);

cfe/trunk/lib/CodeGen/CGDebugInfo.cpp

void CGDebugInfo::EmitInlineFunctionStart(CGBuilderTy &Builder, GlobalDecl GD) {
   FnBeginRegionCount.push_back(LexicalBlockStack.size());
   LexicalBlockStack.emplace_back(SP);
   setInlinedAt(Builder.getCurrentDebugLocation());
   EmitLocation(Builder, FD->getLocation());
 }
 
 void CGDebugInfo::EmitInlineFunctionEnd(CGBuilderTy &Builder) {
   assert(CurInlinedAt && "unbalanced inline scope stack");
-  EmitFunctionEnd(Builder);
+  EmitFunctionEnd(Builder, nullptr);
   setInlinedAt(llvm::DebugLoc(CurInlinedAt).getInlinedAt());
 }
 
 void CGDebugInfo::EmitLocation(CGBuilderTy &Builder, SourceLocation Loc) {
   // Update our current location
   setLocation(Loc);
 
   if (CurLoc.isInvalid() || CurLoc.isMacroID())
void CGDebugInfo::EmitLexicalBlockEnd(CGBuilderTy &Builder,
   EmitLocation(Builder, Loc);
 
   if (DebugKind <= codegenoptions::DebugLineTablesOnly)
     return;
 
   LexicalBlockStack.pop_back();
 }
 
-void CGDebugInfo::EmitFunctionEnd(CGBuilderTy &Builder) {
+void CGDebugInfo::EmitFunctionEnd(CGBuilderTy &Builder, llvm::Function *Fn) {
   assert(!LexicalBlockStack.empty() && "Region stack mismatch, stack empty!");
   unsigned RCount = FnBeginRegionCount.back();
   assert(RCount <= LexicalBlockStack.size() && "Region stack mismatch");
 
   // Pop all regions for this function.
   while (LexicalBlockStack.size() != RCount) {
     // Provide an entry in the line table for the end of the block.
     EmitLocation(Builder, CurLoc);
     LexicalBlockStack.pop_back();
   }
   FnBeginRegionCount.pop_back();
+
+  if (Fn && Fn->getSubprogram())
+    DBuilder.finalizeSubprogram(Fn->getSubprogram());
 }
 
 llvm::DIType *CGDebugInfo::EmitTypeForVarWithBlocksAttr(const VarDecl *VD,
                                                         uint64_t *XOffset) {
 
   SmallVector<llvm::Metadata *, 5> EltTys;
   QualType FType;
   uint64_t FieldSize, FieldOffset;

cfe/trunk/lib/CodeGen/CodeGenFunction.cpp

void CodeGenFunction::FinishFunction(SourceLocation EndLoc) {
   // Emit function epilog (to return).
   llvm::DebugLoc Loc = EmitReturnBlock();
 
   if (ShouldInstrumentFunction())
     EmitFunctionInstrumentation("__cyg_profile_func_exit");
 
   // Emit debug descriptor for function end.
   if (CGDebugInfo *DI = getDebugInfo())
-    DI->EmitFunctionEnd(Builder);
+    DI->EmitFunctionEnd(Builder, CurFn);
 
   // Reset the debug location to that of the simple 'return' expression, if any
   // rather than that of the end of the function's scope '}'.
   ApplyDebugLocation AL(*this, Loc);
   EmitFunctionEpilog(*CurFnInfo, EmitRetDbgLoc, EndLoc);
   EmitEndEHSpec(CurCodeDecl);
 
   assert(EHStack.empty() &&