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.
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.
(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.
(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...
(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.
(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).
(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.
(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?).
(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.
(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.
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.
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.
(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.
(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...
*** Bug 61528 has been marked as a duplicate of this bug. ***
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
Note that we still do not warn for my original testcase.
(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)
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...
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?
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.
Started with fix for PR 43347 to not warn on artificial aggregates https://gcc.gnu.org/bugzilla/show_bug.cgi?id=43347
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.
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)...
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.
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.
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