[PATCH] D110277: [compiler-rt][profile] Do not emit binary IDs for corrupted-profile.c test

Wed Sep 22 14:16:31 PDT 2021

leonardchan added a comment.

> I'm not understanding the description then. Why would the offset calculation be wrong if we're not changing the size of anything?

Perhaps I should clarify in the description then. So after the header, we will also emit the binary IDs, then emit stuff like the data, names, and counters. If binary IDs are emitted, then there will be some extra data in between the header and the counters, which will shift the CounterPtr offset. Right now, with binary IDs, the calculation should be something like `header_size + binary_ids_size + counter_offset`. The calculation now is `11 * 8 + 2 * 8` only takes into account the header_size (`11*8`) and counter_offset (`2*8`) but not the binary_ids_size. If we don't emit binary ids though (via `-Wl,--build-id=none`), then `binary_ids_size` will be zero, making the calculation just `header_size + counter_offset` which matches what we currently have. The header will still contain the uint64_t referring to the binary ids size. It's just that its value will be zero. The alternative solution is to just explicitly calculate `binary_ids_size` which is what is suggested below.

In D110277#3016427 <https://reviews.llvm.org/D110277#3016427>, @phosek wrote:

> This test seems quite fragile, I'd prefer modifying it instead to compute the offset. It shouldn't be that difficult. You should be able to `#include "profile/InstrProfData.inc"` and then cast `Buf` to `__llvm_profile_header` and use it to compute the counter offset similarly to what's being done in https://github.com/llvm/llvm-project/blob/6e60bb6883178cf14e6fd47a6789495636e4322f/compiler-rt/lib/profile/InstrProfilingMerge.c#L95.

If this seems more straightforward, then we can do that. I was just a bit lazy to update the test to account for this.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110277