Page MenuHomePhabricator

Fix DwarfDebug assertions with LTO mixing -g and -gmlt

Authored by probinson on Jan 30 2017, 3:23 PM.



When LTO inlines functions compiled with -g into functions compiled
-gmlt, certain assumptions don't hold any more.

Fixes PR31437.

Diff Detail


Event Timeline

probinson created this revision.Jan 30 2017, 3:23 PM
dblaikie edited edge metadata.Jan 31 2017, 8:38 AM

Given the complexity of the test cases, this might be easier as two separate reviews.

The test cases should be simplified a fair bit further, I think - especially removing user defined types should be helpful in reducing the amount of debug info metadata and possibly making things clearer. (also, in the second test it mentions the possibility of removing the (non-inlined) definition of 'foo' - mark 'foo' as inline in the source and this should happen automatically. If possible, use attribute((always_inline)) to ensure the test case is fairly optimization independent, but I guess since it needs to optimize away the parameter alloca it won't be doable at -O0 anyway, so maybe that's not important)

1151 ↗(On Diff #86356)

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.

1225–1229 ↗(On Diff #86356)

Do bad things happen if we don't clear this? Might be useful if the comment described what undesirable work this avoids?

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.

1151 ↗(On Diff #86356)

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?

1225–1229 ↗(On Diff #86356)

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?

dblaikie added inline comments.Jan 31 2017, 11:40 AM
1 ↗(On Diff #86356)

Here's the simplest example I have for this (also doesn't require optimizations either at compile time or after the link step (though they can be used to tidy up the IR a little):

// first file
void f1();
inline __attribute__((always_inline)) void f2(int) {
void f3() {

// second file
void f() {
1 ↗(On Diff #86356)

Interestingly, this still seems to trigger the assert:

void f()
void foo(int param) {
  if (param)

void foo(int);
void bar() {

Which is vaguely worrying - since in this case the call to foo is not/cannot be optimized away, so I'm not sure what the assertion is telling us here. There should be instructions for the function & so we shouldn't be in that codepath.

probinson added inline comments.Jan 31 2017, 3:05 PM
1 ↗(On Diff #86356)

I have not been able to get your example to crash on an unmodified compiler, can you provide more details?

1 ↗(On Diff #86356)

Why can't it be inlined away? bar() calls foo(), foo() gets inlined into bar(), the constant 0 parameter is propagated to the 'if', the condition is guaranteed false therefore the 'if/then' goes away, poof all gone. Except for the dbg.value describing the inlined copy of 'param'.

probinson updated this revision to Diff 86687.Feb 1 2017, 11:39 AM

Reduce patch to just the DbgValues.empty() assertion; the other will become a new review.
Use dblaikie's simpler test case.

I've just deleted the assertion, because we call DebugHandlerBase::endFunction() shortly after this, which will clear DbgValues for us.

dblaikie accepted this revision.Feb 1 2017, 1:52 PM

Looks good - thanks!

This revision is now accepted and ready to land.Feb 1 2017, 1:52 PM
This revision was automatically updated to reflect the committed changes.