Do not warn with warn_unused_result for alloca(0).

Message ID b1cd7377-e3f7-02b6-2923-715108e71c52@suse.cz
State New
Headers show
Series
  • Do not warn with warn_unused_result for alloca(0).
Related show

Commit Message

Martin Liška June 12, 2019, 11:11 a.m.
On 6/11/19 6:03 PM, Jakub Jelinek wrote:
> On Tue, Jun 11, 2019 at 03:58:27PM +0000, Michael Matz wrote:

>> On Tue, 11 Jun 2019, Martin Liška wrote:

>>

>>> I see 3 occurrences of the alloca (0) in libiberty/regex.c, but there are properly

>>> guarded within:

>>>

>>> # ifdef C_ALLOCA

>>>       alloca (0);

>>> # endif

>>>

>>> and then I noticed 2 more occurrences in gdb that break build right now:

>>>

>>> gdb/regcache.c:  alloca (0);

>>> gdb/top.c:  alloca (0);

>>>

>>> Is it the right approach to remove these 2 in gdb?

>>

>> It's more an indication that the annotation requesting the warning for 

>> unused results is simply overeager (aka wrong) for alloca.  (sure, the 

>> uses in gdb probably could be cleaned up as well, but that doesn't affect 

>> the wrongness of the warning).

> 

> Yeah.  Either we special-case alloca in the warn_unused_result code

> - if the call flags include ECF_MAY_BE_ALLOCA and argument is 0, don't warn,

> or don't add the attribute to alloca, or add yet another attribute that will

> be used for alloca only.

> 

> 	Jakub

> 


Ok, I've got a patch for it.

Patch can bootstrap on x86_64-linux-gnu and survives regression tests.

Ready to be installed?
Thanks,
Martin

Comments

Jakub Jelinek June 12, 2019, 11:22 a.m. | #1
On Wed, Jun 12, 2019 at 01:11:09PM +0200, Martin Liška wrote:
> 2019-06-12  Martin Liska  <mliska@suse.cz>

> 

> 	* calls.c (special_function_p): Make it global.

> 	* calls.h (special_function_p): Declare.


Why?

> 	* tree-cfg.c (do_warn_unused_result): Do not

> 	warn for alloca(0).

> --- a/gcc/tree-cfg.c

> +++ b/gcc/tree-cfg.c

> @@ -63,6 +63,7 @@ along with GCC; see the file COPYING3.  If not see

>  #include "opts.h"

>  #include "asan.h"

>  #include "profile.h"

> +#include "calls.h"

>  

>  /* This file contains functions for building the Control Flow Graph (CFG)

>     for a function tree.  */

> @@ -9447,10 +9448,17 @@ do_warn_unused_result (gimple_seq seq)

>  	      location_t loc = gimple_location (g);

>  

>  	      if (fdecl)

> -		warning_at (loc, OPT_Wunused_result,

> -			    "ignoring return value of %qD "

> -			    "declared with attribute %<warn_unused_result%>",

> -			    fdecl);

> +		{

> +		  if ((special_function_p (fdecl, 0) & ECF_MAY_BE_ALLOCA)


Why not instead gimple_maybe_alloca_call_p (g) ?
On the other side, you want && gimple_call_num_args (g) == 1,
if some alloca call had wrong declaration, you might ICE otherwise.

> +		      && TREE_CODE (gimple_call_arg (g, 0)) == INTEGER_CST

> +		      && wi::to_wide (gimple_call_arg (g, 0)) == 0)


&& integer_zerop (gimple_call_arg (g, 0))

instead?

Plus you need a comment explaining why we don't warn about alloca with
constant 0 argument.

	Jakub
Martin Liška June 12, 2019, 11:30 a.m. | #2
On 6/12/19 1:22 PM, Jakub Jelinek wrote:
> On Wed, Jun 12, 2019 at 01:11:09PM +0200, Martin Liška wrote:

>> 2019-06-12  Martin Liska  <mliska@suse.cz>

>>

>> 	* calls.c (special_function_p): Make it global.

>> 	* calls.h (special_function_p): Declare.

> 

> Why?


Not needed any longer.

> 

>> 	* tree-cfg.c (do_warn_unused_result): Do not

>> 	warn for alloca(0).

>> --- a/gcc/tree-cfg.c

>> +++ b/gcc/tree-cfg.c

>> @@ -63,6 +63,7 @@ along with GCC; see the file COPYING3.  If not see

>>  #include "opts.h"

>>  #include "asan.h"

>>  #include "profile.h"

>> +#include "calls.h"

>>  

>>  /* This file contains functions for building the Control Flow Graph (CFG)

>>     for a function tree.  */

>> @@ -9447,10 +9448,17 @@ do_warn_unused_result (gimple_seq seq)

>>  	      location_t loc = gimple_location (g);

>>  

>>  	      if (fdecl)

>> -		warning_at (loc, OPT_Wunused_result,

>> -			    "ignoring return value of %qD "

>> -			    "declared with attribute %<warn_unused_result%>",

>> -			    fdecl);

>> +		{

>> +		  if ((special_function_p (fdecl, 0) & ECF_MAY_BE_ALLOCA)

> 

> Why not instead gimple_maybe_alloca_call_p (g) ?


Because I was not aware of the function ;)

> On the other side, you want && gimple_call_num_args (g) == 1,


Sure.

> if some alloca call had wrong declaration, you might ICE otherwise.

> 

>> +		      && TREE_CODE (gimple_call_arg (g, 0)) == INTEGER_CST

>> +		      && wi::to_wide (gimple_call_arg (g, 0)) == 0)

> 

> && integer_zerop (gimple_call_arg (g, 0))

> 

> instead?


Yep.

> 

> Plus you need a comment explaining why we don't warn about alloca with

> constant 0 argument.


Also done.

Is it fine now?
Martin

> 

> 	Jakub

>
From dfdb688206cadd7fb9450ba005e8aa465ae61f7e Mon Sep 17 00:00:00 2001
From: Martin Liska <mliska@suse.cz>

Date: Wed, 12 Jun 2019 12:22:36 +0200
Subject: [PATCH] Do not warn with warn_unused_result for alloca(0).

gcc/ChangeLog:

2019-06-12  Martin Liska  <mliska@suse.cz>

	* tree-cfg.c (do_warn_unused_result): Do not
	warn for alloca(0) as it's used by some C
	libraries to release previously allocated memory
	via alloca calls.

gcc/testsuite/ChangeLog:

2019-06-12  Martin Liska  <mliska@suse.cz>

	* gcc.dg/pr78902.c: Add testing of alloca(0).
	* gcc.dg/attr-alloc_size-5.c: Do not warn
	about alloca(0).
---
 gcc/testsuite/gcc.dg/attr-alloc_size-5.c |  2 +-
 gcc/testsuite/gcc.dg/pr78902.c           |  1 +
 gcc/tree-cfg.c                           | 18 ++++++++++++++----
 3 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/gcc/testsuite/gcc.dg/attr-alloc_size-5.c b/gcc/testsuite/gcc.dg/attr-alloc_size-5.c
index 7aa7cbf0c72..26ee43f87de 100644
--- a/gcc/testsuite/gcc.dg/attr-alloc_size-5.c
+++ b/gcc/testsuite/gcc.dg/attr-alloc_size-5.c
@@ -230,5 +230,5 @@ test_alloca (size_t n)
 {
   extern void* alloca (size_t);
 
-  alloca (0); /* { dg-warning "ignoring return value of '.*' declared with attribute 'warn_unused_result'" } */
+  alloca (0);
 }
diff --git a/gcc/testsuite/gcc.dg/pr78902.c b/gcc/testsuite/gcc.dg/pr78902.c
index 49efc970475..f0f4314d684 100644
--- a/gcc/testsuite/gcc.dg/pr78902.c
+++ b/gcc/testsuite/gcc.dg/pr78902.c
@@ -7,6 +7,7 @@ void foo(void)
  __builtin_malloc (1); /* { dg-warning "ignoring return value of '__builtin_malloc' declared with attribute 'warn_unused_result'" } */
  __builtin_calloc (10, 20); /* { dg-warning "ignoring return value of '__builtin_calloc' declared with attribute 'warn_unused_result'" } */
  __builtin_alloca (10); /* { dg-warning "ignoring return value of '__builtin_alloca' declared with attribute 'warn_unused_result'" } */
+ __builtin_alloca (0);
  __builtin_realloc (ptr, 100); /* { dg-warning "ignoring return value of '__builtin_realloc' declared with attribute 'warn_unused_result'" } */
  __builtin_aligned_alloc (10, 16); /* { dg-warning "ignoring return value of '__builtin_aligned_alloc' declared with attribute 'warn_unused_result'" } */
  __builtin_strdup ("pes"); /* { dg-warning "ignoring return value of '__builtin_strdup' declared with attribute 'warn_unused_result'" } */
diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
index a585efea3d8..0d21f3624d7 100644
--- a/gcc/tree-cfg.c
+++ b/gcc/tree-cfg.c
@@ -63,6 +63,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "opts.h"
 #include "asan.h"
 #include "profile.h"
+#include "calls.h"
 
 /* This file contains functions for building the Control Flow Graph (CFG)
    for a function tree.  */
@@ -9447,10 +9448,19 @@ do_warn_unused_result (gimple_seq seq)
 	      location_t loc = gimple_location (g);
 
 	      if (fdecl)
-		warning_at (loc, OPT_Wunused_result,
-			    "ignoring return value of %qD "
-			    "declared with attribute %<warn_unused_result%>",
-			    fdecl);
+		{
+		  /* Some C libraries use alloca(0) in order to free previously
+		     allocated memory by alloca calls.  */
+		  if (gimple_maybe_alloca_call_p (g)
+		      && gimple_call_num_args (g) == 1
+		      && integer_zerop (gimple_call_arg (g, 0)))
+		    ;
+		  else
+		    warning_at (loc, OPT_Wunused_result,
+				"ignoring return value of %qD declared "
+				"with attribute %<warn_unused_result%>",
+				fdecl);
+		}
 	      else
 		warning_at (loc, OPT_Wunused_result,
 			    "ignoring return value of function "
-- 
2.21.0
Jakub Jelinek June 12, 2019, 11:37 a.m. | #3
On Wed, Jun 12, 2019 at 01:30:14PM +0200, Martin Liška wrote:
> @@ -9447,10 +9448,19 @@ do_warn_unused_result (gimple_seq seq)

>  	      location_t loc = gimple_location (g);

>  

>  	      if (fdecl)

> -		warning_at (loc, OPT_Wunused_result,

> -			    "ignoring return value of %qD "

> -			    "declared with attribute %<warn_unused_result%>",

> -			    fdecl);

> +		{

> +		  /* Some C libraries use alloca(0) in order to free previously

> +		     allocated memory by alloca calls.  */

> +		  if (gimple_maybe_alloca_call_p (g)

> +		      && gimple_call_num_args (g) == 1

> +		      && integer_zerop (gimple_call_arg (g, 0)))

> +		    ;

> +		  else


Wouldn't it be easier to negate the condition and avoid the weird ; else ?
I.e. if (!gimple_maybe... || gimple_call_num != 1 || !integer_zerop?

> +		    warning_at (loc, OPT_Wunused_result,

> +				"ignoring return value of %qD declared "

> +				"with attribute %<warn_unused_result%>",

> +				fdecl);

> +		}

>  	      else

>  		warning_at (loc, OPT_Wunused_result,

>  			    "ignoring return value of function "


Otherwise LGTM as the patch, but I'd like to hear from others whether
it is kosher to add such a special case to the warn_unused_result attribute
warning.  And if the agreement is yes, I think it should be documented
somewhere that alloca (0) will not warn even when the call has such an
attribute (probably in the description of warn_unused_result attribute).

	Jakub
Martin Sebor June 12, 2019, 2:59 p.m. | #4
On 6/12/19 5:37 AM, Jakub Jelinek wrote:
> On Wed, Jun 12, 2019 at 01:30:14PM +0200, Martin Liška wrote:

>> @@ -9447,10 +9448,19 @@ do_warn_unused_result (gimple_seq seq)

>>   	      location_t loc = gimple_location (g);

>>   

>>   	      if (fdecl)

>> -		warning_at (loc, OPT_Wunused_result,

>> -			    "ignoring return value of %qD "

>> -			    "declared with attribute %<warn_unused_result%>",

>> -			    fdecl);

>> +		{

>> +		  /* Some C libraries use alloca(0) in order to free previously

>> +		     allocated memory by alloca calls.  */

>> +		  if (gimple_maybe_alloca_call_p (g)

>> +		      && gimple_call_num_args (g) == 1

>> +		      && integer_zerop (gimple_call_arg (g, 0)))

>> +		    ;

>> +		  else

> 

> Wouldn't it be easier to negate the condition and avoid the weird ; else ?

> I.e. if (!gimple_maybe... || gimple_call_num != 1 || !integer_zerop?

> 

>> +		    warning_at (loc, OPT_Wunused_result,

>> +				"ignoring return value of %qD declared "

>> +				"with attribute %<warn_unused_result%>",

>> +				fdecl);

>> +		}

>>   	      else

>>   		warning_at (loc, OPT_Wunused_result,

>>   			    "ignoring return value of function "

> 

> Otherwise LGTM as the patch, but I'd like to hear from others whether

> it is kosher to add such a special case to the warn_unused_result attribute

> warning.  And if the agreement is yes, I think it should be documented

> somewhere that alloca (0) will not warn even when the call has such an

> attribute (probably in the description of warn_unused_result attribute).


I'm not very happy about adding another special case to alloca
(on top of not diagnosing zero allocation by -Walloc-zero).
There is no valid use case for the zero argument, whether or not
the return value is used.  When it is used it's just as dangerous
as allocating a zero-length VLA.  When it isn't used the call is
pointless entirely, and although it might be harmless, it's worth
removing from code just like any other pointless construct GCC
warns about.  So I would prefer not to suppress this warning (from
what I've read in the GDB bug it sounds like its calls to alloca(0)
are being removed).  I would also prefer to re-enable -Walloc-zero
for alloca(0).

But if there is insufficient support for this I agree that
documenting the built-ins GCC applies attribute warn_unused_result
to is a good idea.  (In fact, documenting all the attributes GCC
applies to each built-in would be helpful, but that's a much bigger
project.)

Martin
Michael Matz June 12, 2019, 3:25 p.m. | #5
Hi,

On Wed, 12 Jun 2019, Martin Sebor wrote:

> > Otherwise LGTM as the patch, but I'd like to hear from others whether 

> > it is kosher to add such a special case to the warn_unused_result 

> > attribute warning.  And if the agreement is yes, I think it should be 

> > documented somewhere that alloca (0) will not warn even when the call 

> > has such an attribute (probably in the description of 

> > warn_unused_result attribute).

> 

> I'm not very happy about adding another special case to alloca

> (on top of not diagnosing zero allocation by -Walloc-zero).

> There is no valid use case for the zero argument, whether or not

> the return value is used.


That's the thing, there _is_ a valid use case for supplying a zero 
argument and then the returned value should _not_ be used.  There are 
alloca implementations that do something (freeing memory) when 
called with a zero size, so some (older) programs contain such calls.  
Warning on those calls for the unused results is exactly the wrong thing 
to do, if anything if the result is used we'd have to warn.  (That's of 
course non-standard, but so is alloca itself)  And just removing these 
calls isn't correct either except if it's ensured to not use an alloca 
implementation with that behaviour.

(In fact I think our builtin_alloca implementation could benefit when we 
added that behaviour as well; it's a natural wish to be able to free 
memory that you allocated).


Ciao,
Michael.
Martin Sebor June 12, 2019, 4:13 p.m. | #6
On 6/12/19 9:25 AM, Michael Matz wrote:
> Hi,

> 

> On Wed, 12 Jun 2019, Martin Sebor wrote:

> 

>>> Otherwise LGTM as the patch, but I'd like to hear from others whether

>>> it is kosher to add such a special case to the warn_unused_result

>>> attribute warning.  And if the agreement is yes, I think it should be

>>> documented somewhere that alloca (0) will not warn even when the call

>>> has such an attribute (probably in the description of

>>> warn_unused_result attribute).

>>

>> I'm not very happy about adding another special case to alloca

>> (on top of not diagnosing zero allocation by -Walloc-zero).

>> There is no valid use case for the zero argument, whether or not

>> the return value is used.

> 

> That's the thing, there _is_ a valid use case for supplying a zero

> argument and then the returned value should _not_ be used.  There are

> alloca implementations that do something (freeing memory) when

> called with a zero size, so some (older) programs contain such calls.

> Warning on those calls for the unused results is exactly the wrong thing

> to do, if anything if the result is used we'd have to warn.  (That's of

> course non-standard, but so is alloca itself)  And just removing these

> calls isn't correct either except if it's ensured to not use an alloca

> implementation with that behaviour.


But GCC doesn't support such an implementation, does it?  The only
way to use such an alloca is with -fno-builtin-alloca which should
suppress the warning.

The Linux man page highlights this and the risks of defining one's
own alloca function:

   http://man7.org/linux/man-pages/man3/alloca.3.html

In any event, the warning, just like all others, exists to help
catch common mistakes: "constructions that are not inherently
erroneous but that are risky or suggest there may have been
an error".  It's not meant to accommodate every conceivable
corner case or oddball implementation.  Users of those can
easily disable the warning #pragma GCC diagnostic.  Doing that
makes the intent explicit both to the compiler and to other
tools and programmers.

Martin

> 

> (In fact I think our builtin_alloca implementation could benefit when we

> added that behaviour as well; it's a natural wish to be able to free

> memory that you allocated).

> 

> 

> Ciao,

> Michael.

>
Jakub Jelinek June 12, 2019, 4:40 p.m. | #7
On Wed, Jun 12, 2019 at 10:13:57AM -0600, Martin Sebor wrote:
> But GCC doesn't support such an implementation, does it?


Why would that be relevant?  The warning would cause people to make portable
code less portable (by removing the alloca (0) calls that were added there
for portability reasons), or add hacks to work around the warning (whether
#pragma GCC diagnostic or something else).  That is something we do not
want people to do.

	Jakub
Martin Sebor June 12, 2019, 5:26 p.m. | #8
On 6/12/19 10:40 AM, Jakub Jelinek wrote:
> On Wed, Jun 12, 2019 at 10:13:57AM -0600, Martin Sebor wrote:

>> But GCC doesn't support such an implementation, does it?

> 

> Why would that be relevant?


Obviously because it makes no sense to cater to all conceivable
extensions provided by all sorts of implementations out there.

There are libc implementations out there that accept null pointer
arguments to functions like memcpy with zero sizes, for example.
Or those that accept overlapping objects in calls to strcpy.  GCC
itself handles those gracefully, yet warns for such constructs
nonetheless.  It's useful because those misuses are likely hidden
bugs, even if they don't always manifest themselves.

> The warning would cause people to make portable

> code less portable (by removing the alloca (0) calls that were added there

> for portability reasons), or add hacks to work around the warning (whether

> #pragma GCC diagnostic or something else).  That is something we do not

> want people to do.


You asked for input and I gave it to you.  If your mind was already
made up and you're only willing to accept feedback that agrees with
your view, don't ask.

Martin
Jeff Law June 13, 2019, 3:31 p.m. | #9
On 6/12/19 9:25 AM, Michael Matz wrote:
> Hi,

> 

> On Wed, 12 Jun 2019, Martin Sebor wrote:

> 

>>> Otherwise LGTM as the patch, but I'd like to hear from others whether 

>>> it is kosher to add such a special case to the warn_unused_result 

>>> attribute warning.  And if the agreement is yes, I think it should be 

>>> documented somewhere that alloca (0) will not warn even when the call 

>>> has such an attribute (probably in the description of 

>>> warn_unused_result attribute).

>>

>> I'm not very happy about adding another special case to alloca

>> (on top of not diagnosing zero allocation by -Walloc-zero).

>> There is no valid use case for the zero argument, whether or not

>> the return value is used.

> 

> That's the thing, there _is_ a valid use case for supplying a zero 

> argument and then the returned value should _not_ be used.  There are 

> alloca implementations that do something (freeing memory) when 

> called with a zero size, so some (older) programs contain such calls.  

> Warning on those calls for the unused results is exactly the wrong thing 

> to do, if anything if the result is used we'd have to warn.  (That's of 

> course non-standard, but so is alloca itself)  And just removing these 

> calls isn't correct either except if it's ensured to not use an alloca 

> implementation with that behaviour.

Agreed.  Removing those calls simply can't be right unless you're
dropping support for the old C-alloca implementations completely.

> 

> (In fact I think our builtin_alloca implementation could benefit when we 

> added that behaviour as well; it's a natural wish to be able to free 

> memory that you allocated).

But do we really want to cater to alloca more than we already are?  I'd
argue that programmers simply aren't capable of using alloca
appropriately :-)  That's based on seeing mis-uses exploited repeatedly
through the years.

Also note that simply sprinkling alloca(0) calls won't magically release
memory.  In a stack implementation releasing happens when the frame is
removed.  Doing something more complex than that seems unwise.

In a C implementation, allocations (or groups of allocations) carry
additional information -- specifically the stack pointer at the time the
object was allocated.   Deallocation of the areas only occurs at
subsequent calls to alloca when the current stack pointer is larger than
the saved stack pointer (or if you're on a PA, the opposite :-)

So something like this

for (...)  {
    alloca (space);
}
alloca (0);

Won't release anything because the saved stack pointer for the
allocations in the loop is the same as the stack pointer at the point of
the alloca(0) call.

Where alloca(0) is useful is in something like this:

for (...) {
   somefunc ();
}

Where somefunc allocates space with alloca.  If you think about the
implementation details here, you'll realize that calls to alloca from
within somefunc called in this loop will all have the same stack
pointer.  Thus garbage will keep accumulating on the stack across
iterations of the loop.  To avoid that you put an alloca(0) in the loop
like this:

for (...) {
  somefunc ();
  alloca (0);
}

Now when we call alloca (0) the stack pointer will indicate that the
alloca calls that occurred within somefunc are all dead and thus the
alloca(0) will release memory.

Jeff
Jeff Law June 13, 2019, 3:49 p.m. | #10
On 6/12/19 10:40 AM, Jakub Jelinek wrote:
> On Wed, Jun 12, 2019 at 10:13:57AM -0600, Martin Sebor wrote:

>> But GCC doesn't support such an implementation, does it?

> 

> Why would that be relevant?  The warning would cause people to make portable

> code less portable (by removing the alloca (0) calls that were added there

> for portability reasons), or add hacks to work around the warning (whether

> #pragma GCC diagnostic or something else).  That is something we do not

> want people to do.

I'd like to move C-alloca support to the ash heap of history.  But I'm
not sure we can realistically do that.  Given that reality I think we
need to honor the intent of alloca(0).

I also understand Martin's point that not warning on it could easily
miss programming errors.

I wonder if we could set TREE_NOWARNING on the CALL_EXPR early in the
front-end of the argument is a constant 0 (before simplification,
folding, etc).

That way we'd be able to support alloca(0), but also capture cases where
0 flows into alloca via other mechansisms (which are likely programming
errors).

Jeff
Michael Matz June 13, 2019, 3:59 p.m. | #11
Hi,

On Thu, 13 Jun 2019, Jeff Law wrote:

> > (In fact I think our builtin_alloca implementation could benefit when we 

> > added that behaviour as well; it's a natural wish to be able to free 

> > memory that you allocated).

> 

> Also note that simply sprinkling alloca(0) calls won't magically release

> memory.  In a stack implementation releasing happens when the frame is

> removed.  Doing something more complex than that seems unwise.


Yeah, on reconsideration I think I'm pedaling back on this suggestion 
(which really was to do something more complex).  We have the necessary 
support for this in GCC (for VLAs), and then we'd be able to support code 
like this:

  for () {
    dostuff (alloca (size));
    morestuff (alloca (size2));
    alloca(0);   // free all allocas
  }

without running the risk of unlimited stack use.  But of course this would 
promote a programming style that'd only work with our alloca (and not even 
C-alloca), and we want to avoid that.  I thought it a cute idea, but was 
carried away by the cuteness ;-)

I like the suggestion of setting (and carrying through the pipeline) the 
TREE_NO_WARNING flag on an source 'alloca(0)'.


Ciao,
Michael.
Jakub Jelinek June 13, 2019, 4:02 p.m. | #12
On Thu, Jun 13, 2019 at 03:59:33PM +0000, Michael Matz wrote:
> without running the risk of unlimited stack use.  But of course this would 

> promote a programming style that'd only work with our alloca (and not even 

> C-alloca), and we want to avoid that.  I thought it a cute idea, but was 

> carried away by the cuteness ;-)

> 

> I like the suggestion of setting (and carrying through the pipeline) the 

> TREE_NO_WARNING flag on an source 'alloca(0)'.


Ditto, though it would be nice one day to implement
TREE_NO_WARNING/gimple_no_warning_p as a bit indicating on the side
sets of disabled warnings rather than the current big hammer where
that bit disables all warnings.

	Jakub
Tom Tromey June 13, 2019, 4:46 p.m. | #13
>>>>> "Jeff" == Jeff Law <law@redhat.com> writes:


Jeff> I'd like to move C-alloca support to the ash heap of history.  But I'm
Jeff> not sure we can realistically do that.

Are there still platforms or compilers in use where it's needed?

For gdb I was planning to just remove these calls.

Tom
Martin Sebor June 13, 2019, 7:37 p.m. | #14
On 6/13/19 10:46 AM, Tom Tromey wrote:
>>>>>> "Jeff" == Jeff Law <law@redhat.com> writes:

> 

> Jeff> I'd like to move C-alloca support to the ash heap of history.  But I'm

> Jeff> not sure we can realistically do that.

> 

> Are there still platforms or compilers in use where it's needed?

> 

> For gdb I was planning to just remove these calls.


The only implementation I know about is Doug Gwyn's alloca.
A Google search returns a pointer to QNX CAR that documents it
with the following note:

  *  By default, alloca() is implemented as __builtin_alloca().
     If you compile with the -fno-builtin option, you'll use
     the libc version of alloca(), which is a cover for malloc()
     and behaves in a slightly different manner:
     -  It keeps track of all blocks allocated with alloca() and
        reclaims any that are found to be deeper in the stack than
        the current invocation. It therefore doesn't reclaim storage
        as soon as it becomes invalid, but it will do so eventually.
     -  As a special case, alloca(0) reclaims storage without
        allocating any. You can use this feature to force garbage
        collection.

The implementation details aren't important.  What matters is
that to call the library implementation one has to prevent GCC
from treating alloca as a built-in and eliminating the zero size
calls.

When the function is not treated as a built-in no warnings for
calls to it are issued and so no problem for us to solve.

Martin

http://www.qnx.com/developers/docs/qnxcar2/index.jsp?topic=%2Fcom.qnx.doc.neutrino.lib_ref%2Ftopic%2Fa%2Falloca.html
Jeff Law June 14, 2019, 1:33 a.m. | #15
On 6/13/19 10:46 AM, Tom Tromey wrote:
>>>>>> "Jeff" == Jeff Law <law@redhat.com> writes:

> 

> Jeff> I'd like to move C-alloca support to the ash heap of history.  But I'm

> Jeff> not sure we can realistically do that.

> 

> Are there still platforms or compilers in use where it's needed?

> 

> For gdb I was planning to just remove these calls.

Probably not that we care about for building gcc, gdb & friends.  I'd be
more hesitant to declare C-alloca dead in the more general sense.

jeff

Patch

From b659c00e54ff3bee736f502e7fa4dc233a814b66 Mon Sep 17 00:00:00 2001
From: Martin Liska <mliska@suse.cz>
Date: Wed, 12 Jun 2019 12:22:36 +0200
Subject: [PATCH] Do not warn with warn_unused_result for alloca(0).

gcc/ChangeLog:

2019-06-12  Martin Liska  <mliska@suse.cz>

	* calls.c (special_function_p): Make it global.
	* calls.h (special_function_p): Declare.
	* tree-cfg.c (do_warn_unused_result): Do not
	warn for alloca(0).

gcc/testsuite/ChangeLog:

2019-06-12  Martin Liska  <mliska@suse.cz>

	* gcc.dg/pr78902.c: Add testing of alloca(0).
	* gcc.dg/attr-alloc_size-5.c: Do not warn
	about alloca(0).
---
 gcc/calls.c                              |  3 +--
 gcc/calls.h                              |  1 +
 gcc/testsuite/gcc.dg/attr-alloc_size-5.c |  2 +-
 gcc/testsuite/gcc.dg/pr78902.c           |  1 +
 gcc/tree-cfg.c                           | 16 ++++++++++++----
 5 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/gcc/calls.c b/gcc/calls.c
index c8a42680041..be9b2ff046a 100644
--- a/gcc/calls.c
+++ b/gcc/calls.c
@@ -153,7 +153,6 @@  static void compute_argument_addresses (struct arg_data *, rtx, int);
 static rtx rtx_for_function_call (tree, tree);
 static void load_register_parameters (struct arg_data *, int, rtx *, int,
 				      int, int *);
-static int special_function_p (const_tree, int);
 static int check_sibcall_argument_overlap_1 (rtx);
 static int check_sibcall_argument_overlap (rtx_insn *, struct arg_data *, int);
 
@@ -578,7 +577,7 @@  emit_call_1 (rtx funexp, tree fntree ATTRIBUTE_UNUSED, tree fndecl ATTRIBUTE_UNU
    Set ECF_MAY_BE_ALLOCA for any memory allocation function that might allocate
    space from the stack such as alloca.  */
 
-static int
+int
 special_function_p (const_tree fndecl, int flags)
 {
   tree name_decl = DECL_NAME (fndecl);
diff --git a/gcc/calls.h b/gcc/calls.h
index 128bb513074..0d2bc888b26 100644
--- a/gcc/calls.h
+++ b/gcc/calls.h
@@ -42,5 +42,6 @@  extern tree get_attr_nonstring_decl (tree, tree * = NULL);
 extern void maybe_warn_nonstring_arg (tree, tree);
 extern bool get_size_range (tree, tree[2], bool = false);
 extern rtx rtx_for_static_chain (const_tree, bool);
+extern int special_function_p (const_tree, int);
 
 #endif // GCC_CALLS_H
diff --git a/gcc/testsuite/gcc.dg/attr-alloc_size-5.c b/gcc/testsuite/gcc.dg/attr-alloc_size-5.c
index 7aa7cbf0c72..26ee43f87de 100644
--- a/gcc/testsuite/gcc.dg/attr-alloc_size-5.c
+++ b/gcc/testsuite/gcc.dg/attr-alloc_size-5.c
@@ -230,5 +230,5 @@  test_alloca (size_t n)
 {
   extern void* alloca (size_t);
 
-  alloca (0); /* { dg-warning "ignoring return value of '.*' declared with attribute 'warn_unused_result'" } */
+  alloca (0);
 }
diff --git a/gcc/testsuite/gcc.dg/pr78902.c b/gcc/testsuite/gcc.dg/pr78902.c
index 49efc970475..f0f4314d684 100644
--- a/gcc/testsuite/gcc.dg/pr78902.c
+++ b/gcc/testsuite/gcc.dg/pr78902.c
@@ -7,6 +7,7 @@  void foo(void)
  __builtin_malloc (1); /* { dg-warning "ignoring return value of '__builtin_malloc' declared with attribute 'warn_unused_result'" } */
  __builtin_calloc (10, 20); /* { dg-warning "ignoring return value of '__builtin_calloc' declared with attribute 'warn_unused_result'" } */
  __builtin_alloca (10); /* { dg-warning "ignoring return value of '__builtin_alloca' declared with attribute 'warn_unused_result'" } */
+ __builtin_alloca (0);
  __builtin_realloc (ptr, 100); /* { dg-warning "ignoring return value of '__builtin_realloc' declared with attribute 'warn_unused_result'" } */
  __builtin_aligned_alloc (10, 16); /* { dg-warning "ignoring return value of '__builtin_aligned_alloc' declared with attribute 'warn_unused_result'" } */
  __builtin_strdup ("pes"); /* { dg-warning "ignoring return value of '__builtin_strdup' declared with attribute 'warn_unused_result'" } */
diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
index a585efea3d8..9419de8a756 100644
--- a/gcc/tree-cfg.c
+++ b/gcc/tree-cfg.c
@@ -63,6 +63,7 @@  along with GCC; see the file COPYING3.  If not see
 #include "opts.h"
 #include "asan.h"
 #include "profile.h"
+#include "calls.h"
 
 /* This file contains functions for building the Control Flow Graph (CFG)
    for a function tree.  */
@@ -9447,10 +9448,17 @@  do_warn_unused_result (gimple_seq seq)
 	      location_t loc = gimple_location (g);
 
 	      if (fdecl)
-		warning_at (loc, OPT_Wunused_result,
-			    "ignoring return value of %qD "
-			    "declared with attribute %<warn_unused_result%>",
-			    fdecl);
+		{
+		  if ((special_function_p (fdecl, 0) & ECF_MAY_BE_ALLOCA)
+		      && TREE_CODE (gimple_call_arg (g, 0)) == INTEGER_CST
+		      && wi::to_wide (gimple_call_arg (g, 0)) == 0)
+		    ;
+		  else
+		    warning_at (loc, OPT_Wunused_result,
+				"ignoring return value of %qD declared "
+				"with attribute %<warn_unused_result%>",
+				fdecl);
+		}
 	      else
 		warning_at (loc, OPT_Wunused_result,
 			    "ignoring return value of function "
-- 
2.21.0