* [PATCH] target/68972 - g++.dg/cpp1y/vla-initlist1.C test case fails on powerpc64le
@ 2016-01-28 19:48 Martin Sebor
2016-01-28 22:53 ` Jason Merrill
0 siblings, 1 reply; 6+ messages in thread
From: Martin Sebor @ 2016-01-28 19:48 UTC (permalink / raw)
To: Gcc Patch List; +Cc: Jason Merrill, seurer
[-- Attachment #1: Type: text/plain, Size: 676 bytes --]
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.
Martin
[-- Attachment #2: gcc-68972.patch --]
[-- Type: text/x-patch, Size: 988 bytes --]
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 ();
}
}
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] target/68972 - g++.dg/cpp1y/vla-initlist1.C test case fails on powerpc64le
2016-01-28 19:48 [PATCH] target/68972 - g++.dg/cpp1y/vla-initlist1.C test case fails on powerpc64le Martin Sebor
@ 2016-01-28 22:53 ` Jason Merrill
2016-01-29 1:25 ` Martin Sebor
0 siblings, 1 reply; 6+ messages in thread
From: Jason Merrill @ 2016-01-28 22:53 UTC (permalink / raw)
To: Martin Sebor, Gcc Patch List; +Cc: seurer
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] target/68972 - g++.dg/cpp1y/vla-initlist1.C test case fails on powerpc64le
2016-01-28 22:53 ` Jason Merrill
@ 2016-01-29 1:25 ` Martin Sebor
2016-01-29 3:00 ` Martin Sebor
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Martin Sebor @ 2016-01-29 1:25 UTC (permalink / raw)
To: Jason Merrill, Gcc Patch List; +Cc: seurer
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] target/68972 - g++.dg/cpp1y/vla-initlist1.C test case fails on powerpc64le
2016-01-29 1:25 ` Martin Sebor
@ 2016-01-29 3:00 ` Martin Sebor
2016-02-01 3:09 ` [PING] " Martin Sebor
2016-02-02 12:23 ` Jason Merrill
2 siblings, 0 replies; 6+ messages in thread
From: Martin Sebor @ 2016-01-29 3:00 UTC (permalink / raw)
To: Jason Merrill, Gcc Patch List; +Cc: seurer
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PING] [PATCH] target/68972 - g++.dg/cpp1y/vla-initlist1.C test case fails on powerpc64le
2016-01-29 1:25 ` Martin Sebor
2016-01-29 3:00 ` Martin Sebor
@ 2016-02-01 3:09 ` Martin Sebor
2016-02-02 12:23 ` Jason Merrill
2 siblings, 0 replies; 6+ messages in thread
From: Martin Sebor @ 2016-02-01 3:09 UTC (permalink / raw)
To: Jason Merrill, Gcc Patch List; +Cc: seurer
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] target/68972 - g++.dg/cpp1y/vla-initlist1.C test case fails on powerpc64le
2016-01-29 1:25 ` Martin Sebor
2016-01-29 3:00 ` Martin Sebor
2016-02-01 3:09 ` [PING] " Martin Sebor
@ 2016-02-02 12:23 ` Jason Merrill
2 siblings, 0 replies; 6+ messages in thread
From: Jason Merrill @ 2016-02-02 12:23 UTC (permalink / raw)
To: Martin Sebor, Gcc Patch List; +Cc: seurer
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-02-02 12:23 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-01-28 19:48 [PATCH] target/68972 - g++.dg/cpp1y/vla-initlist1.C test case fails on powerpc64le Martin Sebor
2016-01-28 22:53 ` Jason Merrill
2016-01-29 1:25 ` Martin Sebor
2016-01-29 3:00 ` Martin Sebor
2016-02-01 3:09 ` [PING] " Martin Sebor
2016-02-02 12:23 ` Jason Merrill
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).