[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, BinaryIdSize, __llvm_binary_ids_size())
 INSTR_PROF_RAW_HEADER(uint64_t, DataSize, DataSize)
----------------
I'd prefer to put this field at the end of the header to match the position of binary ID in the profile.


================
Comment at: compiler-rt/lib/profile/InstrProfilingInternal.h:206
+COMPILER_RT_VISIBILITY COMPILER_RT_WEAK int
+__llvm_get_binary_ids_size_or_write(ProfDataWriter *Writer);
+
----------------
This name is a bit of a mouthful. I'd consider calling it just `__llvm_write_binary_ids` and say in the comment that the function always returns the number of binary ids even if writer is `NULL`. 


================
Comment at: compiler-rt/lib/profile/InstrProfilingPlatformLinux.c:105
+      if (Writer == NULL)
+        NumOfBinaryIds++;
+      else {
----------------
I think we should be returning the number of binary ids in every case, even if `writer != NULL`.


================
Comment at: compiler-rt/lib/profile/InstrProfilingPlatformLinux.c:102
+
+      Note = Note + sizeof(ElfW(Nhdr)) + RoundUp(Note->n_namesz, 4) +
+             RoundUp(Note->n_descsz, 4);
----------------
gulfem wrote:
> mcgrathr wrote:
> > This arithmetic is not right since Note has a type with sizeof>1.  An example of correct arithmetic is:
> > ```
> >   Note = (const ElfW(Nhdr)*)((const char *)(Note + 1) + RoundUp(Note->n_namesz, 4) + RoundUp(Note->n_descsz, 4));
> > ```
> My understanding is that each note in notes section starts with a `struct` which includes `n_namesz`, `n_descsz`, and `n_type` members. 
> It is followed by the name (whose length is defined in `n_namesz`) and then by the descriptor (whose length is defined in `n_descsz`).
> So, type is already part of the struct. The arithmetic I have increases Note by struct size, and name data, and descriptor data.
> Am I missing anything?
When you increment a pointer, the address is moved by the size of the underlying type:
```
char* c = ...;
c++; // c increments by 1 == sizeof(char)
uint64_t* l = ...;
l++; // l increments by 8 == sizeof(uint64_t)
```
In this case `sizeof(Note) > 1` so you first need to cast it to `const char *`.


================
Comment at: llvm/lib/ProfileData/InstrProfReader.cpp:534
+const BinaryIdTy *RawInstrProfReader<IntPtrT>::getBinaryIds() {
+  return new BinaryIdTy(BinaryIdsSize, BinaryIdsStart);
+}
----------------
Where is this deallocated? It seems like this memory gets leaked. `BinaryIdTy` is just a plain struct with just two 8 byte fields (on a 64-bit machine), can we avoid allocating it on the heap and return/pass it by value to simplify lifetime management?


================
Comment at: llvm/lib/ProfileData/InstrProfReader.cpp:558
+
+    BinaryId += BinaryIdLen;
+  }
----------------
gulfem wrote:
> gulfem wrote:
> > mcgrathr wrote:
> > > This is wildly susceptible to bad data.
> > > 
> > > This is wildly susceptible to bad data.
> > > 
> > 
> > 
> How can I improve that? I need to somehow increment the pointer by binary id length.
I think at minimum, you need to check that you aren't reading past the end of the file in the case the length is incorrect.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102039