| Message ID | 56AA70A1.8030303@gmail.com |
|---|---|
| State | New |
| Headers | show |
On 01/28/2016 02:48 PM, Martin Sebor wrote: > The g++.dg/cpp1y/vla-initlist1.C test relies on undefined behavior > (reading an uninitialized array element) to verify that two VLAs > are allocated in memory starting at the same address. The test > has been seen to fail on powerpc64-*-linux-gnu and spu-*-elf. > The attached patch removes the undefined behavior while still > verifying the same thing. It passes on powerpc64le. > > Jason, based on your comment in the bug I've removed the part > of the test that verifies that the no-op initializer-list ctor > for the user-defined VLA leaves the array elements uninitialized. > If that's something that should be tested let me know and I'll > add another test or test case for that. Actually, yes, the testcase is testing for that as well. Is that the part that breaks on PPC64LE? Jason
On 01/28/2016 03:53 PM, Jason Merrill wrote: > On 01/28/2016 02:48 PM, Martin Sebor wrote: >> The g++.dg/cpp1y/vla-initlist1.C test relies on undefined behavior >> (reading an uninitialized array element) to verify that two VLAs >> are allocated in memory starting at the same address. The test >> has been seen to fail on powerpc64-*-linux-gnu and spu-*-elf. >> The attached patch removes the undefined behavior while still >> verifying the same thing. It passes on powerpc64le. >> >> Jason, based on your comment in the bug I've removed the part >> of the test that verifies that the no-op initializer-list ctor >> for the user-defined VLA leaves the array elements uninitialized. >> If that's something that should be tested let me know and I'll >> add another test or test case for that. > > Actually, yes, the testcase is testing for that as well. Is that the > part that breaks on PPC64LE? Yes, that's the only thing the existing test exercises (i.e., that the second element of the array is unchanged). What seems to happen is that the call to __builtin_alloca_with_align uses the stdu (store with update) instruction to store and bump down the stack pointer (SP) at the same time (standard for powerpc63le) to make room for the VLA. The subsequent code then reads the saved value of the SP and uses it as the address of the VLA. Since the SP is 64 bits wide, it clobbers the first two words of the VLA. The test looks at the second element, expecting it to be unchanged, but what it finds is the upper word of the saved SP. Since the saved SP value doesn't get read I don't see anything wrong with this. Other than that, I did notice one thing that doesn't look right. There's an empty loop for the no-op initializer_list ctor to initialize the rest of the array. I should open a bug for that. But that's not a unique to the powerpc64 back end. The x86_64 code has the same empty loop. Martin
> Other than that, I did notice one thing that doesn't look right. > There's an empty loop for the no-op initializer_list ctor to > initialize the rest of the array. I should open a bug for that. > But that's not a unique to the powerpc64 back end. The x86_64 > code has the same empty loop. I raised bug 69547 - [6 regression] no-op array initializer emits an empty loop, and bisected it to a constexpr change in r222135. Martin
Jason, Do you have any further comments on the changes to the test or is it okay to check it? https://gcc.gnu.org/ml/gcc-patches/2016-01/msg02245.html In light of the powerpc64le behavior I did not make any changes to verify the no-op VLA initialization does not in fact write into the space allocated for the array. I'm sure it's possible to do but probably not without making assumptions about the implementation that might change or not hold on some targets. I reviewed bug 57402 whose fix that test was committed with and couldn't find anything related to the ctor scribbling over the array. If there is a need for such a test, I think it can be added independently of the fix for the test suite regression (i.e., bug 68972). Thanks Martin On 01/28/2016 06:25 PM, Martin Sebor wrote: > On 01/28/2016 03:53 PM, Jason Merrill wrote: >> On 01/28/2016 02:48 PM, Martin Sebor wrote: >>> The g++.dg/cpp1y/vla-initlist1.C test relies on undefined behavior >>> (reading an uninitialized array element) to verify that two VLAs >>> are allocated in memory starting at the same address. The test >>> has been seen to fail on powerpc64-*-linux-gnu and spu-*-elf. >>> The attached patch removes the undefined behavior while still >>> verifying the same thing. It passes on powerpc64le. >>> >>> Jason, based on your comment in the bug I've removed the part >>> of the test that verifies that the no-op initializer-list ctor >>> for the user-defined VLA leaves the array elements uninitialized. >>> If that's something that should be tested let me know and I'll >>> add another test or test case for that. >> >> Actually, yes, the testcase is testing for that as well. Is that the >> part that breaks on PPC64LE? > > Yes, that's the only thing the existing test exercises (i.e., that > the second element of the array is unchanged). > > What seems to happen is that the call to __builtin_alloca_with_align > uses the stdu (store with update) instruction to store and bump down > the stack pointer (SP) at the same time (standard for powerpc63le) > to make room for the VLA. The subsequent code then reads the saved > value of the SP and uses it as the address of the VLA. Since the SP > is 64 bits wide, it clobbers the first two words of the VLA. The > test looks at the second element, expecting it to be unchanged, but > what it finds is the upper word of the saved SP. Since the saved SP > value doesn't get read I don't see anything wrong with this. > > Other than that, I did notice one thing that doesn't look right. > There's an empty loop for the no-op initializer_list ctor to > initialize the rest of the array. I should open a bug for that. > But that's not a unique to the powerpc64 back end. The x86_64 > code has the same empty loop. > > Martin
On 01/29/2016 02:25 AM, Martin Sebor wrote: > What seems to happen is that the call to __builtin_alloca_with_align > uses the stdu (store with update) instruction to store and bump down > the stack pointer (SP) at the same time (standard for powerpc63le) > to make room for the VLA. The subsequent code then reads the saved > value of the SP and uses it as the address of the VLA. Since the SP > is 64 bits wide, it clobbers the first two words of the VLA. The > test looks at the second element, expecting it to be unchanged, but > what it finds is the upper word of the saved SP. Since the saved SP > value doesn't get read I don't see anything wrong with this. Interesting. What's the point of storing the SP if it isn't going to be used to restore it later? I think I'd prefer to disable the test on targets with quirks like this rather than everywhere. Jason
2016-01-28 Martin Sebor <msebor@redhat.com> target/68972 * g++.dg/cpp1y/vla-initlist1.C: Prevent test failure on powepc64le by making the test less undefined. diff --git a/gcc/testsuite/g++.dg/cpp1y/vla-initlist1.C b/gcc/testsuite/g++.dg/cpp1y/vla-initlist1.C index 8f5709d..339aacc 100644 --- a/gcc/testsuite/g++.dg/cpp1y/vla-initlist1.C +++ b/gcc/testsuite/g++.dg/cpp1y/vla-initlist1.C @@ -12,12 +12,21 @@ struct A }; int x = 4; -int main(int argc, char **argv) +int main(int argc, char **) { - { int i[x] = { 42, 42, 42, 42 }; } + typedef __UINTPTR_TYPE__ uintptr_t; + uintptr_t vla1_addr, vla2_addr; + + // Verify that both arrays are allocated at the same address. + { + int vla1[x] = { 42, 42, 42, 42 }; + vla1_addr = reinterpret_cast<uintptr_t>(vla1); + } { - A a[x] = { argc }; - if (a[1].i != 42) + A vla2[x] = { argc }; + vla2_addr = reinterpret_cast<uintptr_t>(vla2); + + if (vla1_addr != vla2_addr) __builtin_abort (); } }