Re: [PATCH] D12247: [libc++] remove possible trailing padding from aligned_storage

[Yiran Wang via 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@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D12247: [libc++] remove possible trailing padding from aligned_storage

[Yiran Wang via cfe-commits]

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 example, the 
member __buf_ of std::function can be used to store pointers to parameters, and 
the compiler could fail to figure out there is a pointer in the padding area 
points to some local variable.
The fix enlarges the buffer so that the size is exact multiple of "_Align". It 
is of no run time overhead.


http://reviews.llvm.org/D12247

Files:
  include/type_traits

Index: include/type_traits
===
--- include/type_traits
+++ include/type_traits
@@ -1143,7 +1143,7 @@
 union type
 {
 _Aligner __align;
-unsigned char __data[_Len];
+unsigned char __data[(_Len + _Align - 1)/_Align * _Align];
 };
 };
 
@@ -1158,7 +1158,7 @@
 {\
 struct _ALIGNAS(n) type\
 {\
-unsigned char __lx[_Len];\
+unsigned char __lx[(_Len + n - 1)/n * n];\
 };\
 }
 


Index: include/type_traits
===
--- include/type_traits
+++ include/type_traits
@@ -1143,7 +1143,7 @@
 union type
 {
 _Aligner __align;
-unsigned char __data[_Len];
+unsigned char __data[(_Len + _Align - 1)/_Align * _Align];
 };
 };
 
@@ -1158,7 +1158,7 @@
 {\
 struct _ALIGNAS(n) type\
 {\
-unsigned char __lx[_Len];\
+unsigned char __lx[(_Len + n - 1)/n * n];\
 };\
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12247: [libc++] remove possible trailing padding from aligned_storage

[Yiran Wang via cfe-commits]

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-to analysis. But 
this could happen for other ISA+ABI.
The fundamental issue is that for this combination, std::function has member 
__buf_ declared as 
aligned_storage<3*sizoef(void*)>::type __buf_;
Basically, it is
aligned_storage<24>::type;
This will generate aligned_storage of, _Len==24 and _Align==16;
While std::function will use the __buf_ to sizeof(__buf_) bytes (at line 1593 
and 1628 of ), which is 32. Basically, the pointer to "tbool' will 
be stored at "&__buf_+24". 
This is not a well defined memory area, and GCC alias analysis is going to 
ignore the "ESCAPE" of address of "tbool". Basically, the function 
"test_simple" would always return "false".

#include 
extern void external_test(std::functionfn);
extern bool test_simple(){

  bool tbool = false;
  int a, b;
  external_test([&a, &b, &tbool](){
tbool = true;
return true;
  });
  return tbool;

}


http://reviews.llvm.org/D12247



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12247: [libc++] remove possible trailing padding from aligned_storage

[Yiran Wang via cfe-commits]

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 attempt at fixing the 
> problem in `std::function` that might not be ABI breaking..
>  https://gist.github.com/EricWF/3a35b140a66d4826a9


I think following your thought, we should abandon the usage of 
"aligned_storage" in "function" totally. Instead, just say "void *__buf_[3];", 
and check alignment_of _Fp (can be arbitrarily big, but usually small), and 
sizeof the two to guard use of __buf_. The alignment bump to 16 by 
aligned_storage on AARCH64 is not what we want, performance wise.

Also, such kind of change need to applied for some more places, at least in 
libc++, such like 


http://reviews.llvm.org/D12247



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12247: [libc++] remove possible trailing padding from aligned_storage

[Yiran Wang via cfe-commits]

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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12247: [libc++] remove possible trailing padding from aligned_storage

[Yiran Wang via cfe-commits]

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 mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12247: [libc++] remove possible trailing padding from aligned_storage

[Yiran Wang via cfe-commits]

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 alignment requirements it did before the change. I'm not sure 
> what the best way to test this is.
> 2. I would like @mclow.lists to approve this as well. I just want to be 
> cautious.


I think now we are on the same page about these bugs. 
Also, thanks for your review and findings (hopefully fix it soon too).

For the test, we compile it with -O3 -fstrict-aliasing -finline-limit=300, and 
as mentioned before the target is AARCH64+ANDROID.


http://reviews.llvm.org/D12247



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12247: [libc++] remove possible trailing padding from aligned_storage

[Yiran Wang via cfe-commits]

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 compatible.
Are we good to go?


http://reviews.llvm.org/D12247



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits