[PATCH] D104745: [llvm-cov] Enforce alignment of function records

[Vedant Kumar via Phabricator via llvm-commits]

vsk added a comment.

In D104745#2835126 <https://reviews.llvm.org/D104745#2835126>, @serge-sans-paille wrote:

> In D104745#2834796 <https://reviews.llvm.org/D104745#2834796>, @vsk wrote:
>
>> The requirement is that the advancing by sizeof(CovMapFunctionRecordV3) bytes in the record buffer advances to the start of the next record. The address of the buffer doesn't need the same alignment.
>
> The function `advanceByOne(...)` starts with the following assert: ` assert(isAddrAligned(Align(8), this) && "Function record not aligned");` so I tend to think each record needs to be aligned, including the first.

I see, thanks for highlighting the inconsistency there. It seems like `assert(isAddrAligned(Align(8), this) && ...)` is overzealous: the important thing is that `this` points to the start of a function record, *not* that `this` has some particular alignment. Removing the assert would make sense to me.

> Take the case of the first record created, and assume it has a size multiple of 8. In that case, upon creation no padding is added (independently of it's start address). That's what the snippet below does
>
>   while (FuncRecords.size() % 8 != 0)
>     FuncRecords += '\0';
>
> Now let's assume the start address is not a multiple of 8. In that case next record won't have an address aligned to 8, etc.

Yes, and that's fine. If that runs afoul of alignment rules (and as far as I know it doesn't, since we go through endian::byte_swap to read fields from the struct), marking the function record struct 'packed' should appease the compiler.

Do you feel I'm missing something here, e.g. are you seeing an alignment-related crash in this code?

> A potential patch would be yo compute padding based on the start address instead of the size, but then when the std::string grows, the alignment of the data may change upon reallocation. Thus my patch.
> I also tend to think that using a MemoryBuffer conveys more meaning than an `std::string` for what is actually a view on raw data.

I'm completely happy to see refactors/cleanups in this area. I just wanted to make sure we're all on the same page about whether or not there's an alignment bug here. E.g. if there is such a bug that can lead to a crash, it would make sense to add a regression test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104745