Page MenuHomePhabricator

[llvm] Prevent duplicate files in debug line header in dwarf 5.
ClosedPublic

Authored by tamur on Mar 18 2019, 3:21 PM.

Details

Summary

Motivation: In previous dwarf versions, file name indexes started from 1, and
the primary source file was not explicit. Dwarf 5 standard (6.2.4) prescribes
the primary source file to be explicitly given an entry with an index number 0.

The current implementation honors the specification by just duplicating the
main source file, once with index number 0, and later maybe with another
index number. While this is compliant with the letter of the standard, the
duplication causes problems for consumers of this information such as lldb.
(Some files are duplicated, where only some of them have a line table although
all refer to the same file)

With this change, dwarf 5 debug line section files always start from 0, and
the zeroth entry is not duplicated whenever possible. This requires different
handling of dwarf 4 and dwarf 5 during generation (e.g. when a function returns
an index zero for a file name, it signals an error in dwarf 4, but not in dwarf 5)
However, I think the minor complication is worth it, because it enables all
consumers (lldb, gdb, dwarfdump, objdump, and so on) to treat all files in the
file name list homogenously.

Diff Detail

Repository
rL LLVM

Event Timeline

tamur created this revision.Mar 18 2019, 3:21 PM
echristo edited reviewers, added: dblaikie, probinson, aprantl; removed: espindola.Mar 18 2019, 4:10 PM

Thanks for working on this! I knew I had left duplications behind, but the task to take care of them hadn't ever gotten to the top of the list.
Mostly style things to start with.

llvm/include/llvm/MC/MCContext.h
528 ↗(On Diff #191187)

You could constify as a separate NFC preliminary.

llvm/include/llvm/MC/MCDwarf.h
221 ↗(On Diff #191187)

Because the only constructor requires a DwarfVersion parameter, you don't need to initialize here?

224 ↗(On Diff #191187)

explicit ? Don't want this to be a converting constructor.

llvm/lib/MC/MCAsmStreamer.cpp
1914 ↗(On Diff #191187)

You could constify as a separate NFC preliminary.

llvm/lib/MC/MCDwarf.cpp
545 ↗(On Diff #191187)

Current style is lowerCamelCase for methods, so isRootFile here.
I know MC is inconsistent on this point, but new code should use the new style.

llvm/test/CodeGen/AMDGPU/flat-error-unsupported-gpu-hsa.ll
11 ↗(On Diff #191187)

This looks unrelated, please rebase.

llvm/test/tools/llvm-objdump/X86/function-sections-line-numbers.s
33 ↗(On Diff #191187)

Curious why this .file 1 is still present, it looks redundant now.

tamur updated this revision to Diff 191781.Mar 21 2019, 2:26 PM
tamur marked 6 inline comments as done.Mar 21 2019, 2:28 PM

I moved nfc changes to a separate patch.

llvm/test/tools/llvm-objdump/X86/function-sections-line-numbers.s
33 ↗(On Diff #191187)

(sorry, it was an oversight)

probinson accepted this revision.Mar 22 2019, 4:04 AM

A couple more style nits and LGTM.

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
1024 ↗(On Diff #191781)

No 'else' after a 'return'

1033 ↗(On Diff #191781)

No 'else' after a 'return'

This revision is now accepted and ready to land.Mar 22 2019, 4:04 AM
dblaikie added inline comments.Mar 22 2019, 11:05 AM
llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.h
104 ↗(On Diff #191781)

accessor probably could/should be a const member function

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
1034 ↗(On Diff #191781)

Perhaps in v4 we could keep Index == array index, and leave FileNames[0] unused? (I guess then it might need a special case when being emitted to skip over FileNames[0]?)

llvm/lib/MC/MCDwarf.cpp
549–550 ↗(On Diff #191781)

Probably more common to write a nullptr check as "!X" in LLVM code.

tamur updated this revision to Diff 191947.Mar 22 2019, 2:09 PM
tamur marked an inline comment as done.
tamur retitled this revision from Prevent duplicate files in debug line header in dwarf 5. to [llvm] Prevent duplicate files in debug line header in dwarf 5..
tamur marked 3 inline comments as done.Mar 22 2019, 2:11 PM
tamur added inline comments.
llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
1034 ↗(On Diff #191781)

I'd prefer not to change dwarf4 behavior as much as possible, if that's ok with you?

dblaikie added inline comments.Mar 22 2019, 2:22 PM
llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
1034 ↗(On Diff #191781)

Within this change, that makes sense - but if you have time to make this change as a separate patch, it'd be worth trying (to see if it's a good simplification, etc) as a follow-up to this.

LLVM's testing is pretty robust such that we can usually make changes like this (especially to existing functionality like DWARFv4 support that's been directly and indirectly tested by lots of test cases over the years) with pretty high confidence that if the tests pass, the change is OK & ideally the chaneg I suggesting wouldn't change the overall behavior, but be a refactoring/change in implementation only.

This revision was automatically updated to reflect the committed changes.