-
Notifications
You must be signed in to change notification settings - Fork 15.7k
[llvm-profgen] Loading binary functions from .symtab when DWARF info is incomplete #163654
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.
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
Changes from 3 commits
f34ab2d
a1f32b5
0fd352d
c097d37
a19064d
e12e694
5eead6b
8f59bfa
a967994
0dc2c66
75dc996
5600e83
0df504e
644fce9
9188eff
b6ae0ba
b1cbdd0
8927a27
108bc08
6e3a5ac
3d8ad53
1344cdd
2978e21
eaab9df
c53e0ae
b707e80
57ab1cd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -449,29 +449,61 @@ bool ProfileGeneratorBase::collectFunctionsFromRawProfile( | |||||||||||
| // Go through all the stacks, ranges and branches in sample counters, use | ||||||||||||
| // the start of the range to look up the function it belongs and record the | ||||||||||||
| // function. | ||||||||||||
| uint64_t ErrStkAddr = 0, ErrFuncRange = 0, ErrSrc = 0, ErrTgt = 0; | ||||||||||||
| uint64_t TotalStkAddr = 0, TotalFuncRange = 0, TotalSrc = 0, TotalTgt = 0; | ||||||||||||
| for (const auto &CI : *SampleCounters) { | ||||||||||||
| if (const auto *CtxKey = dyn_cast<AddrBasedCtxKey>(CI.first.getPtr())) { | ||||||||||||
| for (auto StackAddr : CtxKey->Context) { | ||||||||||||
| uint64_t inc = Binary->addressIsCode(StackAddr) ? 1 : 0; | ||||||||||||
|
||||||||||||
| TotalStkAddr += inc; | ||||||||||||
| if (FuncRange *FRange = Binary->findFuncRange(StackAddr)) | ||||||||||||
| ProfiledFunctions.insert(FRange->Func); | ||||||||||||
| else | ||||||||||||
| ErrStkAddr += inc; | ||||||||||||
|
||||||||||||
| } | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| for (auto Item : CI.second.RangeCounter) { | ||||||||||||
| uint64_t StartAddress = Item.first.first; | ||||||||||||
| uint64_t inc = Binary->addressIsCode(StartAddress) ? 1 : 0; | ||||||||||||
| TotalFuncRange += inc; | ||||||||||||
| if (FuncRange *FRange = Binary->findFuncRange(StartAddress)) | ||||||||||||
| ProfiledFunctions.insert(FRange->Func); | ||||||||||||
| else | ||||||||||||
| ErrFuncRange += inc; | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| for (auto Item : CI.second.BranchCounter) { | ||||||||||||
| uint64_t SourceAddress = Item.first.first; | ||||||||||||
| uint64_t TargetAddress = Item.first.second; | ||||||||||||
| uint64_t srcinc = Binary->addressIsCode(SourceAddress) ? 1 : 0; | ||||||||||||
| uint64_t tgtinc = Binary->addressIsCode(TargetAddress) ? 1 : 0; | ||||||||||||
| TotalSrc += srcinc; | ||||||||||||
| if (FuncRange *FRange = Binary->findFuncRange(SourceAddress)) | ||||||||||||
| ProfiledFunctions.insert(FRange->Func); | ||||||||||||
| else | ||||||||||||
| ErrSrc += srcinc; | ||||||||||||
| TotalTgt += tgtinc; | ||||||||||||
| if (FuncRange *FRange = Binary->findFuncRange(TargetAddress)) | ||||||||||||
| ProfiledFunctions.insert(FRange->Func); | ||||||||||||
| else | ||||||||||||
| ErrTgt += tgtinc; | ||||||||||||
| } | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| if (ErrStkAddr) | ||||||||||||
|
||||||||||||
| WithColor::warning() << "Cannot find Stack Address from DWARF Info: " | ||||||||||||
|
||||||||||||
| if (Binary->addressIsCode(StartAddress) && | |
| Binary->addressIsCode(EndAddress) && | |
| isValidFallThroughRange(StartAddress, EndAddress, Binary)) | |
| Counter.recordRangeCount(StartAddress, EndAddress, Repeat); | |
| EndAddress = SourceAddress; |
warning: 0.07%(3501587/4906133148) of samples are from ranges that do not belong to any functions.
warning: 0.07%(3500591/4835840652) of function range samples do not belong to any function
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -257,6 +257,8 @@ void ProfiledBinary::load() { | |||
| if (ShowDisassemblyOnly) | ||||
| decodePseudoProbe(Obj); | ||||
|
|
||||
| populateSymbolsFromElf(Obj); | ||||
|
|
||||
| // Disassemble the text sections. | ||||
| disassemble(Obj); | ||||
|
|
||||
|
|
@@ -820,6 +822,48 @@ void ProfiledBinary::populateSymbolAddressList(const ObjectFile *Obj) { | |||
| } | ||||
| } | ||||
|
|
||||
| void ProfiledBinary::populateSymbolsFromElf(const ObjectFile *Obj) { | ||||
|
||||
| // Load binary functions from ELF symbol table when DWARF info is incomplete | ||||
| StringRef FileName = Obj->getFileName(); | ||||
| for (const ELFSymbolRef Symbol : Obj->symbols()) { | ||||
| const SymbolRef::Type Type = unwrapOrError(Symbol.getType(), FileName); | ||||
| const uint64_t Addr = unwrapOrError(Symbol.getAddress(), FileName); | ||||
|
||||
| const StringRef Name = unwrapOrError(Symbol.getName(), FileName); | ||||
| const uint64_t Size = Symbol.getSize(); | ||||
|
|
||||
| if (Size == 0 || Type != SymbolRef::ST_Function) | ||||
| continue; | ||||
|
|
||||
| SmallVector<StringRef> Suffixes( | ||||
|
||||
| {".destroy", ".resume", ".llvm.", ".cold", ".warm"}); | ||||
| const StringRef SymName = | ||||
| FunctionSamples::getCanonicalFnName(Name, Suffixes); | ||||
|
|
||||
| auto Ret = BinaryFunctions.emplace(SymName, BinaryFunction()); | ||||
| auto &Func = Ret.first->second; | ||||
| if (Ret.second) | ||||
| Func.FuncName = Ret.first->first; | ||||
|
|
||||
| if (auto Range = findFuncRange(Addr)) { | ||||
| if (Ret.second && ShowDetailedWarning) | ||||
| WithColor::warning() | ||||
|
||||
| auto Ret = BinaryFunctions.emplace(SymName, BinaryFunction()); |
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Symbol table finding entry already existing in dwarf should not be a warning.
When that happens, we should check if they both point to same ranges, and only warn when
- symbol table and dwarf conflicts for the actual range.
- something is in symbol table, but not in dwarf (suggesting dwarf corrupted, e.g. due to relo overflow)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated this part and the warning shall now capture case 1. And I've merged (2) into the PerfScriptReader::warnInvalidRange :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Can you test on a big binary to make sure this warning does not fire?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually didn't see this warning firing at all in my test, but it shall capture the naming conflicts between dwarf and symbol table, like... in case I missed to strip some internal suffixes...
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should only be storing additional range, not "updating" existing range, right?
Also suggest to structure the code like below for clarify:
auto &Func = Ret.first->second;
if (Ret.second) {
// Function from symbol table not found previously in DWARF, store ranges.
Func.FuncName = Ret.first->first;
Func.FromSymtab = true;
HashBinaryFunctions[MD5Hash(StringRef(SymName))] = &Func;
...
}
else {
// Function already found from DWARF, check consistency between symbol table and DWARF.
...
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, right.... "updating" is actually not a correct behavior. let me flag that in a warning too :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually... we probably still need to "update" the range.. because like, coroutine creates a few functions: "foo", "foo.destroy", "foo.resume", "foo.cleanup", and each of them has a different address range in the symbol table. Even Ret.second == false could still be some function added from the symbol table...
Maybe the StartAddr works better to check if a function is already found in DWARF or not? I'm changing the logic to something like this:
auto Range = findFuncRange(StartAddr);
if (!Range || Range->StartAddress != StartAddr) {
// Function from symbol table not found previously in DWARF, store ranges.
...
} else if (SymName != Range->getFuncName() && ShowDetailedWarning) {
// Function already found from DWARF, check consistency between symbol table and DWARF.
WithColor::warning()...
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is widely used and maybe be expensive. Now with this changing to use
vector, I suspect it would introduce more overheads(alloc, dealloc ..), if so, would it possible to continue using the static array or constexpr?