This is an archive of the discontinued LLVM Phabricator instance.

[MC] Generate .debug_aranges in the 64-bit DWARF format [4/7]
ClosedPublic

Authored by ikudrin on Jun 4 2020, 5:04 AM.

Diff Detail

Event Timeline

ikudrin created this revision.Jun 4 2020, 5:04 AM
dblaikie added inline comments.Jun 4 2020, 10:55 AM
llvm/test/MC/ELF/gen-dwarf64.s
25–26

Test that the length and CU offset were parsed correctly - since this patch changes how those are emitted.

This patch modifies EmitGenDwarfAranges(), which is used only when the assembler is generating DWARF to describe the assembler source.
I can't imagine anyone is really writing large enough assembler that they need to emit DWARF-64 format. What is the motivation here?

This patch modifies EmitGenDwarfAranges(), which is used only when the assembler is generating DWARF to describe the assembler source.
I can't imagine anyone is really writing large enough assembler that they need to emit DWARF-64 format. What is the motivation here?

Might be best to keep the fundamental design direction/question here in one thread ( https://reviews.llvm.org/D81144 ) rather than in all of them. Patches are in a series, so if that one ends up going another direction I don't think there's a risk these later ones will be reviewed/approved/committed in spite of that fundamental question.

So if we want DWARF-64 .debug_aranges for both asm source and higher-level language source....
This patch only does the asm source path, not the main path. I am not 100% clear where the other path is, but I see code to emit .debug_aranges in lib/DWARFLinker/DWARFStreamer.cpp, is that where the normal path is now?

So if we want DWARF-64 .debug_aranges for both asm source and higher-level language source....
This patch only does the asm source path, not the main path. I am not 100% clear where the other path is, but I see code to emit .debug_aranges in lib/DWARFLinker/DWARFStreamer.cpp, is that where the normal path is now?

Yep, adding support for higher level source/LLVM IR will be a separate patch/independent change. It'll be in llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp - llvm::DwarfDebug::emitDebugARanges

jhenderson added inline comments.Jun 5 2020, 12:33 AM
llvm/lib/MC/MCDwarf.cpp
913–916

As mentioned elsewhere, there are two code paths here, but only one seems to be tested. Do we need another test case?

ikudrin updated this revision to Diff 268811.Jun 5 2020, 8:26 AM
ikudrin marked 3 inline comments as done.
  • Extended a test for Mach-O so that all changed code paths are covered.
  • Added more checks for the .debug_aranges section to validate that it is not skewed.
ikudrin added inline comments.Jun 5 2020, 8:32 AM
llvm/lib/MC/MCDwarf.cpp
913–916

MachO/gen-dwarf64.s is extended to cover another code path.

jhenderson added inline comments.Jun 8 2020, 12:29 AM
llvm/test/MC/ELF/gen-dwarf64.s
25

Optionally, you could delete the bit after the cu_offset since it's not important.

llvm/test/MC/MachO/gen-dwarf64.s
14

Same comment as above.

jhenderson accepted this revision.EditedJun 8 2020, 12:29 AM

LGTM, with or without my inline suggestions, I don't mind.

This revision is now accepted and ready to land.Jun 8 2020, 12:29 AM
ikudrin updated this revision to Diff 269152.Jun 8 2020, 3:34 AM
  • Added --implicit-check-not="R_{{.*}} .debug_" to the test to ensure that all relocations to debug sections are checked.
jhenderson accepted this revision.Jun 8 2020, 3:49 AM
This revision was automatically updated to reflect the committed changes.