Like non-verbose output, so that it is easy to recognize the Line,Column,File,ISA,Discriminator column values.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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.
llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp | ||
---|---|---|
707 | 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). |
llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp | ||
---|---|---|
707 | 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. |
Sorry, thought I posted this comment yesterday, but I guess not.
llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp | ||
---|---|---|
706–707 | 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. | |
llvm/test/tools/llvm-dwarfdump/X86/verbose.test | ||
21–22 | 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. |
LGTM, with one remaining request.
llvm/test/tools/llvm-dwarfdump/X86/debug_line_invalid.test | ||
---|---|---|
41–42 | 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. |
llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp | ||
---|---|---|
711–712 | 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(). |
Thanks for the update. LGTM, with nit.
llvm/test/tools/llvm-dwarfdump/X86/debug_line_invalid.test | ||
---|---|---|
41–42 | Looks like this hasn't been addressed still? |
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).