Re: Warn when returning the address of a temporary (middle-end) v2
On Tue, 29 Jul 2014, David Malcolm wrote:
On Tue, 2014-07-29 at 19:36 +0200, Marc Glisse wrote:
On Sun, 27 Jul 2014, Richard Sandiford wrote:
Marc Glisse marc.gli...@inria.fr writes:
Hello,
I followed the advice in this discussion:
https://gcc.gnu.org/ml/gcc-patches/2014-04/msg00269.html
and here is a new patch. I made an effort to isolate a path in at least
one subcase so it doesn't look too strange that the warning is in this
file. Computing the dominance info just to tweak the warning message may
be a bit excessive.
How about only calculating it once you've decided to issue a message?
+ if (always_executed)
+ msg = function returns address of local variable;
+ else
+ msg = function may return address of local variable;
I think you need _(...) here, unless some magic makes that unnecessary now.
Current version, which passed bootstrap+testsuite on x86_64-linux-gnu.
(the original discussion is at
https://gcc.gnu.org/ml/gcc-patches/2014-06/msg01692.html )
This is possibly a dumb question, but what happens for a static local,
rather than an auto local? e.g.
int *f (void)
{
static int i;
return i;
}
(e.g. should the test case cover this?)
New version with a static local variable in the testcase, and without
using build_compound_expr in the C front-end to avoid extra warnings.
Same ChangeLog, same testsuite results.
--
Marc GlisseIndex: gcc/c/c-typeck.c
===
--- gcc/c/c-typeck.c(revision 213251)
+++ gcc/c/c-typeck.c(working copy)
@@ -9330,22 +9330,26 @@ c_finish_return (location_t loc, tree re
if (DECL_P (inner)
!DECL_EXTERNAL (inner)
!TREE_STATIC (inner)
DECL_CONTEXT (inner) == current_function_decl)
{
if (TREE_CODE (inner) == LABEL_DECL)
warning_at (loc, OPT_Wreturn_local_addr,
function returns address of label);
else
- warning_at (loc, OPT_Wreturn_local_addr,
- function returns address of local variable);
+ {
+ warning_at (loc, OPT_Wreturn_local_addr,
+ function returns address of local variable);
+ tree zero = build_zero_cst (TREE_TYPE (res));
+ t = build2 (COMPOUND_EXPR, TREE_TYPE (res), t, zero);
+ }
}
break;
default:
break;
}
break;
}
Index: gcc/c-family/c.opt
===
--- gcc/c-family/c.opt (revision 213251)
+++ gcc/c-family/c.opt (working copy)
@@ -698,24 +698,20 @@ ObjC ObjC++ Var(warn_protocol) Init(1) W
Warn if inherited methods are unimplemented
Wredundant-decls
C ObjC C++ ObjC++ Var(warn_redundant_decls) Warning
Warn about multiple declarations of the same object
Wreorder
C++ ObjC++ Var(warn_reorder) Warning LangEnabledBy(C++ ObjC++,Wall)
Warn when the compiler reorders code
-Wreturn-local-addr
-C ObjC C++ ObjC++ Var(warn_return_local_addr) Init(1) Warning
-Warn about returning a pointer/reference to a local or temporary variable.
-
Wreturn-type
C ObjC C++ ObjC++ Var(warn_return_type) Warning LangEnabledBy(C ObjC C++
ObjC++,Wall)
Warn whenever a function's return type defaults to \int\ (C), or about
inconsistent return types (C++)
Wselector
ObjC ObjC++ Var(warn_selector) Warning
Warn if a selector has multiple methods
Wshadow-ivar
ObjC ObjC++ Var(warn_shadow_ivar) EnabledBy(Wshadow) Init(1) Warning
Index: gcc/common.opt
===
--- gcc/common.opt (revision 213251)
+++ gcc/common.opt (working copy)
@@ -600,20 +600,24 @@ Common Var(warn_packed) Warning
Warn when the packed attribute has no effect on struct layout
Wpadded
Common Var(warn_padded) Warning
Warn when padding is required to align structure members
Wpedantic
Common Var(pedantic) Warning
Issue warnings needed for strict compliance to the standard
+Wreturn-local-addr
+Common Var(warn_return_local_addr) Init(1) Warning
+Warn about returning a pointer/reference to a local or temporary variable.
+
Wshadow
Common Var(warn_shadow) Warning
Warn when one local variable shadows another
Wstack-protector
Common Var(warn_stack_protect) Warning
Warn when not issuing stack smashing protection for some reason
Wstack-usage=
Common Joined RejectNegative UInteger Var(warn_stack_usage) Init(-1) Warning
Index: gcc/cp/typeck.c
===
--- gcc/cp/typeck.c (revision 213251)
+++ gcc/cp/typeck.c (working copy)
@@ -49,21 +49,21 @@ static tree convert_for_assignment (tree
static tree
Re: Warn when returning the address of a temporary (middle-end) v2
On 07/22/14 03:03, Marc Glisse wrote: I note you don't catch return localvar in the isolation code -- it looks like you just catch those which potentially flow from PHIs. I thought I was handling it in the find_explicit_erroneous_behaviour part of the patch (as opposed to the implicit part which deals with PHIs). Function f1 in the testcase addrtmp.c has no PHI. Am I missing something? I must have missed that whole block of code. So, no, I don't think you missed anything. My bad. I'm going to choose to blame it on using a laptop screen on the road rather than my usual monitor at home. I realize you're primarily catching that in the front-ends, but can't we have cases which aren't caught by the front end, but after optimizations we're able to propagate somelocal into the return statement? We can, and it was my original motivation. I only added PHI handling when you asked for it. Note my comment/question makes no sense now that we've settled that you do have the right code in find_explicit_erroneous_behavior :-) It generally looks good and I'm ready to approve if the answer to the above question is can't happen. If it can happen, then we ought to handle it in the isolation code as well (ought to be relatively easy). Just to be clear, the approval would include the PARM_DECL tweak in https://gcc.gnu.org/ml/gcc-patches/2014-06/msg02327.html Yes. jeff
Re: Warn when returning the address of a temporary (middle-end) v2
On 07/30/14 05:54, Marc Glisse wrote:
On Tue, 29 Jul 2014, David Malcolm wrote:
On Tue, 2014-07-29 at 19:36 +0200, Marc Glisse wrote:
On Sun, 27 Jul 2014, Richard Sandiford wrote:
Marc Glisse marc.gli...@inria.fr writes:
Hello,
I followed the advice in this discussion:
https://gcc.gnu.org/ml/gcc-patches/2014-04/msg00269.html
and here is a new patch. I made an effort to isolate a path in at
least
one subcase so it doesn't look too strange that the warning is in this
file. Computing the dominance info just to tweak the warning
message may
be a bit excessive.
How about only calculating it once you've decided to issue a message?
+ if (always_executed)
+msg = function returns address of local variable;
+ else
+msg = function may return address of local variable;
I think you need _(...) here, unless some magic makes that
unnecessary now.
Current version, which passed bootstrap+testsuite on x86_64-linux-gnu.
(the original discussion is at
https://gcc.gnu.org/ml/gcc-patches/2014-06/msg01692.html )
This is possibly a dumb question, but what happens for a static local,
rather than an auto local? e.g.
int *f (void)
{
static int i;
return i;
}
(e.g. should the test case cover this?)
New version with a static local variable in the testcase, and without
using build_compound_expr in the C front-end to avoid extra warnings.
Same ChangeLog, same testsuite results.
Approved.
Jeff
Re: Warn when returning the address of a temporary (middle-end) v2
On Sun, 27 Jul 2014, Richard Sandiford wrote:
Marc Glisse marc.gli...@inria.fr writes:
Hello,
I followed the advice in this discussion:
https://gcc.gnu.org/ml/gcc-patches/2014-04/msg00269.html
and here is a new patch. I made an effort to isolate a path in at least
one subcase so it doesn't look too strange that the warning is in this
file. Computing the dominance info just to tweak the warning message may
be a bit excessive.
How about only calculating it once you've decided to issue a message?
+ if (always_executed)
+ msg = function returns address of local variable;
+ else
+ msg = function may return address of local variable;
I think you need _(...) here, unless some magic makes that unnecessary now.
Current version, which passed bootstrap+testsuite on x86_64-linux-gnu.
(the original discussion is at
https://gcc.gnu.org/ml/gcc-patches/2014-06/msg01692.html )
2014-07-30 Marc Glisse marc.gli...@inria.fr
PR c++/60517
gcc/c/
* c-typeck.c (c_finish_return): Return 0 instead of the address of
a local variable.
gcc/cp/
* typeck.c (maybe_warn_about_returning_address_of_local): Return
whether it is returning the address of a local variable.
(check_return_expr): Return 0 instead of the address of a local
variable.
gcc/c-family/
* c.opt (-Wreturn-local-addr): Move to common.opt.
gcc/
* common.opt (-Wreturn-local-addr): Moved from c.opt.
* gimple-ssa-isolate-paths.c: Include diagnostic-core.h and intl.h.
(isolate_path): New argument to avoid inserting a trap.
(find_implicit_erroneous_behaviour): Handle returning the address
of a local variable.
(find_explicit_erroneous_behaviour): Likewise.
gcc/testsuite/
* c-c++-common/addrtmp.c: New file.
* c-c++-common/uninit-G.c: Adapt.
--
Marc GlisseIndex: gcc/c/c-typeck.c
===
--- gcc/c/c-typeck.c(revision 213210)
+++ gcc/c/c-typeck.c(working copy)
@@ -9330,22 +9330,26 @@ c_finish_return (location_t loc, tree re
if (DECL_P (inner)
!DECL_EXTERNAL (inner)
!TREE_STATIC (inner)
DECL_CONTEXT (inner) == current_function_decl)
{
if (TREE_CODE (inner) == LABEL_DECL)
warning_at (loc, OPT_Wreturn_local_addr,
function returns address of label);
else
- warning_at (loc, OPT_Wreturn_local_addr,
- function returns address of local variable);
+ {
+ warning_at (loc, OPT_Wreturn_local_addr,
+ function returns address of local variable);
+ tree zero = build_zero_cst (TREE_TYPE (res));
+ t = build_compound_expr (loc, t, zero);
+ }
}
break;
default:
break;
}
break;
}
Index: gcc/c-family/c.opt
===
--- gcc/c-family/c.opt (revision 213210)
+++ gcc/c-family/c.opt (working copy)
@@ -698,24 +698,20 @@ ObjC ObjC++ Var(warn_protocol) Init(1) W
Warn if inherited methods are unimplemented
Wredundant-decls
C ObjC C++ ObjC++ Var(warn_redundant_decls) Warning
Warn about multiple declarations of the same object
Wreorder
C++ ObjC++ Var(warn_reorder) Warning LangEnabledBy(C++ ObjC++,Wall)
Warn when the compiler reorders code
-Wreturn-local-addr
-C ObjC C++ ObjC++ Var(warn_return_local_addr) Init(1) Warning
-Warn about returning a pointer/reference to a local or temporary variable.
-
Wreturn-type
C ObjC C++ ObjC++ Var(warn_return_type) Warning LangEnabledBy(C ObjC C++
ObjC++,Wall)
Warn whenever a function's return type defaults to \int\ (C), or about
inconsistent return types (C++)
Wselector
ObjC ObjC++ Var(warn_selector) Warning
Warn if a selector has multiple methods
Wshadow-ivar
ObjC ObjC++ Var(warn_shadow_ivar) EnabledBy(Wshadow) Init(1) Warning
Index: gcc/common.opt
===
--- gcc/common.opt (revision 213210)
+++ gcc/common.opt (working copy)
@@ -600,20 +600,24 @@ Common Var(warn_packed) Warning
Warn when the packed attribute has no effect on struct layout
Wpadded
Common Var(warn_padded) Warning
Warn when padding is required to align structure members
Wpedantic
Common Var(pedantic) Warning
Issue warnings needed for strict compliance to the standard
+Wreturn-local-addr
+Common Var(warn_return_local_addr) Init(1) Warning
+Warn about returning a pointer/reference to a local or temporary variable.
+
Wshadow
Common Var(warn_shadow) Warning
Warn when one local variable shadows another
Re: Warn when returning the address of a temporary (middle-end) v2
On Tue, 2014-07-29 at 19:36 +0200, Marc Glisse wrote:
On Sun, 27 Jul 2014, Richard Sandiford wrote:
Marc Glisse marc.gli...@inria.fr writes:
Hello,
I followed the advice in this discussion:
https://gcc.gnu.org/ml/gcc-patches/2014-04/msg00269.html
and here is a new patch. I made an effort to isolate a path in at least
one subcase so it doesn't look too strange that the warning is in this
file. Computing the dominance info just to tweak the warning message may
be a bit excessive.
How about only calculating it once you've decided to issue a message?
+if (always_executed)
+ msg = function returns address of local variable;
+else
+ msg = function may return address of local variable;
I think you need _(...) here, unless some magic makes that unnecessary now.
Current version, which passed bootstrap+testsuite on x86_64-linux-gnu.
(the original discussion is at
https://gcc.gnu.org/ml/gcc-patches/2014-06/msg01692.html )
This is possibly a dumb question, but what happens for a static local,
rather than an auto local? e.g.
int *f (void)
{
static int i;
return i;
}
(e.g. should the test case cover this?)
Thanks
Dave
Re: Warn when returning the address of a temporary (middle-end) v2
On Tue, Jul 29, 2014 at 02:58:03PM -0400, David Malcolm wrote:
This is possibly a dumb question, but what happens for a static local,
rather than an auto local? e.g.
int *f (void)
{
static int i;
return i;
}
This is fine. The variable i has a static storage duration, so is allocated
when the program begins and is deallocated when the program ends. There's only
one instance of the variable i.
Marek
Re: Warn when returning the address of a temporary (middle-end) v2
On Tue, 29 Jul 2014, David Malcolm wrote:
On Tue, 2014-07-29 at 19:36 +0200, Marc Glisse wrote:
On Sun, 27 Jul 2014, Richard Sandiford wrote:
Marc Glisse marc.gli...@inria.fr writes:
Hello,
I followed the advice in this discussion:
https://gcc.gnu.org/ml/gcc-patches/2014-04/msg00269.html
and here is a new patch. I made an effort to isolate a path in at least
one subcase so it doesn't look too strange that the warning is in this
file. Computing the dominance info just to tweak the warning message may
be a bit excessive.
How about only calculating it once you've decided to issue a message?
+ if (always_executed)
+ msg = function returns address of local variable;
+ else
+ msg = function may return address of local variable;
I think you need _(...) here, unless some magic makes that unnecessary now.
Current version, which passed bootstrap+testsuite on x86_64-linux-gnu.
(the original discussion is at
https://gcc.gnu.org/ml/gcc-patches/2014-06/msg01692.html )
This is possibly a dumb question, but what happens for a static local,
rather than an auto local? e.g.
int *f (void)
{
static int i;
return i;
}
is_global_var returns true and we don't warn.
(e.g. should the test case cover this?)
I can add it if you want. I already test a global variable (function h)
and I expect this is likely already covered elsewhere, but it would make
the testcase more complete, so I'll do that.
I just noticed an issue with my patch: since I am using a compound
expression in the front-ends to preserve side-effects, I am now sometimes
getting this extra warning:
warning: left-hand operand of comma expression has no effect [-Wunused-value]
(I am surprised the testsuite didn't tell me about it)
I will probably have to use the compound expr only if there are side
effects.
--
Marc Glisse
Re: Warn when returning the address of a temporary (middle-end) v2
On Tue, 2014-07-29 at 21:13 +0200, Marek Polacek wrote:
On Tue, Jul 29, 2014 at 02:58:03PM -0400, David Malcolm wrote:
This is possibly a dumb question, but what happens for a static local,
rather than an auto local? e.g.
int *f (void)
{
static int i;
return i;
}
This is fine. The variable i has a static storage duration, so is allocated
when the program begins and is deallocated when the program ends. There's
only
one instance of the variable i.
Sorry, by what happens I meant, is a warning emitted?, and I see
Marc has answered that.
Re: Warn when returning the address of a temporary (middle-end) v2
Marc Glisse marc.gli...@inria.fr writes: Hello, I followed the advice in this discussion: https://gcc.gnu.org/ml/gcc-patches/2014-04/msg00269.html and here is a new patch. I made an effort to isolate a path in at least one subcase so it doesn't look too strange that the warning is in this file. Computing the dominance info just to tweak the warning message may be a bit excessive. How about only calculating it once you've decided to issue a message? + if (always_executed) + msg = function returns address of local variable; + else + msg = function may return address of local variable; I think you need _(...) here, unless some magic makes that unnecessary now. Thanks, Richard
Re: Warn when returning the address of a temporary (middle-end) v2
On Sun, 27 Jul 2014, Richard Sandiford wrote: Marc Glisse marc.gli...@inria.fr writes: Hello, I followed the advice in this discussion: https://gcc.gnu.org/ml/gcc-patches/2014-04/msg00269.html and here is a new patch. I made an effort to isolate a path in at least one subcase so it doesn't look too strange that the warning is in this file. Computing the dominance info just to tweak the warning message may be a bit excessive. How about only calculating it once you've decided to issue a message? Good idea! I'll post the updated patch after testing. + if (always_executed) + msg = function returns address of local variable; + else + msg = function may return address of local variable; I think you need _(...) here, unless some magic makes that unnecessary now. I just tried to see how the magic happens when someone calls error_at, and it goes through diagnostic_set_info, which contains: diagnostic_set_info_translated (diagnostic, _(gmsgid), args, location, kind); So I think the _(...) is already taken care of. But I don't know that code at all and I could easily have looked at it wrong. Thanks for the help, -- Marc Glisse
Re: Warn when returning the address of a temporary (middle-end) v2
Marc Glisse marc.gli...@inria.fr writes: On Sun, 27 Jul 2014, Richard Sandiford wrote: Marc Glisse marc.gli...@inria.fr writes: + if (always_executed) + msg = function returns address of local variable; + else + msg = function may return address of local variable; I think you need _(...) here, unless some magic makes that unnecessary now. I just tried to see how the magic happens when someone calls error_at, and it goes through diagnostic_set_info, which contains: diagnostic_set_info_translated (diagnostic, _(gmsgid), args, location, kind); So I think the _(...) is already taken care of. But I don't know that code at all and I could easily have looked at it wrong. If the msgid is not a direct argument of the diagnostic function you need to mark the string with N_(...). Andreas. -- Andreas Schwab, sch...@linux-m68k.org GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 And now for something completely different.
Re: Warn when returning the address of a temporary (middle-end) v2
On Sun, 27 Jul 2014, Andreas Schwab wrote: Marc Glisse marc.gli...@inria.fr writes: On Sun, 27 Jul 2014, Richard Sandiford wrote: Marc Glisse marc.gli...@inria.fr writes: + if (always_executed) + msg = function returns address of local variable; + else + msg = function may return address of local variable; I think you need _(...) here, unless some magic makes that unnecessary now. I just tried to see how the magic happens when someone calls error_at, and it goes through diagnostic_set_info, which contains: diagnostic_set_info_translated (diagnostic, _(gmsgid), args, location, kind); So I think the _(...) is already taken care of. But I don't know that code at all and I could easily have looked at it wrong. If the msgid is not a direct argument of the diagnostic function you need to mark the string with N_(...). Ah, ok, thanks. Actually, shouldn't it be G_ instead? That's what ABOUT-GCC-NLS seems to suggest since 2005, though I don't expect it makes any difference for simple strings. -- Marc Glisse
Re: Warn when returning the address of a temporary (middle-end) v2
On Wed, 16 Jul 2014, Jeff Law wrote: On 06/22/14 12:20, Marc Glisse wrote: Hello, I followed the advice in this discussion: https://gcc.gnu.org/ml/gcc-patches/2014-04/msg00269.html and here is a new patch. I made an effort to isolate a path in at least one subcase so it doesn't look too strange that the warning is in this file. Computing the dominance info just to tweak the warning message may be a bit excessive. I kept the same option as the front-ends, I don't know if we want a different one, or maybe a Wmaybe-... version. There will be cases where we get a duplicate warning from -Wtarget-lifetime in fortran, but none in the testsuite, and I would rather have 2 warnings than miss such broken code. The uninit-G testcase is about initialization, not returning, so I am changing that, even if it is unnecessary with the current version of the patch (only activated at -O2). Bootstrap+testsuite (--enable-languages=all,obj-c++,ada,go) on x86_64-unknown-linux-gnu. (by the way, contrib/compare_tests is confused when I use all languages, it prints comm: file 1 is not in sorted order and tons of spurious differences) 2014-06-23 Marc Glisse marc.gli...@inria.fr PR c++/60517 gcc/c/ * c-typeck.c (c_finish_return): Return 0 instead of the address of a local variable. gcc/cp/ * typeck.c (maybe_warn_about_returning_address_of_local): Return whether it is returning the address of a local variable. (check_return_expr): Return 0 instead of the address of a local variable. gcc/c-family/ * c.opt (-Wreturn-local-addr): Move to common.opt. gcc/ * common.opt (-Wreturn-local-addr): Moved from c.opt. * gimple-ssa-isolate-paths.c: Include diagnostic-core.h. (isolate_path): New argument to avoid inserting a trap. (find_implicit_erroneous_behaviour): Handle returning the address of a local variable. (find_explicit_erroneous_behaviour): Likewise. (gimple_ssa_isolate_erroneous_paths): Calculate dominance info. gcc/testsuite/ * c-c++-common/addrtmp.c: New file. * c-c++-common/uninit-G.c: Adapt. I note you don't catch return localvar in the isolation code -- it looks like you just catch those which potentially flow from PHIs. I thought I was handling it in the find_explicit_erroneous_behaviour part of the patch (as opposed to the implicit part which deals with PHIs). Function f1 in the testcase addrtmp.c has no PHI. Am I missing something? I realize you're primarily catching that in the front-ends, but can't we have cases which aren't caught by the front end, but after optimizations we're able to propagate somelocal into the return statement? We can, and it was my original motivation. I only added PHI handling when you asked for it. It generally looks good and I'm ready to approve if the answer to the above question is can't happen. If it can happen, then we ought to handle it in the isolation code as well (ought to be relatively easy). Just to be clear, the approval would include the PARM_DECL tweak in https://gcc.gnu.org/ml/gcc-patches/2014-06/msg02327.html ? Thanks, -- Marc Glisse
Re: Warn when returning the address of a temporary (middle-end) v2
On 06/22/14 12:20, Marc Glisse wrote: Hello, I followed the advice in this discussion: https://gcc.gnu.org/ml/gcc-patches/2014-04/msg00269.html and here is a new patch. I made an effort to isolate a path in at least one subcase so it doesn't look too strange that the warning is in this file. Computing the dominance info just to tweak the warning message may be a bit excessive. I kept the same option as the front-ends, I don't know if we want a different one, or maybe a Wmaybe-... version. There will be cases where we get a duplicate warning from -Wtarget-lifetime in fortran, but none in the testsuite, and I would rather have 2 warnings than miss such broken code. The uninit-G testcase is about initialization, not returning, so I am changing that, even if it is unnecessary with the current version of the patch (only activated at -O2). Bootstrap+testsuite (--enable-languages=all,obj-c++,ada,go) on x86_64-unknown-linux-gnu. (by the way, contrib/compare_tests is confused when I use all languages, it prints comm: file 1 is not in sorted order and tons of spurious differences) 2014-06-23 Marc Glisse marc.gli...@inria.fr PR c++/60517 gcc/c/ * c-typeck.c (c_finish_return): Return 0 instead of the address of a local variable. gcc/cp/ * typeck.c (maybe_warn_about_returning_address_of_local): Return whether it is returning the address of a local variable. (check_return_expr): Return 0 instead of the address of a local variable. gcc/c-family/ * c.opt (-Wreturn-local-addr): Move to common.opt. gcc/ * common.opt (-Wreturn-local-addr): Moved from c.opt. * gimple-ssa-isolate-paths.c: Include diagnostic-core.h. (isolate_path): New argument to avoid inserting a trap. (find_implicit_erroneous_behaviour): Handle returning the address of a local variable. (find_explicit_erroneous_behaviour): Likewise. (gimple_ssa_isolate_erroneous_paths): Calculate dominance info. gcc/testsuite/ * c-c++-common/addrtmp.c: New file. * c-c++-common/uninit-G.c: Adapt. I note you don't catch return localvar in the isolation code -- it looks like you just catch those which potentially flow from PHIs. I realize you're primarily catching that in the front-ends, but can't we have cases which aren't caught by the front end, but after optimizations we're able to propagate somelocal into the return statement? It generally looks good and I'm ready to approve if the answer to the above question is can't happen. If it can happen, then we ought to handle it in the isolation code as well (ought to be relatively easy). Jeff
Re: Warn when returning the address of a temporary (middle-end) v2
On Mon, Jun 30, 2014 at 11:37:50PM +0200, Marc Glisse wrote: On Mon, 30 Jun 2014, Jeff Law wrote: On 06/29/14 03:22, Marc Glisse wrote: After looking at PR 61597, I updated the 2 conditions to: + if ((TREE_CODE (valbase) == VAR_DECL +!is_global_var (valbase)) + || TREE_CODE (valbase) == PARM_DECL) a PARM_DECL is a local variable and returning its address is wrong, isn't it? Right. It can live in either a caller or callee allocated slot. The caller case scares me a bit. Is it really wrong to return the address in that case? The slot still exists after returning if the caller is responsible for it. At least on powerpc64, which uses a caller allocated parameter save area, returning the address of something in the parameter save area merits a warning. The ABIs explicitly state that the parameter save area is not preserved over function calls. Also note that anything left in a caller allocated parameter save area will potentially be trashed by arguments written for the next call. -- Alan Modra Australia Development Lab, IBM
Re: Warn when returning the address of a temporary (middle-end) v2
On Wed, 2 Jul 2014, Alan Modra wrote: On Mon, Jun 30, 2014 at 11:37:50PM +0200, Marc Glisse wrote: On Mon, 30 Jun 2014, Jeff Law wrote: On 06/29/14 03:22, Marc Glisse wrote: After looking at PR 61597, I updated the 2 conditions to: + if ((TREE_CODE (valbase) == VAR_DECL +!is_global_var (valbase)) + || TREE_CODE (valbase) == PARM_DECL) a PARM_DECL is a local variable and returning its address is wrong, isn't it? Right. It can live in either a caller or callee allocated slot. The caller case scares me a bit. Is it really wrong to return the address in that case? The slot still exists after returning if the caller is responsible for it. At least on powerpc64, which uses a caller allocated parameter save area, returning the address of something in the parameter save area merits a warning. Note that the patch not only warns, it also returns NULL instead of the pointer (at least the current version does), so it really needs to be certain. The ABIs explicitly state that the parameter save area is not preserved over function calls. Good, that makes me much more confident, thanks. Also note that anything left in a caller allocated parameter save area will potentially be trashed by arguments written for the next call. -- Marc Glisse
Re: Warn when returning the address of a temporary (middle-end) v2
On 07/02/14 06:19, Alan Modra wrote: On Mon, Jun 30, 2014 at 11:37:50PM +0200, Marc Glisse wrote: On Mon, 30 Jun 2014, Jeff Law wrote: On 06/29/14 03:22, Marc Glisse wrote: After looking at PR 61597, I updated the 2 conditions to: + if ((TREE_CODE (valbase) == VAR_DECL +!is_global_var (valbase)) + || TREE_CODE (valbase) == PARM_DECL) a PARM_DECL is a local variable and returning its address is wrong, isn't it? Right. It can live in either a caller or callee allocated slot. The caller case scares me a bit. Is it really wrong to return the address in that case? The slot still exists after returning if the caller is responsible for it. At least on powerpc64, which uses a caller allocated parameter save area, returning the address of something in the parameter save area merits a warning. The ABIs explicitly state that the parameter save area is not preserved over function calls. Also note that anything left in a caller allocated parameter save area will potentially be trashed by arguments written for the next call. Similarly for the PA ABIs. If something returned the address of one of those slots, we'd definitely want a warning. I'd be amazed if that wasn't the case for every ABI with a caller allocated parameter save-back area. jeff
Re: Warn when returning the address of a temporary (middle-end) v2
On 06/30/14 15:37, Marc Glisse wrote: On Mon, 30 Jun 2014, Jeff Law wrote: On 06/29/14 03:22, Marc Glisse wrote: After looking at PR 61597, I updated the 2 conditions to: + if ((TREE_CODE (valbase) == VAR_DECL +!is_global_var (valbase)) + || TREE_CODE (valbase) == PARM_DECL) a PARM_DECL is a local variable and returning its address is wrong, isn't it? Right. It can live in either a caller or callee allocated slot. The caller case scares me a bit. Is it really wrong to return the address in that case? The slot still exists after returning if the caller is responsible for it. The slot exists, but its contents are undefined. The caller never even knows if the callee ever flushed an object back to those slots or if it did flush back, which objects were flushed and in what state. There was a time when I was pondering using those slots for saving callee saved registers on the PA so that leafs that needed a few stack slots wouldn't need to allococate a new frame, instead it'd just use those convenient 4 words. But this turned out to just not be important and it would totally hose the unwinding mechanisms that were in use on the PA at the time. Jeff
Re: Warn when returning the address of a temporary (middle-end) v2
On 06/29/14 03:22, Marc Glisse wrote: After looking at PR 61597, I updated the 2 conditions to: + if ((TREE_CODE (valbase) == VAR_DECL +!is_global_var (valbase)) + || TREE_CODE (valbase) == PARM_DECL) a PARM_DECL is a local variable and returning its address is wrong, isn't it? Right. It can live in either a caller or callee allocated slot. When I first glanced at the patch my thought was why is this in the path isolation pass? But I see you want to modify the returned value to be NULL. I'll have to look at why you want to do that, but at least I understand why it's utilizing the path isolation code. jeff
Re: Warn when returning the address of a temporary (middle-end) v2
On Mon, 30 Jun 2014, Jeff Law wrote: On 06/29/14 03:22, Marc Glisse wrote: After looking at PR 61597, I updated the 2 conditions to: + if ((TREE_CODE (valbase) == VAR_DECL +!is_global_var (valbase)) + || TREE_CODE (valbase) == PARM_DECL) a PARM_DECL is a local variable and returning its address is wrong, isn't it? Right. It can live in either a caller or callee allocated slot. The caller case scares me a bit. Is it really wrong to return the address in that case? The slot still exists after returning if the caller is responsible for it. When I first glanced at the patch my thought was why is this in the path isolation pass? A very pragmatic reason is that you and Richard asked me to after v1 of the patch... It doesn't matter that much to me where this goes. But I see you want to modify the returned value to be NULL. I'll have to look at why you want to do that, but at least I understand why it's utilizing the path isolation code. Originally I mostly wanted to avoid warning several times. Now that the warning is in a pass that runs only once, it isn't that necessary (it remains necessary to do something in the front-end to avoid warning again in the middle-end). It is still an optimization (it probably helps remove dead code), though I could replace 0 with an undefined variable. -- Marc Glisse
Re: Warn when returning the address of a temporary (middle-end) v2
( https://gcc.gnu.org/ml/gcc-patches/2014-06/msg01692.html ) On Sun, 22 Jun 2014, Marc Glisse wrote: Hello, I followed the advice in this discussion: https://gcc.gnu.org/ml/gcc-patches/2014-04/msg00269.html and here is a new patch. I made an effort to isolate a path in at least one subcase so it doesn't look too strange that the warning is in this file. Computing the dominance info just to tweak the warning message may be a bit excessive. I kept the same option as the front-ends, I don't know if we want a different one, or maybe a Wmaybe-... version. There will be cases where we get a duplicate warning from -Wtarget-lifetime in fortran, but none in the testsuite, and I would rather have 2 warnings than miss such broken code. The uninit-G testcase is about initialization, not returning, so I am changing that, even if it is unnecessary with the current version of the patch (only activated at -O2). Bootstrap+testsuite (--enable-languages=all,obj-c++,ada,go) on x86_64-unknown-linux-gnu. (by the way, contrib/compare_tests is confused when I use all languages, it prints comm: file 1 is not in sorted order and tons of spurious differences) 2014-06-23 Marc Glisse marc.gli...@inria.fr PR c++/60517 gcc/c/ * c-typeck.c (c_finish_return): Return 0 instead of the address of a local variable. gcc/cp/ * typeck.c (maybe_warn_about_returning_address_of_local): Return whether it is returning the address of a local variable. (check_return_expr): Return 0 instead of the address of a local variable. gcc/c-family/ * c.opt (-Wreturn-local-addr): Move to common.opt. gcc/ * common.opt (-Wreturn-local-addr): Moved from c.opt. * gimple-ssa-isolate-paths.c: Include diagnostic-core.h. (isolate_path): New argument to avoid inserting a trap. (find_implicit_erroneous_behaviour): Handle returning the address of a local variable. (find_explicit_erroneous_behaviour): Likewise. (gimple_ssa_isolate_erroneous_paths): Calculate dominance info. gcc/testsuite/ * c-c++-common/addrtmp.c: New file. * c-c++-common/uninit-G.c: Adapt. After looking at PR 61597, I updated the 2 conditions to: + if ((TREE_CODE (valbase) == VAR_DECL + !is_global_var (valbase)) + || TREE_CODE (valbase) == PARM_DECL) a PARM_DECL is a local variable and returning its address is wrong, isn't it? (also passes bootstrap+testsuite) -- Marc Glisse
Warn when returning the address of a temporary (middle-end) v2
Hello,
I followed the advice in this discussion:
https://gcc.gnu.org/ml/gcc-patches/2014-04/msg00269.html
and here is a new patch. I made an effort to isolate a path in at least
one subcase so it doesn't look too strange that the warning is in this
file. Computing the dominance info just to tweak the warning message may
be a bit excessive. I kept the same option as the front-ends, I don't know
if we want a different one, or maybe a Wmaybe-... version. There will be
cases where we get a duplicate warning from -Wtarget-lifetime in fortran,
but none in the testsuite, and I would rather have 2 warnings than miss
such broken code. The uninit-G testcase is about initialization, not
returning, so I am changing that, even if it is unnecessary with the
current version of the patch (only activated at -O2).
Bootstrap+testsuite (--enable-languages=all,obj-c++,ada,go) on
x86_64-unknown-linux-gnu.
(by the way, contrib/compare_tests is confused when I use all languages,
it prints comm: file 1 is not in sorted order and tons of spurious
differences)
2014-06-23 Marc Glisse marc.gli...@inria.fr
PR c++/60517
gcc/c/
* c-typeck.c (c_finish_return): Return 0 instead of the address of
a local variable.
gcc/cp/
* typeck.c (maybe_warn_about_returning_address_of_local): Return
whether it is returning the address of a local variable.
(check_return_expr): Return 0 instead of the address of a local
variable.
gcc/c-family/
* c.opt (-Wreturn-local-addr): Move to common.opt.
gcc/
* common.opt (-Wreturn-local-addr): Moved from c.opt.
* gimple-ssa-isolate-paths.c: Include diagnostic-core.h.
(isolate_path): New argument to avoid inserting a trap.
(find_implicit_erroneous_behaviour): Handle returning the address
of a local variable.
(find_explicit_erroneous_behaviour): Likewise.
(gimple_ssa_isolate_erroneous_paths): Calculate dominance info.
gcc/testsuite/
* c-c++-common/addrtmp.c: New file.
* c-c++-common/uninit-G.c: Adapt.
--
Marc GlisseIndex: gcc/c/c-typeck.c
===
--- gcc/c/c-typeck.c(revision 211874)
+++ gcc/c/c-typeck.c(working copy)
@@ -9315,22 +9315,26 @@ c_finish_return (location_t loc, tree re
if (DECL_P (inner)
!DECL_EXTERNAL (inner)
!TREE_STATIC (inner)
DECL_CONTEXT (inner) == current_function_decl)
{
if (TREE_CODE (inner) == LABEL_DECL)
warning_at (loc, OPT_Wreturn_local_addr,
function returns address of label);
else
- warning_at (loc, OPT_Wreturn_local_addr,
- function returns address of local variable);
+ {
+ warning_at (loc, OPT_Wreturn_local_addr,
+ function returns address of local variable);
+ tree zero = build_zero_cst (TREE_TYPE (res));
+ t = build_compound_expr (loc, t, zero);
+ }
}
break;
default:
break;
}
break;
}
Index: gcc/c-family/c.opt
===
--- gcc/c-family/c.opt (revision 211874)
+++ gcc/c-family/c.opt (working copy)
@@ -682,24 +682,20 @@ ObjC ObjC++ Var(warn_protocol) Init(1) W
Warn if inherited methods are unimplemented
Wredundant-decls
C ObjC C++ ObjC++ Var(warn_redundant_decls) Warning
Warn about multiple declarations of the same object
Wreorder
C++ ObjC++ Var(warn_reorder) Warning LangEnabledBy(C++ ObjC++,Wall)
Warn when the compiler reorders code
-Wreturn-local-addr
-C ObjC C++ ObjC++ Var(warn_return_local_addr) Init(1) Warning
-Warn about returning a pointer/reference to a local or temporary variable.
-
Wreturn-type
C ObjC C++ ObjC++ Var(warn_return_type) Warning LangEnabledBy(C ObjC C++
ObjC++,Wall)
Warn whenever a function's return type defaults to \int\ (C), or about
inconsistent return types (C++)
Wselector
ObjC ObjC++ Var(warn_selector) Warning
Warn if a selector has multiple methods
Wshadow-ivar
ObjC ObjC++ Var(warn_shadow_ivar) EnabledBy(Wshadow) Init(1) Warning
Index: gcc/common.opt
===
--- gcc/common.opt (revision 211874)
+++ gcc/common.opt (working copy)
@@ -596,20 +596,24 @@ Common Var(warn_packed) Warning
Warn when the packed attribute has no effect on struct layout
Wpadded
Common Var(warn_padded) Warning
Warn when padding is required to align structure members
Wpedantic
Common Var(pedantic) Warning
Issue warnings needed for strict compliance to the standard
+Wreturn-local-addr
+Common Var(warn_return_local_addr) Init(1) Warning
+Warn about returning a
Re: Warn when returning the address of a temporary (middle-end)
On Sat, 5 Apr 2014, Marc Glisse wrote:
One hard part is avoiding duplicate warnings. Replacing the address with 0 is
a convenient way to do that, so I did it both for my new warning and for the
existing C/C++ ones. The patch breaks gfortran.dg/warn_target_lifetime_2.f90
That's not safe the way you do it; you lose side effects from the original
return value. Consider a testcase such as:
extern void exit (int);
extern void abort (void);
int *
f (void)
{
int a[10];
return a[(exit (0), 0)];
}
int
main (void)
{
f ();
abort ();
}
(which produces the existing warning, but which has behavior fully defined
to exit with status 0).
--
Joseph S. Myers
jos...@codesourcery.com
Re: Warn when returning the address of a temporary (middle-end)
On Mon, Apr 7, 2014 at 8:51 PM, Marc Glisse marc.gli...@inria.fr wrote: On Mon, 7 Apr 2014, Jeff Law wrote: On 04/05/14 07:52, Marc Glisse wrote: Hello, we have front-end warnings about returning the address of a local variable. However, quite often in C++, people don't directly return the address of a temporary, it goes through a few functions which hide that fact. After some inlining, the fact that we are returning the address of a local variable can become obvious to the compiler, to the point where I have used, for debugging purposes, grep 'return ' on the optimized dump produced with -O3 -fkeep-inline-functions (I then had to sort through the global/local variables). fold_stmt looks like a good place for this, but it could go almost anywhere. It has to happen after enough inlining / copy propagation to make it useful though. I was wondering if this would be better implemented as a propagation problem so that cases where some, but not all paths to the return statement have local which reaches the return. ie ... if (foo) x = local else x = global return x; ISTM it ought to be a standard propagation problem and that the problematical ADDR_EXPRs are all going to be in PHI nodes. So I think you just search for problematical ADDR_EXPRs in PHI nodes as your seeds, then forward propagate through the CFG. Ultimately looking for any cases where those ADDR_EXPRs ultimately reach the return statement. Thoughts? I would tend to start from the return statements (assuming the return type is a pointer), look at the defining statement, do things if it is an assignment of an addr_expr, and recurse if it is a PHI. But maybe my brain is cabled backwards ;-) It would be good to piggy back on a pass that already does something similar, if we go that way. Do you know a convenient one? I am also afraid we may get more false positives, but maybe not. If you don't mind false positives you can also do the check in points-to analysis, looking at the points-to result for the SSA name we return. Btw, the idea to add this to -fisolate-errorneous-paths sounds good to me, as well as returning NULL for them. Richard. Last, the simple version actually works well enough that it discovered at least one real bug in real code, and I am afraid that by refining it too much we'll delay and get nothing in the end (my time and my knowledge of the compiler are limited enough to make it a real possibility). But I admit that's not a good argument. Thanks for the comments, -- Marc Glisse
Re: Warn when returning the address of a temporary (middle-end)
On Sat, Apr 5, 2014 at 3:52 PM, Marc Glisse marc.gli...@inria.fr wrote:
Hello,
we have front-end warnings about returning the address of a local variable.
However, quite often in C++, people don't directly return the address of a
temporary, it goes through a few functions which hide that fact. After some
inlining, the fact that we are returning the address of a local variable can
become obvious to the compiler, to the point where I have used, for
debugging purposes, grep 'return ' on the optimized dump produced with -O3
-fkeep-inline-functions (I then had to sort through the global/local
variables).
fold_stmt looks like a good place for this, but it could go almost anywhere.
It has to happen after enough inlining / copy propagation to make it useful
though.
One hard part is avoiding duplicate warnings. Replacing the address with 0
is a convenient way to do that, so I did it both for my new warning and for
the existing C/C++ ones. The patch breaks
gfortran.dg/warn_target_lifetime_2.f90 because it ends up warning twice. I
didn't touch that front-end because I don't know fortran, and the warning
message Pointer at .1. in pointer assignment might outlive the pointer
target doesn't seem very confident that the thing really is broken enough
to be replaced by 0. I only tested (bootstrap+regression) the default
languages, so ada/go may have a similar issue, to be handled if the approach
seems ok. (I personally wouldn't care about duplicate warnings, but I know
some people can't help complaining about it)
This doesn't actually fix PR 60517, for that I was thinking of checking for
memory reads if the first stop of walk_aliased_vdefs is a clobber (could
also check __builtin_free), though I don't know in which pass to do that
yet.
Note that this will break working programs where inlining causes
those references to no longer be returned (though maybe they
already break with the clobbers we insert). I remember that POOMA/PETE
was full of this ... (and made it reliably work by flattening all call trees
with such calls).
Richard.
2014-04-05 Marc Glisse marc.gli...@inria.fr
PR c++/60517
gcc/c/
* c-typeck.c (c_finish_return): Return 0 instead of the address of
a local variable.
gcc/cp/
* typeck.c (check_return_expr): Likewise.
(maybe_warn_about_returning_address_of_local): Tell the caller if
we warned.
gcc/
* gimple-fold.c (fold_stmt_1): Warn if returning the address of a
local variable.
gcc/testsuite/
* c-c++-common/addrtmp.c: New testcase.
* c-c++-common/uninit-G.c: Adjust.
--
Marc Glisse
Index: gcc/c/c-typeck.c
===
--- gcc/c/c-typeck.c(revision 209157)
+++ gcc/c/c-typeck.c(working copy)
@@ -9254,23 +9254,25 @@ c_finish_return (location_t loc, tree re
inner = TREE_OPERAND (inner, 0);
while (REFERENCE_CLASS_P (inner)
TREE_CODE (inner) != INDIRECT_REF)
inner = TREE_OPERAND (inner, 0);
if (DECL_P (inner)
!DECL_EXTERNAL (inner)
!TREE_STATIC (inner)
DECL_CONTEXT (inner) == current_function_decl)
- warning_at (loc,
- OPT_Wreturn_local_addr, function returns
address
- of local variable);
+ {
+ warning_at (loc, OPT_Wreturn_local_addr,
+ function returns address of local variable);
+ t = build_zero_cst (TREE_TYPE (res));
+ }
break;
default:
break;
}
break;
}
retval = build2 (MODIFY_EXPR, TREE_TYPE (res), res, t);
Index: gcc/cp/typeck.c
===
--- gcc/cp/typeck.c (revision 209157)
+++ gcc/cp/typeck.c (working copy)
@@ -49,21 +49,21 @@ static tree convert_for_assignment (tree
static tree cp_pointer_int_sum (enum tree_code, tree, tree,
tsubst_flags_t);
static tree rationalize_conditional_expr (enum tree_code, tree,
tsubst_flags_t);
static int comp_ptr_ttypes_real (tree, tree, int);
static bool comp_except_types (tree, tree, bool);
static bool comp_array_types (const_tree, const_tree, bool);
static tree pointer_diff (tree, tree, tree, tsubst_flags_t);
static tree get_delta_difference (tree, tree, bool, bool, tsubst_flags_t);
static void casts_away_constness_r (tree *, tree *, tsubst_flags_t);
static bool casts_away_constness (tree, tree, tsubst_flags_t);
-static void maybe_warn_about_returning_address_of_local (tree);
+static bool maybe_warn_about_returning_address_of_local (tree);
static tree lookup_destructor (tree, tree, tree, tsubst_flags_t);
static void warn_args_num (location_t, tree,
Re: Warn when returning the address of a temporary (middle-end)
On Mon, 7 Apr 2014, Richard Biener wrote: One hard part is avoiding duplicate warnings. Replacing the address with 0 is a convenient way to do that, so I did it both for my new warning and for the existing C/C++ ones. The patch breaks gfortran.dg/warn_target_lifetime_2.f90 because it ends up warning twice. I didn't touch that front-end because I don't know fortran, and the warning message Pointer at .1. in pointer assignment might outlive the pointer target doesn't seem very confident that the thing really is broken enough to be replaced by 0. I only tested (bootstrap+regression) the default languages, so ada/go may have a similar issue, to be handled if the approach seems ok. (I personally wouldn't care about duplicate warnings, but I know some people can't help complaining about it) This doesn't actually fix PR 60517, for that I was thinking of checking for memory reads if the first stop of walk_aliased_vdefs is a clobber (could also check __builtin_free), though I don't know in which pass to do that yet. Note that this will break working programs where inlining causes those references to no longer be returned (though maybe they already break with the clobbers we insert). I remember that POOMA/PETE was full of this ... (and made it reliably work by flattening all call trees with such calls). I mostly see programs that are written and tested using debug mode and that appear to be working, but break as soon as they are compiled in release mode because of dangling references. Breaking them more seems like a helpful thing to me ;-) (especially with a warning). But I would be happy not replacing the pointer and only emitting a warning, if people don't mind duplicate warnings or if we can find another way to avoid them. + if (warning_at (gimple_location (stmt), OPT_Wreturn_local_addr, + function returns address of local variable)) That's a front-end option, I should either move it to gcc/common.opt or use a different option. -- Marc Glisse
Re: Warn when returning the address of a temporary (middle-end)
One hard part is avoiding duplicate warnings. Replacing the address with 0 is a convenient way to do that, so I did it both for my new warning and for the existing C/C++ ones. The patch breaks gfortran.dg/warn_target_lifetime_2.f90 because it ends up warning twice. I didn't touch that front-end because I don't know fortran, and the warning message Pointer at .1. in pointer assignment might outlive the pointer target doesn't seem very confident that the thing really is broken enough to be replaced by 0. I only tested (bootstrap+regression) the default languages, so ada/go may have a similar issue, to be handled if the approach seems ok. Ada is designed to make such a thing impossible (although you can work around the design with kludges like 'Unrestricted_Access) so the compiler will stop. -- Eric Botcazou
Re: Warn when returning the address of a temporary (middle-end)
On 04/05/14 07:52, Marc Glisse wrote: Hello, we have front-end warnings about returning the address of a local variable. However, quite often in C++, people don't directly return the address of a temporary, it goes through a few functions which hide that fact. After some inlining, the fact that we are returning the address of a local variable can become obvious to the compiler, to the point where I have used, for debugging purposes, grep 'return ' on the optimized dump produced with -O3 -fkeep-inline-functions (I then had to sort through the global/local variables). fold_stmt looks like a good place for this, but it could go almost anywhere. It has to happen after enough inlining / copy propagation to make it useful though. I was wondering if this would be better implemented as a propagation problem so that cases where some, but not all paths to the return statement have local which reaches the return. ie ... if (foo) x = local else x = global return x; ISTM it ought to be a standard propagation problem and that the problematical ADDR_EXPRs are all going to be in PHI nodes. So I think you just search for problematical ADDR_EXPRs in PHI nodes as your seeds, then forward propagate through the CFG. Ultimately looking for any cases where those ADDR_EXPRs ultimately reach the return statement. Thoughts? jeff
Re: Warn when returning the address of a temporary (middle-end)
On Mon, 7 Apr 2014, Jeff Law wrote: On 04/05/14 07:52, Marc Glisse wrote: Hello, we have front-end warnings about returning the address of a local variable. However, quite often in C++, people don't directly return the address of a temporary, it goes through a few functions which hide that fact. After some inlining, the fact that we are returning the address of a local variable can become obvious to the compiler, to the point where I have used, for debugging purposes, grep 'return ' on the optimized dump produced with -O3 -fkeep-inline-functions (I then had to sort through the global/local variables). fold_stmt looks like a good place for this, but it could go almost anywhere. It has to happen after enough inlining / copy propagation to make it useful though. I was wondering if this would be better implemented as a propagation problem so that cases where some, but not all paths to the return statement have local which reaches the return. ie ... if (foo) x = local else x = global return x; ISTM it ought to be a standard propagation problem and that the problematical ADDR_EXPRs are all going to be in PHI nodes. So I think you just search for problematical ADDR_EXPRs in PHI nodes as your seeds, then forward propagate through the CFG. Ultimately looking for any cases where those ADDR_EXPRs ultimately reach the return statement. Thoughts? I would tend to start from the return statements (assuming the return type is a pointer), look at the defining statement, do things if it is an assignment of an addr_expr, and recurse if it is a PHI. But maybe my brain is cabled backwards ;-) It would be good to piggy back on a pass that already does something similar, if we go that way. Do you know a convenient one? I am also afraid we may get more false positives, but maybe not. Last, the simple version actually works well enough that it discovered at least one real bug in real code, and I am afraid that by refining it too much we'll delay and get nothing in the end (my time and my knowledge of the compiler are limited enough to make it a real possibility). But I admit that's not a good argument. Thanks for the comments, -- Marc Glisse
Re: Warn when returning the address of a temporary (middle-end)
On 04/07/14 12:51, Marc Glisse wrote: I would tend to start from the return statements (assuming the return type is a pointer), look at the defining statement, do things if it is an assignment of an addr_expr, and recurse if it is a PHI. But maybe my brain is cabled backwards ;-) It works either way. I am also afraid we may get more false positives, but maybe not. The only false positives should come from paths which are unexecutable. One could argue that if we find any that we should warn, then isolate the path so that we get an immediate runtime trap rather than letting the address of the local escape through the return value. That in turn would argue for dumping it into gimple-isolate-erroneous-paths ;-) Last, the simple version actually works well enough that it discovered at least one real bug in real code, and I am afraid that by refining it too much we'll delay and get nothing in the end (my time and my knowledge of the compiler are limited enough to make it a real possibility). But I admit that's not a good argument. The difference is I see the enhanced version as being simple enough that we ought to just do it. jeff
Re: Warn when returning the address of a temporary (middle-end)
On Mon, 7 Apr 2014, Jeff Law wrote: I am also afraid we may get more false positives, but maybe not. The only false positives should come from paths which are unexecutable. One could argue that if we find any that we should warn, then isolate the path so that we get an immediate runtime trap rather than letting the address of the local escape through the return value. That in turn would argue for dumping it into gimple-isolate-erroneous-paths ;-) I wonder if trapping may be too strong? If a function sometimes returns a pointer to a local variable but the caller always ignores the return value in those cases, we can warn, but maybe we don't want to introduce a trap? Replacing the address with a null pointer seemed like a compromise, it will trap when you try to read it, but not if you ignore it. But if you think we can trap, ok. -- Marc Glisse
Warn when returning the address of a temporary (middle-end)
Hello,
we have front-end warnings about returning the address of a local
variable. However, quite often in C++, people don't directly return the
address of a temporary, it goes through a few functions which hide that
fact. After some inlining, the fact that we are returning the address of a
local variable can become obvious to the compiler, to the point where I
have used, for debugging purposes, grep 'return ' on the optimized dump
produced with -O3 -fkeep-inline-functions (I then had to sort through the
global/local variables).
fold_stmt looks like a good place for this, but it could go almost
anywhere. It has to happen after enough inlining / copy propagation to
make it useful though.
One hard part is avoiding duplicate warnings. Replacing the address with 0
is a convenient way to do that, so I did it both for my new warning and
for the existing C/C++ ones. The patch breaks
gfortran.dg/warn_target_lifetime_2.f90 because it ends up warning twice. I
didn't touch that front-end because I don't know fortran, and the warning
message Pointer at .1. in pointer assignment might outlive the pointer
target doesn't seem very confident that the thing really is broken enough
to be replaced by 0. I only tested (bootstrap+regression) the default
languages, so ada/go may have a similar issue, to be handled if the
approach seems ok. (I personally wouldn't care about duplicate warnings,
but I know some people can't help complaining about it)
This doesn't actually fix PR 60517, for that I was thinking of checking
for memory reads if the first stop of walk_aliased_vdefs is a clobber
(could also check __builtin_free), though I don't know in which pass to do
that yet.
2014-04-05 Marc Glisse marc.gli...@inria.fr
PR c++/60517
gcc/c/
* c-typeck.c (c_finish_return): Return 0 instead of the address of
a local variable.
gcc/cp/
* typeck.c (check_return_expr): Likewise.
(maybe_warn_about_returning_address_of_local): Tell the caller if
we warned.
gcc/
* gimple-fold.c (fold_stmt_1): Warn if returning the address of a
local variable.
gcc/testsuite/
* c-c++-common/addrtmp.c: New testcase.
* c-c++-common/uninit-G.c: Adjust.
--
Marc GlisseIndex: gcc/c/c-typeck.c
===
--- gcc/c/c-typeck.c(revision 209157)
+++ gcc/c/c-typeck.c(working copy)
@@ -9254,23 +9254,25 @@ c_finish_return (location_t loc, tree re
inner = TREE_OPERAND (inner, 0);
while (REFERENCE_CLASS_P (inner)
TREE_CODE (inner) != INDIRECT_REF)
inner = TREE_OPERAND (inner, 0);
if (DECL_P (inner)
!DECL_EXTERNAL (inner)
!TREE_STATIC (inner)
DECL_CONTEXT (inner) == current_function_decl)
- warning_at (loc,
- OPT_Wreturn_local_addr, function returns address
- of local variable);
+ {
+ warning_at (loc, OPT_Wreturn_local_addr,
+ function returns address of local variable);
+ t = build_zero_cst (TREE_TYPE (res));
+ }
break;
default:
break;
}
break;
}
retval = build2 (MODIFY_EXPR, TREE_TYPE (res), res, t);
Index: gcc/cp/typeck.c
===
--- gcc/cp/typeck.c (revision 209157)
+++ gcc/cp/typeck.c (working copy)
@@ -49,21 +49,21 @@ static tree convert_for_assignment (tree
static tree cp_pointer_int_sum (enum tree_code, tree, tree, tsubst_flags_t);
static tree rationalize_conditional_expr (enum tree_code, tree,
tsubst_flags_t);
static int comp_ptr_ttypes_real (tree, tree, int);
static bool comp_except_types (tree, tree, bool);
static bool comp_array_types (const_tree, const_tree, bool);
static tree pointer_diff (tree, tree, tree, tsubst_flags_t);
static tree get_delta_difference (tree, tree, bool, bool, tsubst_flags_t);
static void casts_away_constness_r (tree *, tree *, tsubst_flags_t);
static bool casts_away_constness (tree, tree, tsubst_flags_t);
-static void maybe_warn_about_returning_address_of_local (tree);
+static bool maybe_warn_about_returning_address_of_local (tree);
static tree lookup_destructor (tree, tree, tree, tsubst_flags_t);
static void warn_args_num (location_t, tree, bool);
static int convert_arguments (tree, vectree, va_gc **, tree, int,
tsubst_flags_t);
/* Do `exp = require_complete_type (exp);' to make sure exp
does not have an incomplete type. (That includes void types.)
Returns error_mark_node if the VALUE does not have
complete type when this function returns. */
@@ -8253,79 +8253,81 @@ convert_for_initialization (tree exp, tr
return rhs;
