Page MenuHomePhabricator

[Debug-Info] make DIE attributes generating under control of strict dwarf
ClosedPublic

Authored by shchenz on Apr 22 2021, 12:15 AM.

Details

Summary

We added strict dwarf support in D100826.

This is a suggestion from @probinson and @dblaikie

Make use of this flag for attribute generating.

We should also guard other DWARF info(tags, forms, loc expressions) generating under strict dwarf flag, but currently, there is no obvious way. Leave for later improvement.

Diff Detail

Event Timeline

shchenz created this revision.Apr 22 2021, 12:15 AM
shchenz requested review of this revision.Apr 22 2021, 12:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 22 2021, 12:15 AM
dblaikie added inline comments.Apr 22 2021, 7:55 AM
llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp
1801–1805 ↗(On Diff #339495)

This patch is adding this boilerplate in several places.

Perhaps it'd be cleaner to refactor the underlying "Die.addValue" call to a helper in DwarfUnit in a preparatory patch, then the boilerplate can be added to that one place?

probinson added inline comments.Apr 22 2021, 8:18 AM
llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp
1801–1805 ↗(On Diff #339495)

That was my first thought too, but it looks like class DIE does not know the DwarfUnit (only the DIEUnit) so it doesn't know the version or have access to the strictness flag.
But, there could be a predicate in DwarfUnit that takes the Attribute and hides the details, instead of repeating them in each of these places.

shchenz updated this revision to Diff 339844.Apr 22 2021, 7:16 PM

1: use new wrapper function addAttribute()

@dblaikie @probinson thanks for your review. I added a new wrapper function in DwarfUnit class in NFC patch D101125 and update this patch according to that NFC changes. Could you please help to have another look?

dblaikie added inline comments.Apr 22 2021, 7:38 PM
llvm/lib/CodeGen/AsmPrinter/DwarfUnit.h
82

When is the Attribute 0?

shchenz updated this revision to Diff 339862.Apr 22 2021, 8:44 PM

1: update due to parent change

shchenz added inline comments.Apr 22 2021, 8:47 PM
llvm/lib/CodeGen/AsmPrinter/DwarfUnit.h
82

one example:

/// Add a Dwarf expression attribute data and value.
void DwarfCompileUnit::addExpr(DIELoc &Die, dwarf::Form Form,
                               const MCExpr *Expr) {
  Die.addValue(DIEValueAllocator, (dwarf::Attribute)0, Form, DIEExpr(Expr));
}

This is not for adding an attribute but for adding an expression for an attribute. Even dwarf::AttributeVersion(0) returns 0 and we will still go to return branch, but I think it is meaningless to get a attribute version for attribute 0?

dblaikie added inline comments.Apr 22 2021, 8:50 PM
llvm/lib/CodeGen/AsmPrinter/DwarfUnit.h
82

Ah, I think that's used for putting things in blocks, etc.

I suspect that not adding the attribute if the attribute is 0 is probably the wrong thing to do - perhaps you could check some test cases/try some small pieces of code to see what this breaks/omits? I think it'll end up omitting DWARF location expressions, for instance - so any non-trivial variable location would be incorrect/missing?

shchenz added inline comments.Apr 22 2021, 9:00 PM
llvm/lib/CodeGen/AsmPrinter/DwarfUnit.h
82

sorry, I made a mistake here. We should always call the Die.addValue()(will not go to return branch) even there is no Attribute != 0 when Attribute == 0 So I think for not a real attribute(Attribute == 0), we will always emit them.

dblaikie added inline comments.Apr 22 2021, 9:20 PM
llvm/lib/CodeGen/AsmPrinter/DwarfUnit.h
82

Could you add a test case for some situation that has attribute 0 to exercise this test/show why it's the right thing to do?

shchenz added inline comments.Apr 23 2021, 12:18 AM
llvm/lib/CodeGen/AsmPrinter/DwarfUnit.h
82

Do you mean adding a case without changes before and after this patch? If so, I think there should be many cases in LIT already. See DwarfCompileUnit::addLocationAttribute(), in this function there are many calls to addUInt which also contains calling to 0 attribute.

void DwarfUnit::addUInt(DIEValueList &Block, dwarf::Form Form,
                        uint64_t Integer) {
  addUInt(Block, (dwarf::Attribute)0, Form, Integer);
}

What do you think? @dblaikie

dblaikie added inline comments.Apr 23 2021, 1:57 PM
llvm/lib/CodeGen/AsmPrinter/DwarfUnit.h
82

I mean a test that specifies strict dwarf, but yes - produces the same output as without it. (or a test that specifies strict-dwarf, does have some differences, but verifies this particular thing doesn't have differences)

eg: the PowerPC/strict-dwarf.ll test could include a variable with a location, and showing the location has the correct location expression (when it would not have had that if the Attribute != 0 test was inverted or not present

shchenz updated this revision to Diff 340320.Apr 24 2021, 6:38 PM

1: add tests for attribute 0

llvm/lib/CodeGen/AsmPrinter/DwarfUnit.h
82

Ah, OK. Got your means. I add one more check point in the strict-dwarf test, does it make sense to you? Thanks. @dblaikie

dblaikie accepted this revision.Apr 25 2021, 9:57 AM

Looks good, thanks!

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

Might be worth expanding the comment to describe the Attribute 0 case - "Attribute 0 is used when emitting form-encoded values in blocks, which don't have attributes (only forms) so we cannot detect their DWARF version compatibility here and assume they are compatible"

llvm/test/DebugInfo/PowerPC/strict-dwarf.ll
11

Rather than using a comment (or in addition to the comment, you can use a CHECK for DW_AT_name of "var" to make it clear that the location checking is for that variable. (with a CHECK-NOT: DW_TAG between the attributes to ensure the test didn't skip from one tag to another between the two attributes)

This revision is now accepted and ready to land.Apr 25 2021, 9:57 AM
shchenz marked an inline comment as done.May 12 2021, 8:46 PM
shchenz added inline comments.
llvm/lib/CodeGen/AsmPrinter/DwarfUnit.h
85

Thanks, will update in the commit

llvm/test/DebugInfo/PowerPC/strict-dwarf.ll
11

Thanks for the good idea. Will update this in the commit.

This revision was automatically updated to reflect the committed changes.
shchenz marked an inline comment as done.