[profile] Fix profile merging with binary IDs

[phosek]

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.

[hans]

Thanks for fixing!

compiler-rt/include/profile/InstrProfData.inc

648

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

[phosek]

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]

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

[hans]

In D107143#2917112, @phosek wrote:

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

Thanks! lgtm2

[hans]

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

[MaskRay]

In D107143#2918145, @hans wrote:

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) :)

[phosek]

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]

In D107143#2918587, @phosek wrote:

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!

[glandium]

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]

In D107143#3165914, @glandium wrote:

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]

In D107143#3171287, @phosek wrote:

In D107143#3165914, @glandium wrote:

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                                                                                                                                                                               
>>>>>>

compiler-rt/include/profile/InstrProfData.inc

 /* Definition of member fields of the raw profile header data structure. */
 #ifndef INSTR_PROF_RAW_HEADER
 #define INSTR_PROF_RAW_HEADER(Type, Name, Initializer)
 #else
 #define INSTR_PROF_DATA_DEFINED
 #endif
 INSTR_PROF_RAW_HEADER(uint64_t, Magic, __llvm_profile_get_magic())
 INSTR_PROF_RAW_HEADER(uint64_t, Version, __llvm_profile_get_version())
+INSTR_PROF_RAW_HEADER(uint64_t, BinaryIdsSize, __llvm_write_binary_ids(NULL))
 INSTR_PROF_RAW_HEADER(uint64_t, DataSize, DataSize)
 INSTR_PROF_RAW_HEADER(uint64_t, PaddingBytesBeforeCounters, PaddingBytesBeforeCounters)
 INSTR_PROF_RAW_HEADER(uint64_t, CountersSize, CountersSize)
 INSTR_PROF_RAW_HEADER(uint64_t, PaddingBytesAfterCounters, PaddingBytesAfterCounters)
 INSTR_PROF_RAW_HEADER(uint64_t, NamesSize,  NamesSize)
 INSTR_PROF_RAW_HEADER(uint64_t, CountersDelta,
                       (uintptr_t)CountersBegin - (uintptr_t)DataBegin)
 INSTR_PROF_RAW_HEADER(uint64_t, NamesDelta, (uintptr_t)NamesBegin)
 INSTR_PROF_RAW_HEADER(uint64_t, ValueKindLast, IPVK_Last)
-INSTR_PROF_RAW_HEADER(uint64_t, BinaryIdsSize, __llvm_write_binary_ids(NULL))
 #undef INSTR_PROF_RAW_HEADER
 /* INSTR_PROF_RAW_HEADER  end */
 
 /* VALUE_PROF_FUNC_PARAM start */
 /* Definition of parameter types of the runtime API used to do value profiling
  * for a given value site.
  */
 #ifndef VALUE_PROF_FUNC_PARAM

 #define INSTR_PROF_RAW_MAGIC_64 (uint64_t)255 << 56 | (uint64_t)'l' << 48 | \
        (uint64_t)'p' << 40 | (uint64_t)'r' << 32 | (uint64_t)'o' << 24 |  \
         (uint64_t)'f' << 16 | (uint64_t)'r' << 8 | (uint64_t)129
 #define INSTR_PROF_RAW_MAGIC_32 (uint64_t)255 << 56 | (uint64_t)'l' << 48 | \
        (uint64_t)'p' << 40 | (uint64_t)'r' << 32 | (uint64_t)'o' << 24 |  \
         (uint64_t)'f' << 16 | (uint64_t)'R' << 8 | (uint64_t)129
 
 /* Raw profile format version (start from 1). */
-#define INSTR_PROF_RAW_VERSION 6
+#define INSTR_PROF_RAW_VERSION 7
 /* Indexed profile format version (start from 1). */
 #define INSTR_PROF_INDEX_VERSION 7
 /* Coverage mapping format version (start from 0). */
 #define INSTR_PROF_COVMAP_VERSION 5
 
 /* Profile version is always of type uint64_t. Reserve the upper 8 bits in the
  * version for other variants of profile. We set the lowest bit of the upper 8
  * bits (i.e. bit 56) to 1 to indicate if this is an IR-level instrumentaiton

compiler-rt/lib/profile/InstrProfilingBuffer.c

uint64_t __llvm_profile_get_size_for_buffer_internal(
   /* Determine how much padding is needed before/after the counters and after
    * the names. */
   uint64_t PaddingBytesBeforeCounters, PaddingBytesAfterCounters,
       PaddingBytesAfterNames;
   __llvm_profile_get_padding_sizes_for_counters(
       DataSize, CountersSize, NamesSize, &PaddingBytesBeforeCounters,
       &PaddingBytesAfterCounters, &PaddingBytesAfterNames);
 
-  return sizeof(__llvm_profile_header) +
+  return sizeof(__llvm_profile_header) + __llvm_write_binary_ids(NULL) +
          (DataSize * sizeof(__llvm_profile_data)) + PaddingBytesBeforeCounters +
          (CountersSize * sizeof(uint64_t)) + PaddingBytesAfterCounters +
          NamesSize + PaddingBytesAfterNames;
 }
 
 COMPILER_RT_VISIBILITY
 void initBufferWriter(ProfDataWriter *BufferWriter, char *Buffer) {
   BufferWriter->Write = lprofBufferWriter;

compiler-rt/lib/profile/InstrProfilingMerge.c

 #include "profile/InstrProfData.inc"
 
 COMPILER_RT_VISIBILITY
 void (*VPMergeHook)(ValueProfData *, __llvm_profile_data *);
 
 COMPILER_RT_VISIBILITY
 uint64_t lprofGetLoadModuleSignature() {
   /* A very fast way to compute a module signature.  */
+  uint64_t Version = __llvm_profile_get_version();
   uint64_t CounterSize = (uint64_t)(__llvm_profile_end_counters() -
                                     __llvm_profile_begin_counters());
   uint64_t DataSize = __llvm_profile_get_data_size(__llvm_profile_begin_data(),
                                                    __llvm_profile_end_data());
   uint64_t NamesSize =
       (uint64_t)(__llvm_profile_end_names() - __llvm_profile_begin_names());
   uint64_t NumVnodes =
       (uint64_t)(__llvm_profile_end_vnodes() - __llvm_profile_begin_vnodes());
   const __llvm_profile_data *FirstD = __llvm_profile_begin_data();
 
   return (NamesSize << 40) + (CounterSize << 30) + (DataSize << 20) +
-         (NumVnodes << 10) + (DataSize > 0 ? FirstD->NameRef : 0);
+         (NumVnodes << 10) + (DataSize > 0 ? FirstD->NameRef : 0) + Version;
 }
 
 /* Returns 1 if profile is not structurally compatible.  */
 COMPILER_RT_VISIBILITY
 int __llvm_profile_check_compatibility(const char *ProfileData,
                                        uint64_t ProfileSize) {
   /* Check profile header only for now  */
   __llvm_profile_header *Header = (__llvm_profile_header *)ProfileData;
   __llvm_profile_data *SrcDataStart, *SrcDataEnd, *SrcData, *DstData;
   SrcDataStart =
-      (__llvm_profile_data *)(ProfileData + sizeof(__llvm_profile_header));
+      (__llvm_profile_data *)(ProfileData + sizeof(__llvm_profile_header) +
+                              Header->BinaryIdsSize);
   SrcDataEnd = SrcDataStart + Header->DataSize;
 
   if (ProfileSize < sizeof(__llvm_profile_header))
     return 1;
 
   /* Check the header first.  */
   if (Header->Magic != __llvm_profile_get_magic() ||
       Header->Version != __llvm_profile_get_version() ||
       Header->DataSize !=
           __llvm_profile_get_data_size(__llvm_profile_begin_data(),
                                        __llvm_profile_end_data()) ||
       Header->CountersSize != (uint64_t)(__llvm_profile_end_counters() -
                                          __llvm_profile_begin_counters()) ||
       Header->NamesSize != (uint64_t)(__llvm_profile_end_names() -
                                       __llvm_profile_begin_names()) ||
       Header->ValueKindLast != IPVK_Last)
     return 1;
 
-  if (ProfileSize < sizeof(__llvm_profile_header) +
+  if (ProfileSize < sizeof(__llvm_profile_header) + Header->BinaryIdsSize +
                         Header->DataSize * sizeof(__llvm_profile_data) +
                         Header->NamesSize + Header->CountersSize)
     return 1;
 
   for (SrcData = SrcDataStart,
        DstData = (__llvm_profile_data *)__llvm_profile_begin_data();
        SrcData < SrcDataEnd; ++SrcData, ++DstData) {
     if (SrcData->NameRef != DstData->NameRef ||
int __llvm_profile_merge_from_buffer(const char *ProfileData,
   __llvm_profile_data *SrcDataStart, *SrcDataEnd, *SrcData, *DstData;
   __llvm_profile_header *Header = (__llvm_profile_header *)ProfileData;
   uint64_t *SrcCountersStart;
   const char *SrcNameStart;
   const char *SrcValueProfDataStart, *SrcValueProfData;
   uintptr_t CountersDelta = Header->CountersDelta;
 
   SrcDataStart =
-      (__llvm_profile_data *)(ProfileData + sizeof(__llvm_profile_header));
+      (__llvm_profile_data *)(ProfileData + sizeof(__llvm_profile_header) +
+                              Header->BinaryIdsSize);
   SrcDataEnd = SrcDataStart + Header->DataSize;
   SrcCountersStart = (uint64_t *)SrcDataEnd;
   SrcNameStart = (const char *)(SrcCountersStart + Header->CountersSize);
   SrcValueProfDataStart =
       SrcNameStart + Header->NamesSize +
       __llvm_profile_get_num_padding_bytes(Header->NamesSize);
   if (SrcNameStart < (const char *)SrcCountersStart)
     return 1;

compiler-rt/test/profile/Linux/binary-id.c

 // REQUIRES: linux
 // RUN: %clang_profgen -Wl,--build-id=none -O2 -o %t %s
 // RUN: env LLVM_PROFILE_FILE=%t.profraw %run %t
 // RUN: llvm-profdata show --binary-ids  %t.profraw > %t.out
 // RUN: FileCheck %s --check-prefix=NO-BINARY-ID < %t.out
 // RUN: llvm-profdata merge -o %t.profdata %t.profraw
 
 // RUN: %clang_profgen -Wl,--build-id -O2 -o %t %s
 // RUN: env LLVM_PROFILE_FILE=%t.profraw %run %t
 // RUN: llvm-profdata show --binary-ids  %t.profraw > %t.profraw.out
 // RUN: FileCheck %s --check-prefix=BINARY-ID-RAW-PROF < %t.profraw.out
 
+// RUN: rm -rf %t.profdir
+// RUN: env LLVM_PROFILE_FILE=%t.profdir/default_%m.profraw %run %t
+// RUN: env LLVM_PROFILE_FILE=%t.profdir/default_%m.profraw %run %t
+// RUN: env LLVM_PROFILE_FILE=%t.profdir/default_%m.profraw %run %t
+// RUN: llvm-profdata show --binary-ids  %t.profdir/default_*.profraw > %t.profraw.out
+// RUN: FileCheck %s --check-prefix=BINARY-ID-MERGE-PROF < %t.profraw.out
+
 void foo() {
 }
 
 void bar() {
 }
 
 int main() {
   foo();
   bar();
   return 0;
 }
 
 // NO-BINARY-ID: Instrumentation level: Front-end
 // NO-BINARY-ID-NEXT: Total functions: 3
 // NO-BINARY-ID-NEXT: Maximum function count: 1
 // NO-BINARY-ID-NEXT: Maximum internal block count: 0
 // NO-BINARY-ID-NOT: Binary IDs:
 
 // BINARY-ID-RAW-PROF: Instrumentation level: Front-end
 // BINARY-ID-RAW-PROF-NEXT: Total functions: 3
 // BINARY-ID-RAW-PROF-NEXT: Maximum function count: 1
 // BINARY-ID-RAW-PROF-NEXT: Maximum internal block count: 0
 // BINARY-ID-RAW-PROF-NEXT: Binary IDs:
 // BINARY-ID-RAW-PROF-NEXT: {{[0-9a-f]+}}
+
+// BINARY-ID-MERGE-PROF: Instrumentation level: Front-end
+// BINARY-ID-MERGE-PROF-NEXT: Total functions: 3
+// BINARY-ID-MERGE-PROF-NEXT: Maximum function count: 3
+// BINARY-ID-MERGE-PROF-NEXT: Maximum internal block count: 0
+// BINARY-ID-MERGE-PROF-NEXT: Binary IDs:
+// BINARY-ID-MERGE-PROF-NEXT: {{[0-9a-f]+}}

llvm/include/llvm/ProfileData/InstrProfData.inc

 /* Definition of member fields of the raw profile header data structure. */
 #ifndef INSTR_PROF_RAW_HEADER
 #define INSTR_PROF_RAW_HEADER(Type, Name, Initializer)
 #else
 #define INSTR_PROF_DATA_DEFINED
 #endif
 INSTR_PROF_RAW_HEADER(uint64_t, Magic, __llvm_profile_get_magic())
 INSTR_PROF_RAW_HEADER(uint64_t, Version, __llvm_profile_get_version())
+INSTR_PROF_RAW_HEADER(uint64_t, BinaryIdsSize, __llvm_write_binary_ids(NULL))
 INSTR_PROF_RAW_HEADER(uint64_t, DataSize, DataSize)
 INSTR_PROF_RAW_HEADER(uint64_t, PaddingBytesBeforeCounters, PaddingBytesBeforeCounters)
 INSTR_PROF_RAW_HEADER(uint64_t, CountersSize, CountersSize)
 INSTR_PROF_RAW_HEADER(uint64_t, PaddingBytesAfterCounters, PaddingBytesAfterCounters)
 INSTR_PROF_RAW_HEADER(uint64_t, NamesSize,  NamesSize)
 INSTR_PROF_RAW_HEADER(uint64_t, CountersDelta,
                       (uintptr_t)CountersBegin - (uintptr_t)DataBegin)
 INSTR_PROF_RAW_HEADER(uint64_t, NamesDelta, (uintptr_t)NamesBegin)
 INSTR_PROF_RAW_HEADER(uint64_t, ValueKindLast, IPVK_Last)
-INSTR_PROF_RAW_HEADER(uint64_t, BinaryIdsSize, __llvm_write_binary_ids(NULL))
 #undef INSTR_PROF_RAW_HEADER
 /* INSTR_PROF_RAW_HEADER  end */
 
 /* VALUE_PROF_FUNC_PARAM start */
 /* Definition of parameter types of the runtime API used to do value profiling
  * for a given value site.
  */
 #ifndef VALUE_PROF_FUNC_PARAM

 #define INSTR_PROF_RAW_MAGIC_64 (uint64_t)255 << 56 | (uint64_t)'l' << 48 | \
        (uint64_t)'p' << 40 | (uint64_t)'r' << 32 | (uint64_t)'o' << 24 |  \
         (uint64_t)'f' << 16 | (uint64_t)'r' << 8 | (uint64_t)129
 #define INSTR_PROF_RAW_MAGIC_32 (uint64_t)255 << 56 | (uint64_t)'l' << 48 | \
        (uint64_t)'p' << 40 | (uint64_t)'r' << 32 | (uint64_t)'o' << 24 |  \
         (uint64_t)'f' << 16 | (uint64_t)'R' << 8 | (uint64_t)129
 
 /* Raw profile format version (start from 1). */
-#define INSTR_PROF_RAW_VERSION 6
+#define INSTR_PROF_RAW_VERSION 7
 /* Indexed profile format version (start from 1). */
 #define INSTR_PROF_INDEX_VERSION 7
 /* Coverage mapping format version (start from 0). */
 #define INSTR_PROF_COVMAP_VERSION 5
 
 /* Profile version is always of type uint64_t. Reserve the upper 8 bits in the
  * version for other variants of profile. We set the lowest bit of the upper 8
  * bits (i.e. bit 56) to 1 to indicate if this is an IR-level instrumentaiton

llvm/lib/ProfileData/InstrProfReader.cpp

 
 template <class IntPtrT>
 Error RawInstrProfReader<IntPtrT>::readHeader(
     const RawInstrProf::Header &Header) {
   Version = swap(Header.Version);
   if (GET_VERSION(Version) != RawInstrProf::Version)
     return error(instrprof_error::unsupported_version);
 
+  BinaryIdsSize = swap(Header.BinaryIdsSize);
   CountersDelta = swap(Header.CountersDelta);
   NamesDelta = swap(Header.NamesDelta);
   auto DataSize = swap(Header.DataSize);
   auto PaddingBytesBeforeCounters = swap(Header.PaddingBytesBeforeCounters);
   auto CountersSize = swap(Header.CountersSize);
   auto PaddingBytesAfterCounters = swap(Header.PaddingBytesAfterCounters);
   NamesSize = swap(Header.NamesSize);
   ValueKindLast = swap(Header.ValueKindLast);
-  BinaryIdsSize = swap(Header.BinaryIdsSize);
 
   auto DataSizeInBytes = DataSize * sizeof(RawInstrProf::ProfileData<IntPtrT>);
   auto PaddingSize = getNumPaddingBytes(NamesSize);
 
   // Profile data starts after profile header and binary ids if exist.
   ptrdiff_t DataOffset = sizeof(RawInstrProf::Header) + BinaryIdsSize;
   ptrdiff_t CountersOffset =
       DataOffset + DataSizeInBytes + PaddingBytesBeforeCounters;