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

[Vedant Kumar via Phabricator via llvm-commits]

vsk accepted this revision.
vsk added a comment.
This revision is now accepted and ready to land.

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

>> 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.
>
> Actually, the test already exist: current code crashes the test suite on `llvm-conv/*branch*` tests on RHEL7 on i686 machine. Let me try to explain the bug in a different way:

My apologies, I wasn't aware of the breakage. Thanks for taking the time to patiently explain the situation.

> Assume the buffer allocation starts at address 0x04. Then we have a record of, say, 0x10 bytes. That size is a multiple of 8 so no padding is added, and the second record starts at address 0x14.
>
> Now let's consider the record loading. Loading starts at address 0x04 and that's fine. It loads the expected 0x10 bytes and that's fine too. Then it wants to load the second record, and to do so it computes it's alignment *based on the address*. 0x14 is not aligned on 8, the aligned address for the second record is computed as 0x18, and that's wrong.
> Why does this happen? The padding for the record buffer construction is computed based on *size* while the padding for the record loading is computed based on *address*. What my patch does  is basically make sure that both abstraction are in sync: size multiple of 8 means address multiple of 8.

I think I see the issue now:

  template <support::endianness Endian>
  std::pair<const char *, const CovMapFunctionRecordV3 *>
  advanceByOne(const char *) const {
    assert(isAddrAligned(Align(8), this) && "Function record not aligned");
    const char *Next = ((const char *)this) + sizeof(CovMapFunctionRecordV3) -
                       sizeof(char) + getDataSize<Endian>();
    // Each function record has an alignment of 8, so we need to adjust
    // alignment before reading the next record.
    Next += offsetToAlignedAddr(Next, Align(8)); //<< HERE (offset to next aligned record calculated from potentially unaligned address)
    return {nullptr, reinterpret_cast<const CovMapFunctionRecordV3 *>(Next)};
  }

The proposed fix lgtm.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104745