diff --git a/bolt/include/bolt/Core/BinaryContext.h b/bolt/include/bolt/Core/BinaryContext.h --- a/bolt/include/bolt/Core/BinaryContext.h +++ b/bolt/include/bolt/Core/BinaryContext.h @@ -601,6 +601,9 @@ /// Indicates if the binary is stripped bool IsStripped{false}; + /// Indicates if the binary contains split functions. + bool HasSplitFunctions{false}; + /// Is the binary always loaded at a fixed address. Shared objects and /// position-independent executables (PIEs) are examples of binaries that /// will have HasFixedLoadAddress set to false. diff --git a/bolt/include/bolt/Rewrite/RewriteInstance.h b/bolt/include/bolt/Rewrite/RewriteInstance.h --- a/bolt/include/bolt/Rewrite/RewriteInstance.h +++ b/bolt/include/bolt/Rewrite/RewriteInstance.h @@ -95,6 +95,9 @@ /// from meta data in the file. void discoverFileObjects(); + /// Process fragments, locate parent functions. + void registerFragments(); + /// Read info from special sections. E.g. eh_frame and .gcc_except_table /// for exception and stack unwinding information. Error readSpecialSections(); diff --git a/bolt/lib/Core/BinaryContext.cpp b/bolt/lib/Core/BinaryContext.cpp --- a/bolt/lib/Core/BinaryContext.cpp +++ b/bolt/lib/Core/BinaryContext.cpp @@ -478,18 +478,6 @@ return MemoryContentsType::UNKNOWN; } -/// Check if == .cold(.\d+)? -bool isPotentialFragmentByName(BinaryFunction &Fragment, - BinaryFunction &Parent) { - for (StringRef Name : Parent.getNames()) { - std::string NamePrefix = Regex::escape(NameResolver::restore(Name)); - std::string NameRegex = Twine(NamePrefix, "\\.cold(\\.[0-9]+)?").str(); - if (Fragment.hasRestoredNameRegex(NameRegex)) - return true; - } - return false; -} - bool BinaryContext::analyzeJumpTable( const uint64_t Address, const JumpTable::JumpTableType Type, BinaryFunction &BF, const uint64_t NextJTAddress, @@ -512,14 +500,9 @@ // Nothing to do if we failed to identify the containing function. if (!TargetBF) return false; - // Case 1: check if BF is a fragment and TargetBF is its parent. - if (BF.isFragment()) { - // Parent function may or may not be already registered. - // Set parent link based on function name matching heuristic. - return registerFragment(BF, *TargetBF); - } - // Case 2: check if TargetBF is a fragment and BF is its parent. - return TargetBF->isFragment() && registerFragment(*TargetBF, BF); + // Check if BF is a fragment of TargetBF or vice versa. + return (BF.isFragment() && BF.isParentFragment(TargetBF)) || + (TargetBF->isFragment() && TargetBF->isParentFragment(&BF)); }; ErrorOr Section = getSectionForAddress(Address); @@ -1115,8 +1098,6 @@ bool BinaryContext::registerFragment(BinaryFunction &TargetFunction, BinaryFunction &Function) const { - if (!isPotentialFragmentByName(TargetFunction, Function)) - return false; assert(TargetFunction.isFragment() && "TargetFunction must be a fragment"); if (TargetFunction.isParentFragment(&Function)) return true; @@ -1241,7 +1222,7 @@ if (TargetFunction) { if (TargetFunction->isFragment() && - !registerFragment(*TargetFunction, Function)) { + !TargetFunction->isParentFragment(&Function)) { errs() << "BOLT-WARNING: interprocedural reference between unrelated " "fragments: " << Function.getPrintName() << " and " diff --git a/bolt/lib/Rewrite/RewriteInstance.cpp b/bolt/lib/Rewrite/RewriteInstance.cpp --- a/bolt/lib/Rewrite/RewriteInstance.cpp +++ b/bolt/lib/Rewrite/RewriteInstance.cpp @@ -52,6 +52,7 @@ #include "llvm/Support/FileSystem.h" #include "llvm/Support/LEB128.h" #include "llvm/Support/ManagedStatic.h" +#include "llvm/Support/Regex.h" #include "llvm/Support/Timer.h" #include "llvm/Support/ToolOutputFile.h" #include "llvm/Support/raw_ostream.h" @@ -733,6 +734,8 @@ adjustCommandLineOptions(); discoverFileObjects(); + registerFragments(); + preprocessProfileData(); // Skip disassembling if we have a translation table and we are running an @@ -925,6 +928,9 @@ BinaryFunction *PreviousFunction = nullptr; unsigned AnonymousId = 0; + // Regex object for matching cold fragments. + Regex ColdFragment(".*\\.cold(\\.[0-9]+)?"); + const auto SortedSymbolsEnd = LastSymbol == SortedFileSymbols.end() ? LastSymbol : std::next(LastSymbol); @@ -1183,6 +1189,21 @@ if (!IsSimple) BF->setSimple(false); } + + // Check if it's a cold function fragment. + if (ColdFragment.match(SymName)) { + static bool PrintedWarning = false; + if (!PrintedWarning) { + PrintedWarning = true; + errs() << "BOLT-WARNING: split function detected on input : " + << SymName; + if (BC->HasRelocations) + errs() << ". The support is limited in relocation mode\n"; + } + BC->HasSplitFunctions = true; + BF->IsFragment = true; + } + if (!AlternativeName.empty()) BF->addAlternativeName(AlternativeName); @@ -1265,6 +1286,44 @@ } } +void RewriteInstance::registerFragments() { + if (!BC->HasSplitFunctions) + return; + + for (auto &BFI : BC->getBinaryFunctions()) { + BinaryFunction &Function = BFI.second; + if (!Function.isFragment()) + continue; + for (StringRef Name : Function.getNames()) { + size_t ColdSuffixPos = Name.find(".cold"); + if (ColdSuffixPos == StringRef::npos) + continue; + StringRef ParentName = Name.substr(0, ColdSuffixPos); + const BinaryData *BD = BC->getBinaryDataByName(ParentName); + if (!BD) { + // Parent might be a local symbol which is registered under a uniquified + // name ("foo/1"); + StringRef LHS, RHS; + std::tie(LHS, RHS) = Name.split("/"); + BD = BC->getBinaryDataByName((ParentName + Twine("/") + RHS).str()); + (void)LHS; + } + if (!BD) { + errs() << "BOLT-ERROR: parent function not found for " << Name << "\n"; + exit(1); + } + const uint64_t Address = BD->getAddress(); + BinaryFunction *BF = BC->getBinaryFunctionAtAddress(Address); + if (!BF) { + errs() << "BOLT-ERROR: parent function not found at " + << formatv("{0:x}\n", Address); + exit(1); + } + BC->registerFragment(Function, *BF); + } + } +} + void RewriteInstance::createPLTBinaryFunction(uint64_t TargetAddress, uint64_t EntryAddress, uint64_t EntrySize) { @@ -1453,26 +1512,6 @@ if (std::next(BFI) != BFE) NextFunction = &std::next(BFI)->second; - // Check if it's a fragment of a function. - std::optional FragName = - Function.hasRestoredNameRegex(".*\\.cold(\\.[0-9]+)?"); - if (FragName) { - static bool PrintedWarning = false; - if (!PrintedWarning) { - PrintedWarning = true; - errs() << "BOLT-WARNING: split function detected on input : " - << *FragName; - if (BC->HasRelocations) - errs() << ". The support is limited in relocation mode"; - if (opts::Lite) { - opts::Lite = false; - errs() << "\nBOLT-WARNING: disabling lite mode (-lite) when split " - << "functions are present\n"; - } - } - Function.IsFragment = true; - } - // Check if there's a symbol or a function with a larger address in the // same section. If there is - it determines the maximum size for the // current function. Otherwise, it is the size of a containing section @@ -2765,8 +2804,18 @@ } uint64_t NumFunctionsToProcess = 0; - auto shouldProcess = [&](const BinaryFunction &Function) { + auto mustSkip = [&](const BinaryFunction &Function) { if (opts::MaxFunctions && NumFunctionsToProcess > opts::MaxFunctions) + return true; + for (std::string &Name : opts::SkipFunctionNames) + if (Function.hasNameRegex(Name)) + return true; + + return false; + }; + + auto shouldProcess = [&](const BinaryFunction &Function) { + if (mustSkip(Function)) return false; // If the list is not empty, only process functions from the list. @@ -2784,10 +2833,6 @@ return Match.has_value(); } - for (std::string &Name : opts::SkipFunctionNames) - if (Function.hasNameRegex(Name)) - return false; - if (opts::Lite) { // Forcibly include functions specified in the -function-order file. if (opts::ReorderFunctions == ReorderFunctions::RT_USER) { @@ -2819,9 +2864,15 @@ continue; } + // Decide what to do with fragments after parent functions are processed. + if (Function.isFragment()) + continue; + if (!shouldProcess(Function)) { - LLVM_DEBUG(dbgs() << "BOLT-INFO: skipping processing of function " - << Function << " per user request\n"); + if (opts::Verbosity >= 1) { + outs() << "BOLT-INFO: skipping processing " << Function + << " per user request\n"; + } Function.setIgnored(); } else { ++NumFunctionsToProcess; @@ -2829,6 +2880,52 @@ outs() << "BOLT-INFO: processing ending on " << Function << '\n'; } } + + if (!BC->HasSplitFunctions) + return; + + // Fragment overrides: + // - If the fragment must be skipped, then the parent must be skipped as well. + // Otherwise, fragment should follow the parent function: + // - if the parent is skipped, skip fragment, + // - if the parent is processed, process the fragment(s) as well. + for (auto &BFI : BC->getBinaryFunctions()) { + BinaryFunction &Function = BFI.second; + if (!Function.isFragment()) + continue; + if (mustSkip(Function)) { + for (BinaryFunction *Parent : Function.ParentFragments) { + if (opts::Verbosity >= 1) { + outs() << "BOLT-INFO: skipping processing " << *Parent + << " together with fragment function\n"; + } + Parent->setIgnored(); + --NumFunctionsToProcess; + } + Function.setIgnored(); + continue; + } + + bool IgnoredParent = + llvm::any_of(Function.ParentFragments, [&](BinaryFunction *Parent) { + return Parent->isIgnored(); + }); + if (IgnoredParent) { + if (opts::Verbosity >= 1) { + outs() << "BOLT-INFO: skipping processing " << Function + << " together with parent function\n"; + } + Function.setIgnored(); + } else { + ++NumFunctionsToProcess; + if (opts::Verbosity >= 1) { + outs() << "BOLT-INFO: processing " << Function + << " as a sibling of non-ignored function\n"; + } + if (opts::MaxFunctions && NumFunctionsToProcess == opts::MaxFunctions) + outs() << "BOLT-INFO: processing ending on " << Function << '\n'; + } + } } void RewriteInstance::readDebugInfo() { diff --git a/bolt/test/X86/fragment-lite-reverse.s b/bolt/test/X86/fragment-lite-reverse.s new file mode 100644 --- /dev/null +++ b/bolt/test/X86/fragment-lite-reverse.s @@ -0,0 +1,81 @@ +# Check that BOLT in lite mode processes fragments as expected. + +# RUN: llvm-mc -filetype=obj -triple x86_64-unknown-unknown %s -o %t.o +# RUN: link_fdata %s %t.o %t.fdata +# RUN: %clang %cflags %t.o -o %t.exe -Wl,-q +# RUN: llvm-bolt %t.exe -o %t.out --lite=1 --data %t.fdata -v=1 2>&1 \ +# RUN: | FileCheck %s + +# CHECK: BOLT-INFO: skipping processing main.cold.1 together with parent function +# CHECK: BOLT-INFO: skipping processing foo.cold.1/1 together with parent function +# CHECK: BOLT-INFO: skipping processing bar.cold.1/1 together with parent function + + .globl main + .type main, %function +main: + .cfi_startproc + cmpl $0x0, %eax + je main.cold.1 + retq + .cfi_endproc +.size main, .-main + + .globl foo + .type foo, %function +foo: + .cfi_startproc + cmpl $0x0, %eax + je foo.cold.1 + retq + .cfi_endproc +.size foo, .-foo + + .local bar + .type bar, %function +bar: + .cfi_startproc + cmpl $0x0, %eax + je bar.cold.1 + retq + .cfi_endproc +.size bar, .-bar + + .section .text.cold + .globl main.cold.1 + .type main.cold.1, %function +main.cold.1: +# FDATA: 0 [unknown] 0 1 main.cold.1 0 1 0 + .cfi_startproc + pushq %rbp + movq %rsp, %rbp + movl $0x0, %eax + popq %rbp + retq + .cfi_endproc +.size main.cold.1, .-main.cold.1 + + .local foo.cold.1 + .type foo.cold.1, %function +foo.cold.1: +# FDATA: 0 [unknown] 0 1 foo.cold.1/1 0 1 0 + .cfi_startproc + pushq %rbp + movq %rsp, %rbp + movl $0x0, %eax + popq %rbp + retq + .cfi_endproc +.size foo.cold.1, .-foo.cold.1 + + .local bar.cold.1 + .type bar.cold.1, %function +bar.cold.1: +# FDATA: 0 [unknown] 0 1 bar.cold.1/1 0 1 0 + .cfi_startproc + pushq %rbp + movq %rsp, %rbp + movl $0x0, %eax + popq %rbp + retq + .cfi_endproc +.size bar.cold.1, .-bar.cold.1 diff --git a/bolt/test/X86/fragment-lite.s b/bolt/test/X86/fragment-lite.s new file mode 100644 --- /dev/null +++ b/bolt/test/X86/fragment-lite.s @@ -0,0 +1,81 @@ +# Check that BOLT in lite mode processes fragments as expected. + +# RUN: llvm-mc -filetype=obj -triple x86_64-unknown-unknown %s -o %t.o +# RUN: link_fdata %s %t.o %t.fdata +# RUN: %clang %cflags %t.o -o %t.exe -Wl,-q +# RUN: llvm-bolt %t.exe -o %t.out --lite=1 --data %t.fdata -v=1 2>&1 \ +# RUN: | FileCheck %s + +# CHECK: BOLT-INFO: processing main.cold.1 as a sibling of non-ignored function +# CHECK: BOLT-INFO: processing foo.cold.1/1 as a sibling of non-ignored function +# CHECK: BOLT-INFO: processing bar.cold.1/1 as a sibling of non-ignored function + + .globl main + .type main, %function +main: + .cfi_startproc +# FDATA: 0 [unknown] 0 1 main 0 1 0 + cmpl $0x0, %eax + je main.cold.1 + retq + .cfi_endproc +.size main, .-main + + .globl foo + .type foo, %function +foo: + .cfi_startproc +# FDATA: 0 [unknown] 0 1 foo 0 1 0 + cmpl $0x0, %eax + je foo.cold.1 + retq + .cfi_endproc +.size foo, .-foo + + .local bar + .type bar, %function +bar: + .cfi_startproc +# FDATA: 0 [unknown] 0 1 bar/1 0 1 0 + cmpl $0x0, %eax + je bar.cold.1 + retq + .cfi_endproc +.size bar, .-bar + + .section .text.cold + .globl main.cold.1 + .type main.cold.1, %function +main.cold.1: + .cfi_startproc + pushq %rbp + movq %rsp, %rbp + movl $0x0, %eax + popq %rbp + retq + .cfi_endproc +.size main.cold.1, .-main.cold.1 + + .local foo.cold.1 + .type foo.cold.1, %function +foo.cold.1: + .cfi_startproc + pushq %rbp + movq %rsp, %rbp + movl $0x0, %eax + popq %rbp + retq + .cfi_endproc +.size foo.cold.1, .-foo.cold.1 + + .local bar.cold.1 + .type bar.cold.1, %function +bar.cold.1: + .cfi_startproc + pushq %rbp + movq %rsp, %rbp + movl $0x0, %eax + popq %rbp + retq + .cfi_endproc +.size bar.cold.1, .-bar.cold.1 diff --git a/bolt/test/X86/split-func-icf.s b/bolt/test/X86/split-func-icf.s --- a/bolt/test/X86/split-func-icf.s +++ b/bolt/test/X86/split-func-icf.s @@ -11,8 +11,8 @@ # RUN: llvm-bolt %t.exe -o %t.out -v=1 --print-only=main2.cold.1 --print-disasm 2>&1 | FileCheck %s # CHECK-NOT: unclaimed PC-relative relocations left in data -# CHECK-DAG: BOLT-INFO: marking main2.cold.1(*2) as a fragment of main # CHECK-DAG: BOLT-INFO: marking main2.cold.1(*2) as a fragment of main2 +# CHECK-DAG: BOLT-INFO: marking main2.cold.1(*2) as a fragment of main # CHECK: Binary Function "main2.cold.1(*2)" after disassembly # CHECK: End of Function "main2.cold.1(*2)" # CHECK-DAG: BOLT-WARNING: Ignoring main2