aprantl 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(); ---------------- 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? ================ 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( ---------------- What does "unusable" mean? Incorrect? Invalid? ================ Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:2404 MachineLocation Location = Value.getLoc(); - if (Location.isIndirect()) + if (Location.isIndirect() && !DIExpr->isEntryValue()) DwarfExpr.setMemoryLocationKind(); ---------------- same here ================ Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:2412 } const TargetRegisterInfo &TRI = *AP.MF->getSubtarget().getRegisterInfo(); ---------------- factoring out this snippet into a template and reusing it in DwarfCompileUnit.cpp is probably not going to make this more readable, is it? ================ Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfExpression.h:146 /// The flags of location description being produced. + enum { ---------------- This comment is weird :-) `Additional flags that may be combined with any location description kind.`? ================ Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfExpression.h:149 + EntryValue = 1 << 0, + IndirectEntryValue = 1 << 1, + CallSiteParamValue = 1 << 2 ---------------- 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? ================ 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; ---------------- Is the dependency on MachineLocation really necessary, or could this just take a bool / enum IsIndirect? ================ Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfExpression.h:309 + void setEntryValueFlagFromLoc(MachineLocation Loc) { + LocationFlags |= Loc.isIndirect() ? IndirectEntryValue : EntryValue; + } ---------------- see comment in header... 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