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

[Akira Hatanaka via cfe-commits]

On Thu, Feb 18, 2016 at 8:47 AM, Akira Hatanaka <ahatanak at gmail.com> wrote:

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

Also, in case it wasn't clear, when I say the new element (of size 8)
starts at Index=24, I mean the first byte of the element is byte 24 and the
last byte is byte 31.


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