Skip to content

[SampleProfile] Fix bug where remapper returns empty string and crashing Sample Profile loader #71479

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 2 commits into from
Nov 10, 2023
Merged

[SampleProfile] Fix bug where remapper returns empty string and crashing Sample Profile loader #71479

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
453e690
[SampleProfile] Fix bug where intentionally constructed function with
huangjd Nov 7, 2023
7ea2ed8
Added test case
huangjd Nov 8, 2023
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
7 changes: 5 additions & 2 deletions llvm/lib/ProfileData/SampleProfReader.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1788,8 +1788,11 @@ void SampleProfileReaderItaniumRemapper::applyRemapping(LLVMContext &Ctx) {

std::optional<StringRef>
SampleProfileReaderItaniumRemapper::lookUpNameInProfile(StringRef Fname) {
if (auto Key = Remappings->lookup(Fname))
return NameMap.lookup(Key);
if (auto Key = Remappings->lookup(Fname)) {
StringRef Result = NameMap.lookup(Key);
if (!Result.empty())
return Result;
}
return std::nullopt;
}

Expand Down
6 changes: 3 additions & 3 deletions llvm/lib/Transforms/IPO/SampleProfile.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1630,7 +1630,7 @@ void SampleProfileLoader::promoteMergeNotInlinedContextSamples(
// separate map so that it does not rehash the original profile.
if (!OutlineFS)
OutlineFS = &OutlineFunctionSamples[
FunctionSamples::getCanonicalFnName(Callee->getName())];
FunctionId(FunctionSamples::getCanonicalFnName(Callee->getName()))];
Copy link
Contributor

Choose a reason for hiding this comment

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

pretty much a drive-by comment

It seems to me SampleContext could take either a StringRef or a FunctionId in its constructor, and the former asserts names are not empty (

llvm-project/llvm/include/llvm/ProfileData/SampleProf.h

Line 529 in 5f29555

assert(!Name.empty() && "Name is empty");
) while the latter doesn't (FunctionId ctor doesn't require names are not empty either); and this PR wants to fix a crash for the former constructor.

If I understand correctly, is this FunctionId needed here and below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is intentionally using FunctionId constructor here to avoid that assert, and any other potential situation where a function with empty name is passed in. Since the front end is relatively independent, so we are not making any assumption there. Accepting an empty string in profile matching will at worst degrade the profile's accuracy but still result in a correct binary, which is something we want to ensure here: If both the source code and the sample profile file are valid, the compiler should not exit abnormally.

Also there's an upcoming patch which will make this moot (https://github.com/huangjd/llvm-project/tree/md5phase3) I am still validating it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should the StringRef based constructor in SampleConext be removed to avoid future confusion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That will be addressed in MD5phase3 (pending), in that patch FunctionId no longer requires explicit constructor since the difference between MD5 and String FunctionId will be minimized

OutlineFS->merge(*FS, 1);
// Set outlined profile to be synthetic to not bias the inliner.
OutlineFS->SetContextSynthetic();
Expand Down Expand Up @@ -2675,12 +2675,12 @@ bool SampleProfileLoader::runOnFunction(Function &F, ModuleAnalysisManager *AM)
// into base.
if (!Samples) {
StringRef CanonName = FunctionSamples::getCanonicalFnName(F);
auto It = OutlineFunctionSamples.find(CanonName);
auto It = OutlineFunctionSamples.find(FunctionId(CanonName));
if (It != OutlineFunctionSamples.end()) {
Samples = &It->second;
} else if (auto Remapper = Reader->getRemapper()) {
if (auto RemppedName = Remapper->lookUpNameInProfile(CanonName)) {
It = OutlineFunctionSamples.find(*RemppedName);
It = OutlineFunctionSamples.find(FunctionId(*RemppedName));
if (It != OutlineFunctionSamples.end())
Samples = &It->second;
}
Expand Down
8 changes: 8 additions & 0 deletions llvm/test/Transforms/SampleProfile/remap-unmatched.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
; This test should not crash.
; RUN: opt %s -passes=sample-profile -sample-profile-file=%S/Inputs/remap.prof -sample-profile-remapping-file=%S/Inputs/remap.map

define void @foo() #0 {
ret void
}

attributes #0 = { "use-sample-profile" }