[Lldb-commits] [PATCH] D68270: DWARFDebugLoc: Add a function to get the address range of an entry

Fri Oct 4 05:48:05 PDT 2019

labath marked 8 inline comments as done.
labath added inline comments.

================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugLoc.h:91

+  class EntryIterator {
+  public:
----------------
I went for an iterator-like approach (instead of a callback or direct materialization) because it's easier to use. In particular it makes it possible to reuse this stuff in the dumping code, which would have been pretty hard with callbacks.

================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugLoc.h:138
+    llvm::Optional<object::SectionedAddress> BaseAddr;
+    std::function<Optional<object::SectionedAddress>(uint32_t)>
+        AddrOffsetResolver;
----------------
Using a callback is consistent with what rnglists code does. This uses a std::function instead of a function_ref because it's easier for iterators to escape out of scopes. However, I've wondered if we shouldn't define an AddrOffsetResolver interface that llvm, lldb DWARFUnits and anyone else who wishes so (unit tests ?) can implement.

================
Comment at: lib/DebugInfo/DWARF/DWARFDebugLoc.cpp:164
+    if (!BaseAddr)
+      return createStringError(errc::invalid_argument,
+                               "Cannot interpret DW_LLE_offset_pair entry due "
----------------
A significant deviation from the rnglists code: Here I return an error if it is not possible to compute the address range correctly. The rnglists parser cannot compute the value, it will just return something, which can be very far from the correct range -- for instance, it's happy to use the base_addressx index as the offset, if the index cannot be resolved correctly. And it doesn't provide any indication that it has done so, which doesn't seem like a very useful behavior.

If this is the behavior we want, I can also try to make the rnglists parser do something similar.

================
Comment at: lib/DebugInfo/DWARF/DWARFDebugLoc.cpp:291-295
+  EntryIterator Absolute =
+      getAbsoluteLocations(
+          SectionedAddress{BaseAddr, SectionedAddress::UndefSection},
+          LookupPooledAddress)
+          .begin();
----------------
This parallel iteration is not completely nice, but I think it's worth being able to reuse the absolute range computation code. I'm open to ideas for improvement though.

================
Comment at: test/CodeGen/X86/debug-loclists.ll:16
 ; CHECK-NEXT: 0x00000000:
-; CHECK-NEXT:  [0x0000000000000000, 0x0000000000000004): DW_OP_breg5 RDI+0
-; CHECK-NEXT:  [0x0000000000000004, 0x0000000000000012): DW_OP_breg3 RBX+0
-
-; There is no way to use llvm-dwarfdump atm (2018, october) to verify the DW_LLE_* codes emited,
-; because dumper is not yet implements that. Use asm code to do this check instead.
-;
-; RUN: llc -mtriple=x86_64-pc-linux -filetype=asm < %s -o - | FileCheck %s --check-prefix=ASM
-; ASM:      .section .debug_loclists,"", at progbits
-; ASM-NEXT: .long .Ldebug_loclist_table_end0-.Ldebug_loclist_table_start0 # Length
-; ASM-NEXT: .Ldebug_loclist_table_start0:
-; ASM-NEXT:  .short 5                              # Version
-; ASM-NEXT:  .byte 8                               # Address size
-; ASM-NEXT:  .byte 0                               # Segment selector size
-; ASM-NEXT:  .long 0                               # Offset entry count
-; ASM-NEXT: .Lloclists_table_base0:                
-; ASM-NEXT: .Ldebug_loc0:
-; ASM-NEXT:  .byte 4                               # DW_LLE_offset_pair
-; ASM-NEXT:  .uleb128 .Lfunc_begin0-.Lfunc_begin0  # starting offset
-; ASM-NEXT:  .uleb128 .Ltmp0-.Lfunc_begin0         # ending offset
-; ASM-NEXT:  .byte 2                               # Loc expr size
-; ASM-NEXT:  .byte 117                             # DW_OP_breg5
-; ASM-NEXT:  .byte 0                               # 0
-; ASM-NEXT:  .byte 4                               # DW_LLE_offset_pair
-; ASM-NEXT:  .uleb128 .Ltmp0-.Lfunc_begin0         # starting offset
-; ASM-NEXT:  .uleb128 .Ltmp1-.Lfunc_begin0         # ending offset
-; ASM-NEXT:  .byte 2                               # Loc expr size
-; ASM-NEXT:  .byte 115                             # DW_OP_breg3
-; ASM-NEXT:  .byte 0                               # 0
-; ASM-NEXT:  .byte 0                               # DW_LLE_end_of_list
-; ASM-NEXT: .Ldebug_loclist_table_end0:
+; CHECK-NEXT:    [DW_LLE_offset_pair ]: 0x0000000000000000, 0x0000000000000004 => [0x0000000000000000, 0x0000000000000004) DW_OP_breg5 RDI+0
+; CHECK-NEXT:    [DW_LLE_offset_pair ]: 0x0000000000000004, 0x0000000000000012 => [0x0000000000000004, 0x0000000000000012) DW_OP_breg3 RBX+0
----------------
This tries to follow the RLE format as closely as possible, but I think something like
```
[DW_LLE_offset_pair,  0x0000000000000000, 0x0000000000000004] => [0x0000000000000000, 0x0000000000000004): DW_OP_breg5 RDI+0
```
would make more sense (both here and for RLE).

================
Comment at: test/DebugInfo/X86/fission-ranges.ll:48
 ; CHECK:      [[A]]:
-; CHECK-NEXT:   Addr idx 2 (w/ length 15): DW_OP_consts +0, DW_OP_stack_value
-; CHECK-NEXT:   Addr idx 3 (w/ length 15): DW_OP_reg0 RAX
-; CHECK-NEXT:   Addr idx 4 (w/ length 18): DW_OP_breg7 RSP-8
+; CHECK-NEXT:   [DW_LLE_startx_length]: 0x00000002, 0x0000000f => <Failed to read address offset 2> DW_OP_consts +0, DW_OP_stack_value
+; CHECK-NEXT:   [DW_LLE_startx_length]: 0x00000003, 0x0000000f => <Failed to read address offset 3> DW_OP_reg0 RAX
----------------
This is somewhat annoying, because the entries printed through the loclists section will always have this error (as we don't have the DWARFUnit). I'll have to figure out a way to suppress those, while still keeping them around when printing from DWARFDie (as there a failure means a real error).

Repository:
  rL LLVM

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

https://reviews.llvm.org/D68270