[PATCH] D16843: [Sema] Fix bug in TypeLocBuilder::pushImpl

[Manman Ren via cfe-commits]

On Wed, Feb 17, 2016 at 10:33 PM, Akira Hatanaka <ahatanak at gmail.com> wrote:

> ahatanak added a comment.
>
> OK, I now understand what you meant.
>
> > How about the following?
>
> >
>
> >   else if (LocalAlignment == 8) {
>
> >     if (NumBytesAtAlign8 == 0) {
>
> >       // We have not seen any 8-byte aligned element yet. There is no
> padding and we are either 4-byte
>
> >       // aligned or 8-byte aligned depending on NumBytesAtAlign4.
>
> >       // Add in 4 bytes padding if we are not 8-byte aligned including
> this element.
>
> >       if ((LocalSize + NumBytesAtAlign4) % 8 != 0) {
>
> >         memmove(&Buffer[Index - 4], &Buffer[Index], NumBytesAtAlign4);
>
> >         Index -= 4;
>
> >       }
>
>
> If Capacity is not a multiple of 8, (LocalSize + NumBytesAtAlign4) % 8
> doesn't tell you whether the new element will be 8-byte aligned. For
> example, if Capacity==36, NumBytesAtAlign4==4, and LocalSize==8, (LocalSize
> + NumBytesAtAlign4) equals 12 but padding is not needed as the new element
> can start at Index=24.


I don't quite get why the new element can start at Index of 24, does it
have a LocalAlignment of 8?


> Note that it's possible to have a Capacity that isn't a multiple of 8 by
> calling TypeLocBuilder::reserve.

Yeah, I probably missed the case where Capacity is not aligned.

Please update the patch.

Manman

> I think padding is needed if the new index (Index - LocalSize) is not a
> multiple of 8.
>
>
> http://reviews.llvm.org/D16843
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160218/07ffa7ac/attachment.html>