This is an archive of the discontinued LLVM Phabricator instance.

[llvm-profgen] Fix a dangling vector reference in CS line number based generator
ClosedPublic

Authored by wlei on Sep 22 2021, 11:19 AM.

Details

Summary

It seems we missed one spot to persist SampleContextFrameVector into the global table (CSProfileGenerator::populateFunctionBoundarySamples:340) which causes a crash.

This change tried to fix it in a centralized way i. e. where we generate the FunctionSamples.

Diff Detail

Event Timeline

wlei created this revision.Sep 22 2021, 11:19 AM
wlei requested review of this revision.Sep 22 2021, 11:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 22 2021, 11:19 AM
wlei edited the summary of this revision. (Show Details)Sep 22 2021, 11:31 AM
wlei added reviewers: hoy, wenlei.
wenlei accepted this revision.Sep 22 2021, 11:42 AM

lgtm. so the problem was with profile generation without probe, right?

This revision is now accepted and ready to land.Sep 22 2021, 11:42 AM
hoy accepted this revision.Sep 22 2021, 11:52 AM

lgtm, thanks for the fix.

wlei added a comment.Sep 22 2021, 1:18 PM

lgtm. so the problem was with profile generation without probe, right?

Yeah, this is for CS line-number based generation which we didn't test for large service.

llvm/tools/llvm-profgen/ProfileGenerator.cpp
225

Side note:

This might be improved in the future, if it's a new context, it will always do a twice search(find and emplace), I guess context search is heavy.

wenlei added inline comments.Sep 22 2021, 1:50 PM
llvm/tools/llvm-profgen/ProfileGenerator.cpp
225

I think emplace alone is enough. The return value tells you whether the element exists, and insertion only happens when the element does not exist. Though we would need to create NewContext in Contexts first in order to combine find with emplace.

wlei updated this revision to Diff 374380.Sep 22 2021, 3:04 PM

using const SampleContextFrameVector &Context instead of ArrayRef to avoid obj copy

This revision was landed with ongoing or failed builds.Sep 22 2021, 6:34 PM
This revision was automatically updated to reflect the committed changes.