This is an archive of the discontinued LLVM Phabricator instance.

Differential D147606

[lldb] fix #61942, discard "empty" ranges in DWARF to better handle gcc binary
Needs ReviewPublic

Authored by jwnhy on Apr 5 2023, 5:41 AM.

Details

Reviewers
Michael137
DavidSpickett
JDevlieghere
Summary

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".

Diff Detail

Event Timeline

jwnhy created this revision.Apr 5 2023, 5:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 5 2023, 5:41 AM
jwnhy requested review of this revision.Apr 5 2023, 5:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 5 2023, 5:41 AM
Herald added a subscriber: lldb-commits. · View Herald Transcript
Harbormaster completed remote builds in B223773: Diff 511063.Apr 5 2023, 5:45 AM
DavidSpickett added a comment.Apr 5 2023, 6:41 AM

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)

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
1318 ↗(On Diff #511063)

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).

JDevlieghere added a comment.Apr 5 2023, 12:44 PM

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.

jwnhy added a comment.Apr 5 2023, 6:35 PM
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.

jwnhy added a comment.Apr 5 2023, 6:38 PM
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...

JDevlieghere added a comment.Apr 6 2023, 10:35 AM
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).

labath added a subscriber: labath.Apr 11 2023, 1:10 AM
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

jwnhy updated this revision to Diff 512362.Apr 11 2023, 1:31 AM

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

Harbormaster completed remote builds in B224714: Diff 512362.Apr 11 2023, 1:34 AM
jwnhy added a comment.Apr 11 2023, 1:43 AM
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));
}
jwnhy added a comment.Apr 27 2023, 12:35 AM

Ping!

Revision Contents

PathSize
lldb/
source/
Plugins/
SymbolFile/
DWARF/
5 lines
test/
Shell/
SymbolFile/
DWARF/
x86/
481 lines

Diff 512362

lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp

Show First 20 Lines • Show All 1,059 Lines • ▼ Show 20 Lines llvm::Expected<llvm::DWARFAddressRangesVector> llvm_ranges =
m_dwarf.GetDWARFContext().getOrLoadAddrData().GetMaxU64( m_dwarf.GetDWARFContext().getOrLoadAddrData().GetMaxU64(
&offset, index_size)}; &offset, index_size)};
}); });
if (!llvm_ranges) if (!llvm_ranges)
return llvm_ranges.takeError(); return llvm_ranges.takeError();
DWARFRangeList ranges; DWARFRangeList ranges;
for (const llvm::DWARFAddressRange &llvm_range : *llvm_ranges) { for (const llvm::DWARFAddressRange &llvm_range : *llvm_ranges) {
if (llvm_range.HighPC - llvm_range.LowPC > 0)
ranges.Append(DWARFRangeList::Entry(llvm_range.LowPC, ranges.Append(DWARFRangeList::Entry(llvm_range.LowPC,
llvm_range.HighPC - llvm_range.LowPC)); llvm_range.HighPC - llvm_range.LowPC));
} }
return ranges; return ranges;
} }
llvm::Expected<DWARFRangeList> llvm::Expected<DWARFRangeList>
DWARFUnit::FindRnglistFromIndex(uint32_t index) { DWARFUnit::FindRnglistFromIndex(uint32_t index) {
llvm::Expected<uint64_t> maybe_offset = GetRnglistOffset(index); llvm::Expected<uint64_t> maybe_offset = GetRnglistOffset(index);
if (!maybe_offset) if (!maybe_offset)
Show All 18 Lines

lldb/test/Shell/SymbolFile/DWARF/x86/debug_empty_ranges.s

  • This file was added.
# 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: <user expression 0>: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
.uleb128 0x19
.uleb128 0x1
.uleb128 0x13
.byte 0
.byte 0
.uleb128 0x7
.uleb128 0x1d
.byte 0x1
.uleb128 0x31
.uleb128 0x13
.uleb128 0x11
.uleb128 0x1
.uleb128 0x12
.uleb128 0x7
.uleb128 0x58
.uleb128 0xb
.uleb128 0x59
.uleb128 0xb
.uleb128 0x57
.uleb128 0xb
.byte 0
.byte 0
.uleb128 0x8
.uleb128 0x5
.byte 0
.uleb128 0x31
.uleb128 0x13
.byte 0
.byte 0
.uleb128 0x9
.uleb128 0xb
.byte 0x1
.uleb128 0x55
.uleb128 0x17
.byte 0
.byte 0
.uleb128 0xa
.uleb128 0x34
.byte 0
.uleb128 0x31
.uleb128 0x13
.uleb128 0x2
.uleb128 0x17
.byte 0
.byte 0
.uleb128 0xb
.uleb128 0x2e
.byte 0x1
.uleb128 0x3
.uleb128 0xe
.uleb128 0x3a
.uleb128 0xb
.uleb128 0x3b
.uleb128 0xb
.uleb128 0x39
.uleb128 0xb
.uleb128 0x27
.uleb128 0x19
.uleb128 0x49
.uleb128 0x13
.uleb128 0x20
.uleb128 0xb
.byte 0
.byte 0
.uleb128 0xc
.uleb128 0x5
.byte 0
.uleb128 0x3
.uleb128 0x8
.uleb128 0x3a
.uleb128 0xb
.uleb128 0x3b
.uleb128 0xb
.uleb128 0x39
.uleb128 0xb
.uleb128 0x49
.uleb128 0x13
.byte 0
.byte 0
.uleb128 0xd
.uleb128 0xb
.byte 0x1
.byte 0
.byte 0
.byte 0
.section .debug_loclists,"",@progbits
.long .Ldebug_loc3-.Ldebug_loc2
.Ldebug_loc2:
.value 0x5
.byte 0x8
.byte 0
.long 0
.Ldebug_loc0:
.LLST1:
.byte 0x4
.uleb128 .LVL0-.Ltext0
.uleb128 .LVL1-.Ltext0
.uleb128 0x2
.byte 0x3a
.byte 0x9f
.byte 0
.Ldebug_loc3:
.section .debug_aranges,"",@progbits
.long 0x2c
.value 0x2
.long .Ldebug_info0
.byte 0x8
.byte 0
.value 0
.value 0
.quad .Ltext0
.quad .Letext0-.Ltext0
.quad 0
.quad 0
.section .debug_rnglists,"",@progbits
.Ldebug_ranges0:
.long .Ldebug_ranges3-.Ldebug_ranges2
.Ldebug_ranges2:
.value 0x5
.byte 0x8
.byte 0
.long 0
.LLRL0:
.byte 0x4
.uleb128 .LBB6-.Ltext0
.uleb128 .LBE6-.Ltext0
.byte 0x4
.uleb128 .LBB7-.Ltext0
.uleb128 .LBE7-.Ltext0
.byte 0x4
.uleb128 .LBB8-.Ltext0
.uleb128 .LBE8-.Ltext0
.byte 0x4
.uleb128 .LBB9-.Ltext0
.uleb128 .LBE9-.Ltext0
.byte 0x4
.uleb128 .LBB10-.Ltext0
.uleb128 .LBE10-.Ltext0
.byte 0x4
.uleb128 .LBB11-.Ltext0
.uleb128 .LBE11-.Ltext0
.byte 0x4
.uleb128 .LBB12-.Ltext0
.uleb128 .LBE12-.Ltext0
.byte 0x4
.uleb128 .LBB13-.Ltext0
.uleb128 .LBE13-.Ltext0
.byte 0x4
.uleb128 .LBB14-.Ltext0
.uleb128 .LBE14-.Ltext0
.byte 0x4
.uleb128 .LBB15-.Ltext0
.uleb128 .LBE15-.Ltext0
.byte 0x4
.uleb128 .LBB16-.Ltext0
.uleb128 .LBE16-.Ltext0
.byte 0
.Ldebug_ranges3:
.section .debug_line,"",@progbits
.Ldebug_line0:
.section .debug_str,"MS",@progbits,1
.LASF4:
.string "func"
.LASF3:
.string "main"
.LASF2:
.string "GNU C17 12.2.0 -mtune=generic -march=x86-64 -g -gno-as-locview-support -O1 -fPIE"
.section .debug_line_str,"MS",@progbits,1
.LASF1:
.string ""
.LASF0:
.string "r.c"
.ident "GCC: (GNU) 12.2.0"
.section .note.GNU-stack,"",@progbits