This is an archive of the discontinued LLVM Phabricator instance.

[memprof] Print out profile build ids in the error message.
ClosedPublic

Authored by snehasish on Apr 13 2023, 10:15 PM.

Details

Summary

When no --profiled-binary flag is provided we can print out the build
ids of the modules in the profile. This can help the user fetch the
correct binary from e.g. remote object store.

Diff Detail

Event Timeline

snehasish created this revision.Apr 13 2023, 10:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 13 2023, 10:16 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
snehasish requested review of this revision.Apr 13 2023, 10:16 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 13 2023, 10:16 PM

Remove unused include.

tejohnson added inline comments.Apr 14 2023, 5:22 AM
llvm/lib/ProfileData/RawMemProfReader.cpp
204

Should we pass the Path instead of "" for the report() Context parameter? If not useful, probably document the constant "" parameter.

542

Why do we need them in the order they were stored?

554

formatting seems off - run through clang format.

snehasish updated this revision to Diff 513736.Apr 14 2023, 1:49 PM

Address comments.

snehasish marked an inline comment as done.Apr 14 2023, 2:01 PM

Thanks for the quick review!

llvm/lib/ProfileData/RawMemProfReader.cpp
204

Opted to document the param since I don't think printing the profile path is useful (the binary path is unset).

542

If we keep them in order then the profiled binary build id *should* be the first entry in this list. We rely on the sanitizer tooling (which calls dl_iterate_phdr [1]) to maintain the order which is read from the program headers and we preserve the order in the profile. Added a short comment.

[1] https://github.com/llvm/llvm-project/blob/main/compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp#L737

"The first object visited by callback is the main program."
https://man7.org/linux/man-pages/man3/dl_iterate_phdr.3.html

tejohnson accepted this revision.Apr 17 2023, 7:55 AM

lgtm

llvm/lib/ProfileData/RawMemProfReader.cpp
542

Thanks for the clarification. I would just add the man page link and excerpt to the comment to document why it should be first.

This revision is now accepted and ready to land.Apr 17 2023, 7:55 AM

Rebase, address comment.

snehasish marked an inline comment as done.Apr 17 2023, 10:53 AM
This revision was landed with ongoing or failed builds.Apr 17 2023, 10:54 AM
This revision was automatically updated to reflect the committed changes.