[PATCH] D101024: [Debug-Info] make DIE attributes generating under control of strict dwarf

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfUnit.h:85
+    // DWARF version.
+    if (Attribute != 0 && Asm->TM.Options.DebugStrictDwarf &&
+        DD->getDwarfVersion() < dwarf::AttributeVersion(Attribute))
----------------
shchenz wrote:
> dblaikie wrote:
> > shchenz wrote:
> > > dblaikie wrote:
> > > > shchenz wrote:
> > > > > dblaikie wrote:
> > > > > > When is the Attribute 0?
> > > > > 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?
> > > > 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?
> > > 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.
> > 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?
> 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 
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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101024/new/

https://reviews.llvm.org/D101024