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
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
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
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
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
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_storageN::type)` and
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.
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 not
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
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
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,
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
test for
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. This was my
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
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 risk of
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
On Wed, Aug 26, 2015 at 3:38 PM, Eric Fiselier e...@efcs.ca 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
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
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
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
21 matches
Mail list logo
