[PATCH] D125784: [llvm-dva] 09 - CodeView Reader

[Carlos Alberto Enciso via Phabricator via llvm-commits]

CarlosAlbertoEnciso added inline comments.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVSupport.cpp:154
+
+std::string llvm::logicalview::getScopedName(LVStringRefs Components,
+                                             StringRef BaseName) {
----------------
psamolysov wrote:
> `LVStringRefs` is defined as `std::vector<StringRef>`, this object may be heavy and it could make sense to pass it by reference.
Nice change.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewReader.cpp:790
+    // Find the associate symbol table information.
+    LVSymbolTableEntry SymbolTableEntry = getSymbolTableEntry(SymbolName);
+    LVScope *Function = SymbolTableEntry.Scope;
----------------
psamolysov wrote:
> As I get, `LVSymbolTableEntry`'s cost is about 18-24 bytes, so it makes sense to not copy the entire entry but has a constant reference to it. A constant reference makes the lifecycle of the entry as long as the current scope, so no problem should be here.
Good catch. Changing to return a constant reference.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewVisitor.cpp:1599
+      // We can have a LF_FUNC_ID, LF_PROCEDURE or LF_MFUNCTION.
+      CVFunctionType = Ids.tryGetType(TIFunctionType);
+      if (!getRecordType()) {
----------------
psamolysov wrote:
> This line is not needed, `CVFunctionType` will be set in exactly this value in the `getRecordType` lambda which is called on the next line.
Good finding. Removed the extra call.


================
Comment at: llvm/test/tools/llvm-dva/COFF/01-coff-compare-logical-elements.test:41
+; ONE-NEXT:  [004]                   {Variable} 'CONSTANT' -> 'const int'
+; ONE-NEXT: +[004]                   {TypeAlias} 'INTEGER' -> 'int'
+
----------------
psamolysov wrote:
> Just a question:  does `+` mean the element was added in the Target and omitted in the Reference?
That is correct. Using the command line
```
llvm-dva --attribute=level --print=types test-codeview-clang.o test-codeview-msvc.o
```
The logical views are:
```
Logical View:
[000]           {File} 'test-codeview-clang.o'

[001]             {CompileUnit} 'test.cpp'
[002]               {TypeAlias} 'INTPTR' -> '* const int'
[002]               {Function} extern not_inlined 'foo' -> 'int'
[003]                 {TypeAlias} 'INTEGER' -> 'int'

Logical View:
[000]           {File} 'test-codeview-msvc.o'

[001]             {CompileUnit} 'test.cpp'
[002]               {TypeAlias} 'INTPTR' -> '* const int'
[002]               {Function} extern not_inlined 'foo' -> 'int'
[003]                 {Block}
[004]                   {TypeAlias} 'INTEGER' -> 'int'
```
For the comparison, with
`Reference`: 'test-codeview-clang.o'
`Target`: 'test-codeview-msvc.o'

```
; ONE-NEXT: -[003]                 {TypeAlias} 'INTEGER' -> 'int'
```
It means, that element (type) was deleted from the target.
```
; ONE-NEXT: +[004]                   {TypeAlias} 'INTEGER' -> 'int'
```
It means, that element (type) was added in the target.



================
Comment at: llvm/unittests/DebugInfo/LogicalView/CodeViewReaderTest.cpp:411
+      {"movl", &LVElement::getIsLine},
+      {"movl", &LVElement::getIsLine}};
+  LVReader *Reader = createReader(ReaderHandler, InputsDir, CodeViewClang);
----------------
psamolysov wrote:
> Line duplication?
No.
Using the command line:
```
llvm-dva --attribute=level test-codeview-clang.o --print=instructions
```
The logical view is:
```
Logical View:
[000]           {File} 'test-codeview-clang.o'

[001]             {CompileUnit} 'test.cpp'
[002]               {Function} extern not_inlined 'foo' -> 'int'
[003]                 {Block}
[004]                   {Code} 'movl    $0x7, 0x4(%rsp)'
[004]                   {Code} 'movl    $0x7, 0x1c(%rsp)'
[004]                   {Code} 'jmp     0x8'
[003]                 {Code} 'subq      $0x20, %rsp'
[003]                 {Code} 'andb      $0x1, %r8b'
[003]                 {Code} 'movb      %r8b, 0x1b(%rsp)'
[003]                 {Code} 'movl      %edx, 0x14(%rsp)'
[003]                 {Code} 'movq      %rcx, 0x8(%rsp)'
[003]                 {Code} 'testb     $0x1, 0x1b(%rsp)'
[003]                 {Code} 'je        0x15'
[003]                 {Code} 'movl      0x14(%rsp), %eax'
[003]                 {Code} 'movl      %eax, 0x1c(%rsp)'
[003]                 {Code} 'movl      0x1c(%rsp), %eax'
[003]                 {Code} 'addq      $0x20, %rsp'
[003]                 {Code} 'retq'
```
For the giving selection pattern `"movl[ \t]?%"`, these are the matched elements.
```
[003]                 {Code} 'movl      %edx, 0x14(%rsp)'
[003]                 {Code} 'movl      %eax, 0x1c(%rsp)'
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125784