[PATCH] D125783: [llvm-debuginfo-analyzer] 08 - ELF Reader

[Paul Robinson via Phabricator via llvm-commits]

probinson accepted this revision.
probinson added a comment.
This revision is now accepted and ready to land.

LGTM, although I did notice one comment from @psamolysov that had not been answered.



================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVBinaryReader.cpp:289
+  Iter = SectionAddresses.lower_bound(Address);
+  if (Iter != SectionAddresses.begin())
+    --Iter;
----------------
psamolysov wrote:
> Should the `Iter` be checked that it is not `SectionAddresses.end()`? If the `Iter` is the `end()` iterator, `--Iter` just returns it to a "normal" value and all the checks for `end` in the callers (if these checks are there) will positive (sometimes false positive).
This comment hasn't been answered.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVELFReader.cpp:692
+    if (options().getPrintSizes() && Upper)
+      CompileUnit->addSize(Scope, Lower, Upper);
+  }
----------------
CarlosAlbertoEnciso wrote:
> probinson wrote:
> > It looks like Upper will not include the size of the last child?
> I debugged the code using the following DWARF test case:
> ```
> 0x0000000b: DW_TAG_compile_unit
> 0x0000002d:   DW_TAG_variable
> 0x00000043:   DW_TAG_base_type
> 0x0000004a:   DW_TAG_variable
> 0x00000060:   DW_TAG_subprogram
> 0x00000077:   DW_TAG_subprogram
> 0x00000092:     DW_TAG_formal_parameter
> 0x000000a1:     DW_TAG_formal_parameter
> 0x000000b0:     NULL
> 0x000000b1:   DW_TAG_subprogram
> 0x000000cf:   DW_TAG_subprogram
> 0x000000f5:     DW_TAG_formal_parameter
> 0x00000103:     DW_TAG_formal_parameter
> 0x00000111:     DW_TAG_variable
> 0x00000120:     DW_TAG_variable
> 0x0000012f:     DW_TAG_lexical_block
> 0x00000140:       DW_TAG_variable
> 0x0000014f:       DW_TAG_variable
> 0x0000015e:       NULL
> 0x0000015f:     NULL
> 0x00000160:   DW_TAG_subprogram
> 0x00000186:     DW_TAG_formal_parameter
> 0x00000195:     NULL
> 0x00000196:   DW_TAG_subprogram
> 0x000001b8:     DW_TAG_formal_parameter
> 0x000001c7:     NULL
> 0x000001c8:   NULL
> ```
> These are the `Lower` and `Upper` values for each logical scope, in the order they are recorded in the  compile unit.
> ```
> Offset                                  Lower   Upper
> 0x00000060:   DW_TAG_subprogram         0x060   0x077
> 0x00000077:   DW_TAG_subprogram         0x077   0x0b0
> 0x000000b1:   DW_TAG_subprogram         0x0b1   0x0cf
> 0x0000012f:     DW_TAG_lexical_block    0x12f   0x15e
> 0x000000cf:   DW_TAG_subprogram         0x0cf   0x15f
> 0x00000160:   DW_TAG_subprogram         0x160   0x195
> 0x00000196:   DW_TAG_subprogram         0x196   0x1c7
> 0x0000000b: DW_TAG_compile_unit         0x00b   0x1c8
> ```
> Looking at that table, it seems that `Upper` is including the size of the last child.
> May be I am missing something?
No, my misunderstanding.


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

https://reviews.llvm.org/D125783