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

[Akira Hatanaka via cfe-commits]

On Thu, Feb 18, 2016 at 8:10 AM, Manman Ren <manman.ren at gmail.com> wrote:

>
>
> 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?
>
>

This part is enclosed in "else if (LocalAlignment == 8)", so it's
LocalAlignment should be 8 and it can start at Index=24 (which is 8-byte
aligned), no? By new element, I meant the element that is currently being
pushed.


> 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/d7c91ea4/attachment.html>