Page MenuHomePhabricator

[llvm-dwarfdump] Add a table header for -debug-line -verbose output

Authored by MaskRay on May 30 2020, 12:23 PM.



Like non-verbose output, so that it is easy to recognize the Line,Column,File,ISA,Discriminator column values.

Diff Detail

Event Timeline

MaskRay created this revision.May 30 2020, 12:23 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 30 2020, 12:23 PM
MaskRay retitled this revision from [llvm-dwarfdump] Add a table header for -debug-info -verbose output to [llvm-dwarfdump] Add a table header for -debug-line -verbose output.May 30 2020, 12:35 PM

Column headers probably make some sense. This might clash with my ongoing patches in the area, but I think that's okay - we can both adapt patches as needed without any real dififculty. You should probably update the primary verbose test (i.e. verbose.test) rather than (or possibly in addition to) the test you have modified though. If you do it in addition, you probably don't need the --strict-whitespace/--match-full-lines options in the debug-line-dw-lne-end-sequence.s test.


Will 12 be the right number if using DWARF64 (i.e. with 64-bit section offsets)?

(probably not, but maybe okay for now with a FIXME/TODO for DWARF64, as it looks like we aren't currently printing the offsets as fixed width for DWARF64).

ikudrin added inline comments.Jun 1 2020, 2:43 AM

I'd prefer to keep the current convention of printing offsets. They are not something that has a real size in the file, like fields that we started printing with extended widths in D79997. They are just positions in sections and in most cases they will not exceed 4G boundary, even for the DWARF64 case. Printing all offsets in a unified way makes the report tidier.

MaskRay updated this revision to Diff 267692.Jun 1 2020, 12:10 PM

Update verbose.test instead

This revision is now accepted and ready to land.Jun 1 2020, 1:08 PM

Sorry, thought I posted this comment yesterday, but I guess not.


Another thought: maybe we should only print the column headers if there is no PrologueErr. I'm also not convinced we need the extra new line before the header. I'd ideally like to see a test for that if it is to be included.


By the way, I have a patch ready to put up for review to fix all these extra blank lines, so no need for others to fix them themselves.

MaskRay updated this revision to Diff 267903.Jun 2 2020, 9:17 AM
MaskRay marked 3 inline comments as done.

Remove empty line. Don't dump table header if PrologueErr

jhenderson accepted this revision.Jun 3 2020, 12:02 AM

LGTM, with one remaining request.


Since the rest of this test doesn't line up with the header here, I'd just get rid of the extra spacing. The test looks odd to me with it.

@MaskRay - are you okay to land this today? I have an update for D80989 that relies on it, and it'll be easier to work with this patch if it's already in master!

jhenderson requested changes to this revision.Jun 3 2020, 6:29 AM
jhenderson added inline comments.

Actually, just realised this is the wrong place for this - this will cause, in verbose output, the header to be printed, even if there are no rows, unlike non-verbose output. We should try to match the two up. Also, there's a new line in non-verbose output between the prologue and this header, please add it here too.

I think the best thing to do might be to move this code to just inside the while loop, executed on the first run only. This would mean the header is printed before any verbose opcodes are printed, although it would also mean it was printed even if there are no rows emitted to the table, due to a malformed table, which is slightly inconsistent. I'm open to alternatives. For now, I'm going to use my suggestion in my update to D80989. That will also cause the difference between non-verbose and verbose output to disappear, but not between those outputs and the LineTable::dump().

This revision now requires changes to proceed.Jun 3 2020, 6:29 AM
MaskRay updated this revision to Diff 268200.Jun 3 2020, 7:53 AM

Move the header within while (*OffsetPtr < EndOffset)

jhenderson accepted this revision.Jun 4 2020, 1:00 AM

Thanks for the update. LGTM, with nit.


Looks like this hasn't been addressed still?

This revision is now accepted and ready to land.Jun 4 2020, 1:00 AM
MaskRay updated this revision to Diff 268498.Jun 4 2020, 8:54 AM
MaskRay marked 2 inline comments as done.

Delete excess indentation

This revision was automatically updated to reflect the committed changes.