[PATCH] D125779: [llvm-dva] 04 - Locations and ranges

[Carlos Alberto Enciso via Phabricator via llvm-commits]

CarlosAlbertoEnciso added inline comments.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVLocation.cpp:323
+  //-------------------------------------------------------------------------
+  case 0:
+    Stream << "offset " << int(Operands[0]);
----------------
probinson wrote:
> CarlosAlbertoEnciso wrote:
> > probinson wrote:
> > > CarlosAlbertoEnciso wrote:
> > > > psamolysov wrote:
> > > > > Is there any enum in the `dwarf` namespace which is equals to zero? If not it could make sense to use a named constant instead of magic number `0`.
> > > > @probinson How do you feel in creating something like dwarf::DW_OP_null similar to the existing dwarf::DW_TAG_null.
> > > DW_TAG_NULL is meaningful in the sense that a 0 tag indicates no more siblings at this nesting level, in the debug info.  But a 0 operator should never appear; are you inserting these yourself for some reason?
> > Thanks for the clear answer on `DW_TAG_null`.
> > 
> > This is an extract from the llvm-dwarfdump output for an ELF test case:
> > ```
> > DW_TAG_member
> >      DW_AT_name	("name")
> >      DW_AT_type	("int")
> >      DW_AT_decl_line	(1858)
> >      DW_AT_data_member_location	(0x00)
> > ```
> > Which `llvm-dva` converts into
> > ```
> > 1858           {Member} private 'name' -> 'int'
> >                  {Location}
> >                    {Entry} offset 0
> > ```
> > The `0 operator` corresponds to the `DW_AT_data_member_location` value.
> What I'm understanding from this is that when `llvm-dva` reads the DIE for "name" it finds `DW_AT_data_member_location`, and encodes that into an `LVOperation` with `Opcode` = 0. Then `LVOperation::getOperandsDWARFInfo()` sees the 0 opcode and knows it means this is a simple offset.
> If I am understanding that correctly, then this is something `llvm-dva` is doing for its own benefit, and should not be considered a DWARF expression opcode of any kind.  `llvm-dva` should define its own constant for this, and really should not use a `DW_OP_*` spelling for the name.
Your analysis is correct.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVLocation.cpp:323
+  //-------------------------------------------------------------------------
+  case 0:
+    Stream << "offset " << int(Operands[0]);
----------------
CarlosAlbertoEnciso wrote:
> probinson wrote:
> > CarlosAlbertoEnciso wrote:
> > > probinson wrote:
> > > > CarlosAlbertoEnciso wrote:
> > > > > psamolysov wrote:
> > > > > > Is there any enum in the `dwarf` namespace which is equals to zero? If not it could make sense to use a named constant instead of magic number `0`.
> > > > > @probinson How do you feel in creating something like dwarf::DW_OP_null similar to the existing dwarf::DW_TAG_null.
> > > > DW_TAG_NULL is meaningful in the sense that a 0 tag indicates no more siblings at this nesting level, in the debug info.  But a 0 operator should never appear; are you inserting these yourself for some reason?
> > > Thanks for the clear answer on `DW_TAG_null`.
> > > 
> > > This is an extract from the llvm-dwarfdump output for an ELF test case:
> > > ```
> > > DW_TAG_member
> > >      DW_AT_name	("name")
> > >      DW_AT_type	("int")
> > >      DW_AT_decl_line	(1858)
> > >      DW_AT_data_member_location	(0x00)
> > > ```
> > > Which `llvm-dva` converts into
> > > ```
> > > 1858           {Member} private 'name' -> 'int'
> > >                  {Location}
> > >                    {Entry} offset 0
> > > ```
> > > The `0 operator` corresponds to the `DW_AT_data_member_location` value.
> > What I'm understanding from this is that when `llvm-dva` reads the DIE for "name" it finds `DW_AT_data_member_location`, and encodes that into an `LVOperation` with `Opcode` = 0. Then `LVOperation::getOperandsDWARFInfo()` sees the 0 opcode and knows it means this is a simple offset.
> > If I am understanding that correctly, then this is something `llvm-dva` is doing for its own benefit, and should not be considered a DWARF expression opcode of any kind.  `llvm-dva` should define its own constant for this, and really should not use a `DW_OP_*` spelling for the name.
> Your analysis is correct.
@psamolysov, @probinson: based on the previous analysis any suggestions to define a `llvm-dva` constant to describe the `zero` as an indication for member-offset?

May be `LV_LOCATION_MEMBER_OFFSET`

That constant will be used in

```
// Add a Location Entry.
void LVSymbol::addLocationConstant(dwarf::Attribute Attr, LVUnsigned Constant,
                                   uint64_t LocDescOffset) {
  ...
  // Add records to Location Entry.
  addLocationOperands(/*Opcode=*/LV_LOCATION_MEMBER_OFFSET,
                      /*Operand1=*/Constant, /*Operand2=*/0);
}

std::string LVOperation::getOperandsDWARFInfo() {
  ...
  switch (Opcode) {
  ...
  //-------------------------------------------------------------------------
  // Member location.
  //-------------------------------------------------------------------------
  case LV_LOCATION_MEMBER_OFFSET:
  ...
  }
}
```




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125779