Bug 60517 - warning/error for taking address of member of a temporary object
Summary: warning/error for taking address of member of a temporary object
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: c++ (show other bugs)
Version: 4.9.0
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords: diagnostic
: 61528 (view as bug list)
Depends on:
Blocks: 45821 50476 51270
  Show dependency treegraph
 
Reported: 2014-03-13 15:54 UTC by Manuel López-Ibáñez
Modified: 2019-08-06 00:09 UTC (History)
7 users (show)

See Also:
Host:
Target:
Build:
Known to work: 9.1.0
Known to fail: 4.9.1, 5.5.0, 6.4.0, 7.4.0, 8.3.0
Last reconfirmed: 2014-08-22 00:00:00


Attachments
first try (1.82 KB, patch)
2014-04-05 23:01 UTC, Marc Glisse
Details | Diff
first try (1.83 KB, patch)
2014-04-06 08:23 UTC, Marc Glisse
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Manuel López-Ibáñez 2014-03-13 15:54:51 UTC
class B {
public:
    double x[2];
};
class A {
    B b;
public:
    B getB(void) { return b; }
};

double foo(A a) {
    double * x = &(a.getB().x[0]);
    return x[0];
}

In the code above, x is not pointing to anything useful. I am not sure how hard would be to diagnose this, but it would be nice if it was possible. It would have saved me a few hours of bug-hunting today.
Comment 1 Marc Glisse 2014-03-13 20:42:15 UTC
I see in the dump:

  # .MEM_4 = VDEF <.MEM_8>
  D.2272 ={v} {CLOBBER};
  # VUSE <.MEM_4>
  _5 = MEM[(doubleD.39 *)&D.2272];

which looks like something we could warn about in the middle-end.
Comment 2 Manuel López-Ibáñez 2014-03-13 23:44:47 UTC
(In reply to Marc Glisse from comment #1)
> I see in the dump:
> 
>   # .MEM_4 = VDEF <.MEM_8>
>   D.2272 ={v} {CLOBBER};
>   # VUSE <.MEM_4>
>   _5 = MEM[(doubleD.39 *)&D.2272];
> 
> which looks like something we could warn about in the middle-end.

Could you elaborate? My middle-end foo is not as good as it used to be.
Comment 3 Marc Glisse 2014-03-14 07:19:09 UTC
(In reply to Manuel López-Ibáñez from comment #2)
> (In reply to Marc Glisse from comment #1)
> > I see in the dump:
> > 
> >   # .MEM_4 = VDEF <.MEM_8>
> >   D.2272 ={v} {CLOBBER};
> >   # VUSE <.MEM_4>
> >   _5 = MEM[(doubleD.39 *)&D.2272];
> > 
> > which looks like something we could warn about in the middle-end.
> 
> Could you elaborate? My middle-end foo is not as good as it used to be.

_5 = MEM[(doubleD.39 *)&D.2272];
says we are reading something inside variable D.2272. And right in the previous instruction:
D.2272 ={v} {CLOBBER};
we clobbered the content of that variable, so what we are reading is nonsense. These clobbers are specifically added to indicate when variables die (like your temporary).
And we don't need to rely on the clobber being exactly the previous instruction. The state of the memory (VUSE) when we execute the last instruction was defined (VDEF) by the clobber instruction, typically walk_aliased_vdefs would help you find the last instructions that touched something related to the variable (get_ref_base_and_extent to get the variable maybe?), the visitor would always stop, recording if the instruction was a clobber, and you would check if all branches (or one branch for a -Wmaybe- type of warning?) stopped on a clobber. Some passes may record enough information to make the walk unnecessary, but I am not sure. And you may want to record objects you have already warned about, to avoid flooding if a dead variable is used a lot.

Now that I am describing it, there may already be code that does the same thing for use-after-free or other cases?

Anyway, a hard part would be deciding in which pass to warn. And turning this vague description (assuming it makes at least a bit of sense) into something that actually works. And finding a right compromise between false positives and false negatives. The usual...
Comment 4 Manuel López-Ibáñez 2014-03-14 10:41:21 UTC
(In reply to Marc Glisse from comment #3)
> (In reply to Manuel López-Ibáñez from comment #2)
> > (In reply to Marc Glisse from comment #1)
> > > I see in the dump:
> > > 
> > >   # .MEM_4 = VDEF <.MEM_8>
> > >   D.2272 ={v} {CLOBBER};
> > >   # VUSE <.MEM_4>
> > >   _5 = MEM[(doubleD.39 *)&D.2272];
> > > 
> > > which looks like something we could warn about in the middle-end.
> > 
> > Could you elaborate? My middle-end foo is not as good as it used to be.
> 
> _5 = MEM[(doubleD.39 *)&D.2272];
> says we are reading something inside variable D.2272. And right in the
> previous instruction:
> D.2272 ={v} {CLOBBER};
> we clobbered the content of that variable, so what we are reading is
> nonsense. These clobbers are specifically added to indicate when variables
> die (like your temporary).

Where is the clobber added? The closer to the FE that we warn, the better diagnostic we can generate. I'm not very concerned about maybe-branches, since 
I expect most bugs to appear in temporaries created in the middle of expressions (such as a.getB().getA().x), where no branching occurs.
Comment 5 Marc Glisse 2014-03-14 12:12:53 UTC
(In reply to Manuel López-Ibáñez from comment #4)
> Where is the clobber added?

front-end, I expect (sorry, I'm trying to get something to work on windows and don't have my usual sources at hand).

> The closer to the FE that we warn, the better
> diagnostic we can generate.

Then do it in the front-end and ignore my message ;-)

> I'm not very concerned about maybe-branches, since 
> I expect most bugs to appear in temporaries created in the middle of
> expressions (such as a.getB().getA().x), where no branching occurs.

An exemple I have in mind is (pseudo):
ref id(ref x){return x;}
ref fun(){return id(temporary);}
where the function call hides that we are returning a reference to a temporary (id could return a reference to a global object, for all fun knows) and we know more after inlining (I need id inlined into fun, and either fun inlined into something that uses its returned ref (so my previous technique works) or I would need a different (middle-end) warning that detects return &local_var, which seems even easier except that it would give a lot of duplicates with front-end warnings).
Comment 6 Marc Glisse 2014-03-14 15:41:16 UTC
(In reply to Marc Glisse from comment #5)
> I would need a different (middle-end) warning that
> detects return &local_var,

To confirm this, I looked at the last dangling reference I debugged, recompiled it with -O3 -fkeep-inline-functions, and a grep for "return &" in the .optimized dump showed:
  return &one;
  return &one;
  return &D.495451.c_;
(where "one" is a global variable, but D.495451 is a local variable)
so even this trivial version of the warning would be useful.
Comment 7 Manuel López-Ibáñez 2014-03-14 16:51:11 UTC
(In reply to Marc Glisse from comment #6)
> (In reply to Marc Glisse from comment #5)
> > I would need a different (middle-end) warning that
> > detects return &local_var,
> 
> To confirm this, I looked at the last dangling reference I debugged,
> recompiled it with -O3 -fkeep-inline-functions, and a grep for "return &" in
> the .optimized dump showed:
>   return &one;
>   return &one;
>   return &D.495451.c_;
> (where "one" is a global variable, but D.495451 is a local variable)
> so even this trivial version of the warning would be useful.

To avoid duplicates, the front-end could just return something else, like NULL, when it detects this case (I guess the behavior is undefined and we can do whatever we want, no?).
Comment 8 Marc Glisse 2014-03-14 17:29:10 UTC
(In reply to Manuel López-Ibáñez from comment #7)
> To avoid duplicates, the front-end could just return something else, like
> NULL, when it detects this case (I guess the behavior is undefined and we
> can do whatever we want, no?).

I guess... Then we might as well introduce a call to __builtin_unreachable right before the return, at the same time as we replace the return value.
Comment 9 Marc Glisse 2014-03-14 18:02:29 UTC
(In reply to Manuel López-Ibáñez from comment #7)
> To avoid duplicates, the front-end could just return something else, like
> NULL, when it detects this case (I guess the behavior is undefined and we
> can do whatever we want, no?).

Actually, for a function that returns a reference, we may also want to warn if we detect that it is returning NULL. And a function returning a pointer could be marked returns_nonnull. In both cases we would be introducing a new warning, more confusing than a duplicate. But there are probably small variations on this idea that would work.
Comment 10 Marc Glisse 2014-04-05 23:01:29 UTC
Created attachment 32549 [details]
first try

With -O -fdisable-tree-esra (see PR 60770), it warns on the testcase. Twice because the DSE pass is run twice.

The commented out code was supposed to remove the clobber, but it crashes in unlink_stmt_vdef (probably because I am exiting FOR_EACH_IMM_USE_STMT too violently) so I removed it for now.
Comment 11 Marc Glisse 2014-04-06 08:23:51 UTC
Created attachment 32551 [details]
first try

With clobber removal now.

It isn't so great, the statements before the clobber have already been removed as dead code, so by removing the clobber we produce an uninitialized read for which we get a warning later.
Comment 12 Manuel López-Ibáñez 2014-04-06 09:00:23 UTC
(In reply to Marc Glisse from comment #11)
> Created attachment 32551 [details]
> first try
> 
> With clobber removal now.

Why do you want to remove the clobber? 

I understood your idea to avoid duplicated warnings was to add __builtin_unreachable and replace the value.

Could be possible to have a special var/value that is marked as artificial such that the warning code never warns for statements containing this variable? We already have DECL_ARTIFICIAL in the FE but I am not sure whether that survives to the middle-end.
Comment 13 Marc Glisse 2014-04-06 10:17:10 UTC
(In reply to Manuel López-Ibáñez from comment #12)
> Why do you want to remove the clobber? 

I am in the DSE pass, so removing the clobber as a dead store is the easiest thing to do ;-)

Ok, it might not be such a good idea.

> I understood your idea to avoid duplicated warnings was to add
> __builtin_unreachable and replace the value.

adding __builtin_unreachable seems doable. I am not sure what to replace the value with though, as the variable could have any type.

Note that I also posted:
http://gcc.gnu.org/ml/gcc-patches/2014-04/msg00269.html
(hmm, OPT_Wreturn_local_addr is a C/C++ FE flag, I should move it or pick a different one)
where I do replace the pointer by zero as you suggested. I am not adding __builtin_unreachable, because if nothing uses the return value, I am not sure it would be right to break the code.

> Could be possible to have a special var/value that is marked as artificial
> such that the warning code never warns for statements containing this
> variable? We already have DECL_ARTIFICIAL in the FE but I am not sure
> whether that survives to the middle-end.

There is probably some unused bit somewhere...
Comment 14 Paolo Carlini 2014-06-26 17:22:09 UTC
*** Bug 61528 has been marked as a duplicate of this bug. ***
Comment 15 Marc Glisse 2014-07-31 09:34:30 UTC
Author: glisse
Date: Thu Jul 31 09:33:58 2014
New Revision: 213323

URL: https://gcc.gnu.org/viewcvs?rev=213323&root=gcc&view=rev
Log:
2014-07-31  Marc Glisse  <marc.glisse@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.

Added:
    trunk/gcc/testsuite/c-c++-common/addrtmp.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/c-family/ChangeLog
    trunk/gcc/c-family/c.opt
    trunk/gcc/c/ChangeLog
    trunk/gcc/c/c-typeck.c
    trunk/gcc/common.opt
    trunk/gcc/cp/ChangeLog
    trunk/gcc/cp/typeck.c
    trunk/gcc/gimple-ssa-isolate-paths.c
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/c-c++-common/uninit-G.c
Comment 16 Manuel López-Ibáñez 2014-08-22 13:02:17 UTC
Note that we still do not warn for my original testcase.
Comment 17 Marc Glisse 2014-08-22 13:22:00 UTC
(In reply to Manuel López-Ibáñez from comment #16)
> Note that we still do not warn for my original testcase.

I didn't close the PR ;-)

There are patches around that may need more work, see these discussions (I picked a random message for the link):
https://gcc.gnu.org/ml/gcc-patches/2014-07/msg00376.html
https://gcc.gnu.org/ml/gcc-patches/2014-07/msg00422.html

(apparently the patches do manage to warn on your original testcase, at least I said so in one of the emails)
Comment 18 Marc Glisse 2014-11-22 14:48:06 UTC
The .uninit dump for the original testcase now looks like:

double foo(A) (struct A a)
{
  double SR.1;

  <bb 2>:
  return SR.1_2(D);

}

which the uninit pass would warn about, except that SR.1 is marked TREE_NO_WARNING (possibly since the temporary created by the front-end). If someone wants to look into that...
Comment 19 Fredrik Hederstierna 2017-07-24 10:45:33 UTC
It is the SRA pass that sets TREE_NO_WARNING in function from "tree-sra.c":
"create_access_replacement (struct access *access)".

Since acccess-base is set to ARTIFICIAL and IGNORED, the else-case is taken, setting:

  TREE_NO_WARNING (repl) = 1;

If removing this line we will get a warning in "tree-ssa-uninit.c":

test.c:120:14: warning: 'SR.1' is used uninitialized in this function [-Wuninitialized]
   return x[0];
              ^
Though the generated name is not looking good,
if forcing SRA to make a name it end up with instead:

test.c:120:14: warning: '<anonymous>' is used uninitialized in this function [-Wuninitialized]
   return x[0];

I'm not sure where to take it from here, suggestions?
Comment 20 Fredrik Hederstierna 2017-07-24 11:00:56 UTC
Simplest fix might be something like?
- else
+ else if (access->grp_no_warning)
so we do not always suppress warnings, but name will look funny for temp.
Comment 21 Fredrik Hederstierna 2017-07-24 17:07:45 UTC
Started with fix for PR 43347 to not warn on artificial aggregates

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=43347
Comment 22 Manuel López-Ibáñez 2017-07-24 18:13:42 UTC
I honestly think the uninitialized warning and fixing TREE_NOWARNING is a
red-herring. My testcase should get a warning even if .x[0] is initialized.
The problem is taking the address of a temporary. That should be detected
way before SRA happens. No control flow needed, only scope info.

On 24 Jul 2017 6:07 pm, "fredrik.hederstierna@securitas-direct.com" <
gcc-bugzilla@gcc.gnu.org> wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=60517
>
> --- Comment #21 from Fredrik Hederstierna <fredrik.hederstierna@
> securitas-direct.com> ---
> Started with fix for PR 43347 to not warn on artificial aggregates
>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=43347
>
> --
> You are receiving this mail because:
> You reported the bug.
Comment 23 Fredrik Hederstierna 2017-07-25 15:26:20 UTC
Ah ok, yes I think you are right. The check could possibly be in "cp/typeck.c" and "cp/tree.c"? but I'm not familiar with this C++ parsing code.

Interesting that this code gets warning:

  B* foo(A a) {
    B *b = &(a.getB());
    return b;
  }

test.c: In function ‘B* foo2(A)’:
test.c:28:20: error: taking address of temporary [-fpermissive]
   B *b = &(a.getB());

but this does not (as you example)

  double foo(A a) {
    double *x = &(a.getB().x[0]);
    return x[0];
  }

though the addressing taken was done implicit when taking addressing of the array x?

Checking flow in "typeck.c", it does not give warning since the 'kind' of lvalue is not clk_class but clk_ordinary in latter case since its ARRAY_REF. The warning-code in typeck.c does check for this, but I saw that recently some code was made to consider an array to be a 'class' in PR 80415 patch:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80415

Could it be possible to also include more 'kinds' or types in the warning-test to also include this example?

Though checking this small patch will trigger the warning, but probably its not the right thing to do, but maybe a starting point to seek for a solution?

diff --git a/gcc/cp/tree.c b/gcc/cp/tree.c                                                                                                                                                                                                        
index 2122450..4dab925 100644                                                                                                                                                                                                                     
--- a/gcc/cp/tree.c                                                                                                                                                                                                                               
+++ b/gcc/cp/tree.c                                                                                                                                                                                                                               
@@ -163,7 +163,6 @@ lvalue_kind (const_tree ref)                                                                                                                                                                                                  
       /* FALLTHRU */
     case INDIRECT_REF:
     case ARROW_EXPR:
-    case ARRAY_REF:
     case ARRAY_NOTATION_REF:
     case PARM_DECL:
     case RESULT_DECL:
@@ -212,6 +211,7 @@ lvalue_kind (const_tree ref)
     case COMPOUND_EXPR:
       return lvalue_kind (TREE_OPERAND (ref, 1));
 
+    case ARRAY_REF:
     case TARGET_EXPR:
       return clk_class;

This will return lvalue 'kind' as clk_class also for arrays, and then the warning will trigger in typeck.c. Probably this is just nonsense, so I will leave this to someone else to solve that understands this part of code (not me).

Still I think other path is also interesting about warning for uninitialized artificials (that was removed in PR 43347 by unclear reasons - I think the warning was correct, and the issue was rather that generated SRA name was not the real variable name, which is a completely other problem related to debug info I think)...
Comment 24 Manuel López-Ibáñez 2017-07-25 17:49:15 UTC
How does typeck.c check that it is a temporary? The important thing is not
that it is an ARRAY_REF but that it is a member of a temporary object. Not
sure how to check that.

Marc points above that the FE introduces a clobber to mark the death of the
temporary, so it knows.

The problem of warning for artificials is that optimization passes create
and read uninitialized memory if this provides an advantage and it doesn't
change the behaviour of the code. Warning about those would be very
annoying, if the user code is correct.
Comment 25 Martin Sebor 2019-08-05 23:57:34 UTC
GCC 9 rejects the test case with the error below:

$ cat pr60517.C && gcc -Wall -S pr60517.C
class B {
public:
    double x[2];
};
class A {
    B b;
public:
    B getB(void) { return b; }
};

double foo(A a) {
    double * x = &(a.getB().x[0]);
    return x[0];
}

pr60517.C: In function ‘double foo(A)’:
pr60517.C:12:33: error: taking address of rvalue [-fpermissive]
   12 |     double * x = &(a.getB().x[0]);
      |                                 ^

The change that introduced the error is r260621:

	CWG 616, 1213 - value category of subobject references.

	* tree.c (lvalue_kind): A reference to a subobject of a prvalue is
	an xvalue.
	* typeck2.c (build_m_component_ref): Likewise.
	* typeck.c (cp_build_addr_expr_1, lvalue_or_else): Remove diagnostic
	distinction between temporary and xvalue.

I don't see a test case for this error so let me add one and resolve the bug.
Comment 26 Martin Sebor 2019-08-06 00:09:16 UTC
Author: msebor
Date: Tue Aug  6 00:08:45 2019
New Revision: 274130

URL: https://gcc.gnu.org/viewcvs?rev=274130&root=gcc&view=rev
Log:
PR c++/60517 - warning/error for taking address of member of a temporary object

testsuite/ChangeLog:
	* g++.dg/pr60517.C: New test.

Added:
    trunk/gcc/testsuite/g++.dg/pr60517.C
Modified:
    trunk/gcc/testsuite/ChangeLog