[Lldb-commits] [PATCH] D147606: [lldb] fix #61942, discard "empty" ranges in DWARF to better handle gcc binary

2023-04-27 Thread LU Hongyi via Phabricator via lldb-commits 
jwnhy added a comment.

Ping!


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

https://reviews.llvm.org/D147606

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

[Lldb-commits] [PATCH] D147606: [lldb] fix #61942, discard "empty" ranges in DWARF to better handle gcc binary

2023-04-11 Thread LU Hongyi via Phabricator via lldb-commits 
jwnhy added a comment.

In D147606#4246832 , @JDevlieghere 
wrote:

> The change looks fine and regardless of whether this makes sense or even 
> complies with the standard, we should be resilient against it. I would like 
> to see a test though.

It seems that DWARFv5 indeed specifies how to deal with these empty ranges

> A bounded range entry whose beginning and ending address offsets are equal
> (including zero) indicates an empty range and may be ignored.

Also, I kind of searched through the codebase, and noticed that there are 
multiple places like this, and they adapt a similar approach discarding those 
empty ranges.
e.g. in `DWARFDebugRanges.cpp`

// Filter out empty ranges
if (begin < end)
  range_list.Append(DWARFRangeList::Entry(begin + base_addr, end - begin));
  }


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

https://reviews.llvm.org/D147606

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

[Lldb-commits] [PATCH] D147606: [lldb] fix #61942, discard "empty" ranges in DWARF to better handle gcc binary

2023-04-11 Thread LU Hongyi via Phabricator via lldb-commits 
jwnhy updated this revision to Diff 512362.
jwnhy added a comment.

Updated the patch to remove unncessary checks and added a testcase.


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

https://reviews.llvm.org/D147606

Files:
  lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
  lldb/test/Shell/SymbolFile/DWARF/x86/debug_empty_ranges.s

Index: lldb/test/Shell/SymbolFile/DWARF/x86/debug_empty_ranges.s
===
--- /dev/null
+++ lldb/test/Shell/SymbolFile/DWARF/x86/debug_empty_ranges.s
@@ -0,0 +1,481 @@
+# Test handling of empty DWARF ranges generated by GCC
+
+# RUN: %clang --target=x86_64-pc-linux -o %t %s 
+# RUN: %lldb %t -o "b func" -o "r" -o "expression -T -- i" \
+# RUN: -o "exit" | FileCheck %s
+
+# Failing case was:
+# error: expression failed to parse:
+# error: :1:1: use of undeclared identifier 'i'
+# CHECK: (int) $0 = 10
+	.file	"r.c"
+	.file 1 "r.c"
+	.text
+.Ltext0:
+	.file 0 "" "r.c"
+	.globl	main
+	.type	main, @function
+main:
+.LFB1:
+	.file 1 "r.c"
+	.loc 1 12 12
+	.cfi_startproc
+	.loc 1 12 14
+	movl	b(%rip), %eax
+.LBB5:
+.LBB6:
+	.loc 1 4 3
+.LVL0:
+	.loc 1 5 3
+	.loc 1 5 12
+.LBE6:
+	.loc 1 6 5
+	.loc 1 7 5
+.LBB7:
+	.loc 1 5 19
+	.loc 1 5 12
+.LBE7:
+	.loc 1 6 5
+	.loc 1 7 5
+.LBB8:
+	.loc 1 5 19
+	.loc 1 5 12
+.LBE8:
+	.loc 1 6 5
+	.loc 1 7 5
+.LBB9:
+	.loc 1 5 19
+	.loc 1 5 12
+.LBE9:
+	.loc 1 6 5
+	.loc 1 7 5
+.LBB10:
+	.loc 1 5 19
+	.loc 1 5 12
+.LBE10:
+	.loc 1 6 5
+	.loc 1 7 5
+.LBB11:
+	.loc 1 5 19
+	.loc 1 5 12
+.LBE11:
+	.loc 1 6 5
+	.loc 1 7 5
+.LBB12:
+	.loc 1 5 19
+	.loc 1 5 12
+.LBE12:
+	.loc 1 6 5
+	.loc 1 7 5
+.LBB13:
+	.loc 1 5 19
+	.loc 1 5 12
+.LBE13:
+	.loc 1 6 5
+	.loc 1 7 5
+.LBB14:
+	.loc 1 5 19
+	.loc 1 5 12
+.LBE14:
+	.loc 1 6 5
+	.loc 1 7 5
+.LBB15:
+	.loc 1 5 19
+	.loc 1 5 12
+.LBE15:
+	.loc 1 6 5
+	.loc 1 7 5
+.LBB16:
+	.loc 1 5 19
+	.loc 1 5 12
+	.loc 1 9 3
+	.loc 1 9 5 is_stmt 0
+	movl	$0, a(%rip)
+	.loc 1 10 3 is_stmt 1
+.LVL1:
+.LBE16:
+.LBE5:
+	.loc 1 12 26 is_stmt 0
+	movl	$0, %eax
+	ret
+	.cfi_endproc
+.LFE1:
+	.size	main, .-main
+	.globl	b
+	.bss
+	.align 4
+	.type	b, @object
+	.size	b, 4
+b:
+	.zero	4
+	.globl	a
+	.align 4
+	.type	a, @object
+	.size	a, 4
+a:
+	.zero	4
+	.text
+.Letext0:
+	.section	.debug_info,"",@progbits
+.Ldebug_info0:
+	.long	0xd9
+	.value	0x5
+	.byte	0x1
+	.byte	0x8
+	.long	.Ldebug_abbrev0
+	.uleb128 0x3
+	.long	.LASF2
+	.byte	0x1d
+	.long	.LASF0
+	.long	.LASF1
+	.quad	.Ltext0
+	.quad	.Letext0-.Ltext0
+	.long	.Ldebug_line0
+	.uleb128 0x1
+	.string	"a"
+	.byte	0x1
+	.byte	0x5
+	.long	0x41
+	.uleb128 0x9
+	.byte	0x3
+	.quad	a
+	.uleb128 0x4
+	.byte	0x4
+	.byte	0x5
+	.string	"int"
+	.uleb128 0x5
+	.long	0x41
+	.uleb128 0x1
+	.string	"b"
+	.byte	0x2
+	.byte	0xe
+	.long	0x48
+	.uleb128 0x9
+	.byte	0x3
+	.quad	b
+	.uleb128 0x6
+	.long	.LASF3
+	.byte	0x1
+	.byte	0xc
+	.byte	0x5
+	.long	0x41
+	.quad	.LFB1
+	.quad	.LFE1-.LFB1
+	.uleb128 0x1
+	.byte	0x9c
+	.long	0xb0
+	.uleb128 0x7
+	.long	0xb0
+	.quad	.LBB5
+	.quad	.LBE5-.LBB5
+	.byte	0x1
+	.byte	0xc
+	.byte	0xe
+	.uleb128 0x8
+	.long	0xbd
+	.uleb128 0x9
+	.long	.LLRL0
+	.uleb128 0xa
+	.long	0xc7
+	.long	.LLST1
+	.byte	0
+	.byte	0
+	.byte	0
+	.uleb128 0xb
+	.long	.LASF4
+	.byte	0x1
+	.byte	0x3
+	.byte	0xc
+	.long	0x41
+	.byte	0x1
+	.uleb128 0xc
+	.string	"b"
+	.byte	0x1
+	.byte	0x3
+	.byte	0x18
+	.long	0x41
+	.uleb128 0x2
+	.string	"i"
+	.byte	0x4
+	.byte	0x7
+	.long	0x41
+	.uleb128 0xd
+	.uleb128 0x2
+	.string	"c"
+	.byte	0x6
+	.byte	0x9
+	.long	0x41
+	.byte	0
+	.byte	0
+	.byte	0
+	.section	.debug_abbrev,"",@progbits
+.Ldebug_abbrev0:
+	.uleb128 0x1
+	.uleb128 0x34
+	.byte	0
+	.uleb128 0x3
+	.uleb128 0x8
+	.uleb128 0x3a
+	.uleb128 0x21
+	.sleb128 1
+	.uleb128 0x3b
+	.uleb128 0xb
+	.uleb128 0x39
+	.uleb128 0xb
+	.uleb128 0x49
+	.uleb128 0x13
+	.uleb128 0x3f
+	.uleb128 0x19
+	.uleb128 0x2
+	.uleb128 0x18
+	.byte	0
+	.byte	0
+	.uleb128 0x2
+	.uleb128 0x34
+	.byte	0
+	.uleb128 0x3
+	.uleb128 0x8
+	.uleb128 0x3a
+	.uleb128 0x21
+	.sleb128 1
+	.uleb128 0x3b
+	.uleb128 0xb
+	.uleb128 0x39
+	.uleb128 0xb
+	.uleb128 0x49
+	.uleb128 0x13
+	.byte	0
+	.byte	0
+	.uleb128 0x3
+	.uleb128 0x11
+	.byte	0x1
+	.uleb128 0x25
+	.uleb128 0xe
+	.uleb128 0x13
+	.uleb128 0xb
+	.uleb128 0x3
+	.uleb128 0x1f
+	.uleb128 0x1b
+	.uleb128 0x1f
+	.uleb128 0x11
+	.uleb128 0x1
+	.uleb128 0x12
+	.uleb128 0x7
+	.uleb128 0x10
+	.uleb128 0x17
+	.byte	0
+	.byte	0
+	.uleb128 0x4
+	.uleb128 0x24
+	.byte	0
+	.uleb128 0xb
+	.uleb128 0xb
+	.uleb128 0x3e
+	.uleb128 0xb
+	.uleb128 0x3
+	.uleb128 0x8
+	.byte	0
+	.byte	0
+	.uleb128 0x5
+	.uleb128 0x35
+	.byte	0
+	.uleb128 0x49
+	.uleb128 0x13
+	.byte	0
+	.byte	0
+	.uleb128 0x6
+	.uleb128 0x2e
+	.byte	0x1
+	.uleb128 0x3f
+	.uleb128 0x19
+	.uleb128 0x3
+	.uleb128 0xe
+	.uleb128 0x3a
+	.uleb128 0xb
+	.uleb128 0x3b
+	.uleb128 0xb
+	.uleb128 0x39
+	.uleb128 0xb
+	.uleb128 0x49
+	.uleb128 0x13
+	.uleb128 0x11
+	.uleb128 0x1
+	.uleb128 0x12
+	.uleb128 0x7
+	.uleb128 0x40
+	.uleb128 0x18
+	.uleb128 0x7a
+

[Lldb-commits] [PATCH] D147606: [lldb] fix #61942, discard "empty" ranges in DWARF to better handle gcc binary

2023-04-11 Thread Pavel Labath via Phabricator via lldb-commits 
labath added a comment.

In D147606#4249283 , @JDevlieghere 
wrote:

> In D147606#4247462 , @jwnhy wrote:
>
>> In D147606#4246832 , @JDevlieghere 
>> wrote:
>>
>>> The change looks fine and regardless of whether this makes sense or even 
>>> complies with the standard, we should be resilient against it. I would like 
>>> to see a test though.
>>
>> Thanks a lot for the comment, I am new to lldb community, and got one thing 
>> a bit silly to ask.
>> Up to now, a few patches I submitted is kind of "depending on the 
>> compiler-generated" binary?
>> What am I supposed to do to **ensure the compiler generates these 
>> "easy-to-fault" binaries in the test?**
>>
>> Like in this one, not every compiler will generate "empty ranges", and in 
>> the other one that is "DW_OP_div"...
>
> Yes, this would require a test that checks in what the compiler generates (as 
> opposed to the majority of our "API tests" that build test cases from 
> source). This can either be an assembly file (something like 
> `dwarf5-implicit-const.s`) or a YAML file created with `obj2yaml` (something 
> like `section-overlap.yaml`).

You may be able to adapt/extend 
`lldb/test/Shell/SymbolFile/DWARF/x86/debug_ranges.s`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147606

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

[Lldb-commits] [PATCH] D147606: [lldb] fix #61942, discard "empty" ranges in DWARF to better handle gcc binary

2023-04-06 Thread Jonas Devlieghere via Phabricator via lldb-commits 
JDevlieghere added a comment.

In D147606#4247462 , @jwnhy wrote:

> In D147606#4246832 , @JDevlieghere 
> wrote:
>
>> The change looks fine and regardless of whether this makes sense or even 
>> complies with the standard, we should be resilient against it. I would like 
>> to see a test though.
>
> Thanks a lot for the comment, I am new to lldb community, and got one thing a 
> bit silly to ask.
> Up to now, a few patches I submitted is kind of "depending on the 
> compiler-generated" binary?
> What am I supposed to do to **ensure the compiler generates these 
> "easy-to-fault" binaries in the test?**
>
> Like in this one, not every compiler will generate "empty ranges", and in the 
> other one that is "DW_OP_div"...

Yes, this would require a test that checks in what the compiler generates (as 
opposed to the majority of our "API tests" that build test cases from source). 
This can either be an assembly file (something like `dwarf5-implicit-const.s`) 
or a YAML file created with `obj2yaml` (something like `section-overlap.yaml`).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147606

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

[Lldb-commits] [PATCH] D147606: [lldb] fix #61942, discard "empty" ranges in DWARF to better handle gcc binary

2023-04-05 Thread LU Hongyi via Phabricator via lldb-commits 
jwnhy added a comment.

In D147606#4246832 , @JDevlieghere 
wrote:

> The change looks fine and regardless of whether this makes sense or even 
> complies with the standard, we should be resilient against it. I would like 
> to see a test though.

Thanks a lot for the comment, I am new to lldb community, and got one thing a 
bit silly to ask.
Up to now, a few patches I submitted is kind of "depending on the 
compiler-generated" binary?
What am I supposed to do to **ensure the compiler generates these 
"easy-to-fault" binaries in the test?**

Like in this one, not every compiler will generate "empty ranges", and in the 
other one that is "DW_OP_div"...

I would like to add tests once I figure these out...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147606

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

[Lldb-commits] [PATCH] D147606: [lldb] fix #61942, discard "empty" ranges in DWARF to better handle gcc binary

2023-04-05 Thread LU Hongyi via Phabricator via lldb-commits 
jwnhy added a comment.

In D147606#4245804 , @DavidSpickett 
wrote:

> I'm not familiar with this code, but the issue as explained I think I 
> understand.
>
> It seems like you're checking for empty ranges in two places, what does each 
> one do?
>
> Is there anything else these ranges indicate or do we think it is simply a 
> missed optimisation in the debug info producer? If they do sometimes mean 
> something, then perhaps improving the searching of the ranges is preferable 
> to discarding the empty ones.
> (though if all tests pass with this patch then these empty ranges can't be 
> that important to us can they)

Thanks for the comments, I didn't improve the search because that searching 
logic seems being used everywhere and not just restricted to filter through 
these "address ranges", I am a bit afraid of changing that also changes 
something subtle.
though it seems not making much sense to returning a "empty range" as searching 
results.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147606

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

[Lldb-commits] [PATCH] D147606: [lldb] fix #61942, discard "empty" ranges in DWARF to better handle gcc binary

2023-04-05 Thread Jonas Devlieghere via Phabricator via lldb-commits 
JDevlieghere added a comment.

The change looks fine and regardless of whether this makes sense or even 
complies with the standard, we should be resilient against it. I would like to 
see a test though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147606

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

[Lldb-commits] [PATCH] D147606: [lldb] fix #61942, discard "empty" ranges in DWARF to better handle gcc binary

2023-04-05 Thread David Spickett via Phabricator via lldb-commits 
DavidSpickett added a comment.

I'm not familiar with this code, but the issue as explained I think I 
understand.

It seems like you're checking for empty ranges in two places, what does each 
one do?

Is there anything else these ranges indicate or do we think it is simply a 
missed optimisation in the debug info producer? If they do sometimes mean 
something, then perhaps improving the searching of the ranges is preferable to 
discarding the empty ones.
(though if all tests pass with this patch then these empty ranges can't be that 
important to us can they)




Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:1318
+  if (range_base >= subprogram_low_pc) {
+if (range.IsValid())
+  block->AddRange(Block::Range(range_base - subprogram_low_pc,

Combine this into one if. Also I'd put the Valid check first, just seems like 
the right order for it (not that it matters really).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147606

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

[Lldb-commits] [PATCH] D147606: [lldb] fix #61942, discard "empty" ranges in DWARF to better handle gcc binary

2023-04-05 Thread LU Hongyi via Phabricator via lldb-commits 
jwnhy created this revision.
jwnhy added reviewers: Michael137, DavidSpickett, JDevlieghere.
Herald added a project: All.
jwnhy requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

This patch resolves an issue that lldb not be able to
match the correct range of certain address.

This issue caused by other compilers like gcc generates
"empty ranges" like [address, address) in the DIE. These
"empty ranges" cause lldb matches address with these
ranges unintentionally and causes unpredictable result.
(In #61942, a variable dispearred due to this issue)

This patch resolves this issue by discarding these "empty
ranges" during the parsing of debugging information.

Another approach in fixing this might be changing the
logic of "FindEntryThatContains" and let it try harder
if met "empty ranges".


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D147606

Files:
  lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp


Index: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -1314,10 +1314,11 @@
 for (size_t i = 0; i < num_ranges; ++i) {
   const DWARFRangeList::Entry  = ranges.GetEntryRef(i);
   const addr_t range_base = range.GetRangeBase();
-  if (range_base >= subprogram_low_pc)
-block->AddRange(Block::Range(range_base - subprogram_low_pc,
+  if (range_base >= subprogram_low_pc) {
+if (range.IsValid())
+  block->AddRange(Block::Range(range_base - subprogram_low_pc,
  range.GetByteSize()));
-  else {
+  } else {
 GetObjectFile()->GetModule()->ReportError(
 "{0:x8}: adding range [{1:x16}-{2:x16}) which has a base "
 "that is less than the function's low PC {3:x16}. Please file "
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
@@ -1065,8 +1065,9 @@
 
   DWARFRangeList ranges;
   for (const llvm::DWARFAddressRange _range : *llvm_ranges) {
-ranges.Append(DWARFRangeList::Entry(llvm_range.LowPC,
-llvm_range.HighPC - llvm_range.LowPC));
+if (llvm_range.HighPC - llvm_range.LowPC > 0)
+  ranges.Append(DWARFRangeList::Entry(llvm_range.LowPC,
+  llvm_range.HighPC - 
llvm_range.LowPC));
   }
   return ranges;
 }


Index: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -1314,10 +1314,11 @@
 for (size_t i = 0; i < num_ranges; ++i) {
   const DWARFRangeList::Entry  = ranges.GetEntryRef(i);
   const addr_t range_base = range.GetRangeBase();
-  if (range_base >= subprogram_low_pc)
-block->AddRange(Block::Range(range_base - subprogram_low_pc,
+  if (range_base >= subprogram_low_pc) {
+if (range.IsValid())
+  block->AddRange(Block::Range(range_base - subprogram_low_pc,
  range.GetByteSize()));
-  else {
+  } else {
 GetObjectFile()->GetModule()->ReportError(
 "{0:x8}: adding range [{1:x16}-{2:x16}) which has a base "
 "that is less than the function's low PC {3:x16}. Please file "
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
@@ -1065,8 +1065,9 @@
 
   DWARFRangeList ranges;
   for (const llvm::DWARFAddressRange _range : *llvm_ranges) {
-ranges.Append(DWARFRangeList::Entry(llvm_range.LowPC,
-llvm_range.HighPC - llvm_range.LowPC));
+if (llvm_range.HighPC - llvm_range.LowPC > 0)
+  ranges.Append(DWARFRangeList::Entry(llvm_range.LowPC,
+  llvm_range.HighPC - llvm_range.LowPC));
   }
   return ranges;
 }
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits