[Lldb-commits] [PATCH] D106466: [llvm+lldb] Fix#2 of DW_AT_ranges DW_FORM_sec_offset not using DW_AT_rnglists_base (used by GCC)

2021-08-17 Thread Jan Kratochvil via Phabricator via lldb-commits
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGe21a21a977b4: [lldb] Fix#2 of DW_AT_ranges 
DW_FORM_sec_offset not using DW_AT_rnglists_baseā€¦ (authored by jankratochvil).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106466

Files:
  lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
  lldb/test/Shell/SymbolFile/DWARF/DW_AT_range-DW_FORM_sec_offset.s


Index: lldb/test/Shell/SymbolFile/DWARF/DW_AT_range-DW_FORM_sec_offset.s
===
--- lldb/test/Shell/SymbolFile/DWARF/DW_AT_range-DW_FORM_sec_offset.s
+++ lldb/test/Shell/SymbolFile/DWARF/DW_AT_range-DW_FORM_sec_offset.s
@@ -29,7 +29,7 @@
 # RUN:   -o exit 2>&1 | FileCheck --check-prefix=RNGLISTBASE %s
 
 # RNGLISTBASE-LABEL: image lookup -v -s lookup_rnglists
-# RNGLISTBASE: error: {{.*}}-rnglistbase {0x0043}: DIE has 
DW_AT_ranges(DW_FORM_rnglistx 0x0) attribute, but range extraction failed 
(invalid range list table index 0; OffsetEntryCount is 0, DW_AT_rnglists_base 
is 12), please file a bug and attach the file at the start of this error message
+# RNGLISTBASE: error: {{.*}}-rnglistbase {0x0043}: DIE has 
DW_AT_ranges(DW_FORM_rnglistx 0x0) attribute, but range extraction failed 
(invalid range list table index 0; OffsetEntryCount is 0, DW_AT_rnglists_base 
is 24), please file a bug and attach the file at the start of this error message
 
 .text
 rnglists:
@@ -97,7 +97,7 @@
 .long   .Lrnglists_end-rnglists # DW_AT_high_pc
 .long   .Laddr_table_base0  # DW_AT_addr_base
 .ifdef RNGLISTBASE
-.long   .Ldebug_ranges0 # DW_AT_rnglists_base
+.long   .Ldebug_ranges1 # DW_AT_rnglists_base
 .endif
 .byte   2   # Abbrev [2] 0x2b:0x37 
DW_TAG_subprogram
 .quad   rnglists# DW_AT_low_pc
@@ -105,7 +105,7 @@
 .asciz  "rnglists"  # DW_AT_name
 .byte   5   # Abbrev [5] 0x52:0xf 
DW_TAG_lexical_block
 .ifndef RNGLISTX
-.long   .Ldebug_ranges0 # DW_AT_ranges DW_FORM_sec_offset
+.long   .Ldebug_ranges1 # DW_AT_ranges DW_FORM_sec_offset
 .else
 .uleb128 0  # DW_AT_ranges DW_FORM_rnglistx
 .endif
@@ -130,9 +130,17 @@
 .byte   8   # Address size
 .byte   0   # Segment selector size
 .long   0   # Offset entry count
-.Ldebug_ranges0:
+.Ldebug_rnglist_table_end0:
+
+.long   .Ldebug_rnglist_table_end1-.Ldebug_rnglist_table_start1 # 
Length
+.Ldebug_rnglist_table_start1:
+.short  5   # Version
+.byte   8   # Address size
+.byte   0   # Segment selector size
+.long   0   # Offset entry count
+.Ldebug_ranges1:
 .byte   4   # DW_RLE_offset_pair
 .uleb128 .Lblock1_begin-rnglists  #   starting offset
 .uleb128 .Lblock1_end-rnglists#   ending offset
 .byte   0   # DW_RLE_end_of_list
-.Ldebug_rnglist_table_end0:
+.Ldebug_rnglist_table_end1:
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
@@ -437,15 +437,20 @@
   // We are expected to be called with Offset 0 or pointing just past the table
   // header. Correct Offset in the latter case so that it points to the start
   // of the header.
-  if (offset > 0) {
-uint64_t HeaderSize = llvm::DWARFListTableHeader::getHeaderSize(format);
-if (offset < HeaderSize)
-  return llvm::createStringError(errc::invalid_argument,
- "did not detect a valid"
- " list table with base = 0x%" PRIx64 "\n",
- offset);
-offset -= HeaderSize;
+  if (offset == 0) {
+// This means DW_AT_rnglists_base is missing and therefore DW_FORM_rnglistx
+// cannot be handled. Returning a default-constructed ListTableType allows
+// DW_FORM_sec_offset to be supported.
+return ListTableType();
   }
+
+  uint64_t HeaderSize = llvm::DWARFListTableHeader::getHeaderSize(format);
+  if (offset < HeaderSize)
+return llvm::createStringError(errc::invalid_argument,
+   "did not detect a valid"
+   " list table with base = 0x%" PRIx64 "\n",
+   offset);
+  offset -= HeaderSize;
   ListTableType Table;
   if (llvm::Error E = Table.extractHeaderAndOffsets(data, ))
 return std::move(E);
@@ -996,8 +1001,12 @@
 return 

[Lldb-commits] [PATCH] D106466: [llvm+lldb] Fix#2 of DW_AT_ranges DW_FORM_sec_offset not using DW_AT_rnglists_base (used by GCC)

2021-08-17 Thread David Blaikie via Phabricator via lldb-commits
dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

In D106466#2950268 , @jankratochvil 
wrote:

> In D106466#2947710 , @dblaikie 
> wrote:
>
>> I assume there's already test coverage for rnglistx in debug_info.dwo/split 
>> unit? (because in that case there's no rnglists_base, but rnglistx is usable)
>
> I admit I did not check it, thanks for catching it. But it is already tested 
> by lldb/test/Shell/SymbolFile/DWARF/x86/debug_rnglists-dwo.s 
> 
>  implemented by Pavel Labath in 2019 
> .
>  It is using DWO `DW_TAG_lexical_block->DW_AT_ranges->DW_FORM_rnglistx`.
>
>> - could you explain how this code avoids treating the split unit 
>> rnglists_base == 0 case as "there is no rnglists_base and so rnglistx isn't 
>> usable"?
>
> `m_ranges_base` is not zero in such case as it has been set from the skeleton 
> .

ah, OK - was going to say that setting it from the skeleton would be incorrect 
(for DWARF 5, at least) - but I see that's handled a few lines down: 
https://github.com/llvm/llvm-project/blob/f58a642da19c64cb6ee1badb0f176a872c1d7a0a/lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp#L110

Looks all good to me, then.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106466

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D106466: [llvm+lldb] Fix#2 of DW_AT_ranges DW_FORM_sec_offset not using DW_AT_rnglists_base (used by GCC)

2021-08-17 Thread Jan Kratochvil via Phabricator via lldb-commits
jankratochvil added a comment.

In D106466#2947710 , @dblaikie wrote:

> I assume there's already test coverage for rnglistx in debug_info.dwo/split 
> unit? (because in that case there's no rnglists_base, but rnglistx is usable)

I admit I did not check it, thanks for catching it. But it is already tested by 
lldb/test/Shell/SymbolFile/DWARF/x86/debug_rnglists-dwo.s 

 implemented by Pavel Labath in 2019 
.
 It is using DWO `DW_TAG_lexical_block->DW_AT_ranges->DW_FORM_rnglistx`.

> - could you explain how this code avoids treating the split unit 
> rnglists_base == 0 case as "there is no rnglists_base and so rnglistx isn't 
> usable"?

`m_ranges_base` is not zero in such case as it has been set from the skeleton 
.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106466

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D106466: [llvm+lldb] Fix#2 of DW_AT_ranges DW_FORM_sec_offset not using DW_AT_rnglists_base (used by GCC)

2021-08-16 Thread David Blaikie via Phabricator via lldb-commits
dblaikie added a comment.

I assume there's already test coverage for rnglistx in debug_info.dwo/split 
unit? (because in that case there's no rnglists_base, but rnglistx is usable) - 
could you explain how this code avoids treating the split unit rnglists_base == 
0 case as "there is no rnglists_base and so rnglistx isn't usable"?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106466

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D106466: [llvm+lldb] Fix#2 of DW_AT_ranges DW_FORM_sec_offset not using DW_AT_rnglists_base (used by GCC)

2021-08-16 Thread Jan Kratochvil via Phabricator via lldb-commits
jankratochvil updated this revision to Diff 366676.
jankratochvil edited the summary of this revision.
jankratochvil added a comment.

@dblaikie do you think it is OK for check-in now?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106466

Files:
  lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
  lldb/test/Shell/SymbolFile/DWARF/DW_AT_range-DW_FORM_sec_offset.s


Index: lldb/test/Shell/SymbolFile/DWARF/DW_AT_range-DW_FORM_sec_offset.s
===
--- lldb/test/Shell/SymbolFile/DWARF/DW_AT_range-DW_FORM_sec_offset.s
+++ lldb/test/Shell/SymbolFile/DWARF/DW_AT_range-DW_FORM_sec_offset.s
@@ -29,7 +29,7 @@
 # RUN:   -o exit 2>&1 | FileCheck --check-prefix=RNGLISTBASE %s
 
 # RNGLISTBASE-LABEL: image lookup -v -s lookup_rnglists
-# RNGLISTBASE: error: {{.*}}-rnglistbase {0x0043}: DIE has 
DW_AT_ranges(DW_FORM_rnglistx 0x0) attribute, but range extraction failed 
(invalid range list table index 0; OffsetEntryCount is 0, DW_AT_rnglists_base 
is 12), please file a bug and attach the file at the start of this error message
+# RNGLISTBASE: error: {{.*}}-rnglistbase {0x0043}: DIE has 
DW_AT_ranges(DW_FORM_rnglistx 0x0) attribute, but range extraction failed 
(invalid range list table index 0; OffsetEntryCount is 0, DW_AT_rnglists_base 
is 24), please file a bug and attach the file at the start of this error message
 
 .text
 rnglists:
@@ -97,7 +97,7 @@
 .long   .Lrnglists_end-rnglists # DW_AT_high_pc
 .long   .Laddr_table_base0  # DW_AT_addr_base
 .ifdef RNGLISTBASE
-.long   .Ldebug_ranges0 # DW_AT_rnglists_base
+.long   .Ldebug_ranges1 # DW_AT_rnglists_base
 .endif
 .byte   2   # Abbrev [2] 0x2b:0x37 
DW_TAG_subprogram
 .quad   rnglists# DW_AT_low_pc
@@ -105,7 +105,7 @@
 .asciz  "rnglists"  # DW_AT_name
 .byte   5   # Abbrev [5] 0x52:0xf 
DW_TAG_lexical_block
 .ifndef RNGLISTX
-.long   .Ldebug_ranges0 # DW_AT_ranges DW_FORM_sec_offset
+.long   .Ldebug_ranges1 # DW_AT_ranges DW_FORM_sec_offset
 .else
 .uleb128 0  # DW_AT_ranges DW_FORM_rnglistx
 .endif
@@ -130,9 +130,17 @@
 .byte   8   # Address size
 .byte   0   # Segment selector size
 .long   0   # Offset entry count
-.Ldebug_ranges0:
+.Ldebug_rnglist_table_end0:
+
+.long   .Ldebug_rnglist_table_end1-.Ldebug_rnglist_table_start1 # 
Length
+.Ldebug_rnglist_table_start1:
+.short  5   # Version
+.byte   8   # Address size
+.byte   0   # Segment selector size
+.long   0   # Offset entry count
+.Ldebug_ranges1:
 .byte   4   # DW_RLE_offset_pair
 .uleb128 .Lblock1_begin-rnglists  #   starting offset
 .uleb128 .Lblock1_end-rnglists#   ending offset
 .byte   0   # DW_RLE_end_of_list
-.Ldebug_rnglist_table_end0:
+.Ldebug_rnglist_table_end1:
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
@@ -437,15 +437,20 @@
   // We are expected to be called with Offset 0 or pointing just past the table
   // header. Correct Offset in the latter case so that it points to the start
   // of the header.
-  if (offset > 0) {
-uint64_t HeaderSize = llvm::DWARFListTableHeader::getHeaderSize(format);
-if (offset < HeaderSize)
-  return llvm::createStringError(errc::invalid_argument,
- "did not detect a valid"
- " list table with base = 0x%" PRIx64 "\n",
- offset);
-offset -= HeaderSize;
+  if (offset == 0) {
+// This means DW_AT_rnglists_base is missing and therefore DW_FORM_rnglistx
+// cannot be handled. Returning a default-constructed ListTableType allows
+// DW_FORM_sec_offset to be supported.
+return ListTableType();
   }
+
+  uint64_t HeaderSize = llvm::DWARFListTableHeader::getHeaderSize(format);
+  if (offset < HeaderSize)
+return llvm::createStringError(errc::invalid_argument,
+   "did not detect a valid"
+   " list table with base = 0x%" PRIx64 "\n",
+   offset);
+  offset -= HeaderSize;
   ListTableType Table;
   if (llvm::Error E = Table.extractHeaderAndOffsets(data, ))
 return std::move(E);
@@ -996,8 +1001,12 @@
 return llvm::createStringError(errc::invalid_argument,
"missing 

[Lldb-commits] [PATCH] D106466: [llvm+lldb] Fix#2 of DW_AT_ranges DW_FORM_sec_offset not using DW_AT_rnglists_base (used by GCC)

2021-08-16 Thread Igor Kudrin via Phabricator via lldb-commits
ikudrin added a comment.

The code looks good, but please improve the comments and wait for approval from 
a more LLDB-knowledgeable person than me,




Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp:441
+  if (offset == 0) {
+// Caller must not use this default initializater for GetRnglistOffset.
+return ListTableType();

Please extend the comment to emphasize that even if `DW_AT_rnglists_base` is 
missing and `DW_FORM_rnglistx` cannot be handled, returning a 
default-constructed Table allows `DW_FORM_sec_offset` to be supported.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp:1005
+
+  // As \a offset can be zero we need to call setAddressSize.
+  data.setAddressSize(m_header.GetAddressByteSize());

This comment is quite misleading as it references `offset` which is the 
argument of the method.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106466

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D106466: [llvm+lldb] Fix#2 of DW_AT_ranges DW_FORM_sec_offset not using DW_AT_rnglists_base (used by GCC)

2021-08-14 Thread Jan Kratochvil via Phabricator via lldb-commits
jankratochvil added a comment.

In D106466#2939551 , @ikudrin wrote:

> In D106466#2926185 , @jankratochvil 
> wrote:
>
>> One needs to set at least `AddrSize` and `OffsetEntryCount` as callers do 
>> use it.
>
> Could you please point me to that usage? I am not that familiar with the LLDB 
> code.

Those are used by 
`DWARFUnit::GetRnglistOffset`->`llvm::DWARFDebugRnglistTable::getOffsetEntry`. 
But in such case `llvm::DWARFDebugRnglistTable::extractHeaderAndOffsets` were 
already called that time.
So that is not the code path I need for DWARF not using `DW_AT_rnglists_base`.

I understand now the `Header.length==0` case so the fix is simple now, thanks 
for showing it to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106466

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D106466: [llvm+lldb] Fix#2 of DW_AT_ranges DW_FORM_sec_offset not using DW_AT_rnglists_base (used by GCC)

2021-08-14 Thread Jan Kratochvil via Phabricator via lldb-commits
jankratochvil updated this revision to Diff 366449.
jankratochvil retitled this revision from "3/3: [llvm+lldb] Fix#2 of 
DW_AT_ranges DW_FORM_sec_offset not using DW_AT_rnglists_base (used by GCC)" to 
"[llvm+lldb] Fix#2 of DW_AT_ranges DW_FORM_sec_offset not using 
DW_AT_rnglists_base (used by GCC)".

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106466

Files:
  lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
  lldb/test/Shell/SymbolFile/DWARF/DW_AT_range-DW_FORM_sec_offset.s


Index: lldb/test/Shell/SymbolFile/DWARF/DW_AT_range-DW_FORM_sec_offset.s
===
--- lldb/test/Shell/SymbolFile/DWARF/DW_AT_range-DW_FORM_sec_offset.s
+++ lldb/test/Shell/SymbolFile/DWARF/DW_AT_range-DW_FORM_sec_offset.s
@@ -29,7 +29,7 @@
 # RUN:   -o exit 2>&1 | FileCheck --check-prefix=RNGLISTBASE %s
 
 # RNGLISTBASE-LABEL: image lookup -v -s lookup_rnglists
-# RNGLISTBASE: error: {{.*}}-rnglistbase {0x0043}: DIE has 
DW_AT_ranges(DW_FORM_rnglistx 0x0) attribute, but range extraction failed 
(invalid range list table index 0; OffsetEntryCount is 0, DW_AT_rnglists_base 
is 12), please file a bug and attach the file at the start of this error message
+# RNGLISTBASE: error: {{.*}}-rnglistbase {0x0043}: DIE has 
DW_AT_ranges(DW_FORM_rnglistx 0x0) attribute, but range extraction failed 
(invalid range list table index 0; OffsetEntryCount is 0, DW_AT_rnglists_base 
is 24), please file a bug and attach the file at the start of this error message
 
 .text
 rnglists:
@@ -97,7 +97,7 @@
 .long   .Lrnglists_end-rnglists # DW_AT_high_pc
 .long   .Laddr_table_base0  # DW_AT_addr_base
 .ifdef RNGLISTBASE
-.long   .Ldebug_ranges0 # DW_AT_rnglists_base
+.long   .Ldebug_ranges1 # DW_AT_rnglists_base
 .endif
 .byte   2   # Abbrev [2] 0x2b:0x37 
DW_TAG_subprogram
 .quad   rnglists# DW_AT_low_pc
@@ -105,7 +105,7 @@
 .asciz  "rnglists"  # DW_AT_name
 .byte   5   # Abbrev [5] 0x52:0xf 
DW_TAG_lexical_block
 .ifndef RNGLISTX
-.long   .Ldebug_ranges0 # DW_AT_ranges DW_FORM_sec_offset
+.long   .Ldebug_ranges1 # DW_AT_ranges DW_FORM_sec_offset
 .else
 .uleb128 0  # DW_AT_ranges DW_FORM_rnglistx
 .endif
@@ -130,9 +130,17 @@
 .byte   8   # Address size
 .byte   0   # Segment selector size
 .long   0   # Offset entry count
-.Ldebug_ranges0:
+.Ldebug_rnglist_table_end0:
+
+.long   .Ldebug_rnglist_table_end1-.Ldebug_rnglist_table_start1 # 
Length
+.Ldebug_rnglist_table_start1:
+.short  5   # Version
+.byte   8   # Address size
+.byte   0   # Segment selector size
+.long   0   # Offset entry count
+.Ldebug_ranges1:
 .byte   4   # DW_RLE_offset_pair
 .uleb128 .Lblock1_begin-rnglists  #   starting offset
 .uleb128 .Lblock1_end-rnglists#   ending offset
 .byte   0   # DW_RLE_end_of_list
-.Ldebug_rnglist_table_end0:
+.Ldebug_rnglist_table_end1:
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
@@ -437,15 +437,18 @@
   // We are expected to be called with Offset 0 or pointing just past the table
   // header. Correct Offset in the latter case so that it points to the start
   // of the header.
-  if (offset > 0) {
-uint64_t HeaderSize = llvm::DWARFListTableHeader::getHeaderSize(format);
-if (offset < HeaderSize)
-  return llvm::createStringError(errc::invalid_argument,
- "did not detect a valid"
- " list table with base = 0x%" PRIx64 "\n",
- offset);
-offset -= HeaderSize;
+  if (offset == 0) {
+// Caller must not use this default initializater for GetRnglistOffset.
+return ListTableType();
   }
+
+  uint64_t HeaderSize = llvm::DWARFListTableHeader::getHeaderSize(format);
+  if (offset < HeaderSize)
+return llvm::createStringError(errc::invalid_argument,
+   "did not detect a valid"
+   " list table with base = 0x%" PRIx64 "\n",
+   offset);
+  offset -= HeaderSize;
   ListTableType Table;
   if (llvm::Error E = Table.extractHeaderAndOffsets(data, ))
 return std::move(E);
@@ -996,8 +999,12 @@
 return llvm::createStringError(errc::invalid_argument,
"missing or invalid