PING: new pass to warn on questionable uses of alloca() and VLAs
diff mbox

Message ID 40a675db-261e-2fbf-9760-673bf0e990d8@redhat.com
State New
Headers show

Commit Message

Aldy Hernandez Oct. 19, 2016, 3:32 p.m. UTC
On 10/19/2016 09:16 AM, Eric Botcazou wrote:
>> m68k-suse-linux
>
> visium-elf too.
>

The attached patch fixes the failures on m68k-suse-linux, visium-elf, 
and arm-eabi.

There were a few problems.

One problem is that on lp64 targets (where sizeof(size_t) != 
sizeof(int)), the warning is slightly different-- and rightly so.  I 
have updated the test to handle both warnings on the respective targets.

The other problem is that the following snippet is incorrectly warning 
on 32-bit targets:

   if (n > 0 && n < 2000)
     p = __builtin_alloca (n);

Looking at the gimple it seems like another case of VRP failing to give 
any range information whatsoever.  I have xfailed it as another case 
where Andrew's upcoming work should theoretically fix this.  The test is 
fine on 64-bit targets.

Can y'all double check it on your respective targets as I only have a 
crude cross build?

Thanks.
Aldy
commit 32b629dcab7b78e8181146338c4b077f1d55a0bf
Author: Aldy Hernandez <aldyh@redhat.com>
Date:   Wed Oct 19 11:24:44 2016 -0400

    	* gcc.dg/Walloca-1.c: Adjust test for !lp64 targets.
    	* gcc.dg/Walloca-2.c: Same.

Comments

Andreas Schwab Oct. 19, 2016, 3:38 p.m. UTC | #1
On Okt 19 2016, Aldy Hernandez <aldyh@redhat.com> wrote:

> commit 32b629dcab7b78e8181146338c4b077f1d55a0bf
> Author: Aldy Hernandez <aldyh@redhat.com>
> Date:   Wed Oct 19 11:24:44 2016 -0400
>
>     	* gcc.dg/Walloca-1.c: Adjust test for !lp64 targets.
>     	* gcc.dg/Walloca-2.c: Same.

This fixes both tests on m68k.

Andreas.
Eric Botcazou Oct. 19, 2016, 3:42 p.m. UTC | #2
> This fixes both tests on m68k.

Likewise on Visium.
Jeff Law Oct. 19, 2016, 3:55 p.m. UTC | #3
On 10/19/2016 09:32 AM, Aldy Hernandez wrote:
> On 10/19/2016 09:16 AM, Eric Botcazou wrote:
>>> m68k-suse-linux
>>
>> visium-elf too.
>>
>
> The attached patch fixes the failures on m68k-suse-linux, visium-elf,
> and arm-eabi.
>
> There were a few problems.
>
> One problem is that on lp64 targets (where sizeof(size_t) !=
> sizeof(int)), the warning is slightly different-- and rightly so.  I
> have updated the test to handle both warnings on the respective targets.
>
> The other problem is that the following snippet is incorrectly warning
> on 32-bit targets:
>
>   if (n > 0 && n < 2000)
>     p = __builtin_alloca (n);
>
> Looking at the gimple it seems like another case of VRP failing to give
> any range information whatsoever.  I have xfailed it as another case
> where Andrew's upcoming work should theoretically fix this.  The test is
> fine on 64-bit targets.
>
> Can y'all double check it on your respective targets as I only have a
> crude cross build?
OK for the trunk whenever you're ready.

jeff
Christophe Lyon Oct. 19, 2016, 4:21 p.m. UTC | #4
On 19 October 2016 at 17:55, Jeff Law <law@redhat.com> wrote:
> On 10/19/2016 09:32 AM, Aldy Hernandez wrote:
>>
>> On 10/19/2016 09:16 AM, Eric Botcazou wrote:
>>>>
>>>> m68k-suse-linux
>>>
>>>
>>> visium-elf too.
>>>
>>
>> The attached patch fixes the failures on m68k-suse-linux, visium-elf,
>> and arm-eabi.
>>
>> There were a few problems.
>>
>> One problem is that on lp64 targets (where sizeof(size_t) !=
>> sizeof(int)), the warning is slightly different-- and rightly so.  I
>> have updated the test to handle both warnings on the respective targets.
>>
>> The other problem is that the following snippet is incorrectly warning
>> on 32-bit targets:
>>
>>   if (n > 0 && n < 2000)
>>     p = __builtin_alloca (n);
>>
>> Looking at the gimple it seems like another case of VRP failing to give
>> any range information whatsoever.  I have xfailed it as another case
>> where Andrew's upcoming work should theoretically fix this.  The test is
>> fine on 64-bit targets.
>>
>> Can y'all double check it on your respective targets as I only have a
>> crude cross build?
>
> OK for the trunk whenever you're ready.
>
> jeff

You are too fast for me :-)
I do not have the build trees for all the configurations ready for
manual testing...
So, I just merged the initial patch and the 2nd one, and started
a validation job to make sure the 2nd patch fixes all the regressions
observed earlier.
It will take a few hours.

Christophe
Aldy Hernandez Oct. 19, 2016, 6:43 p.m. UTC | #5
>> OK for the trunk whenever you're ready.
>>
>> jeff
>
> You are too fast for me :-)
> I do not have the build trees for all the configurations ready for
> manual testing...
> So, I just merged the initial patch and the 2nd one, and started
> a validation job to make sure the 2nd patch fixes all the regressions
> observed earlier.
> It will take a few hours.

There shouldn't be any problems that didn't surface in my cross build, 
since they are not execution tests, but compile tests.

I have committed the patch.  If there any specific failures specific to 
your target, let me know and I'll address those separately.

Thanks.
Aldy
Christophe Lyon Oct. 19, 2016, 8:16 p.m. UTC | #6
On 19 October 2016 at 20:43, Aldy Hernandez <aldyh@redhat.com> wrote:
>
>>> OK for the trunk whenever you're ready.
>>>
>>> jeff
>>
>>
>> You are too fast for me :-)
>> I do not have the build trees for all the configurations ready for
>> manual testing...
>> So, I just merged the initial patch and the 2nd one, and started
>> a validation job to make sure the 2nd patch fixes all the regressions
>> observed earlier.
>> It will take a few hours.
>
>
> There shouldn't be any problems that didn't surface in my cross build, since
> they are not execution tests, but compile tests.
>
> I have committed the patch.  If there any specific failures specific to your
> target, let me know and I'll address those separately.
>

Validation results of the 2 patches combined are OK.

Thanks

> Thanks.
> Aldy

Patch
diff mbox

diff --git a/gcc/testsuite/gcc.dg/Walloca-1.c b/gcc/testsuite/gcc.dg/Walloca-1.c
index 34a20c3..32e4ab8 100644
--- a/gcc/testsuite/gcc.dg/Walloca-1.c
+++ b/gcc/testsuite/gcc.dg/Walloca-1.c
@@ -23,7 +23,8 @@  void foo1 (size_t len, size_t len2, size_t len3)
   char *s = alloca (123);
   useit (s);			// OK, constant argument to alloca
 
-  s = alloca (num);		// { dg-warning "large due to conversion" }
+  s = alloca (num);		// { dg-warning "large due to conversion" "" { target lp64 } }
+  // { dg-warning "unbounded use of 'alloca'" "" { target { ! lp64 } } 26 }
   useit (s);
 
   s = alloca(90000);		/* { dg-warning "is too large" } */
diff --git a/gcc/testsuite/gcc.dg/Walloca-2.c b/gcc/testsuite/gcc.dg/Walloca-2.c
index 284b34e..4695fda 100644
--- a/gcc/testsuite/gcc.dg/Walloca-2.c
+++ b/gcc/testsuite/gcc.dg/Walloca-2.c
@@ -8,7 +8,11 @@  g1 (int n)
 {
   void *p;
   if (n > 0 && n < 2000)
-    p = __builtin_alloca (n);
+    // FIXME: This is a bogus warning, and is currently happening on
+    // 32-bit targets because VRP is not giving us any range info for
+    // the argument to __builtin_alloca.  This should be fixed by the
+    // upcoming range work.
+    p = __builtin_alloca (n); // { dg-bogus "unbounded use of 'alloca'" "" { xfail { ! lp64 } } }
   else
     p = __builtin_malloc (n);
   f (p);
@@ -31,8 +35,9 @@  g3 (int n)
   void *p;
   if (n > 0 && n < 3000)
     {
-      p = __builtin_alloca (n); // { dg-warning "'alloca' may be too large" }
-      // { dg-message "note:.*argument may be as large as 2999" "note" { target *-*-* } 34 }
+      p = __builtin_alloca (n); // { dg-warning "'alloca' may be too large" "" { target lp64} }
+      // { dg-message "note:.*argument may be as large as 2999" "note" { target lp64 } 38 }
+      // { dg-warning "unbounded use of 'alloca'" "" { target { ! lp64 } } 38 }
     }
   else
     p = __builtin_malloc (n);