EricWF added a comment.
Sure sorry for the delay.
http://reviews.llvm.org/D12247
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
EricWF closed this revision.
EricWF added a comment.
Committed as r248309.
http://reviews.llvm.org/D12247
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
yiranwang added a comment.
Thank you, Eric. Also, could you please help to commit the change? I personally
do not have the permissions to change libc++ code, thanks a lot.
http://reviews.llvm.org/D12247
___
cfe-commits mailing list
cfe-commits@list
EricWF accepted this revision.
EricWF added a comment.
This revision is now accepted and ready to land.
LGTM. I don't want to hold this up any longer. @mclow.lists can comment on this
after it's committed if he sees any issues.
http://reviews.llvm.org/D12247
_
yiranwang added a comment.
Thanks for testing.
There is proof as the following, given that alignment of _Aligner is _Align
(the whole purpose of _Aligner here, which we do not want to double check), and
for any type, the "sizeof" is always multiple of "alignof", this change should
be ABI compat
EricWF added a comment.
@mclow.lists I have also tested this patch using libabigail and it couldn't
spot any problems.
http://reviews.llvm.org/D12247
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/list
On Thu, Aug 27, 2015 at 2:57 PM, Yiran Wang wrote:
> yiranwang added a comment.
>
> In http://reviews.llvm.org/D12247#234533, @EricWF wrote:
>
>> So this patch LGTM. Sorry for being slow to understand it. However I want to
>> see two things before it lands.
>>
>> 1. I would like to see a test of
yiranwang added a comment.
In http://reviews.llvm.org/D12247#234533, @EricWF wrote:
> So this patch LGTM. Sorry for being slow to understand it. However I want to
> see two things before it lands.
>
> 1. I would like to see a test of some sort that the resulting type has the
> same size and ali
EricWF added a comment.
So this patch LGTM. Sorry for being slow to understand it. However I want to
see two things before it lands.
1. I would like to see a test of some sort that the resulting type has the same
size and alignment requirements it did before the change. I'm not sure what the
b
davidxl added a comment.
yes -- the intention of Yiran's patch is not to change the size/alignment of
the underlying storage, but to make the original padding space part of the type.
http://reviews.llvm.org/D12247
___
cfe-commits mailing list
cfe-c
EricWF added a comment.
I think I've misunderstood this patch the whole time. I didn't understand that
it was undefined to access the trailing padding of the storage type. Am I
correct in saying that this patch won't change the actual values of
`sizeof(aligned_storage::type)` and `sizeof(aligne
EricWF added a comment.
In http://reviews.llvm.org/D12247#231444, @yiranwang wrote:
> A test case is as following. It has to be build by GCC 4.9 -O3 (maybe or
> later), with latest libc++, and for AARCH64+ANDROID target.
> AARCH64 requires 128 bit alignment for aligned_storage and 64 bit point
On Wed, Aug 26, 2015 at 3:38 PM, Eric Fiselier wrote:
> EricWF added a comment.
>
> In http://reviews.llvm.org/D12247#233717, @yiranwang wrote:
>
>> Hi Eric,
>>
>> Could you please explain a bit more what is broken specifically? As we can
>> see, sizeof(), _Len, and _Align, and alignof() of "alig
EricWF added a comment.
In http://reviews.llvm.org/D12247#233717, @yiranwang wrote:
> Hi Eric,
>
> Could you please explain a bit more what is broken specifically? As we can
> see, sizeof(), _Len, and _Align, and alignof() of "aligned_storage" are all
> not changed.
That's correct. At the ris
yiranwang added a comment.
Hi Eric,
Could you please explain a bit more what is broken specifically? As we can see,
sizeof(), _Len, and _Align, and alignof() of "aligned_storage" are all not
changed.
http://reviews.llvm.org/D12247
___
cfe-commits
EricWF added a comment.
In http://reviews.llvm.org/D12247#233595, @yiranwang wrote:
> In http://reviews.llvm.org/D12247#233349, @EricWF wrote:
>
> > In http://reviews.llvm.org/D12247#233323, @davidxl wrote:
> >
> > > We certainly need a fix without breaking ABI. Is there a ABI conformance
> > >
yiranwang added a comment.
some more explanation of last comment.
For most architecture sizeof(function) is 4*sizeof(void *), but it is
6*sizeof(void*) on AARCH64/LINUX, which does not sound so great as there are
padding of 2*sizeof(void*) bytes.
http://reviews.llvm.org/D12247
_
yiranwang added a comment.
In http://reviews.llvm.org/D12247#233349, @EricWF wrote:
> In http://reviews.llvm.org/D12247#233323, @davidxl wrote:
>
> > We certainly need a fix without breaking ABI. Is there a ABI conformance
> > test for libcxx?
>
>
> Well this patch definitely breaks the ABI. Thi
EricWF added a comment.
In http://reviews.llvm.org/D12247#233323, @davidxl wrote:
> We certainly need a fix without breaking ABI. Is there a ABI conformance test
> for libcxx?
To actually answer @davidxl's question: No. libc++ does not have an ABI test
suite of any sort.
http://reviews.llvm
On Wed, Aug 26, 2015 at 11:11 AM, Eric Fiselier wrote:
> EricWF added a comment.
>
> In http://reviews.llvm.org/D12247#233323, @davidxl wrote:
>
>> We certainly need a fix without breaking ABI. Is there a ABI conformance
>> test for libcxx?
>
>
> Well this patch definitely breaks the ABI. This wa
EricWF added a comment.
In http://reviews.llvm.org/D12247#233323, @davidxl wrote:
> We certainly need a fix without breaking ABI. Is there a ABI conformance test
> for libcxx?
Well this patch definitely breaks the ABI. This was my attempt at fixing the
problem in `std::function` that might no
davidxl added a comment.
We certainly need a fix without breaking ABI. Is there a ABI conformance test
for libcxx?
http://reviews.llvm.org/D12247
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo
EricWF added a comment.
I think I'm starting to understand but I think the correct fix for this would
be to fix function so that:
1. It aligns its buffer by alignof(void*).
2. It doesn't stick over-aligned types in the small object buffer.
Point two can probably be done without an ABI break, bu
davidxl added a subscriber: davidxl.
davidxl added a comment.
In libc++, placement new is used in many places. When selecting the buffer
size for the placed object, it uses the 'actual' size of the buffer including
the padding bytes from alignment, instead of the declared of the buffer. As a
r
yiranwang added a comment.
A test case is as following. It has to be build by GCC 4.9 -O3 (maybe or
later), with latest libc++, and for AARCH64+ANDROID target.
AARCH64 requires 128 bit alignment for aligned_storage and 64 bit pointers,
while gcc 4.9 alias analysis will do field-sensitive points
EricWF added a comment.
I'm not sure I understand the problem. Is it possible to provide a minimal
reproducer of the bug/behavior you are describing?
http://reviews.llvm.org/D12247
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://list
danalbert added reviewers: mclow.lists, EricWF.
danalbert added a comment.
FYI this was found because it can cause issues when used with GCC.
http://reviews.llvm.org/D12247
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.or
yiranwang created this revision.
yiranwang added a subscriber: cfe-commits.
In libc++, there are some usage of aligned_storage which uses "sizeof" bytes of
raw data. This is problematic a bit, as the trailing padding area will be
counted by "sizeof", and it leads to out of bound access. For exam
28 matches
Mail list logo
