Page MenuHomePhabricator

[llvm-profdata]Fix llvm-profdata crash on compact binary profile
ClosedPublic

Authored by wlei on Tue, Sep 15, 9:14 PM.

Details

Summary

llvm-profdata show and overlap will crash in getFuncName on compact binary profile. This change fixed this by switching to use getName.

getFuncName is misused in llvm-profdata. As showed below, GUIDToFuncNameMap is only supported in compilation mode, there is no initialization in llvm-profdata. Compact profile whose MD5 is true would try to query GUIDToFuncNameMap then caused the crash. So fix this by switching to getName

Diff Detail

Event Timeline

wlei created this revision.Tue, Sep 15, 9:14 PM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Sep 15, 9:14 PM
wlei requested review of this revision.Tue, Sep 15, 9:14 PM
wlei retitled this revision from fix llvm-profdata overlap crash to [llvm-profdata]Fix llvm-profdata crash on compact binary profile.Tue, Sep 15, 9:45 PM
wlei edited the summary of this revision. (Show Details)
wlei updated this revision to Diff 292104.Tue, Sep 15, 9:50 PM
wlei edited the summary of this revision. (Show Details)

fix lint

wlei updated this revision to Diff 292255.Wed, Sep 16, 9:31 AM

fix lint

hoy accepted this revision.Wed, Sep 16, 11:48 AM
hoy removed a reviewer: hoyFB.
hoy added a subscriber: hoy.

Thanks for the fix! LGTM.

I'm also seeing InstrProfSymtab is there to handle MD5-based names for PGO instrumentation. Not sure how it is used exactly. @wmi Have we thought about using that for AutoFDO?

This revision is now accepted and ready to land.Wed, Sep 16, 11:48 AM
MaskRay added inline comments.Wed, Sep 16, 12:16 PM
llvm/test/tools/llvm-profdata/compact-sample.proftext
3

| -> |

10

| -> |

(Nit: for FileCheck options, the two-dash --check-prefix is preferred.)

18

Indent this line so that the indentation difference with the next line matches the actual output.

wlei updated this revision to Diff 292314.Wed, Sep 16, 12:43 PM

fix lint in compact-sample.proftext

MaskRay accepted this revision.Wed, Sep 16, 1:42 PM

LGTM. Worth waiting a bit in case there are other opinions.

weihe accepted this revision.Wed, Sep 16, 1:54 PM

Thank you for fixing it!

wenlei accepted this revision.Wed, Sep 16, 5:02 PM

LGTM. A small nit: the code snippet in the description could be removed as it's indeed in the codebase already. :)

wmi added a comment.Wed, Sep 16, 5:36 PM
In D87740#2277446, @hoy wrote:

Thanks for the fix! LGTM.

I'm also seeing InstrProfSymtab is there to handle MD5-based names for PGO instrumentation. Not sure how it is used exactly. @wmi Have we thought about using that for AutoFDO?

Looks like there are some functionality overlap between InstrProfSymtab and AutoFDO MD5 lookup. It is worth noting that for Instrumentation PGO profile, function names present in the profile, and for compact format profile in AutoFDO, there are only MD5 and no names are present in the profile -- in order to keep the profile compact, so it is not that trivial to extract the common part but it is definitely possible.

wmi accepted this revision.Wed, Sep 16, 6:04 PM
wlei edited the summary of this revision. (Show Details)Wed, Sep 16, 6:48 PM
wlei added a comment.Wed, Sep 16, 6:57 PM

LGTM. A small nit: the code snippet in the description could be removed as it's indeed in the codebase already. :)

Yes, thanks for the suggestion

llvm/test/tools/llvm-profdata/compact-sample.proftext
3

Fixed, thanks for your feedback!

10

Fixed, thanks for your feedback!

18

Fixed, thanks for your feedback!

This revision was landed with ongoing or failed builds.Sun, Sep 20, 4:59 PM
This revision was automatically updated to reflect the committed changes.

Landed the change on behalf of @wlei.