Page MenuHomePhabricator

[DebugInfo@O2] Fix PR41992: LiveDebugVariables can drop DBG_VALUEs through misinterpreting fragments
Needs ReviewPublic

Authored by Orlando on Aug 19 2019, 5:43 AM.

Details

Summary

Bug: https://bugs.llvm.org/show_bug.cgi?id=41992

This patch prevents DBG_VALUE instructions being incorrectly coalesced by
liveDebugVariables.

A 'UserValue' roughly represents a source variable. A UserValue was identified
by looking at the source variable, DIExpression, and the inlined-at location
of the source variable.

This is a problem because this means arbitrary DIExpressions were being used
to classify the UserValues. For example, a DW_OP_deref on the value presented
in the DBG_VALUE doesn't change what source variable we're describing. Instead
we should only be looking at the fragment info in the DIExpression.

This patch identifies UserValues by looking at the source vaiable, the variable
fragment info, and the inlined-at location. UserValue no longer tracks the
DIExpression associated with the set of DBG_VALUES.

DbgValueLocation seemed like a good place to keep track of the DIExpression
used by the DBG_VALUE because:

  1. This stops the locations with different DIExpressions being coalesced by the interval map used to track the uses.
  2. There is a 1-to-1 mapping of use locations to DIExpressions.
  3. Minimal source changes required to implement this fix.

I've added a test which covers the case outlined in the bug.

Diff Detail

Event Timeline

Orlando created this revision.Aug 19 2019, 5:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 19 2019, 5:43 AM

The testcase makes perfect sense and that sounds like a good bug to fix!
I do wonder wonder why the patch goes out of its way to save fragment infos when fragments don't appear in the test at all?

I think you may need at least one more test that involves fragments. Two non-overlapping fragments should be handled like DBG_VALUEs that describe different variables. Two (partially) overlapping fragments should be handled as if they are describing the same variable.

Thanks for the review!

I do wonder wonder why the patch goes out of its way to save fragment infos when fragments don't appear in the test at all?

The fragment info changes were a natural path since the DIExpression could no longer be used to identify the UserValues, but failing to test any IR with fragments was an oversight for sure.

Two (partially) overlapping fragments should be handled as if they are describing the same variable.

I hadn't considered the 'partially' case, I'll look into this now. I think my current patch will change shape a bit as a result.