Skip to content

[DWARFVerifier] Allow overlapping ranges for ICF-merged functions #117952

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Dec 17, 2024
Merged

[DWARFVerifier] Allow overlapping ranges for ICF-merged functions #117952

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
a5f73d2
[DWARFVerifier] Allow overlapping ranges for ICF-merged functions
Nov 28, 2024
a466e50
Fix Broken Test
Dec 3, 2024
f6daae5
Address Feedback Nr.1
Dec 4, 2024
0e873b7
Remove Unrelated Formatting
Dec 4, 2024
22f8f8a
Remove unecessary comparisson.
Dec 4, 2024
8fc6a58
Handle lexical blocks also
Dec 16, 2024
d55fd0d
temp last fix
Dec 16, 2024
156614b
Simplify code
Dec 16, 2024
a7f9e46
Fix associated test
Dec 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions llvm/include/llvm/DebugInfo/DWARF/DWARFVerifier.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,17 +77,22 @@ class DWARFVerifier {

/// Inserts the address range info. If any of its ranges overlaps with a
/// range in an existing range info, the range info is *not* added and an
/// iterator to the overlapping range info.
/// iterator to the overlapping range info. If AllowDuplicates is true and
/// RI is an already existing range, the duplicate range will not be added
/// but the returned iterator will point to end().
///
/// This is used for finding overlapping children of the same DIE.
die_range_info_iterator insert(const DieRangeInfo &RI);
die_range_info_iterator insert(const DieRangeInfo &RI,
bool AllowDuplicates = false);

/// Return true if ranges in this object contains all ranges within RHS.
bool contains(const DieRangeInfo &RHS) const;

/// Return true if any range in this object intersects with any range in
/// RHS.
bool intersects(const DieRangeInfo &RHS) const;
/// RHS. If AllowDuplicates is true, identical ranges are not considered to
/// be overlapping.
bool intersects(const DieRangeInfo &RHS,
bool AllowDuplicates = false) const;
};

private:
Expand Down
56 changes: 32 additions & 24 deletions llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,14 +70,15 @@ DWARFVerifier::DieRangeInfo::insert(const DWARFAddressRange &R) {
}

DWARFVerifier::DieRangeInfo::die_range_info_iterator
DWARFVerifier::DieRangeInfo::insert(const DieRangeInfo &RI) {
DWARFVerifier::DieRangeInfo::insert(const DieRangeInfo &RI,
bool AllowDuplicates) {
if (RI.Ranges.empty())
return Children.end();

auto End = Children.end();
auto Iter = Children.begin();
while (Iter != End) {
if (Iter->intersects(RI))
if (Iter->intersects(RI, AllowDuplicates))
return Iter;
++Iter;
}
Expand Down Expand Up @@ -109,12 +110,16 @@ bool DWARFVerifier::DieRangeInfo::contains(const DieRangeInfo &RHS) const {
return false;
}

bool DWARFVerifier::DieRangeInfo::intersects(const DieRangeInfo &RHS) const {
bool DWARFVerifier::DieRangeInfo::intersects(const DieRangeInfo &RHS,
bool AllowDuplicates) const {
auto I1 = Ranges.begin(), E1 = Ranges.end();
auto I2 = RHS.Ranges.begin(), E2 = RHS.Ranges.end();
while (I1 != E1 && I2 != E2) {
if (I1->intersects(*I2))
return true;
if (I1->intersects(*I2)) {
bool IsDuplicate = *I1 == *I2;
if (!AllowDuplicates || !IsDuplicate)
return true;
}
if (I1->LowPC < I2->LowPC)
++I1;
else
Expand Down Expand Up @@ -146,11 +151,13 @@ bool DWARFVerifier::verifyUnitHeader(const DWARFDataExtractor DebugInfoData,
if (Version >= 5) {
UnitType = DebugInfoData.getU8(Offset);
AddrSize = DebugInfoData.getU8(Offset);
AbbrOffset = isUnitDWARF64 ? DebugInfoData.getU64(Offset) : DebugInfoData.getU32(Offset);
AbbrOffset = isUnitDWARF64 ? DebugInfoData.getU64(Offset)
: DebugInfoData.getU32(Offset);
ValidType = dwarf::isUnitType(UnitType);
} else {
UnitType = 0;
AbbrOffset = isUnitDWARF64 ? DebugInfoData.getU64(Offset) : DebugInfoData.getU32(Offset);
AbbrOffset = isUnitDWARF64 ? DebugInfoData.getU64(Offset)
: DebugInfoData.getU32(Offset);
AddrSize = DebugInfoData.getU8(Offset);
}

Expand Down Expand Up @@ -412,7 +419,7 @@ unsigned DWARFVerifier::verifyUnits(const DWARFUnitVector &Units) {
unsigned Index = 1;
for (const auto &Unit : Units) {
OS << "Verifying unit: " << Index << " / " << Units.getNumUnits();
if (const char* Name = Unit->getUnitDIE(true).getShortName())
if (const char *Name = Unit->getUnitDIE(true).getShortName())
OS << ", \"" << Name << '\"';
OS << '\n';
OS.flush();
Expand Down Expand Up @@ -531,14 +538,12 @@ bool DWARFVerifier::handleDebugInfo() {
unsigned NumErrors = 0;

OS << "Verifying .debug_info Unit Header Chain...\n";
DObj.forEachInfoSections([&](const DWARFSection &S) {
NumErrors += verifyUnitSection(S);
});
DObj.forEachInfoSections(
[&](const DWARFSection &S) { NumErrors += verifyUnitSection(S); });

OS << "Verifying .debug_types Unit Header Chain...\n";
DObj.forEachTypesSections([&](const DWARFSection &S) {
NumErrors += verifyUnitSection(S);
});
DObj.forEachTypesSections(
[&](const DWARFSection &S) { NumErrors += verifyUnitSection(S); });

OS << "Verifying non-dwo Units...\n";
NumErrors += verifyUnits(DCtx.getNormalUnitsVector());
Expand Down Expand Up @@ -622,14 +627,17 @@ unsigned DWARFVerifier::verifyDieRanges(const DWARFDie &Die,
}

// Verify that children don't intersect.
const auto IntersectingChild = ParentRI.insert(RI);
bool AllowDuplicates = Die.getTag() == DW_TAG_subprogram;
Copy link
Collaborator

@dwblaikie dwblaikie Dec 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why restrict this to subprograms having duplicates? Presumably anything could have duplicates - if we can have subprograms with duplicates (is this duplicates within a subprogram, or between subprograms, or both?) then we can probably have lexical scopes with duplicates too? (& certainly CUs with duplicates - both within a CU and across multiple CUs)

Copy link
Contributor Author

@alx32 alx32 Dec 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wanting to restrict this change to scenarios that I am familiar with / I'm trying to address. I am not sure about the implications for allowing duplicates for anything else. But if is clear that the right choice is to allow full overlaps for all tags - then I can make that change.

Copy link
Collaborator

@dwblaikie dwblaikie Dec 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess a few questions:
Is this only allowing duplicates on DW_AT_ranges of DW_AT_subprograms? Or only allowing overlap /of/ subprograms /on/ CUs? I guess the latter - so that, sufficiently generalizes, handles the case of subprograms with ranges (basic block sections) that overlap with /other/ subprograms
But presumably that /doesn't/ cover the case where a subprogram's sections might overlap with itself, due to BB sections? (or perhaps it does, if there's no "self" special case - if you're allowing a subprogram to overlap with other subprograms, including itself)

But shouldn't this apply to CUs too? a CU ranges could overlap with another CU due to a subprogram in one CU overlapping with a subprogram in a different CU? (is that tested? does that somehow come out of the subprogram-only-allowed case here?)

But, equally, this should apply to lexical scopes too - should be able to make a scope in a function compiled with basic-block sections have self-duplicate ranges, or collide/duplicate with athore lexical scope's ranges in the same subprogram (by putting the same code in different places in a function, and wrapping it in lexical scopes)

Copy link
Contributor Author

@alx32 alx32 Dec 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this only allowing duplicates on DW_AT_ranges of DW_AT_subprograms

So this is only relating to DW_AT_subprogram's - allowing them to overlap like this:

  • Allows DW_AT_ranges's to overlap in between different DW_AT_subprogram's
  • Allows identical DW_AT_low_pc / DW_AT_high_pc in between DW_AT_subprogram's

doesn't cover the case where a subprogram's sections might overlap with itself

  • No, if multiple DW_AT_ranges's in a DW_AT_subprogram overlap - this will still raise an error. Should we cover this case also ?

But shouldn't this apply to CUs too?

  • llvm-dwarfdump --error-display=details --verify does currently not raise any errors for overlapping CU's.

But, equally, this should apply to lexical scopes too

  • Same as above, llvm-dwarfdump --error-display=details --verify does currently not raise any errors for overlapping lexical blocks.

I tested using this script which generates DWARF dumped here.
The output with this patch shows no errors.
The output without this patch shows the errors for overlapping DW_AT_subprogram's

Copy link
Collaborator

@dwblaikie dwblaikie Dec 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yeah, looks like we don't even try to apply any verification constraints to ranges between CUs shrug they should probably have the same constraints, though - that exact overlap of any range is acceptable, but partial overlap is not. But ah well, I'm not pushing for improving that.

Let's look at lexical scope overlap, then...

I think this example demonstrates almost all the overlapping cases (well, doesn't include distinct subprograms overlapping with each other, but otherwise I think it covers things):

int f1(bool a, bool b) {
  {
    int i = 3;
    {
      int j = 7;
      if (a)
        return 5;
    }
    {
      int j = 8;
      if (b)
        return 5;
    }
  }
  return 3;
}


int main() {
}

clang++-tot test.cpp -g -ffunction-sections -fbasic-block-sections=all -fuse-ld=gold -Wl,--icf=all

error: DIE has overlapping ranges in DW_AT_ranges attribute: [0x00000000000006a2, 0x00000000000006ae) and [0x00000000000006a2, 0x00000000000006ae)

0x0000000c:   DW_TAG_compile_unit [1] *
                DW_AT_producer [DW_FORM_strx1]  (indexed (00000000) string = "clang version 20.0.0git ([email protected]:llvm/llvm-project.git 8dd9f206b518a97132f3e2489ccc93704e638353)")
                DW_AT_language [DW_FORM_data2]  (DW_LANG_C_plus_plus_14)
                DW_AT_name [DW_FORM_strx1]      (indexed (00000001) string = "test.cpp")
                DW_AT_str_offsets_base [DW_FORM_sec_offset]     (0x00000008)
                DW_AT_stmt_list [DW_FORM_sec_offset]    (0x00000000)
                DW_AT_comp_dir [DW_FORM_strx1]  (indexed (00000002) string = "/usr/local/google/home/blaikie/dev/scratch")
                DW_AT_low_pc [DW_FORM_addr]     (0x0000000000000000)
                DW_AT_ranges [DW_FORM_rnglistx] (indexed (0x4) rangelist = 0x00000052
                   [0x0000000000000670, 0x00000000000006a2)
                   [0x00000000000006a2, 0x00000000000006ae)
                   [0x00000000000006ae, 0x00000000000006c4)
                   [0x00000000000006a2, 0x00000000000006ae)
                   [0x00000000000006c4, 0x00000000000006d0)
                   [0x00000000000006d0, 0x00000000000006d5)
                   [0x00000000000006e0, 0x00000000000006e8))
                DW_AT_addr_base [DW_FORM_sec_offset]    (0x00000008)
                DW_AT_rnglists_base [DW_FORM_sec_offset]        (0x0000000c)

error: DIE has overlapping ranges in DW_AT_ranges attribute: [0x00000000000006a2, 0x00000000000006ae) and [0x00000000000006a2, 0x00000000000006ae)

0x0000002b:   DW_TAG_subprogram [2] * (0x0000000c)
                DW_AT_ranges [DW_FORM_rnglistx] (indexed (0x0) rangelist = 0x00000020
                   [0x0000000000000670, 0x00000000000006a2)
                   [0x00000000000006a2, 0x00000000000006ae)
                   [0x00000000000006ae, 0x00000000000006c4)
                   [0x00000000000006a2, 0x00000000000006ae)
                   [0x00000000000006c4, 0x00000000000006d0)
                   [0x00000000000006d0, 0x00000000000006d5))
                DW_AT_frame_base [DW_FORM_exprloc]      (DW_OP_reg6)
                DW_AT_linkage_name [DW_FORM_strx1]      (indexed (00000003) string = "_Z2f1bb")
                DW_AT_name [DW_FORM_strx1]      (indexed (00000004) string = "f1")
                DW_AT_decl_file [DW_FORM_data1] ("/usr/local/google/home/blaikie/dev/scratch/test.cpp")
                DW_AT_decl_line [DW_FORM_data1] (1)
                DW_AT_type [DW_FORM_ref4]       (cu + 0x0087 => {0x00000087} "int")
                DW_AT_external [DW_FORM_flag_present]   (true)

error: DIE has overlapping ranges in DW_AT_ranges attribute: [0x00000000000006a2, 0x00000000000006ae) and [0x00000000000006a2, 0x00000000000006ae)

0x0000004d:   DW_TAG_lexical_block [4] * (0x0000002b)
                DW_AT_ranges [DW_FORM_rnglistx] (indexed (0x1) rangelist = 0x00000033
                   [0x0000000000000685, 0x00000000000006a2)
                   [0x00000000000006a2, 0x00000000000006ae)
                   [0x00000000000006ae, 0x00000000000006c4)
                   [0x00000000000006a2, 0x00000000000006ae))

error: DIEs have overlapping address ranges:
0x00000068: DW_TAG_lexical_block [4] * (0x0000004d)
              DW_AT_ranges [DW_FORM_rnglistx]   (indexed (0x3) rangelist = 0x0000004b
                 [0x00000000000006ae, 0x00000000000006c4)
                 [0x00000000000006a2, 0x00000000000006ae))

0x0000005a: DW_TAG_lexical_block [4] * (0x0000004d)
              DW_AT_ranges [DW_FORM_rnglistx]   (indexed (0x2) rangelist = 0x00000042
                 [0x000000000000068c, 0x00000000000006a2)
                 [0x00000000000006a2, 0x00000000000006ae))

Copy link
Contributor Author

@alx32 alx32 Dec 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the example - the latest version addresses this.

const auto IntersectingChild = ParentRI.insert(RI, AllowDuplicates);
if (IntersectingChild != ParentRI.Children.end()) {
++NumErrors;
ErrorCategory.Report("DIEs have overlapping address ranges", [&]() {
error() << "DIEs have overlapping address ranges:";
dump(Die);
dump(IntersectingChild->Die) << '\n';
});
if (IntersectingChild->Ranges != ParentRI.Children.end()->Ranges) {
++NumErrors;
ErrorCategory.Report("DIEs have overlapping address ranges", [&]() {
error() << "DIEs have overlapping address ranges:";
dump(Die);
dump(IntersectingChild->Die) << '\n';
});
}
}

// Verify that ranges are contained within their parent.
Expand Down Expand Up @@ -914,8 +922,7 @@ unsigned DWARFVerifier::verifyDebugInfoReferences(
return DWARFDie();
};
unsigned NumErrors = 0;
for (const std::pair<const uint64_t, std::set<uint64_t>> &Pair :
References) {
for (const std::pair<const uint64_t, std::set<uint64_t>> &Pair : References) {
if (GetDIEForOffset(Pair.first))
continue;
++NumErrors;
Expand Down Expand Up @@ -2117,7 +2124,8 @@ bool DWARFVerifier::verifyDebugStrOffsets(
});
Success = false;
}
for (uint64_t Index = 0; C && C.tell() + OffsetByteSize <= NextUnit; ++Index) {
for (uint64_t Index = 0; C && C.tell() + OffsetByteSize <= NextUnit;
++Index) {
uint64_t OffOff = C.tell();
uint64_t StrOff = DA.getAddress(C);
// check StrOff refers to the start of a string
Expand Down
131 changes: 131 additions & 0 deletions llvm/test/tools/llvm-dwarfdump/X86/verify_no_overlap_error_icf.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,131 @@
//--- comments.txt

# This test verifies several scenarios with DW_TAG_subprogram address ranges:
# 1. Two subprograms can have identical ranges (shown with foo2 and foo3 having same low_pc/high_pc)
# This is valid and can happen when ICF (Identical Code Folding) merges functions.
# 2. Two subprograms can have overlapping ranges when using DW_AT_ranges
# (shown with func1_with_ranges and func2_with_ranges sharing range 0x5000-0x6000)
# This is also valid and can occur with -fbasic-block-sections=all
# 3. The test also verifies that non-identical overlapping ranges are correctly flagged as errors:
# - When modifying just the first range's high offset from 0x6000 to 0x5999, it creates an invalid subrange overlap
# - When modifying just the first instance of DW_AT_high_pc 0x77 to 0x66, it creates an invalid function overlap
# The test ensures llvm-dwarfdump --verify correctly validates these cases by:
# a) Accepting valid identical overlapping ranges
# b) Rejecting invalid non-identical overlapping ranges

# Need to use split-file in order for `sed` calls below to work correctly
# RUN: split-file %s %t
# RUN: yaml2obj %t/test.yaml | llvm-dwarfdump --error-display=details --verify - | FileCheck %s
# CHECK: No errors.

# RUN: sed '0,/HighOffset: 0x6000/{s//HighOffset: 0x5999/}' %t/test.yaml | yaml2obj | not llvm-dwarfdump --error-display=details --verify - | FileCheck %s --check-prefix=CHECK-RANGES
# CHECK-RANGES: error: DIEs have overlapping address ranges

# RUN: sed '0,/Value: 0x77/{s/Value: 0x77/Value: 0x66/}' %t/test.yaml | yaml2obj | not llvm-dwarfdump --error-display=details --verify - | FileCheck %s --check-prefix=CHECK-HIGH-PC
# CHECK-HIGH-PC: error: DIEs have overlapping address ranges

//--- test.yaml
--- !ELF
FileHeader:
Class: ELFCLASS64
Data: ELFDATA2LSB
Type: ET_REL
Machine: EM_X86_64
DWARF:
debug_abbrev:
- Table:
- Tag: DW_TAG_compile_unit
Children: DW_CHILDREN_yes
Attributes:
- Attribute: DW_AT_producer
Form: DW_FORM_string
- Attribute: DW_AT_language
Form: DW_FORM_data2
- Attribute: DW_AT_name
Form: DW_FORM_string
- Attribute: DW_AT_low_pc
Form: DW_FORM_addr
- Attribute: DW_AT_high_pc
Form: DW_FORM_data8
- Tag: DW_TAG_subprogram
Children: DW_CHILDREN_no
Attributes:
- Attribute: DW_AT_name
Form: DW_FORM_string
- Attribute: DW_AT_low_pc
Form: DW_FORM_addr
- Attribute: DW_AT_high_pc
Form: DW_FORM_data8
- Tag: DW_TAG_subprogram
Children: DW_CHILDREN_no
Attributes:
- Attribute: DW_AT_name
Form: DW_FORM_string
- Attribute: DW_AT_ranges
Form: DW_FORM_sec_offset
- Tag: DW_TAG_base_type
Children: DW_CHILDREN_no
Attributes:
- Attribute: DW_AT_name
Form: DW_FORM_string
debug_ranges:
- Offset: 0x0
AddrSize: 0x8
Entries:
- LowOffset: 0x1000
HighOffset: 0x2000
- LowOffset: 0x3000
HighOffset: 0x4000
- LowOffset: 0x5000 # Overlaps with 2nd range below
HighOffset: 0x6000
- LowOffset: 0x0
HighOffset: 0x0
- Offset: 0x50
AddrSize: 0x8
Entries:
- LowOffset: 0x2500
HighOffset: 0x2800
- LowOffset: 0x5000 # Overlaps with 3rd range above
HighOffset: 0x6000
- LowOffset: 0x7000
HighOffset: 0x8000
- LowOffset: 0x0
HighOffset: 0x0
debug_info:
- Version: 4
Entries:
- AbbrCode: 1
Values:
- CStr: by_hand
- Value: 0x04
- CStr: CU1
- Value: 0x1000
- Value: 0x100
- AbbrCode: 4
Values:
- CStr: int
- AbbrCode: 2
Values:
- CStr: foo1
- Value: 0x1000
- Value: 0x10
- AbbrCode: 2
Values:
- CStr: foo2
- Value: 0x0 # Overlaps with 'foo3' below
- Value: 0x77
- AbbrCode: 2
Values:
- CStr: foo3
- Value: 0x0 # Overlaps with 'foo2' above
- Value: 0x77
- AbbrCode: 3
Values:
- CStr: func1_with_ranges
- Value: 0x0
- AbbrCode: 3
Values:
- CStr: func2_with_ranges
- Value: 0x50
- AbbrCode: 0
...
2 changes: 1 addition & 1 deletion llvm/test/tools/llvm-dwarfutil/ELF/X86/verify.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ DWARF:
Values:
- CStr: foo3
- Value: 0x0
- Value: 0x100
- Value: 0x80
- Value: 0x00000040
- AbbrCode: 0
...
Loading