Bug 63446 - dangling reference results in confusing diagnostic from -Wuninitialized
Summary: dangling reference results in confusing diagnostic from -Wuninitialized
Status: NEW
Alias: None
Product: gcc
Classification: Unclassified
Component: tree-optimization (show other bugs)
Version: 4.6.0
: P3 enhancement
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords: diagnostic
Depends on:
Blocks: Wuninitialized 49974
  Show dependency treegraph
 
Reported: 2014-10-02 22:58 UTC by M8R-ynb11d
Modified: 2021-04-16 17:17 UTC (History)
3 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail: 10.2.0, 11.0, 4.6.4, 4.9.4, 5.5.0, 6.4.0, 7.2.0, 8.3.0, 9.1.0
Last reconfirmed: 2021-04-16 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description M8R-ynb11d 2014-10-02 22:58:57 UTC
struct foo {
    int &ref;
    foo(int &i) : ref(i) {}
};

foo make_foo()
{
    int x = 42;
    return foo(x);
}

int func()
{
    foo f = make_foo();
    return f.ref;
}

This code is obviously broken due to the dangling reference, so I'm glad gcc gives a warning (clang is silent) but the warning is a bit confusing:

$ g++ -O2 -Wall -c wuninit.cpp
wuninit.cpp: In function ‘int func()’:
wuninit.cpp:15:14: warning: ‘x’ is used uninitialized in this function [-Wuninitialized]
     return f.ref;
              ^

I get that the diagnostic is generated after inlining has moved x into func(), but it's still rather confusing as the person that wrote func() might have no knowledge of the internals of make_foo(), and this would be a real head scratcher for them.  Additionally, it mentions x being used uninitialized, but x is initialized.  (I understand that the initialization becomes dead code and is removed, but that's not immediately obvious.)

In an ideal world gcc would warn about the last line of make_foo() instead of func(), and it would mention a dangling reference instead of an uninitialized use.
Comment 1 Manuel López-Ibáñez 2014-10-03 00:16:03 UTC
> In an ideal world gcc would warn about the last line of make_foo() instead
> of func(), and it would mention a dangling reference instead of an
> uninitialized use.

Hum, yes, but I'm not even sure if GCC realizes that there is a dangling reference. However, it should since the code looks like:

foo make_foo() ()
Eh tree:
   1 cleanup
     2 cleanup
{
  intD.9 & SR.1D.2273;
  intD.9 xD.2244;
  struct fooD.2226 D.2254;
  struct fooD.2226 D.2260;

;;   basic block 2, loop depth 0, count 0, freq 10000, maybe hot
;;    prev block 0, next block 1, flags: (NEW, REACHABLE)
;;    pred:       ENTRY (FALLTHRU,EXECUTABLE)
;;   starting at line 10
  [test.c : 10:11] # .MEM_2 = VDEF <.MEM_1(D)>
  xD.2244 = 42;
  [test.c : 11:15] # .MEM_4 = VDEF <.MEM_2>
  [test.c : 11] MEM[(struct fooD.2226 *)&D.2260] = &xD.2244;
  # .MEM_6 = VDEF <.MEM_4>
  xD.2244 ={v} {CLOBBER};
  [test.c : 11:15] # VUSE <.MEM_6>
  return D.2260;
;;    succ:       EXIT (EXECUTABLE)

}

which is just before x = 42 is removed. But for the same reason that it is removed, perhaps also the dangling reference could be detected.
Comment 2 Marc Glisse 2014-10-03 07:09:55 UTC
make_foo:

  MEM[(struct foo *)&D.2281] = &x;
  x ={v} {CLOBBER};
  return D.2281;

That doesn't seem so easy to warn about. We could walk from return to find some of the latest non-clobbered dominating writes to that variable, then if it is an ADDR_EXPR look for dominating clobbers, but that would be very specialized, slight variations on this code would not warn (and all those walks may start to get costly if we do them naively). It is easier to warn in the caller when it tries to dereference the dangling pointer.
Comment 3 Marc Glisse 2014-10-03 07:18:29 UTC
(In reply to Marc Glisse from comment #2)
> make_foo:
> 
>   MEM[(struct foo *)&D.2281] = &x;
>   x ={v} {CLOBBER};
>   return D.2281;
> 
> That doesn't seem so easy to warn about. We could walk from return to find
> some of the latest non-clobbered dominating writes to that variable, then if
> it is an ADDR_EXPR look for dominating clobbers, but that would be very
> specialized,

Hmm, actually, we can probably ignore the clobber, returning an object that contains a pointer to a local variable is the issue. That's already a bit more doable.

Not sure what the best pass would be to add this though. Returning (directly) a pointer to a temporary is handled in isolate-paths IIRC, but that might not be as good a fit for this.
Comment 4 Manuel López-Ibáñez 2014-10-03 11:37:43 UTC
(In reply to Marc Glisse from comment #2)
> make_foo:
> 
>   MEM[(struct foo *)&D.2281] = &x;
>   x ={v} {CLOBBER};
>   return D.2281;
> 
> That doesn't seem so easy to warn about. We could walk from return to find
> some of the latest non-clobbered dominating writes to that variable, then if
> it is an ADDR_EXPR look for dominating clobbers, but that would be very
> specialized, slight variations on this code would not warn (and all those
> walks may start to get costly if we do them naively). It is easier to warn
> in the caller when it tries to dereference the dangling pointer.

At some moment (in dcce1), gcc decides that x = 4 is not needed. For the same reason, it could realize that MEM[(struct foo *)&D.2281] = &x must produce a dangling reference, no?
Comment 5 Marc Glisse 2014-10-03 12:13:29 UTC
(In reply to Manuel López-Ibáñez from comment #4)
> At some moment (in dcce1), gcc decides that x = 4 is not needed. For the
> same reason, it could realize that MEM[(struct foo *)&D.2281] = &x must
> produce a dangling reference, no?

A clobber implies that the content is lost, so it is useless to store something there right before the clobber (I assume that's why the store is removed, I didn't check), but I don't believe it implies that the memory location is reclaimed (does it?), so it would be fine to store a pointer in some struct, kill what that pointer points to, re-create something there, use that, etc. Without the return statement, it is normal to remove x=4 (because of the clobber), but I don't see anything to warn about, while we could warn with a return statement and no clobber, so the 2 things seem quite different.

Of course I may be looking at this the wrong way.
Comment 6 Manuel López-Ibáñez 2014-10-03 12:39:22 UTC
(In reply to Marc Glisse from comment #5)
> A clobber implies that the content is lost, so it is useless to store
> something there right before the clobber (I assume that's why the store is
> removed, I didn't check), but I don't believe it implies that the memory
> location is reclaimed (does it?), so it would be fine to store a pointer in
> some struct, kill what that pointer points to, re-create something there,
> use that, etc. Without the return statement, it is normal to remove x=4
> (because of the clobber), but I don't see anything to warn about, while we
> could warn with a return statement and no clobber, so the 2 things seem
> quite different.

OK, that makes sense. Thanks for the explanation. 

What about checking that the return value, if it is a VUSE, is not assigned to a local? That is, this would be valid, assuming y is not local,

  MEM[(struct foo *)&D.2281] = &x;
  MEM[(struct foo *)&D.2281] = &y;
  return D.2281;

 but this would not be:

  MEM[(struct foo *)&D.2281] = &x;
  return D.2281;

It doesn't matter, whether x is clobbered or has a value and also it doesn't matter what is executed between the assignment and the return as long as it doesn't change the value of MEM[(struct foo *)&D.2281]. It could be a matter of following the chain of VUSE->VDEF, which I think we already do for Wuninitialized.
Comment 7 Marc Glisse 2014-10-03 13:51:40 UTC
(In reply to Manuel López-Ibáñez from comment #6)
> What about [...]

That's roughly what I describe in comment #2, amended by comment #3.

> It could be a matter of following the chain of VUSE->VDEF,
> which I think we already do for Wuninitialized.

IIRC we don't, and the file contains a comment saying that it would be expensive. Actually, properly limiting the maximal depth of the walk (as is done in several other passes) would make it cheap enough and we should probably do it.

Note that if make_foo was static or inline, it wouldn't exist anymore in the uninit pass.
Comment 8 Manuel López-Ibáñez 2014-10-03 14:57:04 UTC
(In reply to Marc Glisse from comment #7)
> IIRC we don't, and the file contains a comment saying that it would be
> expensive. Actually, properly limiting the maximal depth of the walk (as is
> done in several other passes) would make it cheap enough and we should
> probably do it.

I hope so. You may even fix PR19430 in the process, which is one of the oldest PRs we have!
Comment 9 Martin Sebor 2021-04-16 17:17:08 UTC
GCC 11 improves the message by including a note that mentions the uninitialized variable.  In this case it doesn't completely solve the problem because the variable it points to is initialized but it should help.

pr63446.C: In function ‘int func()’:
pr63446.C:15:14: warning: ‘x’ is used uninitialized [-Wuninitialized]
   15 |     return f.ref;
      |              ^~~
pr63446.C:8:9: note: ‘x’ was declared here
    8 |     int x = 42;
      |         ^

But the underlying bug here is returning a reference to a local variable, so the warning to improve is -Wreturn-local-addr.  That will diagnose the return statement in make_foo().  Bug 49974 tracks that enhancement.  With that, the additional -Wuninitialized issued in func() would just help clarify the consequences of the bug but not otherwise be essential.