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

[Gulfem Savrun Yeniceri via Phabricator via llvm-commits]

gulfem marked an inline comment as done.
gulfem 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)
----------------
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?


================
Comment at: compiler-rt/lib/profile/InstrProfilingWriter.c:39
+/* Variable-length structure that consists of build id size and data. */
+struct BuildId {
+  uint32_t  Size;
----------------
phosek wrote:
> I'm not sure if we should be using the term `BuildId` since it can be interpreted as ELF-specific. We want this to generalize to other binary formats like Mach-O and COFF. In Mach-O, it's called `LC_UUID`. In COFF, it's usually referred to as GUID.
> 
> When searching online, I noticed that lld also build ID for COFF, see D36758, but I'm not sure if that's official name or if they just copied the name from the ELF implementation. So one option would be to just use `BuildId` but make it clear that it's a generic term. Another option would be a new name, for example `BinaryId`.
I renamed it to `binary id`,  and I will briefly explain other ids used in other platforms in the RFC. 


================
Comment at: compiler-rt/lib/profile/InstrProfilingWriter.c:300
+       */
+      struct BuildId *BI = malloc(sizeof(struct BuildId));
+      BI->Size = Note->n_descsz;
----------------
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.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102039