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

[Gulfem Savrun Yeniceri via Phabricator via llvm-commits]

gulfem marked 4 inline comments as done.
gulfem added inline comments.


================
Comment at: compiler-rt/lib/profile/InstrProfilingPlatformLinux.c:102
+
+      Note = Note + sizeof(ElfW(Nhdr)) + RoundUp(Note->n_namesz, 4) +
+             RoundUp(Note->n_descsz, 4);
----------------
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?


================
Comment at: llvm/include/llvm/ProfileData/InstrProfReader.h:85
 
+  // Get number of binary ids.
+  /// TODO: Can we implement the following three functions
----------------
mcgrathr wrote:
> Is this the number of distinct IDs, or the byte size of the total block?  (AIUI the total block is a sequence of {uint64_t n, uint8_t[n]} blocks.)  If it's the number of distinct IDs, then I don't understand how a single uint8_t* from getBinaryIds() is meant to be used.  For a number like that, I think Count is a clearer name to use than Size.
> 
> Is this the number of distinct IDs, or the byte size of the total block?  
It is the number of binary IDs. That file consistently uses `Size` to refer to number of elements like `DataSize`, `CounterSize`, `NamesSize`, etc.
I don't like that either, but I just used `Size` to be consistent with the rest of the implementation.
I think `Count` or `NumOf` is more self-explanatory.




================
Comment at: llvm/lib/ProfileData/InstrProfReader.cpp:558
+
+    BinaryId += BinaryIdLen;
+  }
----------------
mcgrathr wrote:
> This is wildly susceptible to bad data.
> 
> This is wildly susceptible to bad data.
> 




================
Comment at: llvm/lib/ProfileData/InstrProfReader.cpp:558
+
+    BinaryId += BinaryIdLen;
+  }
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102039