Page MenuHomePhabricator

Updating .debug_line section version information to match DWARF version.
ClosedPublic

Authored by kromanova on Jan 28 2016, 4:28 PM.

Details

Summary

Hello,
The compiler always emits .debug_line version 2, though some opcodes from DWARF 3 (e.g. DW_LNS_set_prologue_end, DW_LNS_set_epilogue_begin or DW_LNS_set_isa) and from DWARF 4 could be emitted by the compiler. That's not exactly right.

This patch changes version information of .debug_line to exactly match the DWARF version (which is 4 by default).
For .debug_line version 4, a new field maximum_operations_per_instruction is emitted. I'm not exactly sure how to set this field correctly for VLIW architectures. Hopefully, there are some experts in the community that could help out (any Hexagon debuginfo developers out there?).

I’ve tried a few tools to check if they could handle .debug_line version 4.
On a simple testcase, I verified that GDB debugger 7.7 understands .debug_line version 4.
Sony’s proprietary debugger can understand it too.
I don't have LLDB built, so I haven’t tried it.
GCC (older version 4.8.2) still emits version 2 for the .debug_line section, even though the DWARF version is set to 4. I'm not sure about the latest GCC.
GNU's readelf (version 2.24) can handle .debug_line version 4.

I added 3 new tests debug-line-version2.ll, debug-line-version3.ll and debug-line-version4.ll. The IR is generated from one source using different options (-gdwarf-2, -gdwarf-3 and –gdwarf-4). Any idea how to merge these three tests into one?

Let me know what do you think about the patch.

Diff Detail

Repository
rL LLVM

Event Timeline

kromanova updated this revision to Diff 46324.Jan 28 2016, 4:28 PM
kromanova retitled this revision from to Updating .debug_line section version information to match DWARF version..
kromanova updated this object.
kromanova added reviewers: dblaikie, echristo, probinson.
kromanova set the repository for this revision to rL LLVM.
kromanova added a subscriber: cfe-commits.
rafael edited subscribers, added: llvm-commits; removed: cfe-commits.Jan 28 2016, 4:43 PM
dblaikie edited edge metadata.Jan 28 2016, 4:57 PM
dblaikie added a subscriber: dblaikie.

+Adrian for Apple/LLDB perspective

One way to merge the tests would be to add another command line argument
into DwarfDebug (see the way the split-dwarf command line argument is
used), just for testing purposes. The command line argument can override
the value specified in the metadata - so you can have one input file for
the 3 different versions.

probinson edited edge metadata.Jan 28 2016, 5:11 PM

I added 3 new tests debug-line-version2.ll, debug-line-version3.ll and debug-line-version4.ll. The IR is generated from one source using different options (-gdwarf-2, -gdwarf-3 and –gdwarf-4). Any idea how to merge these three tests into one?

Don't start from a .ll file? Your code change is in MC, so it's fine to test this with llvm-mc starting from assembler source. llvm-mc has a -dwarf-version option so you can control the version and the associated checks from the RUN line.
I see that test/MC/ELF/gen-dwarf.s does this kind of thing, for example.
And test/MC/ARM/dwarf-asm-multiple-sections.s seems to have accumulated version tests of this nature.

lib/MC/MCDwarf.cpp
282 ↗(On Diff #46324)

The other state-machine parameters are all pretty obvious, but a raw "1" is not, so it ought to have a comment.

kromanova updated this revision to Diff 74153.Oct 10 2016, 11:54 AM
kromanova edited edge metadata.

Here is the revised patch. I have addressed Paul Robinson's comments.
Sorry for the delay. Somehow this patch patch fell off my radar.

Question: If you grep LLVM sources for " DWARF2Line, you'll see that a lot of identifier names imply that line_table is DWARF2. e.g. "DWARF2LineOpcodeBase". I suspect this should be changed to avoid confusion. Do you agree? Separate NFC patch?

Adding Krzysztof Parzyszek to answer a question on how current patch changing .debug_line match the emitted DWARF version (which is 4 by default) affects Hexagon and to note that additional work is needed to set up maximum_operations_per_instruction field correctly for VLIW platforms.

Question: If you grep LLVM sources for " DWARF2Line, you'll see that a lot of identifier names imply that line_table is DWARF2. e.g. "DWARF2LineOpcodeBase". I suspect this should be changed to avoid confusion. Do you agree? Separate NFC patch?

Seems reasonable to me but I would generally expect that kind of name-change patch to be done separately.

Question: If you grep LLVM sources for " DWARF2Line, you'll see that a lot of identifier names imply that line_table is DWARF2. e.g. "DWARF2LineOpcodeBase". I suspect this should be changed to avoid confusion. Do you agree? Separate NFC patch?

Seems reasonable to me but I would generally expect that kind of name-change patch to be done separately.

That sounds good. I will do it after this patch is accepted. I would really like to get this patch reviewed (and tested) by someone who cares about VLIW.

echristo accepted this revision.Oct 13 2016, 11:04 PM
echristo edited edge metadata.

Seems entirely reasonable. Not sure what to do about the hexagon port - also, what do you have in mind for them to fix this? Check the target?

test/DebugInfo/X86/debug-line-version.s
6 ↗(On Diff #74153)

"generate"

This revision is now accepted and ready to land.Oct 13 2016, 11:04 PM

Not sure what to do about the hexagon port - also, what do you have in mind for them to fix this? Check the target?

Hi Eric,
Thank you for the review!

For Hexagon (or any other VLIW architecture) it will be necessary to check the target and implement support for 'op_index' register. I don’t think I will be the right person to implement support for calculating operation pointer within the bundle for VLIW architectures. I won’t be able to check/test that it works correctly.

I’m also quite concerned that not everyone who cares will actually read this patch. It might break some consumer tools like debuggers and dwarf dumpers, because they are not aware of maximum_operations_per_instruction field. I checked that “main suspects” GDB (version 7.7), readelf (version 2.24), latest llvm-dwarfdump understand maximum_operations_per_instruction; however, obviously, that’s not a complete list of debug info consumers.

Taking all this information into consideration, should I go ahead and commit? I have fixed the typo that you noticed.

Katya

Let's go ahead and add Krzysztof to this thread so he's aware for the Hexagon port and whether or not his debugger understands op_index at all. This patch may need to be conditionalized on that to avoid something going haywire.

-eric

kparzysz edited edge metadata.Oct 18 2016, 2:16 PM

Thanks, this is actually important for us. I'll get someone more familiar with the subject to take a look.

Thanks, this is actually important for us. I'll get someone more familiar with the subject to take a look.

Thank you! Please let me know how this patch will affect your tools.

kparzysz added a subscriber: ted.Oct 19 2016, 11:07 AM

Based on the discussion on lldb-dev, this seems ok. LGTM.

OK, let's go ahead and commit and go from there.

Thanks for your patience here.

-eric

Great! Thank you. I will rebase and commit.

This revision was automatically updated to reflect the committed changes.
kromanova reopened this revision.Oct 28 2016, 12:37 AM

I have committed this patch today r285355 and have to revert it shortly in r285362 because of two reasons:

(1) Unfortunately, some of the sanitizer tests failed with the following error.

Command Output (stdout):
--
while processing /var/folders/09/r4vw4v8n5kb67jl66zvlbljw0000gn/T/lit_tmp_XSTjqx/closed-fds-412f78.o:
warning: line table paramters mismatch. Cannot emit.
Closing streams.

--
Command Output (stderr):
--
/Users/buildslave/jenkins/sharedspace/phase1@2/llvm/projects/compiler-rt/test/asan/TestCases/Posix/closed-fds.cc:31:17: error: expected string not found in input
// CHECK-FILE: {{ #0 0x.* in main .*closed-fds.cc:}}[[@LINE-4]]
                ^
<stdin>:25:2: note: scanning from here
#0 0x49b2d in main (/Users/buildslave/jenkins/sharedspace/phase1@2/clang-build/projects/compiler-rt/test/asan/I386DarwinConfig/TestCases/Posix/Output/closed-fds.cc.tmp+0x1b2d)
^

Here is the link with the failure that buildbot sent me:
http://lab.llvm.org:8080/green/job/clang-stage1-configure-RA_check/22826/
I'm not sure why these tests are failing. I have re-run all the sanitizer tests on my machines (x86-64 linux) and I didn't see any failures. Any clues?

(2) Some tests failed because the generated length of the line table was different from the expected value of
// DWARF2-NEXT: total_length: 0x00000077
For some of the tested configurations the generated total_length was 0x0000008c, 0x00000091 or 0x00000089.

There are lot of tests in LLVM regression test suite that check against hardcoded value of line table length, so I used similar strategy. I wonder what's so special about this particular case and why is the length of line table is different.

I guess, in this particular test it's not necessary to check against a hardcoded value of the line table length. Instead, I could just make sure that for DWARF2 and DWARF3 we generate the length of the table 'X' and for DWARF4, we generate length 'X+1'. I'm not sure if it's easy to do with FileCheck. Can we use pattern matching in FileCheck to capture [[X]], when patterns are in different test configuration (i.e. being tested with different -check-prefix)? Can we use captured value in expressions to calculate [[X+1]] similar to how it's done with [[@LINE]]?

Script:
--
/home/buildbots/ppc64le-clang-lnt-test/clang-ppc64le-lnt/stage1/./bin/llvm-mc -g -dwarf-version 2 -triple  i686-pc-linux-gnu /home/buildbots/ppc64le-clang-lnt-test/clang-ppc64le-lnt/llvm/test/DebugInfo/X86/debug-line-version.s -filetype=obj -o - | /home/buildbots/ppc64le-clang-lnt-test/clang-ppc64le-lnt/stage1/./bin/llvm-dwarfdump - | /home/buildbots/ppc64le-clang-lnt-test/clang-ppc64le-lnt/stage1/./bin/FileCheck --check-prefix=DWARF2 /home/buildbots/ppc64le-clang-lnt-test/clang-ppc64le-lnt/llvm/test/DebugInfo/X86/debug-line-version.s
/home/buildbots/ppc64le-clang-lnt-test/clang-ppc64le-lnt/stage1/./bin/llvm-mc -g -dwarf-version 3 -triple  i686-pc-linux-gnu /home/buildbots/ppc64le-clang-lnt-test/clang-ppc64le-lnt/llvm/test/DebugInfo/X86/debug-line-version.s -filetype=obj -o -  |/home/buildbots/ppc64le-clang-lnt-test/clang-ppc64le-lnt/stage1/./bin/llvm-dwarfdump - | /home/buildbots/ppc64le-clang-lnt-test/clang-ppc64le-lnt/stage1/./bin/FileCheck --check-prefix=DWARF3 /home/buildbots/ppc64le-clang-lnt-test/clang-ppc64le-lnt/llvm/test/DebugInfo/X86/debug-line-version.s
/home/buildbots/ppc64le-clang-lnt-test/clang-ppc64le-lnt/stage1/./bin/llvm-mc -g -dwarf-version 4 -triple  i686-pc-linux-gnu /home/buildbots/ppc64le-clang-lnt-test/clang-ppc64le-lnt/llvm/test/DebugInfo/X86/debug-line-version.s -filetype=obj -o - | /home/buildbots/ppc64le-clang-lnt-test/clang-ppc64le-lnt/stage1/./bin/llvm-dwarfdump - | /home/buildbots/ppc64le-clang-lnt-test/clang-ppc64le-lnt/stage1/./bin/FileCheck  --check-prefix=DWARF4 /home/buildbots/ppc64le-clang-lnt-test/clang-ppc64le-lnt/llvm/test/DebugInfo/X86/debug-line-version.s
--
Exit Code: 1

Command Output (stderr):
--
/home/buildbots/ppc64le-clang-lnt-test/clang-ppc64le-lnt/llvm/test/DebugInfo/X86/debug-line-version.s:20:17: error: expected string not found in input
// DWARF2-NEXT: total_length: 0x00000077
                ^
<stdin>:67:2: note: scanning from here
 total_length: 0x00000091
 ^
This revision is now accepted and ready to land.Oct 28 2016, 12:37 AM
kromanova updated this revision to Diff 81615.Dec 15 2016, 10:40 AM
kromanova edited edge metadata.
kromanova removed rL LLVM as the repository for this revision.

I have committed this patch today r285355 and have to revert it shortly in r285362, because some tests were failing.

Sorry, this patch fell out of my radar for a while. Yesterday I investigated why the tests failed sometimes ( (the size of the line_table varied depending on the full file name and we check the size of line_table). I have updated the test not to depend on the full file name. When this patch gets approved, I will re-commit it.

Test update LGTM.

This revision was automatically updated to reflect the committed changes.