[PATCH] D29310: Fix DwarfDebug assertions with LTO mixing -g and -gmlt

[Paul Robinson via Phabricator via llvm-commits]

probinson added a comment.

I was wondering whether it might be too much for one review.  Easy enough to break it up.

I've already spent a bunch of time trying to reduce the test cases, but I'll have another go at it.



================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:1151
+  // verify this.
+  PrevAVSize = AbstractVariables.size();
+
----------------
dblaikie wrote:
> Rather than adding a new member variable & diagnosing this case where we were before - perhaps it'd make for a more actionable assertion if we change how we're doing this.
> 
> There's exactly one place that's (admittedly, currently - this could change, so might make what I'm proposing more brittle) creating AbstractVariables - in DwarfDebug::createAbstractVariable.
> 
> From that function it should be possible to check the debug info level (CurFn->getFunction()->getSubprogram()->getUnit()->getEmissionKind()) and assert that it is not LineTablesOnly. That way it'll fail faster/more informatively, and requires less wide-scoped state tracking, etc.
I'd be ecstatic to avoid the extra state.  I was really not happy about that.
However it's not actually wrong to create abstract variables in a -gmlt function, *if* that function has inlined a -g function.  I was thinking maybe createAbstractVariable should assert that there are abstract scopes, but looking at the paths into that method, you can only get there if you already have an abstract scope (so the assertion would be pretty pointless).
Should we just abandon the original assertion?


================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:1225-1229
+    // With LTO, a function compiled -gmlt could inline a function compiled -g,
+    // which could have all real instructions optimized/folded away, which in
+    // turn means no abstract scopes would be created. But the DbgValues will
+    // still be there.  Forget them.
+    DbgValues.clear();
----------------
dblaikie wrote:
> Do bad things happen if we don't clear this? Might be useful if the comment described what undesirable work this avoids?
Hmmm maybe nothing bad happens, and we just eliminate the assertion.
DbgValues won't generate any line-table entries, and we've already seen that they won't generate any abstract scopes, so maybe it's benign to just ignore them?


https://reviews.llvm.org/D29310