[PATCH] D102039: [profile] WIP Add build 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)
----------------
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.


================
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;
----------------
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`.


================
Comment at: compiler-rt/lib/profile/InstrProfilingWriter.c:272
+ */
+static const struct BuildId *readBuildId() {
+  extern const ElfW(Ehdr) __ehdr_start __attribute__((visibility("hidden")));
----------------
This function implements ELF-specific logic so it cannot be in `InstrProfilingWriter.c` which is platform agnostic. It should be `InstrProfilingPlatformLinux.c` and we'll need COFF and Mach-O equivalents in `InstrProfilingPlatformWindows.c` and `InstrProfilingPlatformDarwin.c` respectively (which could be empty in the initial implementation).


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102039