Page MenuHomePhabricator

[DebugInfo] Stop undef fragments from closing non-overlapping fragments
ClosedPublic

Authored by dstenb on May 24 2019, 4:49 AM.

Details

Summary

When DwarfDebug::buildLocationList() encountered an undef debug value,
it would truncate all open values, regardless if they were overlapping or
not. This patch fixes so that it only does that for overlapping fragments.

This change unearthed a bug that I had introduced in D57511,
which I have fixed in this patch. The code in DebugHandlerBase that
changes labels for parameter debug values could break DwarfDebug's
assumption that the labels for the entries in the debug value history
are monotonically increasing. Before this patch, that bug could result
in location list entries whose ending address was lower than the
beginning address, and with the changes for undef debug values that this
patch introduces it could trigger an assertion, due to attempting to
emit location list entries with empty ranges. A reproducer for the bug
is added in param-reg-const-mix.mir.

Diff Detail

Repository
rL LLVM

Event Timeline

dstenb created this revision.May 24 2019, 4:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 24 2019, 4:49 AM

LGTM with a couple of questions, will leave it for further comments.

test/DebugInfo/X86/undef-fragment.ll
27–31 ↗(On Diff #201195)

Is it OK to ditch the absolute addresses here? (Assuming that this isn't testing the address monoticity). I don't have any strong feeling, just the impression that this might be brittle to future codegen changes. (No big deal).

51–53 ↗(On Diff #201195)

Can we ditch the attributes?

dstenb updated this revision to Diff 201254.May 24 2019, 8:16 AM

Update test cases after review comments.

dstenb marked 2 inline comments as done.May 24 2019, 8:23 AM
dstenb added inline comments.
test/DebugInfo/X86/undef-fragment.ll
27–31 ↗(On Diff #201195)

Good idea! I think it is okay to test the monoticity in the other attached test case. I generalized the address checks so that they just verify that the entries are placed directly after each other.

51–53 ↗(On Diff #201195)

It should be okay to remove them I think. I kept dbg.value's attributes as I was not sure if it is customary to remove attributes from intrinsics.

aprantl accepted this revision.May 24 2019, 8:45 AM

That looks reasonable, thanks!

lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1178 ↗(On Diff #201254)

This could also become MachineInstr::isUndefDbgValue().

This revision is now accepted and ready to land.May 24 2019, 8:45 AM
dstenb updated this revision to Diff 201529.May 27 2019, 7:02 AM

Address comments (add MachineInstr:isUndefDebugValue query function)

dstenb updated this revision to Diff 201637.May 28 2019, 4:29 AM

Add a comment to the query function (undef debug values are mentioned in SourceLevelDebugging.rst, but I did not find any central explanations of such debug values in the code base).

I'm planning on landing this shortly, as I assume that the changes after the LGTM are small and non-controversial.

This revision was automatically updated to reflect the committed changes.