Page MenuHomePhabricator

[DebugInfo] Print non-verbose output at same point as verbose output
ClosedPublic

Authored by jhenderson on Jun 2 2020, 3:49 AM.

Details

Summary

Verbose and non-verbose parsing of .debug_line produced their output at different points in the program. The most obvious impact of this was that error messages were produced at different times, but it also potentially reduced what clients could do by customising the stream or warning/error handlers.

This change makes the two variants consistent by printing non-verbose output inline, the same as verbose output.

Testing of the error messages has been modified to check the messages always appear in the same location to illustrate the behaviour.

Depends on D80803.
Also depends on D80874.

Diff Detail

Event Timeline

jhenderson created this revision.Jun 2 2020, 3:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 2 2020, 3:49 AM
jhenderson edited the summary of this revision. (Show Details)Jun 2 2020, 3:57 AM
JDevlieghere accepted this revision.Jun 2 2020, 9:41 AM
This revision is now accepted and ready to land.Jun 2 2020, 9:41 AM

Not a huge fan of this - seems like a bit of a contortion & I'm not sure the consistency of where warnings are printed is all that important? (perhaps this is more of the path that I'd have liked to avoid by not syncing/flushing/trying to line up these things in the first place)

Alternatively, though - could the APIs be changed here to make them more similar/consistent - rather than having an API that prints out and returns a result? What about going the other way and having a callback API that's called back for processed lines and unprocessed lines - and then it's printed on a per-line-table-entry basis, rather than buffered and dumped at the end? Eh, guess then you'd need a separate callback for the header too, etc.

Not a huge fan of this - seems like a bit of a contortion & I'm not sure the consistency of where warnings are printed is all that important? (perhaps this is more of the path that I'd have liked to avoid by not syncing/flushing/trying to line up these things in the first place)

Alternatively, though - could the APIs be changed here to make them more similar/consistent - rather than having an API that prints out and returns a result? What about going the other way and having a callback API that's called back for processed lines and unprocessed lines - and then it's printed on a per-line-table-entry basis, rather than buffered and dumped at the end? Eh, guess then you'd need a separate callback for the header too, etc.

There's probably an easier way, thinking about it - rather than have the verbose output behaviour be controlled by the presence of OS or not, simply pass in a separate Verbose argument for verbose output, and OS for verbose or non-verbose output. In other words, the three usages would become:

  1. No output at all - don't pass in OS and set Verbose to false (or possibly it doesn't matter). Code will just populate the rows, sequences etc.
  2. Brief non-verbose output - pass in OS and set Verbose to false. Code will populate the data structures and also print the rows as it goes, like the current verbose output, except without the instruction details etc.
  3. Verbose output - pass in OS and set Verbose to true. Code will behave the same as currently.

The second and third options differ in that some output is only printed in verbose mode, whilst other output is printed always. This feels more natural to me as an interface anyway. I'll take a look at that as an alternative. It will give us more consistent timings of when things are printed (possibly important if people are doing some crazy things with custom streams, plus helps with error message timing) without significantly complicating the interface or having to buffer the output unnecessarily.

jhenderson planned changes to this revision.Jun 3 2020, 3:30 AM

I actually think I prefer my API change suggestion to this patch, so I'm going to look at that now.

jhenderson updated this revision to Diff 268178.Jun 3 2020, 6:47 AM
jhenderson retitled this revision from [DebugInfo] Make verbose line output appear at same point as non-verbose to [DebugInfo] Print non-verbose output at some point as verbose output.
jhenderson edited the summary of this revision. (Show Details)

Completely change approach by changing the parsing API to take both a stream and verbose boolean - the presence of the stream indicates output should be emitted, whilst the boolean indicates whether verbose output should be emitted. This means that verbose and non-verbose output will be emitted at the same time. This patch also rebases the change on top of D80874, but a side-effect is that non-verbose output now includes a header if there are any opcodes whereas before it only included the column headers for a table where there are one or more rows. There is unlikely to be much difference between the two.

This revision is now accepted and ready to land.Jun 3 2020, 6:47 AM
jhenderson requested review of this revision.Jun 3 2020, 6:49 AM
jhenderson marked an inline comment as done.

Could I get another review of this now? I think it solves the problems of the previous version.

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
927–928

This is one of several places where new line printing seems incorrect. I am preparing a patch to fix this, and leaving this code in for now to minimise the number of behaviour changes in this patch.

dblaikie accepted this revision.Jun 3 2020, 11:36 AM

Looks good - some optional feedback.

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
747–749

Maybe put this outside the while loop so it doesn't have to be tested every time?

if (*OffsetPtr < EndOffset && OS)
  Row::dumpTableHeader(*OS << '\n', Verbose ? 12 : 0);
while (*OffsetPtr < EndOffset)
  ...

Though it does mean duplicating the loop condition (could put it in a lambda), which isn't ideal either - certainly up to you, just figured I'd float the idea.

923–925

Could drop the braces here

This revision is now accepted and ready to land.Jun 3 2020, 11:36 AM
jhenderson updated this revision to Diff 268407.Jun 4 2020, 3:24 AM

Rebase on latest version of D80874, and addressed @dblaikie's comments.

jhenderson marked 4 inline comments as done.Jun 4 2020, 3:25 AM
jhenderson added inline comments.
llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
747–749

This was solved by rebasing on @MaskRay's latest version of D80874.

923–925

Done, thanks.

jhenderson marked 2 inline comments as done.Jun 4 2020, 3:25 AM
MaskRay accepted this revision.Jun 4 2020, 11:34 AM

Looks great!

jhenderson retitled this revision from [DebugInfo] Print non-verbose output at some point as verbose output to [DebugInfo] Print non-verbose output at same point as verbose output.Mon, Jun 8, 7:00 AM
jhenderson updated this revision to Diff 269523.Tue, Jun 9, 6:23 AM
jhenderson marked an inline comment as done.

Rebase and fix a number of small issues. Feel free to give me any post-commit comments.

This revision was automatically updated to reflect the committed changes.