Page MenuHomePhabricator

[DebugInfo] Improve new line printing in debug line verbose output
ClosedPublic

Authored by jhenderson on Jun 3 2020, 8:09 AM.

Details

Summary

The new line printing for debug line verbose output was inconsistent. For new rows in the matrix, a blank line followed, whilst the DW_LNS_copy opcode actually resulted in two blank lines. There was also potential inconsistency in the blank lines at the end of the table. This patch mostly resolves these issues - no blank lines should appear in the output except for a single line after the prologue and at table end to separate it from any subsquent table, plus some instances after error messages.

Also add a unit test for verbose output to test the fine details of new line placement and other aspects of verbose output.

Depends on D80989.
Also depends on D80797.

Diff Detail

Event Timeline

jhenderson created this revision.Jun 3 2020, 8:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 3 2020, 8:09 AM
dblaikie added inline comments.Jun 3 2020, 11:31 AM
llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
714–717

This is the sort of "let's try to curate where errors print out relative to output" changes/direction I think are pretty unfortunate - being able to return errors rather than passing in error handlers seems like a better general coding paradigm where possible (while the callback system for warnings is great, and callbacks for errors are sometimes necessary - when there is some ability to print an error and continue) - returning errors seems clearer/more obvious/simpler control flow (there's no ability to accidentally print an error and continue because you forgot the error handler doesn't terminate execution/need to separately return? and what value do you return if you need to communicate to the caller they need to stop too etc)...

1070

Could you snapshot the length of the matrix before and after, and test if they're different rather than having to add a second operation next to every appendToMatrix? That would seem less error-prone (less possible to miss the pairing, have the two lines diverge in any way, etc)?

jhenderson marked an inline comment as done.Jun 4 2020, 3:15 AM
jhenderson added inline comments.
llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
714–717

The problem is that in the common case, most tools are doing the same thing with both recoverable and unrecoverable errors, it's just done at different times. This means that some error messages appear in the middle of parsing the table (before the terminating blank line), whilst others appear after the prologue has been printed, after the blank line, and seemingly with the next table. This is the incosistency and potential confusion I'm trying to avoid here.

I have a patch for this already, but essentially all that it does is change this function to return void, having passed the error to a separate UnrecoverableErrorHandler (which already exists in higher-level code). The change is NFC from the point of view of what gets parsed - the same handler will still get called allowing the client to stop or do whatever etc. The only difference is when the callback is called, and since it's a callback, the client can still handle the error as it did before. In practice, they both just get manifested as warnings most of the time, so the timing difference looks weird to most clients.

Whilst I get your point about simpler control flow, we are currently in a slightly weird hybrid situation where sometimes we use a callback and other times we return the error, so in some ways, I think this idea simplifies the interface.

I'll try to get my follow-up patch up for review to illustrate.

jhenderson updated this revision to Diff 268463.Jun 4 2020, 7:14 AM
jhenderson edited the summary of this revision. (Show Details)

Rebase and use Rows.size() to identify whether a new line should be printed.

jhenderson edited the summary of this revision. (Show Details)Jun 4 2020, 7:14 AM
jhenderson marked an inline comment as done.Mon, Jun 8, 6:40 AM

@dblaikie, I think the issues surrounding the error messages are best dealt with in D81165 (or other patches replacing that one). Would you be happy for this patch to land (possibly without the FIXME) in its current state? The bulk of it is improving the state of things (see the differences in behaviour indicated by the test changes, for example.

dblaikie accepted this revision.Mon, Jun 8, 12:25 PM

@dblaikie, I think the issues surrounding the error messages are best dealt with in D81165 (or other patches replacing that one). Would you be happy for this patch to land (possibly without the FIXME) in its current state? The bulk of it is improving the state of things (see the differences in behaviour indicated by the test changes, for example.

Sure - please remove the FIXME either now or later/once D81165 is resolved in some way.

This revision is now accepted and ready to land.Mon, Jun 8, 12:25 PM
This revision was automatically updated to reflect the committed changes.