Page MenuHomePhabricator

[DWARF5] Enable .debug_line.dwo section emission if macro info is present
AbandonedPublic

Authored by SouraVX on Jun 9 2020, 9:06 AM.

Details

Summary

Previously the .debug_line.dwo section was only emitted only if type
units are present. This patch further extends emission for the case if
.debug_macro.dwo section is present.

For enabling this, I've refactored out SplitLineTable from
DwarfTypeUnit to DwarfUnit so that it can be emitted for when
type units are not present. Emission is further restricted to only
when .debug_macro.dwo section is present.

Ideally, this should've splitted into more than one patches, but
that can be done post review. This patch is intended to convey the overall
direction.

Diff Detail

Event Timeline

SouraVX created this revision.Jun 9 2020, 9:06 AM
Herald added a project: Restricted Project. · View Herald Transcript
SouraVX added a project: debug-info.
SouraVX added a subscriber: alok.
probinson added inline comments.Jun 10 2020, 11:00 AM
llvm/lib/CodeGen/AsmPrinter/DwarfUnit.h
298

either this->SplitLineTable = SplitLineTable; or change the name of the parameter.

SouraVX updated this revision to Diff 270033.Jun 10 2020, 10:05 PM

Addressed @probinson review comments. Thanks for this!

SouraVX marked 2 inline comments as done.Jun 10 2020, 10:07 PM
SouraVX added inline comments.
llvm/lib/CodeGen/AsmPrinter/DwarfUnit.h
298

Let's keep the former. Thanks!

SouraVX marked an inline comment as done.Jun 15 2020, 11:40 AM

Ping @ all reviewers!

This revision is now accepted and ready to land.Jun 15 2020, 11:45 AM
dblaikie requested changes to this revision.Jun 15 2020, 12:52 PM

I think this needs a bit more work - or at least I have a few more questions.

llvm/include/llvm/MC/MCDwarf.h
303

This function appears to be unused (& should be removed/omitted), if I'm not mistaken.

305

Is this necessary? If no one ever retrieves/uses the label anyway.

307

Can this be avoided? Currently the split line table is enabled/emitted implicitly, based on whether or not there's anything in it, I think? Hopefully that behavior can extend to the debug_macro use case as well?

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1018–1025

(hopefully this whole block can be removed/avoided - based on the prior point about "enableSplitLineTable" Being avoided)

Also, putting a CU stmt_list 0 seems like it has a good chance to confuse/break consumers - since they could conclude that this attribute means that all decl_files should be looked up in the debug_line.dwo section, whereas they should probably be looking in the .debug_line section?

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.h
406–410

This is effectively unused and should be removed/avoided - also, comments generally should refer to/explain/justify the current state of the code, not a future state. (variables, etc, can be introduced as/when they are needed, not before)

The future use case for debug_macro.dwo's debug_line_offset might be fine with a hardcoded zero (as you had used below for the stmt_list ("NewCU.addSectionOffset(Die, dwarf::DW_AT_stmt_list, 0);")), avoiding the need for an explicit label here. (though a label could be used, if it is, I'd probably suggest having the MCDwarfDwoLineTable generate it itself, when requested - and could also use that as a signal to emit the debug_line.dwo if needed))

783

This function is unused and should be removed/avoided.

llvm/lib/CodeGen/AsmPrinter/DwarfUnit.h
84–85

Rather than plumbing this through to every unit - might be better to go the other way, removing the existing plumbing into DwarfTypeUnit, and replace it with accessing this directly from DwarfDebug whenever needed. (since there's only one of them anyway - so we don't need to track different ones per-unit)

This revision now requires changes to proceed.Jun 15 2020, 12:52 PM
SouraVX marked 3 inline comments as done.Jun 15 2020, 1:18 PM

Thanks @dblaikie for reviewing this!

llvm/include/llvm/MC/MCDwarf.h
303

Ah Sorry, but I think I conveyed the intent(Splitting up later, if deemed necessary) in patch summary correctly. All these functions will be later used by D80945.

305

Yes this is crucial(setting up label explicitly) for setting up debug_line_offset in debug_line.dwo.
This will be finally emitted in ASM as:
.Lsplit_unit_line_table0
Later in debug_line_offset can be emitted as a difference of labels as:
.Lsplit_unit_line_table0-.debug_line.dwo

307

Not I believe, since SplitLineTable is emitted/enabled for the case when TypeUnits are present. This helper allow us to enable emission(later handeld in MCDwarf)if macro information is present.

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1018–1025

Also, putting a CU stmt_list 0 seems like it has a good chance to confuse/break consumers - since they could conclude that this attribute means that all decl_files should be looked up in the debug_line.dwo section, whereas they should probably be looking in the .debug_line section?

This whole block is bounded by the else condition(also further restricted by macro info) i.e control-flow enters here only for SplitDwarf. So there won't be any ambiguity.

SouraVX marked 3 inline comments as not done.Jun 15 2020, 1:27 PM
SouraVX added inline comments.
llvm/lib/CodeGen/AsmPrinter/DwarfUnit.h
84–85

Sorry, not able to follow you here. SplitLineTable was already present in DwarfTypeUnit hence it was restricting the enablement for present case.
So I moved it from there and promoted to DwarfUnit so that both case(TypeUnit and debug_macro.dwo) where SplitLineTable is required should be able to use it.

dblaikie added inline comments.Jun 15 2020, 3:59 PM
llvm/include/llvm/MC/MCDwarf.h
303

It's great to stage refactorings separately where possible - but generally the state of the codebase should be fairly self-consistent at any point in time. So adjusting the code to make the line table accessible to more places (eg: removing the pointer from DwarfTypeUnit and to just use it from DwarfDebug directly before adding more usage (or the refactoring you've proposed, adding it to all DwarfUnits, if that was the way to go - which I don't think it is)).

305

Could it just be emitted as zero, the same way as the stmt_list you had added?

In any case, it should be added in the patch that uses it, not before.

307

I /believe/ SplitLineTable is enabled not by the presence of type units in general, but by those type units using "getFile" which adds entries to the line table and enables it. So it's enabled implicitly when files are added - I would hope that the same would be true of the macro section. It could implicitly enable the split line table by using it, not by having an explicit invocation that enables it.

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1018–1025

I meant ambiguity in the resulting DWARF.

Now you'll have a split unit with a stmt_list of zero, but the skeleton unit for that split unit will also have a stmt_list. So a consumer, when trying to resolve the index in a decl_file inside the split unit, might see the stmt_list in the split CU and decide that's the line table it should consult - when in reality it's the line table in the skeleton unit that is meant to be consulted in that case.

SouraVX planned changes to this revision.Jun 18 2020, 5:21 AM

Based on the feedback by @dblaikie (Thanks for this)! Filed separate refactoring revison D82084. Planning to revise this after we reach consensus on that(D82084).
Thanks!

Posted an alternative direction/proposal here: D84278