This is an archive of the discontinued LLVM Phabricator instance.

[profile] Fix profile merging with binary IDs
ClosedPublic

Authored by phosek on Jul 30 2021, 2:43 AM.

Details

Summary

This fixes support for merging profiles which broke as a consequence
of e50a38840dc3db5813f74b1cd2e10e6d984d0e67. The issue was missing
adjustment in merge logic to account for the binary IDs which are
now included in the raw profile just after header.

In addition, this change also:

  • Includes the version in module signature that's used for merging

to avoid accidental attempts to merge incompatible profiles.

  • Moves the binary IDs size field after version field in the header

as was suggested in the review.

Diff Detail

Event Timeline

phosek created this revision.Jul 30 2021, 2:43 AM
phosek requested review of this revision.Jul 30 2021, 2:43 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJul 30 2021, 2:43 AM
Herald added subscribers: llvm-commits, Restricted Project. · View Herald Transcript
hans accepted this revision.Jul 30 2021, 6:01 AM

Thanks for fixing!

compiler-rt/include/profile/InstrProfData.inc
648

Does this need to be incremented for the header field move?

This revision is now accepted and ready to land.Jul 30 2021, 6:01 AM
phosek added inline comments.Jul 30 2021, 10:17 AM
compiler-rt/include/profile/InstrProfData.inc
648

I wasn't sure if it's necessary since few projects probably picked up this change so far given that we haven't heard any reports of this issue other than Chromium and the change only landed last week, but I suppose it wouldn't hurt to bump the version.

phosek updated this revision to Diff 363145.Jul 30 2021, 10:42 AM
phosek marked an inline comment as done.
phosek updated this revision to Diff 363150.Jul 30 2021, 10:50 AM
phosek retitled this revision from [profile] Fix merging with binary IDs to [profile] Fix profile merging with binary IDs.
phosek edited the summary of this revision. (Show Details)

@hans I've bumped the version and also included the version in the module signature as discussed on crbug.com/1233361.

hans added a comment.Jul 30 2021, 12:33 PM

@hans I've bumped the version and also included the version in the module signature as discussed on crbug.com/1233361.

Thanks! lgtm2

This revision was landed with ongoing or failed builds.Jul 30 2021, 1:41 PM
This revision was automatically updated to reflect the committed changes.
phosek updated this revision to Diff 363247.Jul 30 2021, 6:55 PM
hans added a comment.Jul 31 2021, 12:44 AM

If this turns out to be tricky to land, can we back out https://reviews.llvm.org/D102039 in the meantime?

If this turns out to be tricky to land, can we back out https://reviews.llvm.org/D102039 in the meantime?

LGTM (drop binary ID) for the 13.0.0 release.

If you revert D102039, please don't misfire on D104556 (which also needed to raw profile format version bump for a few llvm/test/tools/llvm-profdata/ tests) :)

83302c84890e5e6cb74c7d6c9f8eaaa56db0077c seems to be working, I haven't seen any bot failures. I'll ask for it get cherry-picked into the 13.0.0 release branch.

hans added a comment.Aug 2 2021, 6:44 AM

83302c84890e5e6cb74c7d6c9f8eaaa56db0077c seems to be working, I haven't seen any bot failures. I'll ask for it get cherry-picked into the 13.0.0 release branch.

Sounds great. Thanks for the investigation and fix!

Not that it matters anymore after D111123 but please note that trunk as of the landing of this (or even trunk before the landing of D111123) says that raw profiles emitted by clang 13.0.0 (which has a backport of this) are malformed.

phosek added a comment.Dec 3 2021, 7:17 PM

Not that it matters anymore after D111123 but please note that trunk as of the landing of this (or even trunk before the landing of D111123) says that raw profiles emitted by clang 13.0.0 (which has a backport of this) are malformed.

Can you clarify what exactly is the issue (if any)? What version of Clang are you using?

hotwinter added a subscriber: hotwinter.EditedFeb 11 2022, 8:06 AM

Not that it matters anymore after D111123 but please note that trunk as of the landing of this (or even trunk before the landing of D111123) says that raw profiles emitted by clang 13.0.0 (which has a backport of this) are malformed.

Can you clarify what exactly is the issue (if any)? What version of Clang are you using?

This looks like to be an issue for me as well, when checking out the release/13.x branch at ToT 75e33f71c2dae584b13a7d1186ae0a038ba98838, the binary-id.c test still fails to run with this output. Is it possible to backport a patch so that ToT of 13.x is not broken?

compiler-rt/test/profile/Linux/binary-id.c:47:29: error: BINARY-ID-RAW-PROF-NEXT: expected string not found in input                                                                       
// BINARY-ID-RAW-PROF-NEXT: Binary IDs:
                            ^                                                                                                                                                                                                                    
<stdin>:4:32: note: scanning from here                                                                                                                                                                                                           Maximum internal block count: 0                                                                                                                                                                                                                  
                               ^                                                                                        
                                                                                                                                                                                                                                                 
Input file: <stdin>                                                                                                                                                                                                                              Check file: compiler-rt/test/profile/Linux/binary-id.c                                                                                                                                     
                                                                                                                                                                                                                                                 
-dump-input=help explains the following input dump.                                                                                                                                                                                              
                                                                                                                                                                                                                                                 
Input was:                                                                                                              
<<<<<<                                                                                                                  
         1: Instrumentation level: Front-end                                                                                                                                                                                                     
         2: Total functions: 3                                                                                          
         3: Maximum function count: 1                                                                                   
         4: Maximum internal block count: 0                                                                                                                                                                                                      
next:47                                    X error: no match found                                                                                                                                                                               
>>>>>>