vsk marked 11 inline comments as done.
vsk added inline comments.
================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:1288
DwarfExpr.addFragmentOffset(DIExpr);
- if (Location.isIndirect())
+ if (Location.isIndirect() && !DIExpr->isEntryValue())
DwarfExpr.setMemoryLocationKind();
----------------
aprantl wrote:
> Can you add a comment? it's not necessarily obvious why an indirect entry
> value is not a memory location. Is it because an entry value is its own kind?
This has more to do with pre-existing constraints in the dwarf emitter, more
than anything else. `DwarfExpression::addReg` cannot be used with Memory
location kinds, but we need that method to write out
DW_OP_entry_value(DW_OP_regN).
I guess there's no such restriction for `addBReg`, but I'm not sure why we'd
pick that representation (see Djordje's earlier comment).
================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:790
+ // FIXME: This produces unusable descriptions when the register contains
+ // a pointer to a temporary copy of a struct passed by value.
DIExpression *EntryExpr = DIExpression::get(
----------------
aprantl wrote:
> What does "unusable" mean? Incorrect? Invalid?
Should be "incorrect", fixed. It's "unusable" in the sense that the debugger
will error out trying to evaluate the expression: it will think, "struct Foo
/is/ the pointer in this register", instead of, "struct Foo is /pointed to/ by
this register".
================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:2412
}
const TargetRegisterInfo &TRI = *AP.MF->getSubtarget().getRegisterInfo();
----------------
aprantl wrote:
> factoring out this snippet into a template and reusing it in
> DwarfCompileUnit.cpp is probably not going to make this more readable, is it?
I tried doing this a few different ways :). I couldn't find a clean solution.
In the end I opted for adding 'DwarfExpression::adjustLocationKind' to share
just some of the logic. Lmk what you think.
================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfExpression.h:146
/// The flags of location description being produced.
+ enum {
----------------
aprantl wrote:
> This comment is weird :-)
>
> `Additional flags that may be combined with any location description kind.`?
Fixed, that's much better.
================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfExpression.h:149
+ EntryValue = 1 << 0,
+ IndirectEntryValue = 1 << 1,
+ CallSiteParamValue = 1 << 2
----------------
aprantl wrote:
> Would it make more sense call this `Indirect` (w/o EntryValue) and treat it
> as a separate bit, so you can still test for EntryValue independently?
Yes, that does seem better. It's more extensible this way, and anyway, most of
the logic for treating direct/indirect entry values should be the same.
================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfExpression.h:308
/// Lock this down to become an entry value location.
- void setEntryValueFlag() { LocationFlags |= EntryValue; }
+ void setEntryValueFlagFromLoc(MachineLocation Loc) {
+ LocationFlags |= Loc.isIndirect() ? IndirectEntryValue : EntryValue;
----------------
aprantl wrote:
> Is the dependency on MachineLocation really necessary, or could this just
> take a bool / enum IsIndirect?
It aids encapsulation to not require clients to unpack the relevant bit of
MachineLocation, I feel. I've changed this to accept a reference, to at least
get rid of the header dependency. This seems acceptable to me, as
DwarfExpression clients work with MachineLocation anyway.
================
Comment at: llvm/test/DebugInfo/MIR/AArch64/dbgcall-site-indirect-param.mir:18
+# CHECK-NEXT: [0x0000000000000000, 0x0000000000000010): DW_OP_breg0 W0+0
+# CHECK-NEXT: [0x0000000000000010, 0x000000000000001c):
DW_OP_entry_value(DW_OP_reg0 W0))
+# CHECK-NEXT: DW_AT_name ("f")
----------------
djtodoro wrote:
> I know that the final effect is the same, but should this be
> `DW_OP_entry_value 2 DW_OP_bregN 0` instead of `DW_OP_entry_value 1
> DW_OP_regN`?
I'm not sure, the DW_OP_regN description is simply the one that has already
been wired up. Is there a reason to prefer 'DW_OP_bregN 0'?
================
Comment at: llvm/test/DebugInfo/MIR/AArch64/dbgcall-site-indirect-param.mir:71
+ !24 = !DILocation(line: 5, column: 24, scope: !13)
+ !25 = !DILocation(line: 6, column: 23, scope: !13)
+ !31 = !DILocation(line: 6, column: 20, scope: !13)
----------------
djtodoro wrote:
> Nit: !25-36! could be attached to !24
That's a fair point, but I feel this is too onerous of a reduction to do by
hand. As the MIR is already fairly reduced, I would prefer not to do this.
================
Comment at: llvm/test/DebugInfo/MIR/AArch64/dbgcall-site-indirect-param.mir:79
+---
+name: bar
+alignment: 4
----------------
djtodoro wrote:
> I believe we do need all the attributes.
Fixed.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D80345/new/
https://reviews.llvm.org/D80345
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
