| Message ID | b1cd7377-e3f7-02b6-2923-715108e71c52@suse.cz |
|---|---|
| State | New |
| Headers | show |
| Series | Do not warn with warn_unused_result for alloca(0). | expand |
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
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 >
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
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
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.
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. >
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
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
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
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
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.
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
>>>>> "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
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
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
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