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

This revision was automatically updated to reflect the committed changes. Closed by commit rL261260: [Sema] Fix bug in TypeLocBuilder::pushImpl (authored by ahatanak). Changed prior to commit: http://reviews.llvm.org/D16843?vs=48387=48392#toc Repository: rL LLVM

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

manmanren added a comment. Thanks Akira, LGTM. Manman http://reviews.llvm.org/D16843 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

ahatanak updated this revision to Diff 48387. ahatanak added a comment. Fix indentation. http://reviews.llvm.org/D16843 Files: lib/Sema/TypeLocBuilder.cpp test/SemaObjCXX/typeloc-data-alignment.mm Index: test/SemaObjCXX/typeloc-data-alignment.mm

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

ahatanak updated this revision to Diff 48384. ahatanak added a comment. Address Manman's review comments. Handle elements with "LocalAlignment == 8" similarly to those with "LocalAlignment == 4". http://reviews.llvm.org/D16843 Files: lib/Sema/TypeLocBuilder.cpp

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

manmanren added a comment. > 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

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

On Thu, Feb 18, 2016 at 8:47 AM, Akira Hatanaka wrote: > On Thu, Feb 18, 2016 at 8:10 AM, Manman Ren wrote: > >> >> >> On Wed, Feb 17, 2016 at 10:33 PM, Akira Hatanaka >> wrote: >> >>> ahatanak added a comment. >>> >>> OK, I now

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

On Thu, Feb 18, 2016 at 8:10 AM, Manman Ren wrote: > > > On Wed, Feb 17, 2016 at 10:33 PM, Akira Hatanaka > wrote: > >> ahatanak added a comment. >> >> OK, I now understand what you meant. >> >> > How about the following? >> >> > >> >> > else if

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

On Wed, Feb 17, 2016 at 10:33 PM, Akira Hatanaka 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

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

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

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

manmanren added a comment. Hi Akira, 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

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

ahatanak updated this revision to Diff 48241. ahatanak added a comment. Address some of Manman's review comments. - Add comments that explain what the code is doing (I should add some high-level comments too later). - Simplify the if-else statement. I haven't implemented the other changes

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

manmanren added inline comments. Comment at: lib/Sema/TypeLocBuilder.cpp:130 @@ +129,3 @@ + else if (CurrentlyHasPadding) { +// Remove 4-byte padding. +memmove([Index + 4], [Index], NumBytesAtAlign4); Expand this comment a little?

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

ahatanak updated this revision to Diff 48033. ahatanak added a comment. The bug is in the handling of elements whose LocalAlignment is 8, so I've fixed just that part. I'll resend a patch to simplify the logic of TypeLocBuilder::pushImpl later as it's not urgent.

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

manmanren added a comment. Thanks for working on this! Manman Comment at: lib/Sema/TypeLocBuilder.cpp:99 @@ +98,3 @@ +// element was pushed. +RemoveOrInsertPadding = NumBytesAtAlign4 != 0; + } This patch seems to do two things: 1> simplify the logic

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

manmanren added a subscriber: manmanren. manmanren added a comment. Hi Akira, Can you give a high-level explanation of how this patch fixes the problem? This patch enforces that the capacity is 8-byte aligned, is it required to solve the alignment issue here? Thanks, Manman

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

ahatanak updated this revision to Diff 47413. ahatanak added a comment. Rename and add variables. Add comments to make it clearer what the code is doing. http://reviews.llvm.org/D16843 Files: lib/Sema/TypeLocBuilder.cpp lib/Sema/TypeLocBuilder.h test/SemaObjCXX/typeloc-data-alignment.mm

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

ahatanak updated this revision to Diff 47037. ahatanak added a comment. Fixed a couple of bugs: - Make sure Capacity is a multiple of 8-byte. - "||=" isn't a valid operator. Use "||" instead. http://reviews.llvm.org/D16843 Files: lib/Sema/TypeLocBuilder.cpp lib/Sema/TypeLocBuilder.h

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

ahatanak created this revision. ahatanak added reviewers: doug.gregor, rjmccall, vsk. ahatanak added a subscriber: cfe-commits. This patch fixes a bug in the code between line 117-126 of TypeLocBuilder.cpp which was causing the assert in line 132. The bug manifests itself when an element of