[Bug c++/104492] New: Bogus dangling pointer warning (dangling pointer to ‘candidates’ may be used [-Werror=dangling-pointer=])

[Bug c++/104492] New: Bogus dangling pointer warning (dangling pointer to ‘candidates’ may be used [-Werror=dangling-pointer=])

[thiago at kde dot org]

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

            Bug ID: 104492
           Summary: Bogus dangling pointer warning (dangling pointer to
                    ‘candidates’ may be used [-Werror=dangling-pointer=])
           Product: gcc
           Version: 12.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: thiago at kde dot org
  Target Milestone: ---

Created attachment 52409
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=52409&action=edit
Preprocessed sources for qlibrary.cpp

Workaround: https://codereview.qt-project.org/c/qt/qtbase/+/394894
Original sources: 
https://code.qt.io/cgit/qt/qtbase.git/tree/src/corelib/plugin/qlibrary.cpp?id=5e378aaff61c9708e0657f3ee29517c06cb075fa#n618
https://github.com/qt/qtbase/blob/5e378aaff61c9708e0657f3ee29517c06cb075fa/src/corelib/plugin/qlibrary.cpp#L618-L677

Code snippet:

    auto isValidSuffix = [](QStringView s) {
        const QLatin1String candidates[] = {
            QLatin1String("so"),
        };
        return std::find(std::begin(candidates), std::end(candidates), s) !=
std::end(candidates);
    };
    auto suffixes = qTokenize(completeSuffix, u'.');
    auto it = suffixes.begin();
    const auto end = suffixes.end();
    while (it != end) {
        if (isValidSuffix(*it++))
          return true;
    }
    return false;

This code above produces the warning:
qlibrary.cpp:114:9: error: dangling pointer to ‘candidates’ may be used
[-Werror=dangling-pointer=]          
  114 |         if (isValidSuffix(*it++))                                       
      |         ^~                                                              
qlibrary.cpp:76:29: note: ‘candidates’ declared here                            
   76 |         const QLatin1String candidates[] = {                            
      |                             ^~~~~~~~~~                                  

The 'candidates' variable has indeed gone out of scope, but there's no use of
its pointer where the lambda is used. The lambda does a std::find, which
returns a pointer into the array, but that pointer is compared to std::end and
the lambda returns a boolean.

Compile the attached preprocessed sources with:
 g++ -march=x86-64-v3 -Wall -Wextra -O3 -o /dev/null -c qlibrary.cpp.ii

GCC commit 1ce5395977f37e8d0c03394f7b932a584ce85cc7 (master branch dated
2022-02-09).

[Bug c++/104492] Bogus dangling pointer warning (dangling pointer to ‘candidates’ may be used [-Werror=dangling-pointer=])

[pinskia at gcc dot gnu.org]

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

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
This is most likely the case where we have:
....
T = y != &z[0];
Z = {Clobber}
If(T)

And then forward prop the comparison after the clobber.

[Bug middle-end/104492] [12 Regression] Bogus dangling pointer warning at -O3

[msebor at gcc dot gnu.org]

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

Martin Sebor <msebor at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
          Component|c++                         |middle-end
            Summary|Bogus dangling pointer      |[12 Regression] Bogus
                   |warning (dangling pointer   |dangling pointer warning at
                   |to ‘candidates’ may be used |-O3
                   |[-Werror=dangling-pointer=] |
                   |)                           |
     Ever confirmed|0                           |1
             Blocks|                            |104077
   Last reconfirmed|                            |2022-02-11
           Keywords|                            |diagnostic
                 CC|                            |msebor at gcc dot gnu.org

--- Comment #2 from Martin Sebor <msebor at gcc dot gnu.org> ---
The warning is issued for the if condition in the basic block below:

<bb 28> [local count: 1014686340]:
# _106 = PHI <&MEM <const struct QLatin1String[1]> [(void *)&candidates +
16B](26), &candidates(27)>
<L14>:
_274 = _106;
__pred ={v} {CLOBBER(eol)};
_17 = _274;
__pred ={v} {CLOBBER(eol)};
candidates ={v} {CLOBBER(eol)};                                               
<<< clobber
s ={v} {CLOBBER(eol)};
D.169975 ={v} {CLOBBER(eol)};
if (&MEM <const struct QLatin1String[1]> [(void *)&candidates + 16B] != _17)  
<<< -Wdangling-pointer
  goto <bb 30>; [5.50%]
else
  goto <bb 29>; [94.50%]


Referenced Bugs:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104077
[Bug 104077] bogus/missing -Wdangling-pointer

[Bug middle-end/104492] [12 Regression] Bogus dangling pointer warning at -O3

[msebor at gcc dot gnu.org]

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

--- Comment #3 from Martin Sebor <msebor at gcc dot gnu.org> ---
It might be possible to run the pass earlier to avoid this problem but I
haven't managed to find a spot that didn't regress some -Wdangling-pointer
tests (at least g++.dg/warn/Wdangling-pointer-2.C).  Alternatively, if forwprop
moves an access past a clobber (or whatever pass does it) it might be taught to
suppress the warning when it does.

[Bug middle-end/104492] [12 Regression] Bogus dangling pointer warning at -O3

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |12.0

--- Comment #4 from Richard Biener <rguenth at gcc dot gnu.org> ---
As said elsewhere equality compares should IMHO exempt from
-Wdangling-pointers.

[Bug middle-end/104492] [12 Regression] Bogus dangling pointer warning at -O3

[msebor at gcc dot gnu.org]

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

--- Comment #5 from Martin Sebor <msebor at gcc dot gnu.org> ---
Setting aside the question of warning about inequality expressions involving
invalid pointers, it seems that if the annotation 'candidates ={v}
{CLOBBER(eol)};' is to be interpreted as one would intuitively expect -- as
ending the variable's lifetime -- then GCC moving its use past that point
should be considered a bug in that transformation.

[Bug middle-end/104492] [12 Regression] Bogus dangling pointer warning at -O3

[rguenth at gcc dot gnu.org]

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

--- Comment #6 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Martin Sebor from comment #5)
> Setting aside the question of warning about inequality expressions involving
> invalid pointers, it seems that if the annotation 'candidates ={v}
> {CLOBBER(eol)};' is to be interpreted as one would intuitively expect -- as
> ending the variable's lifetime -- then GCC moving its use past that point
> should be considered a bug in that transformation.

The lifetime of the object ends but this is just a value and GCC cannot
distinguish for example between (uintptr_t)&candidates and &candidates
(where "leaking" the former is obviously OK).  It's similar to the
issue with __builtin_object_size and &a.b[0] vs. &a - it's nothing we can
fix.  So diagnosing uses of the _address_ (rather than the pointed to
storage) is going to have GCC generated false positives.

[Bug middle-end/104492] [12 Regression] Bogus dangling pointer warning at -O3

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P1

[Bug middle-end/104492] [12 Regression] Bogus dangling pointer warning at -O3

[msebor at gcc dot gnu.org]

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

Martin Sebor <msebor at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
           Assignee|unassigned at gcc dot gnu.org      |msebor at gcc dot gnu.org

--- Comment #7 from Martin Sebor <msebor at gcc dot gnu.org> ---
So the CLOBBER semantics correspond more closely to those of a C++ destructor
than to a deallocation call.  It would be helpful to document these things.

[Bug middle-end/104492] [12 Regression] Bogus dangling pointer warning at -O3

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #8 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Any progress on this?  It is a P1...
As Richi said, CLOBBERs have vops and prevent moving of memory reads/writes
across them, but can't prevent moving of just addresses of those vars across
them, such a dependence isn't present in the IL.
So, can CLOBBERs be used for warning diagnosing out of scope accesses to
variables?  Sure.  Can CLOBBERs be used for warning diagnosing references to
address of out of scope variables?  No (perhaps with the exception of
GIMPLE_RETURN of such addresses, returning that to a caller is certainly
suspicious).

[Bug middle-end/104492] [12 Regression] Bogus dangling pointer warning at -O3

[rguenth at gcc dot gnu.org]

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

--- Comment #9 from Richard Biener <rguenth at gcc dot gnu.org> ---
So IL wise the issue is that we go from

  <bb 2> :
  candidates(address-taken)[0].m_size = 2;
  candidates(address-taken)[0].m_data = "so";
  _1 = std::end<const QLatin1String, 1> (&candidates(address-taken));
  _2 = std::begin<const QLatin1String, 1> (&candidates(address-taken));
  _11 = std::find<const QLatin1String*, QStringView> (_2, _1,
&s(address-taken));
  _3 = _11;
  _4 = std::end<const QLatin1String, 1> (&candidates(address-taken));
  _13 = _3 != _4;
  candidates(address-taken) ={v} {CLOBBER(eol)};
  return _13;

to

  _32 = std::__find_if<const QLatin1String*,
__gnu_cxx::__ops::_Iter_equals_val<const QStringView> >
(&candidates(address-taken), &MEM <const struct QLatin1String[1]> [(void
*)&candidates(address-taken) + 16B], __pred, D.182436);
  __pred ={v} {CLOBBER(eol)};
  _33 = &MEM <const struct QLatin1String[1]> [(void
*)&candidates(address-taken) + 16B] != _32;
  candidates(address-taken) ={v} {CLOBBER(eol)};
  _66 = _33;
  s(address-taken) ={v} {CLOBBER(eol)};
  _19 = _66;
  retval.0_20 = _19;
  D.169966(address-taken) ={v} {CLOBBER(eol)};
  if (retval.0_20 != 0)

exposing the forwarding opportunity into the conditional:

  _32 = std::__find_if<const QLatin1String*,
__gnu_cxx::__ops::_Iter_equals_val<const QStringView> >
(&candidates(address-taken), &MEM <const struct QLatin1String[1]> [(void
*)&candidates(address-taken) + 16B], __pred, D.182436);
  __pred ={v} {CLOBBER(eol)};
  candidates(address-taken) ={v} {CLOBBER(eol)};
  s(address-taken) ={v} {CLOBBER(eol)};
  D.169966 ={v} {CLOBBER(eol)};
  if (&MEM <const struct QLatin1String[1]> [(void *)&candidates(address-taken)
+ 16B] != _32)

as noted CLOBBERs are not barriers for values but only for memory so any
such forwarding (which would also happen for non-equality compares) interferes
with the intent of -Wdangling-pointer.  I'll note that a CLOBBER does _not_
invalidate the pointer to the storage but only its contents as opposed to
some reading of 'realloc' or 'free' semantics imposed by the C standard.

The documentation mentions two levels of -Wdangling-pointer but all examples
are about either the pointer escaping the function (to the caller) or about
accesses to the storage whose contents became indeterminate.

Unrolling and IVOPTs/SLSR could also expose re-use of storage accessed by
a pointer to the "first" instance of a variable.

I'm not sure what can be done about all this for the late pass_warn_access
(which runs _very_ late).  It's going to be prone to such issues and
maybe -Wanalyzer is a better tool for the purpose.

I was not successful in auto-reducing the testcase to something that
closely resembles the above IL but I guess crafting a manual testcase
from it would be possible.

For the specific case we now pass 'equality' to
pass_waccess::warn_invalid_pointer which is true for the original testcase
but is only used to prune diagnostics after free/realloc and not when
using the (undocumented) -Wdangling-pointer=3 level (level 3 is also rejected
because it has IntegerRange(0, 2)).

This case is about iteration over an auto variable and the "found" check
being forwarded across the storage invalidation.

The following fixes the original (and my misreduced) testcase.  I'm going
to test it and post it for review.

diff --git a/gcc/gimple-ssa-warn-access.cc b/gcc/gimple-ssa-warn-access.cc
index 879dbcc1e52..6c404f18db7 100644
--- a/gcc/gimple-ssa-warn-access.cc
+++ b/gcc/gimple-ssa-warn-access.cc
@@ -3896,13 +3896,13 @@ pass_waccess::warn_invalid_pointer (tree ref, gimple
*us
e_stmt,
        return;
     }

+  if ((equality && warn_use_after_free < 3)
+      || (maybe && warn_use_after_free < 2)
+      || warning_suppressed_p (use_stmt, OPT_Wuse_after_free))
+    return;
+
   if (is_gimple_call (inval_stmt))
     {
-      if ((equality && warn_use_after_free < 3)
-         || (maybe && warn_use_after_free < 2)
-         || warning_suppressed_p (use_stmt, OPT_Wuse_after_free))
-       return;
-
       const tree inval_decl = gimple_call_fndecl (inval_stmt);

       if ((ref && warning_at (use_loc, OPT_Wuse_after_free,
@@ -3923,10 +3923,6 @@ pass_waccess::warn_invalid_pointer (tree ref, gimple
*use_stmt,
       return;
     }

-  if ((maybe && warn_dangling_pointer < 2)
-      || warning_suppressed_p (use_stmt, OPT_Wdangling_pointer_))
-    return;
-
   if (DECL_NAME (var))
     {
       if ((ref

[Bug middle-end/104492] [12 Regression] Bogus dangling pointer warning at -O3

[cvs-commit at gcc dot gnu.org]

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

--- Comment #10 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Richard Biener <rguenth@gcc.gnu.org>:

https://gcc.gnu.org/g:9aaaae7edb781867797d0a553a7db99d52ecd5e1

commit r12-8282-g9aaaae7edb781867797d0a553a7db99d52ecd5e1
Author: Richard Biener <rguenther@suse.de>
Date:   Mon Apr 25 10:46:16 2022 +0200

    middle-end/104492 - avoid all equality compare dangling pointer diags

    The following extends the equality compare dangling pointer diagnostics
    suppression for uses following free or realloc to also cover those
    following invalidation of auto variables via CLOBBERs.  That avoids
    diagnosing idioms like

      return std::find(std::begin(candidates), std::end(candidates), s)
               != std::end(candidates);

    for auto candidates which are prone to forwarding of the final
    comparison across the storage invalidation as then seen by the
    late run access warning pass.

    2022-04-25  Richard Biener  <rguenther@suse.de>

            PR middle-end/104492
            * gimple-ssa-warn-access.cc
            (pass_waccess::warn_invalid_pointer): Exclude equality compare
            diagnostics for all kind of invalidations.
            (pass_waccess::check_dangling_uses): Fix post-dominator query.
            (pass_waccess::check_pointer_uses): Likewise.

[Bug middle-end/104492] [12 Regression] Bogus dangling pointer warning at -O3

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|ASSIGNED                    |RESOLVED

--- Comment #11 from Richard Biener <rguenth at gcc dot gnu.org> ---
Fixed.

[Bug middle-end/104492] [12 Regression] Bogus dangling pointer warning at -O3

[cvs-commit at gcc dot gnu.org]

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

--- Comment #12 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:948e8e401023f6c3153f6d0c449bc5c2899ee7b7

commit r12-8289-g948e8e401023f6c3153f6d0c449bc5c2899ee7b7
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Wed Apr 27 18:47:10 2022 +0200

    testsuite: Add testcase for dangling pointer equality bogus warning
[PR104492]

    On Wed, Apr 27, 2022 at 12:02:33PM +0200, Richard Biener wrote:
    > I did that but the reduction result did not resemble the same failure
    > mode.  I've failed to manually construct a testcase as well.  Possibly
    > a testcase using libstdc++ but less Qt internals might be possible.

    Here is a testcase that I've managed to reduce, FAILs with:
    FAIL: g++.dg/warn/pr104492.C  -std=gnu++14  (test for bogus messages, line
111)
    FAIL: g++.dg/warn/pr104492.C  -std=gnu++17  (test for bogus messages, line
111)
    FAIL: g++.dg/warn/pr104492.C  -std=gnu++20  (test for bogus messages, line
111)
    on both x86_64-linux and i686-linux without your commit and passes with it.

    2022-04-27  Jakub Jelinek  <jakub@redhat.com>

            PR middle-end/104492
            * g++.dg/warn/pr104492.C: New test.