Skip to content

Commit 3fe50b6

Browse files
aaupovaaupov
authored
[BOLT] Store FileSymRefs in a multimap
With aggressive ICF, it's possible to have different local symbols (under different FILE symbols) to be mapped to the same address. FileSymRefs only keeps a single SymbolRef per address, which prevents fragment matching from finding the correct symbol to perform parent function lookup. Work around this issue by switching FileSymRefs to a multimap. In future, uses of FileSymRefs can be replaced with SortedSymbols which keeps essentially the same information. Test Plan: added ambiguous_fragment.test Reviewers: dcci, ayermolo, maksfb, rafaelauler Reviewed By: rafaelauler Pull Request: #98992
1 parent f0ac890 commit 3fe50b6

File tree

5 files changed

+104
-4
lines changed

5 files changed

+104
-4
lines changed

‎bolt/include/bolt/Rewrite/RewriteInstance.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -490,7 +490,7 @@ class RewriteInstance {
490490
std::unordered_map<const MCSymbol *, uint32_t> SymbolIndex;
491491

492492
/// Store all non-zero symbols in this map for a quick address lookup.
493-
std::map<uint64_t, llvm::object::SymbolRef> FileSymRefs;
493+
std::multimap<uint64_t, llvm::object::SymbolRef> FileSymRefs;
494494

495495
/// FILE symbols used for disambiguating split function parents.
496496
std::vector<ELFSymbolRef> FileSymbols;

‎bolt/lib/Rewrite/RewriteInstance.cpp‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -886,7 +886,7 @@ void RewriteInstance::discoverFileObjects() {
886886
if (SymName == "__hot_start" || SymName == "__hot_end")
887887
continue;
888888

889-
FileSymRefs[SymbolAddress] = Symbol;
889+
FileSymRefs.emplace(SymbolAddress, Symbol);
890890

891891
// Skip section symbols that will be registered by disassemblePLT().
892892
if (SymbolType == SymbolRef::ST_Debug) {
@@ -1052,7 +1052,9 @@ void RewriteInstance::discoverFileObjects() {
10521052

10531053
// Remove the symbol from FileSymRefs so that we can skip it from
10541054
// in the future.
1055-
auto SI = FileSymRefs.find(SymbolAddress);
1055+
auto SI = llvm::find_if(
1056+
llvm::make_range(FileSymRefs.equal_range(SymbolAddress)),
1057+
[&](auto SymIt) { return SymIt.second == Symbol; });
10561058
assert(SI != FileSymRefs.end() && "symbol expected to be present");
10571059
assert(SI->second == Symbol && "wrong symbol found");
10581060
FileSymRefs.erase(SI);
@@ -1260,6 +1262,7 @@ void RewriteInstance::discoverFileObjects() {
12601262

12611263
registerFragments();
12621264
FileSymbols.clear();
1265+
FileSymRefs.clear();
12631266

12641267
discoverBOLTReserved();
12651268
}
@@ -1433,7 +1436,11 @@ void RewriteInstance::registerFragments() {
14331436
const uint64_t Address = BF->getAddress();
14341437

14351438
// Get fragment's own symbol
1436-
const auto SymIt = FileSymRefs.find(Address);
1439+
const auto SymIt = llvm::find_if(
1440+
llvm::make_range(FileSymRefs.equal_range(Address)), [&](auto SI) {
1441+
StringRef Name = cantFail(SI.second.getName());
1442+
return Name.contains(ParentName);
1443+
});
14371444
if (SymIt == FileSymRefs.end()) {
14381445
BC->errs()
14391446
<< "BOLT-ERROR: symbol lookup failed for function at address 0x"

‎bolt/test/X86/Inputs/ambiguous_fragment.s‎

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
#--- file1
2+
.file "file1.cpp"
3+
.section .text.cold
4+
.type __func.cold.0, @function
5+
__func.cold.0:
6+
ud2
7+
.size __func.cold.0, .-__func.cold.0
8+
.section .text
9+
.type __func, @function
10+
__func:
11+
ud2
12+
.size __func, .-__func
13+
14+
#--- file2
15+
.file "file2.cpp"
16+
.section .text.cold
17+
.type __func.cold.0, @function
18+
__func.cold.0:
19+
ud2
20+
.size __func.cold.0, .-__func.cold.0
21+
.section .text
22+
.type __func, @function
23+
__func:
24+
ud2
25+
.size __func, .-__func
26+
27+
#--- file3
28+
.file "file3.cpp"
29+
.section .text.cold
30+
.type __func.cold.0, @function
31+
__func.cold.0:
32+
ud2
33+
.size __func.cold.0, .-__func.cold.0
34+
.section .text
35+
.type __func, @function
36+
__func:
37+
ud2
38+
.size __func, .-__func
39+
40+
#--- file4
41+
.file "file4.cpp"
42+
.section .text.cold
43+
.type __func.cold.0, @function
44+
__func.cold.0:
45+
ud2
46+
.size __func.cold.0, .-__func.cold.0
47+
.section .text
48+
.type __func, @function
49+
__func:
50+
ud2
51+
.size __func, .-__func
52+
53+
#--- file5
54+
.file "bolt-pseudo.o"

‎bolt/test/X86/Inputs/ambiguous_fragment.script‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
SECTIONS {
2+
. = 0x10000;
3+
.text : { *(.text) }
4+
. = 0x20000;
5+
.text.cold : { *(.text.cold) }
6+
}

‎bolt/test/X86/ambiguous_fragment.test‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
## This reproduces a bug with misidentification of a parent fragment.
2+
3+
RUN: split-file %p/Inputs/ambiguous_fragment.s %t
4+
5+
RUN: llvm-mc --filetype=obj --triple x86_64-unknown-unknown %t/file1 -o %t1.o
6+
RUN: llvm-mc --filetype=obj --triple x86_64-unknown-unknown %t/file2 -o %t2.o
7+
RUN: llvm-mc --filetype=obj --triple x86_64-unknown-unknown %t/file3 -o %t3.o
8+
RUN: llvm-mc --filetype=obj --triple x86_64-unknown-unknown %t/file4 -o %t4.o
9+
RUN: llvm-mc --filetype=obj --triple x86_64-unknown-unknown %t/file5 -o %t5.o
10+
11+
RUN: ld.lld %t1.o %t2.o %t3.o %t4.o %t5.o -o %t.exe \
12+
RUN: --script %p/Inputs/ambiguous_fragment.script
13+
14+
RUN: llvm-objcopy %t.exe %t.exe2 \
15+
RUN: --add-symbol=_Zfunc.cold.0=.text.cold:0x4,local,function \
16+
RUN: --add-symbol=_Zfunc=.text:0xc,function
17+
18+
RUN: llvm-objdump --syms %t.exe2 | FileCheck %s --check-prefix=CHECK-SYMS
19+
20+
RUN: link_fdata %s %t.exe2 %t.preagg PREAGG
21+
RUN: perf2bolt -v=1 %t.exe2 -p %t.preagg --pa -o %t.fdata -w %t.yaml | FileCheck %s
22+
23+
# PREAGG: B X:0 #__func# 1 0
24+
25+
CHECK-SYMS: 0000000000020004 {{.*}} __func.cold.0
26+
CHECK-SYMS: 0000000000020004 {{.*}} _Zfunc.cold.0
27+
28+
CHECK-NOT: BOLT-ERROR: parent function not found for __func.cold.0
29+
CHECK: BOLT-INFO: marking __func.cold.0/3(*4) as a fragment of __func/4(*3)
30+
CHECK-NEXT: BOLT-INFO: marking __func.cold.0/1(*2) as a fragment of __func/1(*2)
31+
CHECK-NEXT: BOLT-INFO: marking __func.cold.0/2(*2) as a fragment of __func/2(*2)
32+
CHECK-NEXT: BOLT-INFO: marking __func.cold.0/3(*4) as a fragment of __func/3(*2)
33+
CHECK-NEXT: BOLT-INFO: marking __func.cold.0/4(*2) as a fragment of __func/4(*3)

0 commit comments

Comments
 (0)