[Bug c++/98646] New: A static_cast confuses -Wnonnull, causing false positives

[Bug c++/98646] New: A static_cast confuses -Wnonnull, causing false positives

[ville.voutilainen at gmail dot com]

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

            Bug ID: 98646
           Summary: A static_cast confuses -Wnonnull, causing false
                    positives
           Product: gcc
           Version: 11.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: ville.voutilainen at gmail dot com
  Target Milestone: ---

Created attachment 49957
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=49957&action=edit
Preprocessed source

Compile with

/usr/local/bin/c++ -DQT_ASCII_CAST_WARNINGS -DQT_BUILDING_QT
-DQT_BUILD_XCB_PLUGIN -DQT_BUILD_XCB_QPA_LIB_LIB -DQT_CORE_LIB
-DQT_DEPRECATED_WARNINGS -DQT_DEPRECATED_WARNINGS_SINCE=0x060000
-DQT_DISABLE_DEPRECATED_BEFORE=0x050000 -DQT_GUI_LIB -DQT_MOC_COMPAT
-DQT_NO_CAST_TO_ASCII -DQT_NO_EXCEPTIONS -DQT_NO_FOREACH -DQT_OPENGL_LIB
-DQT_USE_QSTRINGBUILDER -DXcbQpa_EXPORTS -D_LARGEFILE64_SOURCE
-D_LARGEFILE_SOURCE -fPIC -fvisibility=hidden -fvisibility-inlines-hidden -Wall
-Wextra -fno-exceptions -fPIC -Werror -Wno-error=cpp
-Wno-error=deprecated-declarations -Wno-error=strict-overflow
-Wno-error=implicit-fallthrough -Wno-error=deprecated-copy
-Wno-error=redundant-move -Wno-error=init-list-lifetime
-Wno-error=format-overflow -Wsuggest-override -std=c++17 -c winid-nonnull.cpp

The result is
/home/vivoutil/kuutti/kuuttikutonen/qt5/qtbase/src/plugins/platforms/xcb/qxcbwindow.cpp:
In member function ‘void QXcbWindow::show()’:
/home/vivoutil/kuutti/kuuttikutonen/qt5/qtbase/src/plugins/platforms/xcb/qxcbwindow.cpp:685:91:
error: ‘this’ pointer null [-Werror=nonnull]
  685 |                 transientXcbParent = static_cast<const QXcbWindow
*>(tp->handle())->winId();
      |                                                                        
                  ^
In file included from
/home/vivoutil/kuutti/kuuttikutonen/qt5/qtbase/src/plugins/platforms/xcb/qxcbwindow.cpp:40:
/home/vivoutil/kuutti/kuuttikutonen/qt5/qtbase/src/plugins/platforms/xcb/qxcbwindow.h:87:9:
note: in a call to non-static member function ‘virtual WId QXcbWindow::winId()
const’
   87 |     WId winId() const override;
      |         ^~~~~

Removing the static_cast removes the warning.

[Bug c++/98646] [11 Regression] A static_cast confuses -Wnonnull, causing false positives

[msebor at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
           See Also|                            |https://gcc.gnu.org/bugzill
                   |                            |a/show_bug.cgi?id=96003
             Blocks|                            |95507
             Status|UNCONFIRMED                 |NEW
           Keywords|                            |diagnostic
            Summary|A static_cast confuses      |[11 Regression] A
                   |-Wnonnull, causing false    |static_cast confuses
                   |positives                   |-Wnonnull, causing false
                   |                            |positives
   Last reconfirmed|                            |2021-01-13

--- Comment #1 from Martin Sebor <msebor at gcc dot gnu.org> ---
Confirmed.  It must be an instance we missed in the fix for pr96003 where the
C++ front end adds a COND_EXPR to static_cast.

The larger context in the translation unit is:

  if (tp && tp->handle())
    transientXcbParent = static_cast<const QXcbWindow
*>(tp->handle())->winId();

The caller guards against tp->handle() being null but the front end doesn't
consider that and adds another guard which then triggers the warning that also
runs in the front end.  The pr96003 fix works around the same problem by
setting the TREE_NO_WARNING bit so the same hack is needed wherever else the
front end builds a static_cast.


Referenced Bugs:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95507
[Bug 95507] [meta-bug] bogus/missing -Wnonnull

[Bug c++/98646] [11 Regression] A static_cast confuses -Wnonnull, causing false positives

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |11.0

[Bug c++/98646] [11 Regression] A static_cast confuses -Wnonnull, causing false positives

[mpolacek at gcc dot gnu.org]

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

Marek Polacek <mpolacek at gcc dot gnu.org> changed:

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

--- Comment #2 from Marek Polacek <mpolacek at gcc dot gnu.org> ---
// PR c++/98646
// { dg-do compile }
// { dg-options "-Wnonnull" }

struct B {
  void foo();
};
struct D : B {
  void show();
};
void D::show() { static_cast<D *>(0)->foo(); }

[Bug c++/98646] [11 Regression] A static_cast confuses -Wnonnull, causing false positives

[mpolacek at gcc dot gnu.org]

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

--- Comment #3 from Marek Polacek <mpolacek at gcc dot gnu.org> ---
Started with r11-1697.

[Bug c++/98646] [11 Regression] A static_cast confuses -Wnonnull, causing false positives

[mpolacek at gcc dot gnu.org]

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

--- Comment #4 from Marek Polacek <mpolacek at gcc dot gnu.org> ---
The reduced testcase is unfortunately overreduced :(

[Bug c++/98646] [11 Regression] A static_cast confuses -Wnonnull, causing false positives

[mpolacek at gcc dot gnu.org]

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

--- Comment #5 from Marek Polacek <mpolacek at gcc dot gnu.org> ---
A better one:

// PR c++/98646
// { dg-do compile }
// { dg-options "-Wnonnull" }

struct B {
  void foo();
};

struct D : B {
  void show();
};

void
D::show()
{
  constexpr void *p = nullptr;
  if (p)
    static_cast<D *>(p)->foo();
}

[Bug c++/98646] [11 Regression] A static_cast confuses -Wnonnull, causing false positives

[rguenth at gcc dot gnu.org]

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

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

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

[Bug c++/98646] [11 Regression] A static_cast confuses -Wnonnull, causing false positives

[mpolacek at gcc dot gnu.org]

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

--- Comment #6 from Marek Polacek <mpolacek at gcc dot gnu.org> ---
(In reply to Martin Sebor from comment #1)
> Confirmed.  It must be an instance we missed in the fix for pr96003 where
> the C++ front end adds a COND_EXPR to static_cast.
> 
> The larger context in the translation unit is:
> 
>   if (tp && tp->handle())
>     transientXcbParent = static_cast<const QXcbWindow
> *>(tp->handle())->winId();
> 
> The caller guards against tp->handle() being null but the front end doesn't
> consider that and adds another guard which then triggers the warning that
> also runs in the front end.  The pr96003 fix works around the same problem
> by setting the TREE_NO_WARNING bit so the same hack is needed wherever else
> the front end builds a static_cast.

This won't work here, because we're not creating the null test; build_base_path
is called from build_over_call:

 8902       tree converted_arg = build_base_path (PLUS_EXPR, arg,
 8903                                             base_binfo, 1, complain);

where the 1 means nonnull is true.

The warning probably has to be moved out of the FE.  Null 'this' has to be
detected in constexpr context, that is bug 97230.

[Bug c++/98646] [11 Regression] A static_cast confuses -Wnonnull, causing false positives

[mpolacek at gcc dot gnu.org]

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

--- Comment #7 from Marek Polacek <mpolacek at gcc dot gnu.org> ---
Or, disable it when we call build_base_path with nonnull == 1?

[Bug c++/98646] [11 Regression] A static_cast confuses -Wnonnull, causing false positives

[jakub at gcc dot gnu.org]

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

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> ---
The warning used to be in the FEs but that didn't work, see PR69835 for
details.

[Bug c++/98646] [11 Regression] A static_cast confuses -Wnonnull, causing false positives

[msebor at gcc dot gnu.org]

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

--- Comment #9 from Martin Sebor <msebor at gcc dot gnu.org> ---
-Wnonnull still is in the front end (in addition to the middle end).  This
instance is issued by check_nonnull_arg in c-family/c-common.c, but other
similar instances are issued from tree-ssa-ccp.c.

Rather than twiddling the NO_WARNING bit I wonder if changing the C++ front end
to emit an internal function for these casts and expanding it in the gimplifier
would be a better solution.  check_nonnull_arg would then have nothing to
complain about.

[Bug c++/98646] [11 Regression] A static_cast confuses -Wnonnull, causing false positives

[jakub at gcc dot gnu.org]

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

--- Comment #10 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
I don't see how either TREE_NO_WARNING or some magic call would help.
Because the user can also write:
// PR c++/98646
// { dg-do compile }
// { dg-options "-Wnonnull" }

struct B { void foo (); };
struct D : B { void show (); };

void
D::show ()
{
  constexpr D *p = nullptr;
  if (p)
    p->foo ();
}

and that also warns and IMNSHO should not.

We should warn for:
struct B { void foo (); };
struct D : B { void show (); };

void
D::show ()
{
  constexpr D *p = nullptr;
  p->foo ();
}

So, the problem is IMHO that the warning about passing NULL to this is
misplaced, it shouldn't be done in the FEs, but later when there was at least
some chance of dead code removal.

[Bug c++/98646] [11 Regression] A static_cast confuses -Wnonnull, causing false positives

[msebor at gcc dot gnu.org]

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

--- Comment #11 from Martin Sebor <msebor at gcc dot gnu.org> ---
(In reply to Marek Polacek from comment #5)
> A better one:
> 
> // PR c++/98646
> // { dg-do compile }
> // { dg-options "-Wnonnull" }
> 
> struct B {
>   void foo();
> };
> 
> struct D : B {
>   void show();
> };
> 
> void
> D::show()
> {
>   constexpr void *p = nullptr;
>   if (p)
>     static_cast<D *>(p)->foo();
> }

This test case isn't representative of the reported problem.  It shows the
general issue with the front-end part of -Wnonnull as well as with and most
other flow-based front end warnings.  An equivalent test case that shows the
same issue with GCC 10 and prior is this:

  __attribute__ ((nonnull)) void f (void*);

  void g (void)
  {
    constexpr void *q = 0;
    if (q)
      f (q);   // -Wnonnull
  }

Because the front ends operate on one expression at a time they can't see past
the expression boundary and so these kinds of examples will always trigger
false positives in such warnings.

The new problem reported in comment #0 that's unique in GCC 11 is due to the
COND_EXPR that's implicitly added by the C++ front end for casts between
related types.  The following is the expression that triggers the warning in
the attached test case, the this argument to virtual WId QXcbWindow::winId()
const:

SAVE_EXPR <QWindow::handle (NON_LVALUE_EXPR <VIEW_CONVERT_EXPR<const struct
QWindow *>(tp)>)> != 0B ? (const struct QXcbWindow *) (SAVE_EXPR
<QWindow::handle (NON_LVALUE_EXPR <VIEW_CONVERT_EXPR<const struct QWindow
*>(tp)>)> + 18446744073709551600) : 0B

The third operand of the COND_EXPR (i.e., the 0B) triggers the front end
warning because check_function_arguments_recurse() is designed to look for it.

The only bug I see here is not that the warning can be triggered in an if
statement with an unreachable branch but that it can be triggered by the
implicit literal null inserted by the front end that's not in the source code.

A test case that more closely reflects the problem is this 

$ cat pr98646.C && gcc -S -Wall -fdump-tree-original=/dev/stdout pr98646.C
struct A { virtual ~A (); };
struct B { virtual ~B (); B* f (); };

struct C: A, B { virtual ~C (); void g () const; };

void f (B *p)
{
  static_cast<C*>(p->f ())->g ();          // no warning
}

void g (B *p)
{
  static_cast<const C*>(p->f ())->g ();    // bogus -Wnonnull
}


;; Function void f(B*) (null)
;; enabled by -tree-original


<<cleanup_point <<< Unknown tree: expr_stmt
  C::g (SAVE_EXPR <B::f (NON_LVALUE_EXPR <p>)> != 0B ? (struct C *) (SAVE_EXPR
<B::f (NON_LVALUE_EXPR <p>)> + 18446744073709551608) : 0B) >>>>>;

pr98646.C: In function ‘void g(B*)’:
pr98646.C:13:38: warning: ‘this’ pointer null [-Wnonnull]
   13 |   static_cast<const C*>(p->f ())->g ();    // bogus -Wnonnull
      |                                      ^
pr98646.C:4:38: note: in a call to non-static member function ‘void C::g()
const’
    4 | struct C: A, B { virtual ~C (); void g () const; };
      |                                      ^

;; Function void g(B*) (null)
;; enabled by -tree-original


<<cleanup_point <<< Unknown tree: expr_stmt
  C::g (SAVE_EXPR <B::f (NON_LVALUE_EXPR <p>)> != 0B ? (const struct C *)
(SAVE_EXPR <B::f (NON_LVALUE_EXPR <p>)> + 18446744073709551608) : 0B) >>>>>;


But the problem isn't new to GCC 11.  The same warning can be triggered in GCC
10 with a slightly modified test case:

$ cat pr98646.C && /build/gcc-10-branch/gcc/xgcc -B /build/gcc-10-branch/gcc -S
-Wall pr98646.C
struct A { virtual ~A (); };
struct B { virtual ~B (); B* f (); };

struct C: A, B { virtual ~C (); };

__attribute__ ((nonnull)) void g (const void*);

void f (B *p)
{
  g (static_cast<C*>(p->f ()));          // bogus -Wnonnull
}

void g (B *p)
{
  g (static_cast<const C*>(p->f ()));    // bogus -Wnonnull
}
pr98646.C: In function ‘void f(B*)’:
pr98646.C:10:30: warning: null argument where non-null required (argument 1)
[-Wnonnull]
   10 |   g (static_cast<C*>(p->f ()));          // bogus -Wnonnull
      |                              ^
pr98646.C: In function ‘void g(B*)’:
pr98646.C:15:36: warning: null argument where non-null required (argument 1)
[-Wnonnull]
   15 |   g (static_cast<const C*>(p->f ()));    // bogus -Wnonnull
      |                                    ^

The only new aspect of it is that in GCC 11 it triggers for member functions.

[Bug c++/98646] [11 Regression] A static_cast confuses -Wnonnull, causing false positives

[msebor at gcc dot gnu.org]

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

--- Comment #12 from Martin Sebor <msebor at gcc dot gnu.org> ---
(In reply to Jakub Jelinek from comment #10)
> So, the problem is IMHO that the warning about passing NULL to this is
> misplaced, it shouldn't be done in the FEs, but later when there was at
> least some chance of dead code removal.

That would be a hack.  First, the this argument to a member function is just
like a plain argument to an ordinary nonnull function, so they all should be
treated the same.  Second, there already is code in tree-ssa-ccp.c to handle
-Wnonnull, so nothing needs to be moved.

The -Wnonnull code could be removed from the front end, but that would
considerably compromise the warning: passing null pointers to inline member
functions would cease to be diagnosed.  See for example the effect on the code
below.  In C++ that wouldn't be a good deal.

$ cat u.C && gcc -O2 -S -Wall -fdump-tree-optimized=/dev/stdout u.C
struct S {
  int i;
  void f () { i = 0; };
  void g ();
};

void f (void)
{
  S *p = 0;
  p->f ();   // missing warning
}

void g (void)
{
  S *p = 0;
  p->g ();   // -Wnonnull (good)
}

;; Function f (_Z1fv, funcdef_no=1, decl_uid=2357, cgraph_uid=2,
symbol_order=1) (executed once)

void f ()
{
  <bb 2> [local count: 1073741824]:
  MEM[(struct S *)0B].i ={v} 0;
  __builtin_trap ();

}


u.C: In function ‘void g()’:
u.C:16:8: warning: ‘this’ pointer null [-Wnonnull]
   16 |   p->g ();   // -Wnonnull (good)
      |   ~~~~~^~
u.C:4:8: note: in a call to non-static member function ‘void S::g()’
    4 |   void g ();
      |        ^

;; Function g (_Z1gv, funcdef_no=2, decl_uid=2360, cgraph_uid=3,
symbol_order=2)

void g ()
{
  <bb 2> [local count: 1073741824]:
  S::g (0B); [tail call]
  return;

}


Also, removing the -Wnonnull handling from the FE wouldn't actually solve the
reported problem.  It would just paper over it because the middle end warning
doesn't consider PHIs yet.  If/when it's enhanced to consider them the warning
would come back again as is plain to see in the dump for the test case from
comment #11:

$ cat pr98646.C && gcc -O2 -S -Wall -fdump-tree-optimized=/dev/stdout pr98646.C
struct A { virtual ~A (); };
struct B { virtual ~B (); B* f (); };

struct C: A, B { virtual ~C (); void g () const; };

void f (B *p)
{
  static_cast<C*>(p->f ())->g ();          // bogus -Wnonnull
}

;; Function f (_Z1fP1B, funcdef_no=0, decl_uid=2430, cgraph_uid=1,
symbol_order=0)

Removing basic block 5
void f (struct B * p)
{
  struct C * iftmp.0_1;
  struct B * _5;
  struct C * iftmp.0_6;

  <bb 2> [local count: 1073741824]:
  _5 = B::f (p_3(D));
  if (_5 != 0B)
    goto <bb 3>; [70.00%]
  else
    goto <bb 4>; [30.00%]

  <bb 3> [local count: 751619281]:
  iftmp.0_6 = _5 + 18446744073709551608;

  <bb 4> [local count: 1073741824]:
  # iftmp.0_1 = PHI <iftmp.0_6(3), _5(2)>
  C::g (iftmp.0_1); [tail call]     <<< should get a warning: pointer may be
null
  return;

}

We can see that already today by running the analyzer on the test case:

pr98646.C: In function ‘void f(B*)’:
pr98646.C:8:31: warning: use of NULL where non-null expected [CWE-476]
[-Wanalyzer-null-argument]
    8 |   static_cast<C*>(p->f ())->g ();          // bogus -Wnonnull
      |   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~
  ‘void f(B*)’: events 1-3
    |
    |
pr98646.C:4:38: note: argument 'this' of ‘void C::g() const’ must be non-null
    4 | struct C: A, B { virtual ~C (); void g () const; };
      |                                      ^

Which is why, if we want to avoid these warnings, we need the front end to
annotate the implicit COND_EXPR somehow.  It could be done by setting
TREE_NO_WARNING but I'd worry about it getting lost or unintentionally
suppressing important warnings.  Adding a special internal function instead and
expanding it after the first -Wnonnull handler would be better.

[Bug c++/98646] [11 Regression] A static_cast confuses -Wnonnull, causing false positives

[msebor at gcc dot gnu.org]

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

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

[Bug c++/98646] [11 Regression] A static_cast confuses -Wnonnull, causing false positives

[msebor at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |patch

--- Comment #13 from Martin Sebor <msebor at gcc dot gnu.org> ---
A simple patch that just makes sure the NO_WARNING bits stays set:
https://gcc.gnu.org/pipermail/gcc-patches/2021-January/563944.html

[Bug c++/98646] [11 Regression] A static_cast confuses -Wnonnull, causing false positives

[cvs-commit at gcc dot gnu.org]

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

--- Comment #14 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Martin Sebor <msebor@gcc.gnu.org>:

https://gcc.gnu.org/g:d6f1cf644c45b76a27b6a6869dedaa030e3c7570

commit r11-6900-gd6f1cf644c45b76a27b6a6869dedaa030e3c7570
Author: Martin Sebor <msebor@redhat.com>
Date:   Mon Jan 25 12:41:28 2021 -0700

    PR c++/98646 - spurious -Wnonnull calling a member on the result of
static_cast

    gcc/c-family/ChangeLog:

            PR c++/98646
            * c-common.c (check_nonnull_arg): Adjust warning text.

    gcc/cp/ChangeLog:

            PR c++/98646
            * cvt.c (cp_fold_convert): Propagate TREE_NO_WARNING.

    gcc/ChangeLog:

            PR c++/98646
            * tree-ssa-ccp.c (pass_post_ipa_warn::execute): Adjust warning
text.

    gcc/testsuite/ChangeLog:

            PR c++/98646
            * g++.dg/warn/Wnonnull5.C: Adjust text of an expected warning.
            * g++.dg/warn/Wnonnull10.C: New test.
            * g++.dg/warn/Wnonnull9.C: New test.

[Bug c++/98646] [11 Regression] A static_cast confuses -Wnonnull, causing false positives

[msebor at gcc dot gnu.org]

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

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

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

--- Comment #15 from Martin Sebor <msebor at gcc dot gnu.org> ---
Fixed in r11-6900.