This patch adds dumping of line table instructions as well as the final
state at each specified pc value in verbose mode. This is essentially
the same as the default in Darwin's dwarfdump. Dumping the actual line
table opcodes can be particularly useful for something like debugging a
bad .debug_line section.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
Doing this during parsing doesn't seem too intrusive and looks like it is the right trade-off.
Do you have a test case, too?
Jup, I put up the code before/while fixing the tests. I've added a verbose.test to check the printing of the opcodes. The existing tests that use verbose mode essentially verify that the resulting rows are still identical. As a verification I also ran a diff against Darwin's dwarfdump and the printed opcodes are identical.
lib/DebugInfo/DWARF/DWARFDebugLine.cpp | ||
---|---|---|
445 ↗ | (On Diff #115662) | If we don't have one already, should probably have a DW_LNE enum stringification function generated from Dwarf.def - and use that to print the DW_LNE_* first, outside the switch, then use the switch to dump the specific parameters, if any, etc. |
462 ↗ | (On Diff #115662) | There's some inconsistency in whitespace (some of the printing "DW_LNE_foo (" and some without the space) - pick one for consistency. |
test/DebugInfo/MIR/X86/empty-inline.mir | ||
14–16 ↗ | (On Diff #115662) | Perhaps it's worth un-verbosifying test cases when they need to be touched manually like this anyway? |
Added a couple more inline comments.
Since you aren't checking for the verbose details in many of the modified tests, perhaps it would be better to replace the RUN command with a --debug-line (non-verbose) invocation?
This is not super important, so feel free to decide what to do on a case-by-case basis.
lib/DebugInfo/DWARF/DWARFContext.cpp | ||
---|---|---|
340 ↗ | (On Diff #115662) | Add: // Verbose dumping is done during parsing and not on the intermediate representation. |
lib/DebugInfo/DWARF/DWARFDebugLine.cpp | ||
445 ↗ | (On Diff #115662) | To avoid typos and make the binary smaller, I'd prefer: |
Thanks Dave, I've updated the diff with your feedback.
test/DebugInfo/MIR/X86/empty-inline.mir | ||
---|---|---|
14–16 ↗ | (On Diff #115662) | That's how I got started before switching to this approach. I wasn't confident enough about the content of the verbose output not being part of the test. I definitely don't want to reduce the test quality for this kind of change. If there are tests that stick out where it would obviously not matter, I'd be more than happy to change it! |
lib/DebugInfo/DWARF/DWARFDebugLine.cpp | ||
---|---|---|
444–445 ↗ | (On Diff #115666) | Could this go outside the switch? So it's written once rather than in every case in the switch? Or are there some cases that need to avoid it or otherwise do something different? (& if possible, the newline printing could also go after the switch to avoid that being repeated in every case) |
test/DebugInfo/MIR/X86/empty-inline.mir | ||
14–16 ↗ | (On Diff #115662) | Seems like the opposite, though - by removing the CHECK-NEXTs you've reduced the quality of the test by allowing other line entries to be present, etc, potentially. So it's probably better here, for instance, to unverbosify the test to keep it closer to the original intent. (possibly similarly on other tests) |
test/DebugInfo/MIR/X86/empty-inline.mir | ||
---|---|---|
14–16 ↗ | (On Diff #115662) | Fair enough, I agree that this is probably better. Thanks! |
test/DebugInfo/MIR/X86/empty-inline.mir | ||
---|---|---|
7 ↗ | (On Diff #115847) | AFAIK only leading and trailing whitespace is ignored. An additional benefit is that most of the lines won't have to be changed if we ever want to make the test verbose again. |
test/MC/ARM/dwarf-asm-multiple-sections.s | ||
55–58 ↗ | (On Diff #115847) | That's because added the 3 run lines at the top of the file, before the code. |
test/DebugInfo/MIR/X86/empty-inline.mir | ||
---|---|---|
7 ↗ | (On Diff #115847) | I am pretty sure that FileCheck treats all whitespace as variable length, unless you tell it to be strict. |
test/DebugInfo/MIR/X86/empty-inline.mir | ||
---|---|---|
7 ↗ | (On Diff #115847) | Seems like you were both right! I've updated the tests. |