[Bug tree-optimization/96003] New: Maybe a false positive for -Werror=nonnull

[Bug tree-optimization/96003] New: Maybe a false positive for -Werror=nonnull

[marxin at gcc dot gnu.org]

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

            Bug ID: 96003
           Summary: Maybe a false positive for -Werror=nonnull
           Product: gcc
           Version: 11.0
            Status: UNCONFIRMED
          Keywords: rejects-valid
          Severity: normal
          Priority: P3
         Component: tree-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: marxin at gcc dot gnu.org
                CC: msebor at gcc dot gnu.org
  Target Milestone: ---

It's reduced from inkscape project. Happens since r11-1697-g75ff24e1920ea6b1:

$ cat /tmp/folis.ii
class Managed {};
class Anchored {
public:
  void release();
  Managed _anchor;
};
template <typename R> void release(R r) {
  static_cast<Anchored *>(r)->release();
}
class Selection : Managed, public Anchored {};
class AppSelectionModel {
  AppSelectionModel() { release(new Selection); }
};

$ g++ /tmp/folis.ii -Werror=nonnull
/tmp/folis.ii: In instantiation of ‘void release(R) [with R = Selection*]’:
/tmp/folis.ii:12:46:   required from here
/tmp/folis.ii:8:38: error: ‘this’ pointer null [-Werror=nonnull]
    8 |   static_cast<Anchored *>(r)->release();
      |   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~
/tmp/folis.ii:4:8: note: in a call to non-static member function ‘void
Anchored::release()’
    4 |   void release();
      |        ^~~~~~~
cc1plus: some warnings being treated as errors

[Bug tree-optimization/96003] Maybe a false positive for -Werror=nonnull

[jakub at gcc dot gnu.org]

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

--- Comment #1 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
static_cast expanded into ptr ? cast(ptr) : NULL and maybe jump threading
threading that into a NULL->release (); separate path?

[Bug tree-optimization/96003] Maybe a false positive for -Werror=nonnull

[marxin at gcc dot gnu.org]

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

--- Comment #2 from Martin Liška <marxin at gcc dot gnu.org> ---
It happens all much sooner (in FE):

$ cat a-folis.ii.004t.original

;; Function AppSelectionModel::AppSelectionModel() (null)
;; enabled by -tree-original


{
  <<cleanup_point <<< Unknown tree: expr_stmt
  release<Selection*> ((struct Selection *) operator new (2)) >>>>>;
}


;; Function void release(R) [with R = Selection*] (null)
;; enabled by -tree-original


<<cleanup_point <<< Unknown tree: expr_stmt
  Anchored::release (r != 0B ? &r->D.2352 : 0B) >>>>>;

[Bug tree-optimization/96003] Maybe a false positive for -Werror=nonnull

[slyfox at inbox dot ru]

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

Sergei Trofimovich <slyfox at inbox dot ru> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |slyfox at inbox dot ru

--- Comment #3 from Sergei Trofimovich <slyfox at inbox dot ru> ---
Don't know if it's the same problem, but firefox-78 also fails to build with
'-Werror=nonnull' seeming false positives.

cvise produced the following example:

$ cat bug.cpp
    struct a {
      void b();
    };
    struct c {
      template <typename d> class e {
      public:
        int operator*();
        void operator++() { d()->b(); }
        bool operator!=(e);
      };
      e<a *> begin();
      e<a *> end();
    };
    class f {
      c g;
      void h();
    };
    void f::h() {
      for (auto i : g)
        ;
    }

$ x86_64-pc-linux-gnu-g++-10.1.0 -o bug.o -c bug.cpp -std=c++17 -Werror=nonnull
-Wno-unused-variable -O0 -fsyntax-only

$ /home/slyfox/dev/git/gcc-native-quick/gcc/xg++ -B
/home/slyfox/dev/git/gcc-native-quick/gcc -o bug.o -c bug.cpp -std=c++17
-Werror=nonnull -Wno-unused-variable -O0 -fsyntax-only
bug.cpp: In instantiation of 'void c::e<d>::operator++() [with d = a*]':
bug.cpp:19:17:   required from here
bug.cpp:8:31: error: 'this' pointer null [-Werror=nonnull]
    8 |     void operator++() { d()->b(); }
      |                         ~~~~~~^~
bug.cpp:2:8: note: in a call to non-static member function 'void a::b()'
    2 |   void b();
      |        ^
cc1plus: some warnings being treated as errors

[Bug tree-optimization/96003] [11 Regression] Maybe a false positive for -Werror=nonnull

[msebor at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |NEW
           Keywords|rejects-valid               |
   Last reconfirmed|                            |2020-07-09
             Blocks|                            |95507
            Summary|Maybe a false positive for  |[11 Regression] Maybe a
                   |-Werror=nonnull             |false positive for
                   |                            |-Werror=nonnull

--- Comment #4 from Martin Sebor <msebor at gcc dot gnu.org> ---
Confirmed for the test case in comment #0.  It's a warning, not an error
(except with -Werror if requested), so removing rejects-valid.  The warning is
doing what it's designed to do.  The corresponding diagnostic is emitted prior
to r11-1697 for the following C code:

$ cat z.c && gcc -S -Wall -fdump-tree-optimized=/dev/stdout z.c
__attribute__ ((nonnull)) void f (void*);

void g (void **p)
{
  f (p ? *p : 0);
}
z.c: In function ‘g’:
z.c:5:3: warning: argument 1 null where non-null expected [-Wnonnull]
    5 |   f (p ? *p : 0);
      |   ^
z.c:1:32: note: in a call to function ‘f’ declared ‘nonnull’
    1 | __attribute__ ((nonnull)) void f (void*);
      |                                ^

Unless the warning code is removed from the front end and made to rely solely
on the middle end I'm not sure what the generic front end part can do to avoid
triggering.  Perhaps the C++ front end could wrap the 'r != 0B ? &r->D.2352 :
0B' expression somehow to briefly hide it.

The warning in the test case in comment #3 looks correct to me.  The function
template

  template <typename d> void e<d>::operator++() { d()->b(); }

is instantiated as a result of the calls to e<a *> begin() and e<a *> end()
emitted by the for (auto i : g) loop.  With d being e*, the d() expression
evaluates to a null pointer, so the call is ((e*)0)->b().


Referenced Bugs:

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

[Bug c++/96003] [11 Regression] Maybe a false positive for -Werror=nonnull

[msebor at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
          Component|tree-optimization           |c++

--- Comment #5 from Martin Sebor <msebor at gcc dot gnu.org> ---
It's the generic front end code where the problem manifests: adjusting
Component.

[Bug c++/96003] [11 Regression] Maybe a false positive for -Werror=nonnull

[slyfox at inbox dot ru]

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

--- Comment #6 from Sergei Trofimovich <slyfox at inbox dot ru> ---
(In reply to Martin Sebor from comment #4)
> The warning in the test case in comment #3 looks correct to me.

Thank you! I'll try to re-reduce and not introduce new NULLs.

[Bug c++/96003] [11 Regression] Maybe a false positive for -Werror=nonnull

[rguenth at gcc dot gnu.org]

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

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

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

[Bug c++/96003] [11 Regression] Maybe a false positive for -Werror=nonnull

[slyfox at inbox dot ru]

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

--- Comment #7 from Sergei Trofimovich <slyfox at inbox dot ru> ---
Similar example from xmms2 project, dynamic_cast<> version:

  #include <typeinfo>

  // main loop interface
  struct I {
    virtual void run();
  };

  struct M : public I {
    virtual void run();
    void setup_M();
  };

  struct Client
  {
   // In real code Client() always initializes mainloop_ with non-NULL 'M*'
pointer;
   I* mainloop_;
   Client();
   void run_client();

   void connect() {
      if (mainloop_ && typeid(mainloop_) == typeid(M)) {
          dynamic_cast<M*>(mainloop_)->setup_M( );
      }
   }
  };

$ LANG=C x86_64-pc-linux-gnu-g++-11.0.0 -c bug.cpp -o a.o -Werror=nonnull
bug.cpp: In member function 'void Client::connect()':
bug.cpp:22:49: error: 'this' pointer null [-Werror=nonnull]
   22 |           dynamic_cast<M*>(mainloop_)->setup_M( );
      |                                                 ^
bug.cpp:10:10: note: in a call to non-static member function 'void
M::setup_M()'
   10 |     void setup_M();
      |          ^~~~~~~
cc1plus: some warnings being treated as errors

Original code is from
https://github.com/xmms2/xmms2-devel/blob/dedc33d7408e140bce714c2c3eb5bcc793f1af6c/src/clients/lib/xmmsclient%2B%2B/client.cpp#L85

[Bug c++/96003] [11 Regression] Maybe a false positive for -Werror=nonnull

[slyfox at inbox dot ru]

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

--- Comment #8 from Sergei Trofimovich <slyfox at inbox dot ru> ---
(In reply to Sergei Trofimovich from comment #6)
> (In reply to Martin Sebor from comment #4)
> > The warning in the test case in comment #3 looks correct to me.
> 
> Thank you! I'll try to re-reduce and not introduce new NULLs.

Here is the second attempt at reducing firefox case, also related to
static_cast<>:

    struct D;
    struct B {
      B* next;
      D* Next();
    };

    struct D : public B {
      virtual ~D();
    };

    struct Iterator {
      D* current;
      void advance() {
        current = static_cast<B*>(current)->Next();
      }
    };

$ x86_64-pc-linux-gnu-g++ -o bug.o -c bug.cpp -Werror=nonnull
-fno-strict-aliasing
bug.cpp: In member function 'void Iterator::advance()':
bug.cpp:14:46: error: 'this' pointer null [-Werror=nonnull]
   14 |     current = static_cast<B*>(current)->Next();
      |                                              ^
bug.cpp:4:6: note: in a call to non-static member function 'D* B::Next()'
    4 |   D* Next();
      |      ^~~~
cc1plus: some warnings being treated as errors

Also, the "'this' pointer null" error wording is not very clear. Should it be
"'this' pointer is null"? Or "'this' pointer may be null"?

[Bug c++/96003] [11 Regression] Maybe a false positive for -Werror=nonnull

[romain.geissler at amadeus dot com]

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

Romain Geissler <romain.geissler at amadeus dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |romain.geissler at amadeus dot com

--- Comment #9 from Romain Geissler <romain.geissler at amadeus dot com> ---
Hi,

FYI, I tried to build rapidjson with the latest gcc 11, and I also see a lot of
Wnonnull warnings (which I think are false positives too) now. As rapidjson
seems to use -Werror by default, it breaks the build, I am going to remove this
-Werror to unblock me.

FYI, among the many warnings reported, you can see this one:

/workdir/src/rapidjson-1.1.0/include/rapidjson/schema.h:2104:93: error: 'this'
pointer null [-Werror=nonnull]

 2104 |     bool Bool(bool b)       { RAPIDJSON_SCHEMA_HANDLE_VALUE_(Bool,  
(CurrentContext(), b), (b)); }

      |                                                                        
                    ^

/workdir/src/rapidjson-1.1.0/include/rapidjson/schema.h:2089:87: note: in
definition of macro 'RAPIDJSON_SCHEMA_HANDLE_PARALLEL_'

 2089 |                
static_cast<GenericSchemaValidator*>(context->validators[i_])->method arg2;\

      |                                                                        
              ^~~~

/workdir/src/rapidjson-1.1.0/include/rapidjson/schema.h:2104:31: note: in
expansion of macro 'RAPIDJSON_SCHEMA_HANDLE_VALUE_'

 2104 |     bool Bool(bool b)       { RAPIDJSON_SCHEMA_HANDLE_VALUE_(Bool,  
(CurrentContext(), b), (b)); }

      |                               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

/workdir/src/rapidjson-1.1.0/include/rapidjson/schema.h:2104:10: note: in a
call to non-static member function 'bool
rapidjson::GenericSchemaValidator<SchemaDocumentType, OutputHandler,
StateAllocator>::Bool(bool) [with SchemaDocumentType =
rapidjson::GenericSchemaDocument<rapidjson::GenericValue<rapidjson::UTF8<> > >;
OutputHandler = rapidjson::BaseReaderHandler<rapidjson::UTF8<>, void>;
StateAllocator = rapidjson::CrtAllocator]'

 2104 |     bool Bool(bool b)       { RAPIDJSON_SCHEMA_HANDLE_VALUE_(Bool,  
(CurrentContext(), b), (b)); }

      |          ^~~~


All the examples I checked were using the static_cast pattern similar to
comment #8.

Cheers,
Romain

[Bug c++/96003] [11 Regression] Maybe a false positive for -Werror=nonnull

[msebor at gcc dot gnu.org]

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

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

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

[Bug c++/96003] [11 Regression] spurious -Wnonnull calling a member on the result of static_cast

[msebor at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|[11 Regression] Maybe a     |[11 Regression] spurious
                   |false positive for          |-Wnonnull calling a member
                   |-Werror=nonnull             |on the result of
                   |                            |static_cast
           Keywords|                            |patch

--- Comment #10 from Martin Sebor <msebor at gcc dot gnu.org> ---
Patch for the static cast:
https://gcc.gnu.org/pipermail/gcc-patches/2020-July/550231.html

[Bug c++/96003] [11 Regression] spurious -Wnonnull calling a member on the result of static_cast

[sbergman at redhat dot com]

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

--- Comment #11 from Stephan Bergmann <sbergman at redhat dot com> ---
(In reply to Martin Sebor from comment #10)
> Patch for the static cast:
> https://gcc.gnu.org/pipermail/gcc-patches/2020-July/550231.html

LibreOffice runs into the same issue, but while the above patch fixes my first
reduced test case

> $ cat test1.cc
> struct S1 {};
> struct S2: S1 {};
> struct S3: S1 {};
> struct S4: S2, S3 { void f(); };
> void g(S3 * p) { static_cast<S4 *>(p)->f(); }

it does not fix the second

> $ cat test2.cc
> struct S1 { virtual ~S1(); };
> struct S2 { virtual ~S2(); };
> struct S3: S1, S2 { void f() const; };
> void g(S2 * p) { static_cast<S3 const *>(p)->f(); }

> $ g++ -Wnonnull -fsyntax-only test2.cc
> test2.cc: In function ‘void g(S2*)’:
> test2.cc:4:48: warning: ‘this’ pointer null [-Wnonnull]
>     4 | void g(S2 * p) { static_cast<S3 const *>(p)->f(); }
>       |                                                ^
> test2.cc:3:26: note: in a call to non-static member function ‘void S3::f() const’
>     3 | struct S3: S1, S2 { void f() const; };
>       |                          ^

[Bug c++/96003] [11 Regression] spurious -Wnonnull calling a member on the result of static_cast

[msebor at gcc dot gnu.org]

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

--- Comment #12 from Martin Sebor <msebor at gcc dot gnu.org> ---
Thanks for the test case.  In it, the no-warning bit set on the conditional
expression to avoid the warning is cleared before the expression reaches the
warning code.  The culprit seems to be the cp_fold_convert() function called on
the expression: it rebuilds the COND_EXPR but fails to propagate the no-warning
bit.  The change below fixes it but I wouldn't be surprised if this wasn't the
only instance where the no-warning bit might get stripped from an expression.

diff --git a/gcc/fold-const.c b/gcc/fold-const.c
index 300d959278b..67153e3f484 100644
--- a/gcc/fold-const.c
+++ b/gcc/fold-const.c
@@ -8754,6 +8754,7 @@ fold_unary_loc (location_t loc, enum tree_code code, tree
type, tree op0)
            arg02 = fold_build1_loc (loc, code, type,
                                 fold_convert_loc (loc,
                                                   TREE_TYPE (op0), arg02));
+         bool nowarn = TREE_NO_WARNING (op0);
          tem = fold_build3_loc (loc, COND_EXPR, type, TREE_OPERAND (arg0, 0),
                             arg01, arg02);

@@ -8788,6 +8789,8 @@ fold_unary_loc (location_t loc, enum tree_code code, tree
type, tree op0)
                                      TREE_OPERAND (TREE_OPERAND (tem, 1), 0),
                                      TREE_OPERAND (TREE_OPERAND (tem, 2),
                                                    0)));
+         if (nowarn)
+           TREE_NO_WARNING (tem) = true;
          return tem;
        }
    }

[Bug c++/96003] [11 Regression] spurious -Wnonnull calling a member on the result of static_cast

[sbergman at redhat dot com]

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

--- Comment #13 from Stephan Bergmann <sbergman at redhat dot com> ---
FTR, with that second patch building recent LibreOffice succeeds without false
positives.

[Bug c++/96003] [11 Regression] spurious -Wnonnull calling a member on the result of static_cast

[marxin at gcc dot gnu.org]

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

--- Comment #14 from Martin Liška <marxin at gcc dot gnu.org> ---
@Martin: Any progress on this bug?

[Bug c++/96003] [11 Regression] spurious -Wnonnull calling a member on the result of static_cast

[msebor at gcc dot gnu.org]

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

--- Comment #15 from Martin Sebor <msebor at gcc dot gnu.org> ---
The patch I posted two weeks ago is only now being reviewed.

[Bug c++/96003] [11 Regression] spurious -Wnonnull calling a member on the result of static_cast

[cvs-commit at gcc dot gnu.org]

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

--- Comment #16 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:df5cf47a978aaeb53fc2b18ff0b22eb4531a27d8

commit r11-2457-gdf5cf47a978aaeb53fc2b18ff0b22eb4531a27d8
Author: Martin Sebor <msebor@redhat.com>
Date:   Fri Jul 31 10:27:33 2020 -0600

    Set and test no-warning bit to avoid -Wnonnull for synthesized expressions.

    Resolves:
    PR c++/96003 spurious -Wnonnull calling a member on the result of
static_cast

    gcc/c-family/ChangeLog:

            PR c++/96003
            * c-common.c (check_function_arguments_recurse): Return early when
            no-warning bit is set.

    gcc/cp/ChangeLog:

            PR c++/96003
            * class.c (build_base_path): Set no-warning bit on the synthesized
            conditional expression in static_cast.

    gcc/testsuite/ChangeLog:

            PR c++/96003
            * g++.dg/warn/Wnonnull7.C: New test.

[Bug c++/96003] [11 Regression] spurious -Wnonnull calling a member on the result of static_cast

[msebor at gcc dot gnu.org]

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

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

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

--- Comment #17 from Martin Sebor <msebor at gcc dot gnu.org> ---
Warnings for the static cast suppressed in r11-2457.

The warning for the dynamic cast is still issued and I would suggest to use a
cast to a reference instead, both to avoid it and as an indication that the
cast is expected to succeed (or throw):

     if (mainloop_ && typeid(mainloop_) == typeid(M)) {
          dynamic_cast<M&>(*mainloop_).setup_M( );

[Bug c++/96003] [11 Regression] spurious -Wnonnull calling a member on the result of static_cast

[msebor at gcc dot gnu.org]

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

--- Comment #18 from Martin Sebor <msebor at gcc dot gnu.org> ---
(In reply to Sergei Trofimovich from comment #8)
> Also, the "'this' pointer null" error wording is not very clear. Should it
> be "'this' pointer is null"? Or "'this' pointer may be null"?

I agree that the text doesn't read quite right.  It's just a slight rewording
of the generic "argument 1 null..." so both might read better if rephrased as
you suggest.

[Bug c++/96003] [11 Regression] spurious -Wnonnull calling a member on the result of static_cast

[marxin at gcc dot gnu.org]

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

--- Comment #19 from Martin Liška <marxin at gcc dot gnu.org> ---
(In reply to Martin Sebor from comment #17)
> Warnings for the static cast suppressed in r11-2457.
> 
> The warning for the dynamic cast is still issued and I would suggest to use
> a cast to a reference instead, both to avoid it and as an indication that
> the cast is expected to succeed (or throw):
> 
>      if (mainloop_ && typeid(mainloop_) == typeid(M)) {
>           dynamic_cast<M&>(*mainloop_).setup_M( );

I still see quite some packages failing due to the warning in the dynamic_cast
context. Can you please write a note into Porting to section?

[Bug c++/96003] [11 Regression] spurious -Wnonnull calling a member on the result of static_cast

[msebor at gcc dot gnu.org]

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

--- Comment #20 from Martin Sebor <msebor at gcc dot gnu.org> ---
Martin, does the code in the packages follow the pattern below?

$ cat t.C && gcc -O2 -S -Wall t.C
struct A { virtual ~A (); };
struct B { virtual ~B (); void f (); };

void f (A *p)
{
  if (dynamic_cast<B*>(p))
    dynamic_cast<B*>(p)->f ();
}
t.C: In function ‘void f(A*)’:
t.C:7:29: warning: ‘this’ pointer is null [-Wnonnull]
    7 |     dynamic_cast<B*>(p)->f ();
      |                             ^
t.C:2:32: note: in a call to non-static member function ‘void B::f()’
    2 | struct B { virtual ~B (); void f (); };
      |                                ^

[Bug c++/96003] [11 Regression] spurious -Wnonnull calling a member on the result of static_cast

[msebor at gcc dot gnu.org]

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

--- Comment #21 from Martin Sebor <msebor at gcc dot gnu.org> ---
Also, how many warnings for this type of code (or other) do you see?  If there
are too many it might be worth revisiting the decision.

[Bug c++/96003] [11 Regression] spurious -Wnonnull calling a member on the result of static_cast

[marxin at gcc dot gnu.org]

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

--- Comment #22 from Martin Liška <marxin at gcc dot gnu.org> ---
(In reply to Martin Sebor from comment #21)
> Also, how many warnings for this type of code (or other) do you see?  If
> there are too many it might be worth revisiting the decision.

I see it only in 4 packages (out of ~3300) we have in a staging project:

inkscape.log:[  133s] ../src/extension/system.cpp:177:78: error: 'this' pointer
is null [-Werror=nonnull]
inkscape.log:[  133s] ../src/extension/system.cpp:387:79: error: 'this' pointer
is null [-Werror=nonnull]
libyui.log:[  116s]
/home/abuild/rpmbuild/BUILD/libyui-3.12.2/examples/ManyWidgets.cc:133:84:
error: 'this' pointer is null [-Werror=nonnull]
libyui.log:[  116s]
/home/abuild/rpmbuild/BUILD/libyui-3.12.2/examples/ManyWidgets.cc:136:66:
error: 'this' pointer is null [-Werror=nonnull]
libyui-ncurses-pkg.log:[   39s]
/home/abuild/rpmbuild/BUILD/libyui-ncurses-pkg-2.50.10/src/NCPackageSelector.cc:872:68:
error: 'this' pointer is null [-Werror=nonnull]
libyui-ncurses-pkg.log:[   39s]
/home/abuild/rpmbuild/BUILD/libyui-ncurses-pkg-2.50.10/src/NCPackageSelector.cc:1032:68:
error: 'this' pointer is null [-Werror=nonnull]
libyui-ncurses-pkg.log:[   39s]
/home/abuild/rpmbuild/BUILD/libyui-ncurses-pkg-2.50.10/src/NCPackageSelector.cc:1149:68:
error: 'this' pointer is null [-Werror=nonnull]
llvm11.log:[  226s] ../lib/Analysis/LazyValueInfo.cpp:1516:45: warning: 'this'
pointer is null [-Wnonnull]
llvm11.log:[  226s] ../lib/Analysis/LazyValueInfo.cpp:1517:41: warning: 'this'
pointer is null [-Wnonnull]

[Bug c++/96003] [11 Regression] spurious -Wnonnull calling a member on the result of static_cast

[marxin at gcc dot gnu.org]

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

--- Comment #23 from Martin Liška <marxin at gcc dot gnu.org> ---
(In reply to Martin Sebor from comment #20)
> Martin, does the code in the packages follow the pattern below?
> 
> $ cat t.C && gcc -O2 -S -Wall t.C
> struct A { virtual ~A (); };
> struct B { virtual ~B (); void f (); };
> 
> void f (A *p)
> {
>   if (dynamic_cast<B*>(p))
>     dynamic_cast<B*>(p)->f ();
> }
> t.C: In function ‘void f(A*)’:
> t.C:7:29: warning: ‘this’ pointer is null [-Wnonnull]
>     7 |     dynamic_cast<B*>(p)->f ();
>       |                             ^
> t.C:2:32: note: in a call to non-static member function ‘void B::f()’
>     2 | struct B { virtual ~B (); void f (); };
>       |                                ^

I guess so, looking at the libyui-ncurses-pkg issue:

class YWidget {...}

class NCWidget : public tnode<NCWidget*>, protected NCursesError {...}

wrect NCPackageSelector::deleteReplacePoint()
{
    // delete current child of the ReplacePoint
    YWidget * replaceChild = replacePoint->firstChild();
    wrect oldSize;

    if ( replaceChild )
    {
        oldSize = dynamic_cast<NCWidget *>(replaceChild)->wGetSize();
...

That seems to me like a bug as NCWidget is not the base of YWidget.
Am I right?

[Bug c++/96003] [11 Regression] spurious -Wnonnull calling a member on the result of static_cast

[msebor at gcc dot gnu.org]

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

--- Comment #24 from Martin Sebor <msebor at gcc dot gnu.org> ---
I see.  Yes, if the types are unrelated, that would be a likely bug.   I think
could and should be diagnosed by the C++ front end, by some more targeted
warning than -Wnonnull (as requested in pr38557).

But I didn't actually mean to write that test case in comment #20 (I forgot to
derive one class from the other).  The following is what I meant where the
-Wnonnull is strictly a false positive because of the test.  It can be avoided
by casting *p to C& which might be argued (as I do in comment #17) makes the
code clearer, but if this is a common idiom we could try to suppress the
warning in these cases as well.  Do you see this pattern a lot?

struct A { virtual ~A (); };
struct C: A { virtual ~C (); void f (); };

void g (A *p)
{
  if (dynamic_cast<C*>(p))
    dynamic_cast<C*>(p)->f ();   // -Wnonnull

  /* Avoid like so:
  if (dynamic_cast<C*>(p))
    dynamic_cast<C&>(*p)->f ();  */

}

[Bug c++/96003] [11 Regression] spurious -Wnonnull calling a member on the result of static_cast

[marxin at gcc dot gnu.org]

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

--- Comment #25 from Martin Liška <marxin at gcc dot gnu.org> ---
(In reply to Martin Sebor from comment #24)
> I see.  Yes, if the types are unrelated, that would be a likely bug.   I
> think could and should be diagnosed by the C++ front end, by some more
> targeted warning than -Wnonnull (as requested in pr38557).
> 
> But I didn't actually mean to write that test case in comment #20 (I forgot
> to derive one class from the other).  The following is what I meant where
> the -Wnonnull is strictly a false positive because of the test.  It can be
> avoided by casting *p to C& which might be argued (as I do in comment #17)
> makes the code clearer, but if this is a common idiom we could try to
> suppress the warning in these cases as well.  Do you see this pattern a lot?

I see the pattern in libyui-3.12.2 package.

> 
> struct A { virtual ~A (); };
> struct C: A { virtual ~C (); void f (); };
> 
> void g (A *p)
> {
>   if (dynamic_cast<C*>(p))
>     dynamic_cast<C*>(p)->f ();   // -Wnonnull
> 
>   /* Avoid like so:
>   if (dynamic_cast<C*>(p))
>     dynamic_cast<C&>(*p)->f ();  */

dynamic_cast<C&>(*p).f ();  */

works for me as a workaround of the issue.

[Bug c++/96003] [11 Regression] spurious -Wnonnull calling a member on the result of static_cast

[marxin at gcc dot gnu.org]

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

--- Comment #26 from Martin Liška <marxin at gcc dot gnu.org> ---
> I see the pattern in libyui-3.12.2 package.

and the second affected package is inkscape:
https://gitlab.com/inkscape/inkscape/-/issues/2206

[Bug c++/96003] [11 Regression] spurious -Wnonnull calling a member on the result of static_cast

[nyh at math dot technion.ac.il]

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

Nadav Har'El <nyh at math dot technion.ac.il> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |nyh at math dot technion.ac.il

--- Comment #27 from Nadav Har'El <nyh at math dot technion.ac.il> ---
I think this problem still exists at least in some form, and should be
reopened. It just hit the Seastar project, with gcc 11.1.1 -
https://github.com/scylladb/seastar/issues/914.

The problem there is that it uses C-style callbacks (of the c-ares library)
that cannot be modified; The callback gets a void* argument. We pass a C++'s
object's address into this void*, and then inside the callback function itself,
cast the void* back (using reinterpret_cast) to the object pointer - and then
attempt to run methods of this pointer. Here is where gcc 11.1.1. warns us that
this pointer may be a null (the message erroneously says "is a null") -
although I know for a fact it cannot be a null, and sadly even adding an
assert(p) to tell the compiler I'm sure it is not a null - doesn't help.