[PATCH] D56587: Introduce DW_OP_LLVM_convert

[Paul Robinson via Phabricator via llvm-commits]

probinson added inline comments.


================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:1937
+    // easiest way to pass custom values as the expressions are inserted into a
+    // byte stream rather early (see DwarfExpression::addExpression).
+    switch (State) {
----------------
aprantl wrote:
> markus wrote:
> > aprantl wrote:
> > > This is to inject the reference to the basic type die, right?
> > > 
> > > This code should probably be factored out into a relocate/fixupTypeRefs() helper function. I also assume that you need to apply the same fixup for the case of a single, non-debug_lo, inline DW_AT_location, right?
> > > 
> > > The fact that the placeholder is encoded as a LEB128 sounds really dangerous. If we ever support any branching operations, it will mess with the offsets. Can we assume that the finalized DIE ref will always be a DW_OP_ref_addr or something with a fixed size? Could we make the placeholder the same fixed size, too?
> > > 
> > > If that doesn't work, the right solution is probably to defer the emission of DwarfExpressions until here, which we could do in a separate, preparatory commit.
> > > This is to inject the reference to the basic type die, right?
> > Yes
> > > This code should probably be factored out into a relocate/fixupTypeRefs() helper function. I also assume that you need to apply the same fixup for the case of a single, non-debug_lo, inline DW_AT_location, right?
> > Sounds reasonable. Not sure I could easily find where in the code the inline expressions are inserted though. If you could point at a file and line number that would be helpful. I guess another option would be to force these expressions (the ones containing a base type reference) to always end up in .debug_loc right?
> > > The fact that the placeholder is encoded as a LEB128 sounds really dangerous. If we ever support any branching operations, it will mess with the offsets. Can we assume that the finalized DIE ref will always be a DW_OP_ref_addr or something with a fixed size? Could we make the placeholder the same fixed size, too?
> > The spec states that the finalized base type DIE offset is encoded as a ULEB128 so not much choice about that but the value we pick up here (the one inserted in `DwarfExpression::addExpression`) is just a index so we could certainly encode that in a fixed size integer. If branches were to be introduced at a later point I imagine that the branch target in the emitted dwarf would be a label (`MCSymbol`) but in the intermediate expression vector a simple offset would probably suffice.
> > > If that doesn't work, the right solution is probably to defer the emission of DwarfExpressions until here, which we could do in a separate, preparatory commit.
> > I think that would be a good thing to do but unfortunately it seems far from easy to get rid of the stuff that is in between.
> > 
> > If you could point at a file and line number that would be helpful. 
> 
> 
> ```
> git grep addBlock.*DW_AT_location lib/CodeGen/AsmPrinter
> lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:    addBlock(*VariableDIE, dwarf::DW_AT_location, DwarfExpr->finalize());
> lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:        addBlock(*VariableDie, dwarf::DW_AT_location, DwarfExpr.finalize());
> lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:  addBlock(*VariableDie, dwarf::DW_AT_location, DwarfExpr.finalize());
> lib/CodeGen/AsmPrinter/DwarfUnit.cpp:        addBlock(ParamDIE, dwarf::DW_AT_location, Loc);
> ```
> 
> 
> 
> > The spec states that the finalized base type DIE offset is encoded as a ULEB128 so not much choice about that but the value we pick up here (the one inserted in DwarfExpression::addExpression) is just a index so we could certainly encode that in a fixed size integer.
> > 
> 
> I see. I didn't think about emitting branch targets a label differences, I thought we'd just hardcode the offsets.  I guess we can defer this until it becomes an issue. Encoding the temporary die reference with a fixed size would probably still be a good idea, just to keep this code simpler.
Re the ULEB as how to find the base_type DIEs, the unstated assumption is that the base_type DIEs would be emitted unconditionally at the top of the CU, so everyone can just use them as needed.  If you want to emit base_types lazily... don't do that.
Re branches, there are already branch operators, if that's what you're talking about (DW_OP_skip and _bra).


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

https://reviews.llvm.org/D56587