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

[Carlos Alberto Enciso via Phabricator via llvm-commits]

CarlosAlbertoEnciso added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Readers/LVBinaryReader.h:81
+  // The virtual address refers to the address where the section is loaded.
+  using LVSectionAddresses = std::map<LVSectionIndex, object::SectionRef>;
+  LVSectionAddresses SectionAddresses;
----------------
probinson wrote:
> So, `LVSectionIndex` isn't actually an (array) index or other small identifier, it's an address?
`LVSectionIndex` is the value returned by `getIndex` and it is the object section number (id).

https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/Object/ObjectFile.h#L470




================
Comment at: llvm/test/tools/llvm-debuginfo-analyzer/DWARF/04-dwarf-missing-nested-enumerators.test:24
+; references to the enumerators 'RED' and 'BLUE'. The DWARF generated
+; by GCC, CodeView generated by Clang and MSVC, it does include such
+; references.
----------------
probinson wrote:
> 
Changed.

```
; In the following logical views, we can see that the DWARF debug
; information generated by the Clang compiler does not include any
; references to the enumerators 'RED' and 'BLUE'. The DWARF generated
; by GCC, does include such references.
```


================
Comment at: llvm/test/tools/llvm-debuginfo-analyzer/DWARF/05-dwarf-incorrect-lexical-scope-variable.test:25
+
+; In the following logical views, we can see that the CodeView debug
+; information generated by the Clang compiler shows the variables
----------------
probinson wrote:
> I'm not seeing a CodeView display in this test.
Changed

```
; In the following logical views, we can see that the DWARF debug
; information generated by the Clang compiler shows the variables
; 'Var_1' and 'Var_2' are at the same lexical scope (4) in the function
; 'InlineFuction'.
; The DWARF generated by GCC/Clang show those variables at the correct
; lexical scope: '3' and '4' respectively.
```


================
Comment at: llvm/unittests/DebugInfo/LogicalView/ELFReaderTest.cpp:80
+  const LVLocations *Ranges = CompileUnit->getRanges();
+  EXPECT_NE(Ranges, nullptr);
+  ASSERT_EQ(Ranges->size(), 1);
----------------
probinson wrote:
> CarlosAlbertoEnciso wrote:
> > psamolysov wrote:
> > > It could make sense to check pointers on the `nullptr` in `ASSERT_NE` instead of `EXPECT_NE`. If a pointer is null, the test will just crash on the first dereference of the pointer.
> > That is a good point.
> > I believe a function that uses `ASSERT_NE` must be declared with `TEST` or `TEST_F` and I can't see how to return a value, which it is our case.
> > In order to use `ASSERT_NE` then we need to change how those functions are implemented and invoked.
> > 
> There are other ASSERT_* macros used in this function, so I don't think your concern is correct.
You are correct. Very good point.
I was confused with functions that return a value.
Changed other instances to use `ASSERT_NE(cond1, nullptr);`


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

https://reviews.llvm.org/D125783