Bug 69850 - [6 Regression] unnecessary -Wnonnull-compare warning
Summary: [6 Regression] unnecessary -Wnonnull-compare warning
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: c++ (show other bugs)
Version: 6.0
: P1 normal
Target Milestone: 6.0
Assignee: Jakub Jelinek
URL:
Keywords: diagnostic
Depends on:
Blocks:
 
Reported: 2016-02-17 08:04 UTC by Markus Trippelsdorf
Modified: 2016-02-19 19:18 UTC (History)
4 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2016-02-17 00:00:00


Attachments
gcc6-pr69850.patch (1010 bytes, patch)
2016-02-17 13:12 UTC, Jakub Jelinek
Details | Diff
unreduced testcase (449.64 KB, application/x-bzip)
2016-02-18 09:44 UTC, Markus Trippelsdorf
Details
gcc6-pr69850.patch (679 bytes, patch)
2016-02-18 13:20 UTC, Jakub Jelinek
Details | Diff
unreduced testcase (459.25 KB, application/x-bzip)
2016-02-18 13:59 UTC, Markus Trippelsdorf
Details
gcc6-pr69850.patch (1.19 KB, patch)
2016-02-18 15:22 UTC, Jakub Jelinek
Details | Diff
gcc6-pr69850-3.patch (796 bytes, patch)
2016-02-18 17:24 UTC, Jakub Jelinek
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Markus Trippelsdorf 2016-02-17 08:04:04 UTC
trippels@gcc2-power8 status % cat cmp_set.ii
class shared_count {
public:
  ~shared_count() { delete this; }
};
class A {
  shared_count pn;
};
class B {
public:
  B(bool);
  A m_message;
};
void fn1(B) { fn1(0); }

trippels@gcc2-power8 status % g++ -Wall -O2 -c cmp_set.ii
cmp_set.ii: In destructor ‘shared_count::~shared_count()’:
cmp_set.ii:3:32: warning: nonnull argument ‘this’ compared to NULL [-Wnonnull-compare]
   ~shared_count() { delete this; }
                                ^
Comment 1 Markus Trippelsdorf 2016-02-17 08:08:23 UTC
Obviously started with r233472.
Comment 2 Jakub Jelinek 2016-02-17 13:12:40 UTC
Created attachment 37723 [details]
gcc6-pr69850.patch

Untested fix.  Another possibility is to STRIP_NOPS and if it is this, set ifexp to integer_one_node.  But we need the TREE_NO_WARNING stuff anyway, because e.g. in function with nonnull parameter other than this, that at some point is changed, the comparison would be still desirable and valid.
Comment 3 Manuel López-Ibáñez 2016-02-17 17:34:11 UTC
(In reply to Jakub Jelinek from comment #2)
> Created attachment 37723 [details]
> gcc6-pr69850.patch
> 
> Untested fix.  Another possibility is to STRIP_NOPS and if it is this, set
> ifexp to integer_one_node.  But we need the TREE_NO_WARNING stuff anyway,
> because e.g. in function with nonnull parameter other than this, that at
> some point is changed, the comparison would be still desirable and valid.

Aren't compiler-generated expressions marked with ARTIFICIAL or some such? In any case, setting TREE_NO_WARNING for compiler-generated expressions seems the right thing to do.

I'd suggest to add "-Wnonnul-compare" explicitly to the testcase, otherwise it may start silently failing if -Wall stops enabling the warning.
Comment 4 Jakub Jelinek 2016-02-17 22:27:56 UTC
Author: jakub
Date: Wed Feb 17 22:27:24 2016
New Revision: 233508

URL: https://gcc.gnu.org/viewcvs?rev=233508&root=gcc&view=rev
Log:
	PR c++/69850
	* gimplify.c (gimplify_cond_expr): Call gimple_set_no_warning
	on the cond_stmt from TREE_NO_WARNING on COND_EXPR_COND.
	* gimple-ssa-nonnull-compare.c (do_warn_nonnull_compare): Don't
	warn on gimple_no_warning_p statements.

	* init.c (build_delete): Set TREE_NO_WARNING on ifexp.

	* g++.dg/warn/Wnonnull-compare-1.C: New test.

Added:
    trunk/gcc/testsuite/g++.dg/warn/Wnonnull-compare-1.C
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/cp/ChangeLog
    trunk/gcc/cp/init.c
    trunk/gcc/gimple-ssa-nonnull-compare.c
    trunk/gcc/gimplify.c
    trunk/gcc/testsuite/ChangeLog
Comment 5 Jakub Jelinek 2016-02-17 22:54:07 UTC
Fixed.
Comment 6 Markus Trippelsdorf 2016-02-18 09:44:50 UTC
Created attachment 37728 [details]
unreduced testcase

trippels@gcc2-power8 llvm_build % g++ -Wnonnull-compare -Werror -c  VariantValue.ii
In file included from /home/trippels/llvm/tools/clang/include/clang/Basic/DiagnosticIDs.h:19:0,
                 from /home/trippels/llvm/tools/clang/include/clang/Basic/Diagnostic.h:18,
                 from /home/trippels/llvm/tools/clang/include/clang/Basic/PartialDiagnostic.h:19,
                 from /home/trippels/llvm/tools/clang/include/clang/AST/DeclarationName.h:17,
                 from /home/trippels/llvm/tools/clang/include/clang/AST/DeclBase.h:18,
                 from /home/trippels/llvm/tools/clang/include/clang/AST/Decl.h:18,
                 from /home/trippels/llvm/tools/clang/include/clang/AST/ASTTypeTraits.h:20,
                 from /home/trippels/llvm/tools/clang/include/clang/AST/ASTContext.h:18,
                 from /home/trippels/llvm/tools/clang/include/clang/ASTMatchers/ASTMatchers.h:48,
                 from /home/trippels/llvm/tools/clang/include/clang/ASTMatchers/Dynamic/VariantValue.h:20,
                 from /home/trippels/llvm/tools/clang/lib/ASTMatchers/Dynamic/VariantValue.cpp:15:
/home/trippels/llvm/include/llvm/ADT/IntrusiveRefCntPtr.h: In member function ‘void llvm::RefCountedBaseVPTR::Release() const’:
/home/trippels/llvm/include/llvm/ADT/IntrusiveRefCntPtr.h:79:38: error: nonnull argument ‘this’ compared to NULL [-Werror=nonnull-compare]
       if (--ref_cnt == 0) delete this;
                                      ^
cc1plus: all warnings being treated as errors
Comment 7 Markus Trippelsdorf 2016-02-18 12:33:55 UTC
(wow, llvm approaches Boost in the complexity of testcases. It takes ages to reduce them.) 


class RefCountedBaseVPTR {
  virtual ~RefCountedBaseVPTR();
  void Release() const { delete this; }
  template <typename> friend struct A;
};
template <typename T> struct A {
  static void release(T *p1) { p1->Release(); }
};
template <typename T> class B {
  T Obj;
public:
  void reset() { A<T>::release(&Obj); }
};
class C {
  class Payload : public RefCountedBaseVPTR {
    ~Payload();
  };
  void reset();
  B<Payload> Value;
};
void C::reset() { Value.reset(); }
Comment 8 Jakub Jelinek 2016-02-18 13:20:21 UTC
Created attachment 37730 [details]
gcc6-pr69850.patch

Untested fix.
Comment 9 Jakub Jelinek 2016-02-18 13:20:57 UTC
Reopening for the #c7 issue.
Comment 10 Markus Trippelsdorf 2016-02-18 13:52:40 UTC
Still happening even with latest patch:

[1763/2142] Building CXX object tools/clang/lib/StaticAnalyzer/Core/CMakeFiles/clangStaticAnalyzerCore.dir/HTMLDiagnostics.cpp.o
In file included from /home/trippels/llvm/tools/clang/include/clang/Rewrite/Core/RewriteBuffer.h:15:0,
                 from /home/trippels/llvm/tools/clang/include/clang/Rewrite/Core/Rewriter.h:19,
                 from /home/trippels/llvm/tools/clang/lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp:21:
/home/trippels/llvm/tools/clang/include/clang/Rewrite/Core/RewriteRope.h: In member function ‘void clang::RopeRefCountString::Release()’:
/home/trippels/llvm/tools/clang/include/clang/Rewrite/Core/RewriteRope.h:43:30: warning: nonnull argument ‘this’ compared to NULL [-Wnonnull-compare]
         delete [] (char*)this;
                              ^
Comment 11 Jakub Jelinek 2016-02-18 13:56:04 UTC
Can you attach it again?
Comment 12 Markus Trippelsdorf 2016-02-18 13:59:17 UTC
Created attachment 37731 [details]
unreduced testcase
Comment 13 Markus Trippelsdorf 2016-02-18 14:32:55 UTC
template <typename T> struct A {
  static void release(T *p1) { p1->Release(); }
};
template <typename T> class IntrusiveRefCntPtr {
  T Obj;
public:
  void operator=(IntrusiveRefCntPtr) { A<T>::release(&Obj); }
};
struct RopeRefCountString {
  void Release() { delete[] this; }
};
struct RopePiece {
  IntrusiveRefCntPtr<RopeRefCountString> StrData;
};
class RopePieceBTreeNode {
protected:
  enum { WidthFactor };
};
class RopePieceBTreeLeaf : RopePieceBTreeNode {
  RopePiece Pieces[WidthFactor];
  RopePieceBTreeNode *insert(const RopePiece &);
};
int a;
RopePieceBTreeNode *RopePieceBTreeLeaf::insert(const RopePiece &) {
  Pieces[a] = Pieces[1];
}
Comment 14 Jakub Jelinek 2016-02-18 15:22:47 UTC
Created attachment 37732 [details]
gcc6-pr69850.patch

Updated fix.
Comment 15 Markus Trippelsdorf 2016-02-18 15:54:59 UTC
markus@x4 c++11 % /var/tmp/gcc_build_dir_/./gcc/xgcc -shared-libgcc -B/var/tmp/gcc_build_dir_/./gcc -nostdinc++ -L/var/tmp/gcc_build_dir_/x86_64-pc-linux-gnu/libstdc++-v3/src -L/var/tmp/gcc_build_dir_/x86_64-pc-linux-gnu/libstdc++-v3/src/.libs -L/var/tmp/gcc_build_dir_/x86_64-pc-linux-gnu/libstdc++-v3/libsupc++/.libs -B/usr/x86_64-pc-linux-gnu/bin/ -B/usr/x86_64-pc-linux-gnu/lib/ -isystem /usr/x86_64-pc-linux-gnu/include -isystem /usr/x86_64-pc-linux-gnu/sys-include -I/var/tmp/gcc/libstdc++-v3/../libgcc -I/var/tmp/gcc_build_dir_/x86_64-pc-linux-gnu/libstdc++-v3/include/x86_64-pc-linux-gnu -I/var/tmp/gcc_build_dir_/x86_64-pc-linux-gnu/libstdc++-v3/include -I/var/tmp/gcc/libstdc++-v3/libsupc++ -std=gnu++11 -D_GLIBCXX_SHARED -fno-implicit-templates -Wall -Wextra -Wwrite-strings -Wcast-qual -Wabi -fdiagnostics-show-location=once -ffunction-sections -fdata-sections -frandom-seed=cow-shim_facets.lo -march=native -O2 -pipe -c -o cow-shim_facets.lo ../../../../../gcc/libstdc++-v3/src/c++11/cow-shim_facets.cc
In file included from ../../../../../gcc/libstdc++-v3/src/c++11/cow-shim_facets.cc:35:0:
../../../../../gcc/libstdc++-v3/src/c++11/cxx11-shim_facets.cc: In member function ‘const std::locale::facet* std::locale::facet::_M_cow_shim(const std::locale::id*) const’:
../../../../../gcc/libstdc++-v3/src/c++11/cxx11-shim_facets.cc:792:19: warning: nonnull argument ‘this’ compared to NULL [-Wnonnull-compare]
     if (auto* p = dynamic_cast<const __shim*>(this))
                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Comment 16 Jakub Jelinek 2016-02-18 17:24:18 UTC
Created attachment 37735 [details]
gcc6-pr69850-3.patch

Untested incremental fix for the dynamic_cast issue.
Comment 17 Jakub Jelinek 2016-02-19 16:03:23 UTC
Author: jakub
Date: Fri Feb 19 16:02:51 2016
New Revision: 233561

URL: https://gcc.gnu.org/viewcvs?rev=233561&root=gcc&view=rev
Log:
	PR c++/69850
	* init.c (build_vec_delete_1): Set TREE_NO_WARNING on the NE_EXPR
	condition.
	* cp-gimplify.c (cp_fold): Propagate TREE_NO_WARNING from binary
	operators if folding preserved the binop, just with different
	arguments.

	* g++.dg/warn/Wnonnull-compare-2.C: New test.
	* g++.dg/warn/Wnonnull-compare-3.C: New test.

Added:
    trunk/gcc/testsuite/g++.dg/warn/Wnonnull-compare-2.C
    trunk/gcc/testsuite/g++.dg/warn/Wnonnull-compare-3.C
Modified:
    trunk/gcc/cp/ChangeLog
    trunk/gcc/cp/cp-gimplify.c
    trunk/gcc/cp/init.c
    trunk/gcc/testsuite/ChangeLog
Comment 18 Jakub Jelinek 2016-02-19 19:17:02 UTC
Author: jakub
Date: Fri Feb 19 19:16:31 2016
New Revision: 233568

URL: https://gcc.gnu.org/viewcvs?rev=233568&root=gcc&view=rev
Log:
	PR c++/69850
	* rtti.c (ifnonnull): Set TREE_NO_WARNING on the condition, use
	NE_EXPR instead of EQ_EXPR and swap last two arguments on COND_EXPR.

	* g++.dg/warn/Wnonnull-compare-4.C: New test.

Added:
    trunk/gcc/testsuite/g++.dg/warn/Wnonnull-compare-4.C
Modified:
    trunk/gcc/cp/ChangeLog
    trunk/gcc/cp/rtti.c
    trunk/gcc/testsuite/ChangeLog
Comment 19 Jakub Jelinek 2016-02-19 19:18:48 UTC
Fixed.  If you encounter some other -Wnonnull-compare false positive, please file a new PR.