[PATCH] D102039: [profile] WIP Add binary id into profiles

[Petr Hosek via Phabricator via llvm-commits]

phosek added inline comments.


================
Comment at: compiler-rt/include/profile/InstrProfData.inc:132
 INSTR_PROF_RAW_HEADER(uint64_t, Version, __llvm_profile_get_version())
+INSTR_PROF_RAW_HEADER(uint64_t, HasBuildId, 0) // TODO: Can use a bool?
 INSTR_PROF_RAW_HEADER(uint64_t, DataSize, DataSize)
----------------
gulfem wrote:
> gulfem wrote:
> > phosek wrote:
> > > Instead of `HasBuildId` as a boolean, I think it might be better to use `BuildIdSize` to allow multiple ids. While raw profiles should have either 0 or 1 in practice, indexed profiles created by merging multiple raw profiles could have multiple ids, so using `BuildIdSize` would allow using the same header format for both  raw and indexed format.
> > Can a raw profile **with** build id and another raw profile **without** a build id be merged and create indexed profiled?
> @phosek, it seems to me like `raw` and `indexed` profile do not share the same profile format.
> For ex, `raw` profile header is defined in:
> https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/ProfileData/InstrProfData.inc#L130
>  
> Whereas,  `indexed` profile header is defined in:
> https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/ProfileData/InstrProf.h#L999
> 
> I think, we might need to extend both formats with binary id then.
> Please let me know if I'm missing anything. 
Yes, looks like we'll need to extend both formats but I still think it'd be preferable to support arbitrary number of binary id's in both formats since at least in ELF, you can have more than one build ID. The format of should also be the same in both the raw and indexed format, that is a sequence of tuples where each tuple is a length and a sequence of bytes of that length.


================
Comment at: compiler-rt/lib/profile/InstrProfiling.h:101
+ */
+const uint8_t *__llvm_read_binary_id(uint32_t *Size);
+
----------------
I'd slightly prefer returning both the data and the size as output parameters for consistency. We might also consider having a boolean/int return argument to signal an error.


================
Comment at: compiler-rt/lib/profile/InstrProfilingWriter.c:287-288
+  uint32_t BinaryIdSize = 0;
+  uint32_t *BinaryIdSizePtr = &BinaryIdSize;
+  const uint8_t *BinaryId = __llvm_read_binary_id(BinaryIdSizePtr);
+
----------------
You shouldn't need an extra variable for the pointer.


================
Comment at: compiler-rt/lib/profile/InstrProfilingWriter.c:300
+       */
+      struct BuildId *BI = malloc(sizeof(struct BuildId));
+      BI->Size = Note->n_descsz;
----------------
gulfem wrote:
> phosek wrote:
> > I think that we should avoid allocating the `BuildId` struct on the heap which could cause an issue when allocator itself is instrumented. Two solutions I can think of is to either allocate the struct as static (it's 8-16 bytes which is not so bad) or alternatively this function could use `ProfDataWriter` to directly write out the struct in which case it could be allocated on stack.
> It seems like ValueProfiling already uses heap allocation `compiler-rt/lib/profile/InstrProfilingValue.c`, but we don't really need heap allocation for build id.
> So, I removed build id struct.
> Please let me know what you think about the new implementation.
> 
That's true but it's also possible to allocate counter for value profiling statically if you use `-mllvm -vp-static-alloc` in which case you can avoid `malloc` which may be desirable on some platforms (this is what we would likely use on Fuchsia for example).


================
Comment at: llvm/tools/llvm-profdata/llvm-profdata.cpp:2227
+  // This is only for testing binary id prototype.
+  Reader->printBinaryId(OS);
   return 0;
----------------
I think we'll want this functionality even in the final version but we should introduce additional flag, for example `-show-binary-id`, and only print the binary id if that flag is set.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102039/new/

https://reviews.llvm.org/D102039