[PATCH] D98798: Produce warning for performing pointer arithmetic on a null pointer.

[Jamie Schmeiser via Phabricator via cfe-commits]

jamieschmeiser added a comment.

@crownyanguan Why do you think this caused the linker to crash?  It is a 
warning in the front end.  Would it even be executed in your posted command?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98798/new/

https://reviews.llvm.org/D98798

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D98798: Produce warning for performing pointer arithmetic on a null pointer.

[crownyanguan via Phabricator via cfe-commits]

crownyanguan added a comment.

This commit also makes the phoronix/pgbench run fail.
clang --gcc-toolchain=/usr/bin/.. -target aarch64-unknown-linux-gnu -Wall 
-Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement 
-Wendif-labels -Wmissing-format-attribute -Wformat-security 
-fno-strict-aliasing -fwrapv -Wno-unused-command-line-argument -Ofast 
-funroll-loops -flto -mcpu=cortex-a57 -fno-fast-math -L../../src/port 
-L../../src/common -Ofast -funroll-loops -flto -mcpu=cortex-a57 -fno-fast-math  
-Wl,--as-needed  -Wl,-E access/brin/brin.o access/brin/brin_pageops.o 
access/brin/brin_revmap.o access/brin/brin_tuple.o access/brin/brin_xlog.o 
access/brin/brin_minmax.o access/brin/brin_inclusion.o 
access/brin/brin_validate.o access/common/bufmask.o access/common/heaptuple.o 
access/common/indextuple.o access/common/printsimple.o access/common/printtup.o 
access/common/reloptions.o access/common/scankey.o access/common/tupconvert.o 
access/common/tupdesc.o access/gin/ginutil.o access/gin/gininsert.o 
access/gin/ginxlog.o access/gin/ginentrypage.o access/gin/gindatapage.o 
access/gin/ginbtree.o access/gin/ginscan.o access/gin/ginget.o 
access/gin/ginvacuum.o access/gin/ginarrayproc.o access/gin/ginbulk.o 
access/gin/ginfast.o access/gin/ginpostinglist.o access/gin/ginlogic.o 
access/gin/ginvalidate.o access/gist/gist.o access/gist/gistutil.o 
access/gist/gistxlog.o access/gist/gistvacuum.o access/gist/gistget.o 
access/gist/gistscan.o access/gist/gistproc.o access/gist/gistsplit.o 
access/gist/gistbuild.o access/gist/gistbuildbuffers.o 
access/gist/gistvalidate.o access/hash/hash.o access/hash/hashfunc.o 
access/hash/hashinsert.o access/hash/hashovfl.o access/hash/hashpage.o 
access/hash/hashsearch.o access/hash/hashsort.o access/hash/hashutil.o 
access/hash/hashvalidate.o access/hash/hash_xlog.o access/heap/heapam.o 
access/heap/hio.o access/heap/pruneheap.o access/heap/rewriteheap.o 
access/heap/syncscan.o access/heap/tuptoaster.o access/heap/visibilitymap.o 
access/index/amapi.o access/index/amvalidate.o access/index/genam.o 
access/index/indexam.o access/nbtree/nbtcompare.o access/nbtree/nbtinsert.o 
access/nbtree/nbtpage.o access/nbtree/nbtree.o access/nbtree/nbtsearch.o 
access/nbtree/nbtutils.o access/nbtree/nbtsort.o access/nbtree/nbtvalidate.o 
access/nbtree/nbtxlog.o access/rmgrdesc/brindesc.o access/rmgrdesc/clogdesc.o 
access/rmgrdesc/committsdesc.o access/rmgrdesc/dbasedesc.o 
access/rmgrdesc/genericdesc.o access/rmgrdesc/gindesc.o 
access/rmgrdesc/gistdesc.o access/rmgrdesc/hashdesc.o 
access/rmgrdesc/heapdesc.o access/rmgrdesc/logicalmsgdesc.o 
access/rmgrdesc/mxactdesc.o access/rmgrdesc/nbtdesc.o 
access/rmgrdesc/relmapdesc.o access/rmgrdesc/replorigindesc.o 
access/rmgrdesc/seqdesc.o access/rmgrdesc/smgrdesc.o access/rmgrdesc/spgdesc.o 
access/rmgrdesc/standbydesc.o access/rmgrdesc/tblspcdesc.o 
access/rmgrdesc/xactdesc.o access/rmgrdesc/xlogdesc.o access/spgist/spgutils.o 
access/spgist/spginsert.o access/spgist/spgscan.o access/spgist/spgvacuum.o 
access/spgist/spgvalidate.o access/spgist/spgdoinsert.o access/spgist/spgxlog.o 
access/spgist/spgtextproc.o access/spgist/spgquadtreeproc.o 
access/spgist/spgkdtreeproc.o access/tablesample/bernoulli.o 
access/tablesample/system.o access/tablesample/tablesample.o 
access/transam/clog.o access/transam/commit_ts.o access/transam/generic_xlog.o 
access/transam/multixact.o access/transam/parallel.o access/transam/rmgr.o 
access/transam/slru.o access/transam/subtrans.o access/transam/timeline.o 
access/transam/transam.o access/transam/twophase.o 
access/transam/twophase_rmgr.o access/transam/varsup.o access/transam/xact.o 
access/transam/xlog.o access/transam/xlogarchive.o access/transam/xlogfuncs.o 
access/transam/xloginsert.o access/transam/xlogreader.o 
access/transam/xlogutils.o bootstrap/bootparse.o bootstrap/bootstrap.o 
catalog/catalog.o catalog/dependency.o catalog/heap.o catalog/index.o 
catalog/indexing.o catalog/namespace.o catalog/aclchk.o catalog/objectaccess.o 
catalog/objectaddress.o catalog/partition.o catalog/pg_aggregate.o 
catalog/pg_collation.o catalog/pg_constraint.o catalog/pg_conversion.o 
catalog/pg_depend.o catalog/pg_enum.o catalog/pg_inherits.o 
catalog/pg_largeobject.o catalog/pg_namespace.o catalog/pg_operator.o 
catalog/pg_proc.o catalog/pg_publication.o catalog/pg_range.o 
catalog/pg_db_role_setting.o catalog/pg_shdepend.o catalog/pg_subscription.o 
catalog/pg_type.o catalog/storage.o catalog/toasting.o parser/analyze.o 
parser/gram.o parser/scan.o parser/parser.o parser/parse_agg.o 
parser/parse_clause.o parser/parse_coerce.o parser/parse_collate.o 
parser/parse_cte.o parser/parse_enr.o parser/parse_expr.o parser/parse_func.o 
parser/parse_node.o parser/parse_oper.o parser/parse_param.o 
parser/parse_relation.o parser/parse_target.o parser/parse_type.o 
parser/parse_utilcmd.o parser/scansup.o commands/amcmds.o 
commands/aggregatecmds.o commands/alter.o commands/analyze.o commands/async.o 
commands/cluster.o 

[PATCH] D98798: Produce warning for performing pointer arithmetic on a null pointer.

[Jamie Schmeiser via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rG9cb00b9ecbe7: Reland Produce warning for performing pointer 
arithmetic on a null pointer. (authored by jamieschmeiser).

Changed prior to commit:
  https://reviews.llvm.org/D98798?vs=346751=360116#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98798/new/

https://reviews.llvm.org/D98798

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaExpr.cpp
  clang/test/Sema/Inputs/pointer-subtraction.h
  clang/test/Sema/pointer-subtraction.c
  clang/test/Sema/pointer-subtraction.cpp

Index: clang/test/Sema/pointer-subtraction.cpp
===
--- /dev/null
+++ clang/test/Sema/pointer-subtraction.cpp
@@ -0,0 +1,19 @@
+// RUN: %clang_cc1 %s -fsyntax-only -verify -pedantic -Wextra -std=c++11 -isystem %S/Inputs
+// RUN: %clang_cc1 %s -fsyntax-only -verify -pedantic -Wnull-pointer-subtraction -std=c++11 -isystem %S/Inputs
+// RUN: %clang_cc1 %s -fsyntax-only -verify -pedantic -Wextra -std=c++11 -isystem %S/Inputs -Wsystem-headers -DSYSTEM_WARNINGS
+// RUN: %clang_cc1 %s -fsyntax-only -verify -pedantic -Wnull-pointer-subtraction -std=c++11 -isystem %S/Inputs -Wsystem-headers -DSYSTEM_WARNINGS
+
+#include 
+
+void a() {
+  char *f = (char *)0;
+  f = (char *)((char *)0 - f); // expected-warning {{performing pointer subtraction with a null pointer may have undefined behavior}}
+  f = (char *)(f - (char *)0); // expected-warning {{performing pointer subtraction with a null pointer may have undefined behavior}}
+  f = (char *)((char *)0 - (char *)0); // valid in C++
+
+#ifndef SYSTEM_WARNINGS
+  SYSTEM_MACRO(f);
+#else
+  SYSTEM_MACRO(f); // expected-warning {{performing pointer subtraction with a null pointer may have undefined behavior}}
+#endif
+}
Index: clang/test/Sema/pointer-subtraction.c
===
--- /dev/null
+++ clang/test/Sema/pointer-subtraction.c
@@ -0,0 +1,19 @@
+// RUN: %clang_cc1 %s -fsyntax-only -verify -pedantic -Wextra -std=c11 -isystem %S/Inputs
+// RUN: %clang_cc1 %s -fsyntax-only -verify -pedantic -Wnull-pointer-subtraction -std=c11 -isystem %S/Inputs
+// RUN: %clang_cc1 %s -fsyntax-only -verify -pedantic -Wextra -std=c11 -isystem %S/Inputs -Wsystem-headers -DSYSTEM_WARNINGS
+// RUN: %clang_cc1 %s -fsyntax-only -verify -pedantic -Wnull-pointer-subtraction -std=c11 -isystem %S/Inputs -Wsystem-headers -DSYSTEM_WARNINGS
+
+#include 
+
+void a() {
+  char *f = (char *)0;
+  f = (char *)((char *)0 - f); // expected-warning {{performing pointer subtraction with a null pointer has undefined behavior}}
+  f = (char *)(f - (char *)0); // expected-warning {{performing pointer subtraction with a null pointer has undefined behavior}}
+  f = (char *)((char *)0 - (char *)0); // expected-warning {{performing pointer subtraction with a null pointer has undefined behavior}} expected-warning {{performing pointer subtraction with a null pointer has undefined behavior}}
+
+#ifndef SYSTEM_WARNINGS
+  SYSTEM_MACRO(f);
+#else
+  SYSTEM_MACRO(f); // expected-warning {{performing pointer subtraction with a null pointer has undefined behavior}}
+#endif
+}
Index: clang/test/Sema/Inputs/pointer-subtraction.h
===
--- /dev/null
+++ clang/test/Sema/Inputs/pointer-subtraction.h
@@ -0,0 +1 @@
+#define SYSTEM_MACRO(x) (x) = (char *)((char *)0 - (x))
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -10432,6 +10432,22 @@
   << S.getLangOpts().CPlusPlus << Pointer->getSourceRange();
 }
 
+/// Diagnose invalid subraction on a null pointer.
+///
+static void diagnoseSubtractionOnNullPointer(Sema , SourceLocation Loc,
+ Expr *Pointer, bool BothNull) {
+  // Null - null is valid in C++ [expr.add]p7
+  if (BothNull && S.getLangOpts().CPlusPlus)
+return;
+
+  // Is this s a macro from a system header?
+  if (S.Diags.getSuppressSystemWarnings() && S.SourceMgr.isInSystemMacro(Loc))
+return;
+
+  S.Diag(Loc, diag::warn_pointer_sub_null_ptr)
+  << S.getLangOpts().CPlusPlus << Pointer->getSourceRange();
+}
+
 /// Diagnose invalid arithmetic on two function pointers.
 static void diagnoseArithmeticOnTwoFunctionPointers(Sema , SourceLocation Loc,
 Expr *LHS, Expr *RHS) {
@@ -10863,7 +10879,16 @@
LHS.get(), RHS.get()))
 return QualType();
 
-  // FIXME: Add warnings for nullptr - ptr.
+  bool LHSIsNullPtr = 

[PATCH] D98798: Produce warning for performing pointer arithmetic on a null pointer.

[Jamie Schmeiser via Phabricator via cfe-commits]

jamieschmeiser added a comment.

This was originally approved and landed on May 11th.  I agreed to let it be 
reverted when it was discovered that some headers were triggering the warning.  
I reworked the code to not generate the warning when coming from system header 
files and also added option control for the warnings.  I was informed that the 
changes fixed the problems with the system headers but have received no other 
feedback since late May, despite numerous pings and requests tagging the 
various people involved.  Since I have received no objections, further comments 
nor further review in approximately 2 months, I am assuming that there are no 
further concerns.  Unless I hear otherwise, I will commit these changes 
tomorrow.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98798/new/

https://reviews.llvm.org/D98798

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D98798: Produce warning for performing pointer arithmetic on a null pointer.

[Kelvin Li via Phabricator via cfe-commits]

kkwli0 added a comment.

Is the diff from HEAD?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98798/new/

https://reviews.llvm.org/D98798

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D98798: Produce warning for performing pointer arithmetic on a null pointer.

[Jamie Schmeiser via Phabricator via cfe-commits]

jamieschmeiser added a comment.

ping


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98798/new/

https://reviews.llvm.org/D98798

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D98798: Produce warning for performing pointer arithmetic on a null pointer.

[Jamie Schmeiser via Phabricator via cfe-commits]

jamieschmeiser added a comment.

@rsmith @thakis @efriedma  If there are no more changes required, please 
approve.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98798/new/

https://reviews.llvm.org/D98798

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D98798: Produce warning for performing pointer arithmetic on a null pointer.

[Jamie Schmeiser via Phabricator via cfe-commits]

jamieschmeiser added a comment.

ping


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98798/new/

https://reviews.llvm.org/D98798

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D98798: Produce warning for performing pointer arithmetic on a null pointer.

[Jamie Schmeiser via Phabricator via cfe-commits]

jamieschmeiser added a comment.

@rsmith I separated the C and C++ messages in response to your comments and 
changed the C++ message to state that "performing pointer subtraction with a 
null pointer may have undefined behavior" to address your concerns.  Are you 
satisfied?  @thakis, the warning no longer fires in the MS headers according to 
@hans and there is separate option control over this warning.  Is this 
sufficient?  @efriedma, thank you for reviewing/approving the original but 
these changes were sufficiently different that I thought a new review would be 
beneficial.  Are you still satisfied with the resulting changes?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98798/new/

https://reviews.llvm.org/D98798

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D98798: Produce warning for performing pointer arithmetic on a null pointer.

[Jamie Schmeiser via Phabricator via cfe-commits]

jamieschmeiser added a comment.

ping


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98798/new/

https://reviews.llvm.org/D98798

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D98798: Produce warning for performing pointer arithmetic on a null pointer.

[Jamie Schmeiser via Phabricator via cfe-commits]

jamieschmeiser added a comment.

The reason I worded it with 'may' is because, in C++, nullptr - nullptr is 
defined.  If the code is "nullptr - p" or "p - nullptr", it is only undefined 
behaviour when p is not nullptr, hence the 'may' part of the warning because 
this is not known at compile time.  The warning is still useful as it is 
suspect code but one cannot state that it is undefined behaviour because it is 
valid if it is null at runtime.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98798/new/

https://reviews.llvm.org/D98798

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D98798: Produce warning for performing pointer arithmetic on a null pointer.

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.

We tried it, and the warning is still firing in a similar (but not exactly the 
same) way:

  In file included from 
../../content/browser/accessibility/browser_accessibility_manager_win.cc:19:
  In file included from 
../..\content/browser/renderer_host/legacy_render_widget_host_win.h:11:
  ../../third_party/wtl/include\atlapp.h(366,12): error: performing pointer 
subtraction with a null pointer may have undefined behavior 
[-Werror,-Wnull-pointer-subtraction]
  uSize = NONCLIENTMETRICS_V1_SIZE;
  ^~~~
  ../../third_party/wtl/include\atlapp.h(248,38): note: expanded from macro 
'NONCLIENTMETRICS_V1_SIZE'
#define NONCLIENTMETRICS_V1_SIZE   _SIZEOF_STRUCT(NONCLIENTMETRICS, 
lfMessageFont)
   
^~~
  ../../third_party/wtl/include\atlapp.h(228,91): note: expanded from macro 
'_SIZEOF_STRUCT'
#define _SIZEOF_STRUCT(structname, member)  
(((int)((LPBYTE)(&((structname*)0)->member) - ((LPBYTE)((structname*)0 + 
sizeof(((structname*)0)->member))

^ ~~

Note that this time the warning is coming from a macro defined in a wtl/ header 
itself , which is not a system header. So I think the "don't warn in system 
headers" part is working correctly.

And it's good that it's got a separate flag, because for Chromium we would need 
to turn it off.

But as a developer I'm confused by the warning saying this _may_ have undefined 
behavior. Does it or doesn't it? If it does, I suppose the warning makes sense 
and keeping it behind a separate flag that's part of -Wextra seems reasonable.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98798/new/

https://reviews.llvm.org/D98798

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D98798: Produce warning for performing pointer arithmetic on a null pointer.

[Jamie Schmeiser via Phabricator via cfe-commits]

jamieschmeiser added a reviewer: thakis.
jamieschmeiser added a comment.

ping


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98798/new/

https://reviews.llvm.org/D98798

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D98798: Produce warning for performing pointer arithmetic on a null pointer.

[Jamie Schmeiser via Phabricator via cfe-commits]

jamieschmeiser added a comment.

@thakis, can you please verify that the changes no longer affect the MS 
headers?  Thanks.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98798/new/

https://reviews.llvm.org/D98798

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D98798: Produce warning for performing pointer arithmetic on a null pointer.

[Jamie Schmeiser via Phabricator via cfe-commits]

jamieschmeiser updated this revision to Diff 346751.
jamieschmeiser added a comment.

Fix formatting.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98798/new/

https://reviews.llvm.org/D98798

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaExpr.cpp
  clang/test/Sema/Inputs/pointer-subtraction.h
  clang/test/Sema/pointer-subtraction.c
  clang/test/Sema/pointer-subtraction.cpp

Index: clang/test/Sema/pointer-subtraction.cpp
===
--- /dev/null
+++ clang/test/Sema/pointer-subtraction.cpp
@@ -0,0 +1,19 @@
+// RUN: %clang_cc1 %s -fsyntax-only -verify -pedantic -Wextra -std=c++11 -isystem %S/Inputs
+// RUN: %clang_cc1 %s -fsyntax-only -verify -pedantic -Wnull-pointer-subtraction -std=c++11 -isystem %S/Inputs
+// RUN: %clang_cc1 %s -fsyntax-only -verify -pedantic -Wextra -std=c++11 -isystem %S/Inputs -Wsystem-headers -DSYSTEM_WARNINGS
+// RUN: %clang_cc1 %s -fsyntax-only -verify -pedantic -Wnull-pointer-subtraction -std=c++11 -isystem %S/Inputs -Wsystem-headers -DSYSTEM_WARNINGS
+
+#include 
+
+void a() {
+  char *f = (char *)0;
+  f = (char *)((char *)0 - f); // expected-warning {{performing pointer subtraction with a null pointer may have undefined behavior}}
+  f = (char *)(f - (char *)0); // expected-warning {{performing pointer subtraction with a null pointer may have undefined behavior}}
+  f = (char *)((char *)0 - (char *)0); // valid in C++
+
+#ifndef SYSTEM_WARNINGS
+  SYSTEM_MACRO(f);
+#else
+  SYSTEM_MACRO(f); // expected-warning {{performing pointer subtraction with a null pointer may have undefined behavior}}
+#endif
+}
Index: clang/test/Sema/pointer-subtraction.c
===
--- /dev/null
+++ clang/test/Sema/pointer-subtraction.c
@@ -0,0 +1,19 @@
+// RUN: %clang_cc1 %s -fsyntax-only -verify -pedantic -Wextra -std=c11 -isystem %S/Inputs
+// RUN: %clang_cc1 %s -fsyntax-only -verify -pedantic -Wnull-pointer-subtraction -std=c11 -isystem %S/Inputs
+// RUN: %clang_cc1 %s -fsyntax-only -verify -pedantic -Wextra -std=c11 -isystem %S/Inputs -Wsystem-headers -DSYSTEM_WARNINGS
+// RUN: %clang_cc1 %s -fsyntax-only -verify -pedantic -Wnull-pointer-subtraction -std=c11 -isystem %S/Inputs -Wsystem-headers -DSYSTEM_WARNINGS
+
+#include 
+
+void a() {
+  char *f = (char *)0;
+  f = (char *)((char *)0 - f); // expected-warning {{performing pointer subtraction with a null pointer has undefined behavior}}
+  f = (char *)(f - (char *)0); // expected-warning {{performing pointer subtraction with a null pointer has undefined behavior}}
+  f = (char *)((char *)0 - (char *)0); // expected-warning {{performing pointer subtraction with a null pointer has undefined behavior}} expected-warning {{performing pointer subtraction with a null pointer has undefined behavior}}
+
+#ifndef SYSTEM_WARNINGS
+  SYSTEM_MACRO(f);
+#else
+  SYSTEM_MACRO(f); // expected-warning {{performing pointer subtraction with a null pointer has undefined behavior}}
+#endif
+}
Index: clang/test/Sema/Inputs/pointer-subtraction.h
===
--- /dev/null
+++ clang/test/Sema/Inputs/pointer-subtraction.h
@@ -0,0 +1 @@
+#define SYSTEM_MACRO(x) (x) = (char *)((char *)0 - (x))
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -10366,6 +10366,22 @@
   << S.getLangOpts().CPlusPlus << Pointer->getSourceRange();
 }
 
+/// Diagnose invalid subraction on a null pointer.
+///
+static void diagnoseSubtractionOnNullPointer(Sema , SourceLocation Loc,
+ Expr *Pointer, bool BothNull) {
+  // Null - null is valid in C++ [expr.add]p7
+  if (BothNull && S.getLangOpts().CPlusPlus)
+return;
+
+  // Is this s a macro from a system header?
+  if (S.Diags.getSuppressSystemWarnings() && S.SourceMgr.isInSystemMacro(Loc))
+return;
+
+  S.Diag(Loc, diag::warn_pointer_sub_null_ptr)
+  << S.getLangOpts().CPlusPlus << Pointer->getSourceRange();
+}
+
 /// Diagnose invalid arithmetic on two function pointers.
 static void diagnoseArithmeticOnTwoFunctionPointers(Sema , SourceLocation Loc,
 Expr *LHS, Expr *RHS) {
@@ -10797,7 +10813,16 @@
LHS.get(), RHS.get()))
 return QualType();
 
-  // FIXME: Add warnings for nullptr - ptr.
+  bool LHSIsNullPtr = LHS.get()->IgnoreParenCasts()->isNullPointerConstant(
+  Context, Expr::NPC_ValueDependentIsNotNull);
+  bool RHSIsNullPtr = RHS.get()->IgnoreParenCasts()->isNullPointerConstant(
+  Context, Expr::NPC_ValueDependentIsNotNull);
+
+  // Subtracting nullptr or from nullptr is suspect
+  if (LHSIsNullPtr)
+ 

[PATCH] D98798: Produce warning for performing pointer arithmetic on a null pointer.

[Jamie Schmeiser via Phabricator via cfe-commits]

jamieschmeiser requested review of this revision.
jamieschmeiser added a comment.

Significant changes made since previously accepted.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98798/new/

https://reviews.llvm.org/D98798

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D98798: Produce warning for performing pointer arithmetic on a null pointer.

[Jamie Schmeiser via Phabricator via cfe-commits]

jamieschmeiser updated this revision to Diff 346515.
jamieschmeiser edited the summary of this revision.
jamieschmeiser added a comment.

As requested, I have added a new warning option -Wnull-pointer-subtraction (and 
added it to -Wextra) that does not trigger on system headers.  In addition, I 
changed the message used such that it states that it is undefined behaviour for 
C but may be undefined behaviour in C++ to address the concerns that performing 
the subtraction with a pointer that dynamically becomes null is not undefined 
behaviour.  Expanded the testing to also test that the warning does not fire on 
system headers.  Also, updated the release notes to indicate the new option.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98798/new/

https://reviews.llvm.org/D98798

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaExpr.cpp
  clang/test/Sema/Inputs/pointer-subtraction.h
  clang/test/Sema/pointer-subtraction.c
  clang/test/Sema/pointer-subtraction.cpp

Index: clang/test/Sema/pointer-subtraction.cpp
===
--- /dev/null
+++ clang/test/Sema/pointer-subtraction.cpp
@@ -0,0 +1,19 @@
+// RUN: %clang_cc1 %s -fsyntax-only -verify -pedantic -Wextra -std=c++11 -isystem %S/Inputs
+// RUN: %clang_cc1 %s -fsyntax-only -verify -pedantic -Wnull-pointer-subtraction -std=c++11 -isystem %S/Inputs
+// RUN: %clang_cc1 %s -fsyntax-only -verify -pedantic -Wextra -std=c++11 -isystem %S/Inputs -Wsystem-headers -DSYSTEM_WARNINGS
+// RUN: %clang_cc1 %s -fsyntax-only -verify -pedantic -Wnull-pointer-subtraction -std=c++11 -isystem %S/Inputs -Wsystem-headers -DSYSTEM_WARNINGS
+
+#include 
+
+void a() {
+  char *f = (char*)0;
+  f = (char*)((char*)0 - f); // expected-warning {{performing pointer subtraction with a null pointer may have undefined behavior}}
+  f = (char*)(f - (char*)0); // expected-warning {{performing pointer subtraction with a null pointer may have undefined behavior}}
+  f = (char*)((char*)0 - (char*)0); // valid in C++
+
+#ifndef SYSTEM_WARNINGS
+  SYSTEM_MACRO(f);
+#else
+  SYSTEM_MACRO(f); // expected-warning {{performing pointer subtraction with a null pointer may have undefined behavior}}
+#endif
+}
Index: clang/test/Sema/pointer-subtraction.c
===
--- /dev/null
+++ clang/test/Sema/pointer-subtraction.c
@@ -0,0 +1,19 @@
+// RUN: %clang_cc1 %s -fsyntax-only -verify -pedantic -Wextra -std=c11 -isystem %S/Inputs
+// RUN: %clang_cc1 %s -fsyntax-only -verify -pedantic -Wnull-pointer-subtraction -std=c11 -isystem %S/Inputs
+// RUN: %clang_cc1 %s -fsyntax-only -verify -pedantic -Wextra -std=c11 -isystem %S/Inputs -Wsystem-headers -DSYSTEM_WARNINGS
+// RUN: %clang_cc1 %s -fsyntax-only -verify -pedantic -Wnull-pointer-subtraction -std=c11 -isystem %S/Inputs -Wsystem-headers -DSYSTEM_WARNINGS
+
+#include 
+
+void a() {
+  char *f = (char*)0;
+  f = (char*)((char*)0 - f); // expected-warning {{performing pointer subtraction with a null pointer has undefined behavior}}
+  f = (char*)(f - (char*)0); // expected-warning {{performing pointer subtraction with a null pointer has undefined behavior}}
+  f = (char*)((char*)0 - (char*)0); // expected-warning {{performing pointer subtraction with a null pointer has undefined behavior}} expected-warning {{performing pointer subtraction with a null pointer has undefined behavior}}
+
+#ifndef SYSTEM_WARNINGS
+  SYSTEM_MACRO(f);
+#else
+  SYSTEM_MACRO(f); // expected-warning {{performing pointer subtraction with a null pointer has undefined behavior}}
+#endif
+}
Index: clang/test/Sema/Inputs/pointer-subtraction.h
===
--- /dev/null
+++ clang/test/Sema/Inputs/pointer-subtraction.h
@@ -0,0 +1 @@
+#define SYSTEM_MACRO(x) (x) = (char*)((char*)0 - (x))
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -10366,6 +10366,22 @@
   << S.getLangOpts().CPlusPlus << Pointer->getSourceRange();
 }
 
+/// Diagnose invalid subraction on a null pointer.
+///
+static void diagnoseSubtractionOnNullPointer(Sema , SourceLocation Loc,
+	 Expr *Pointer, bool BothNull) {
+  // Null - null is valid in C++ [expr.add]p7
+  if (BothNull && S.getLangOpts().CPlusPlus)
+return;
+
+  // Is this s a macro from a system header?
+  if (S.Diags.getSuppressSystemWarnings() && S.SourceMgr.isInSystemMacro(Loc))
+return;
+
+  S.Diag(Loc, diag::warn_pointer_sub_null_ptr)
+<< S.getLangOpts().CPlusPlus << Pointer->getSourceRange();
+}
+
 /// Diagnose invalid arithmetic on two function pointers.
 static void diagnoseArithmeticOnTwoFunctionPointers(Sema , SourceLocation Loc,
 Expr *LHS, Expr *RHS) {
@@ -10797,7 +10813,16 @@
  

[PATCH] D98798: Produce warning for performing pointer arithmetic on a null pointer.

[Jamie Schmeiser via Phabricator via cfe-commits]

jamieschmeiser reopened this revision.
jamieschmeiser added a comment.
This revision is now accepted and ready to land.

Re-opening because it was reverted.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98798/new/

https://reviews.llvm.org/D98798

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D98798: Produce warning for performing pointer arithmetic on a null pointer.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/test/Sema/pointer-addition.cpp:5-6
+  char *f = (char*)0;
+  f = (char*)((char*)0 - f); // expected-warning {{performing pointer 
arithmetic on a null pointer has undefined behavior}}
+  f = (char*)(f - (char*)0); // expected-warning {{performing pointer 
arithmetic on a null pointer has undefined behavior}}
+  f = (char*)((char*)0 - (char*)0); // valid in C++

These two warnings are wrong -- their claim about these expressions having 
undefined behavior is incorrect. We can't prove that `f` is not null (and in 
fact it is null here), so we should not be producing a warning that says the 
code has undefined behavior. If you want to warn in the cases where you can 
prove the other pointer is non-null, and say that that case has undefined 
behavior, that seems fine, but please fix the diagnostic message to be 
technically correct (eg, "computing difference of a null pointer and a non-null 
pointer has undefined behavior").

Perhaps a better approach would be to use the same logic to decide whether to 
warn in C and C++, but produce different warning text. For example, you could 
say "[...] has undefined behavior" in C, but in C++ just say "warning: 
performing pointer arithmetic on a null pointer" without making 
potentially-inaccurate claims about UB? The code is still *suspicious* in C++ 
even if it's not UB.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98798/new/

https://reviews.llvm.org/D98798

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D98798: Produce warning for performing pointer arithmetic on a null pointer.

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

Thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98798/new/

https://reviews.llvm.org/D98798

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D98798: Produce warning for performing pointer arithmetic on a null pointer.

[Jamie Schmeiser via Phabricator via cfe-commits]

jamieschmeiser added a comment.

If this is a problem for you, please revert it and I will take a look when I 
return.  Thanks.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98798/new/

https://reviews.llvm.org/D98798

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D98798: Produce warning for performing pointer arithmetic on a null pointer.

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

@thakis if this is an issue for you, please revert; like you said, we can 
figure out the issues later.

Looking at the warning you showed, we might want to consider suppressing that 
specific pattern, in addition to (or instead of?) suppressing it in system 
headers. The null pointer subtraction is the least of the problems in 
`(char*)&((struct S*)0)->member - (char*)0`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98798/new/

https://reviews.llvm.org/D98798

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D98798: Produce warning for performing pointer arithmetic on a null pointer.

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

(Now completed the reply, see phab.)

In D98798#2754635 , @xbolva00 wrote:

> You dont have to use -Wno-…
>
> You can use pragma to disable this warning for certain location in the code, 
> eg include of commctrl.h.

commctrl.h is included by other system headers too though.

I don't think requiring users to add pragmas in all files that transitively 
include a random subset of windows headers is great :) I'm fine with not 
reverting if a fix for this will happen soon, but it sounds like the author is 
away for a few days. Reverts are cheap.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98798/new/

https://reviews.llvm.org/D98798

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D98798: Produce warning for performing pointer arithmetic on a null pointer.

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

You dont have to use -Wno-…

You can use pragma to disable this warning for certain location in the code.

-1 for revert just because of this reason.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98798/new/

https://reviews.llvm.org/D98798

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D98798: Produce warning for performing pointer arithmetic on a null pointer.

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

(sorry, accidentally hit cmd-enter, not actually done writing)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98798/new/

https://reviews.llvm.org/D98798

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D98798: Produce warning for performing pointer arithmetic on a null pointer.

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

In D98798#2754449 , @jamieschmeiser 
wrote:

> @thakis, I have some questions about your comments.
> Are you sure this is coming from a system header?  The path that you gave has 
> third_party as a directory in the path.

Yes. `third_party\depot_tools\win_toolchain\vs_files\20d5f2553f\Windows 
Kits\10\Include\10.0.19041.0\um\commctrl.h` is a hermetic system directory used 
via `/winsysrootthird_party\depot_tools\win_toolchain\vs_files\20d5f2553f`.

> If the warning were being triggered by code in a system header, I would have 
> expected it to fire on something in the windows build bots but they appear to 
> be clean.

I don't think LLVM uses a lot of code from the windows system headers. The 
compiler doesn't contain any UI :) (commctrl.h is a UI header).

> I don't know if it is possible to suppress a warning based on whether the 
> source is in a system header, or from a macro expansion that is defined in a 
> system header; are you aware about whether or not this is possible?



> If it is, I suspect it would already be in force as a general setting, again 
> making me question whether this is a system header...and if it isn't a system 
> header, that wouldn't help you in any case.
> If I understand your second point correctly, you have a system that 
> previously compiled cleanly with warnings being treated as errors and you are 
> concerned that if you use the existing options to suppress this particular 
> warning, you are concerned that something could creep in.  Therefore you are 
> proposing a different warning group that would be a subgroup of the existing 
> one so that if you set it, you would set both but you would still be able to 
> set the specific one.  I don't know if this is possible or not.  Either way, 
> I suspect that it would require using a different warning for the subtraction 
> case, which would also require significantly changing these changes. Am I 
> understanding correctly?  If so, this implies that you have access to a 
> workaround for your problem, although it may not be the best solution.
> I do not have access to a windows setup to test any of these proposed 
> changes; in particular, given that I suspect that the affected files are 
> specific to some third party vendor from which you have purchased code, I do 
> not have means of investigating the actual problems/solutions.  If it would 
> be helpful, I would be happy to review any changes that you might like to 
> make to remedy the situation.
> I will be on vacation for the next few days so please excuse my delayed 
> responses.




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98798/new/

https://reviews.llvm.org/D98798

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D98798: Produce warning for performing pointer arithmetic on a null pointer.

[Jamie Schmeiser via Phabricator via cfe-commits]

jamieschmeiser added a comment.

@thakis, I have some questions about your comments.
Are you sure this is coming from a system header?  The path that you gave has 
third_party as a directory in the path.  If the warning were being triggered by 
code in a system header, I would have expected it to fire on something in the 
windows build bots but they appear to be clean.
I don't know if it is possible to suppress a warning based on whether the 
source is in a system header, or from a macro expansion that is defined in a 
system header; are you aware about whether or not this is possible?  If it is, 
I suspect it would already be in force as a general setting, again making me 
question whether this is a system header...and if it isn't a system header, 
that wouldn't help you in any case.
If I understand your second point correctly, you have a system that previously 
compiled cleanly with warnings being treated as errors and you are concerned 
that if you use the existing options to suppress this particular warning, you 
are concerned that something could creep in.  Therefore you are proposing a 
different warning group that would be a subgroup of the existing one so that if 
you set it, you would set both but you would still be able to set the specific 
one.  I don't know if this is possible or not.  Either way, I suspect that it 
would require using a different warning for the subtraction case, which would 
also require significantly changing these changes. Am I understanding 
correctly?  If so, this implies that you have access to a workaround for your 
problem, although it may not be the best solution.
I do not have access to a windows setup to test any of these proposed changes; 
in particular, given that I suspect that the affected files are specific to 
some third party vendor from which you have purchased code, I do not have means 
of investigating the actual problems/solutions.  If it would be helpful, I 
would be happy to review any changes that you might like to make to remedy the 
situation.
I will be on vacation for the next few days so please excuse my delayed 
responses.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98798/new/

https://reviews.llvm.org/D98798

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D98798: Produce warning for performing pointer arithmetic on a null pointer.

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

Two pieces of feedback:

1. This fires in system headers on windows, e.g. like so:

  ../../third_party/wtl/include\./atlapp.h(336,12): error: performing pointer 
arithmetic on a null pointer has undefined behavior if the offset is nonzero 
[-Werror,-Wnull-pointer-arithmetic]
  uSize = LVGROUP_V5_SIZE;
  ^~~
  ..\..\third_party\depot_tools\win_toolchain\vs_files\20d5f2553f\Windows 
Kits\10\Include\10.0.19041.0\um\commctrl.h(4074,25): note: expanded from macro 
'LVGROUP_V5_SIZE'
  #define LVGROUP_V5_SIZE CCSIZEOF_STRUCT(LVGROUP, uAlign)
  ^~~~
  ..\..\third_party\depot_tools\win_toolchain\vs_files\20d5f2553f\Windows 
Kits\10\Include\10.0.19041.0\um\commctrl.h(262,90): note: expanded from macro 
'CCSIZEOF_STRUCT'
  #define CCSIZEOF_STRUCT(structname, member)  
(((int)((LPBYTE)(&((structname*)0)->member) - ((LPBYTE)((structname*)0 + 
sizeof(((structname*)0)->member))

   ^ ~~

Can we make this not fire for macros for system headers? And if this takes a 
while to implement, can we revert this until then?

2. This reusing the existing group means we now either have to disable the old, 
existing warning and regress, or we can't build. Can we please put this in a 
new group (Wnull-pointer-arithmetic-sub or what) that's a subgroup of 
Wnull-pointer-arithmetic but that can be turned off separately, to allow 
incremental roll-out of this?

Thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98798/new/

https://reviews.llvm.org/D98798

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D98798: Produce warning for performing pointer arithmetic on a null pointer.

[Jamie Schmeiser via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGdfc1e31d49fe: Produce warning for performing pointer 
arithmetic on a null pointer. (authored by jamieschmeiser).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98798/new/

https://reviews.llvm.org/D98798

Files:
  clang/lib/Sema/SemaExpr.cpp
  clang/test/Sema/pointer-addition.c
  clang/test/Sema/pointer-addition.cpp


Index: clang/test/Sema/pointer-addition.cpp
===
--- /dev/null
+++ clang/test/Sema/pointer-addition.cpp
@@ -0,0 +1,8 @@
+// RUN: %clang_cc1 %s -fsyntax-only -verify -pedantic -Wextra -std=c++11
+
+void a() {
+  char *f = (char*)0;
+  f = (char*)((char*)0 - f); // expected-warning {{performing pointer 
arithmetic on a null pointer has undefined behavior}}
+  f = (char*)(f - (char*)0); // expected-warning {{performing pointer 
arithmetic on a null pointer has undefined behavior}}
+  f = (char*)((char*)0 - (char*)0); // valid in C++
+}
Index: clang/test/Sema/pointer-addition.c
===
--- clang/test/Sema/pointer-addition.c
+++ clang/test/Sema/pointer-addition.c
@@ -29,4 +29,7 @@
   // Cases that don't match the GNU inttoptr idiom get a different warning.
   f = (char*)0 - i; // expected-warning {{performing pointer arithmetic on a 
null pointer has undefined behavior}}
   int *g = (int*)0 + i; // expected-warning {{performing pointer arithmetic on 
a null pointer has undefined behavior}}
+  f = (char*)((char*)0 - f); // expected-warning {{performing pointer 
arithmetic on a null pointer has undefined behavior}}
+  f = (char*)(f - (char*)0); // expected-warning {{performing pointer 
arithmetic on a null pointer has undefined behavior}}
+  f = (char*)((char*)0 - (char*)0); // expected-warning {{performing pointer 
arithmetic on a null pointer has undefined behavior}} expected-warning 
{{performing pointer arithmetic on a null pointer has undefined behavior}}
 }
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -10779,7 +10779,17 @@
LHS.get(), RHS.get()))
 return QualType();
 
-  // FIXME: Add warnings for nullptr - ptr.
+  bool LHSIsNullPtr = LHS.get()->IgnoreParenCasts()->isNullPointerConstant(
+  Context, Expr::NPC_ValueDependentIsNotNull);
+  bool RHSIsNullPtr = RHS.get()->IgnoreParenCasts()->isNullPointerConstant(
+  Context, Expr::NPC_ValueDependentIsNotNull);
+
+  // Subtracting nullptr or from nullptr should produce
+  // a warning expect nullptr - nullptr is valid in C++ [expr.add]p7
+  if (LHSIsNullPtr && (!getLangOpts().CPlusPlus || !RHSIsNullPtr))
+diagnoseArithmeticOnNullPointer(*this, Loc, LHS.get(), false);
+  if (RHSIsNullPtr && (!getLangOpts().CPlusPlus || !LHSIsNullPtr))
+diagnoseArithmeticOnNullPointer(*this, Loc, RHS.get(), false);
 
   // The pointee type may have zero size.  As an extension, a structure or
   // union may have zero size or an array may have zero length.  In this


Index: clang/test/Sema/pointer-addition.cpp
===
--- /dev/null
+++ clang/test/Sema/pointer-addition.cpp
@@ -0,0 +1,8 @@
+// RUN: %clang_cc1 %s -fsyntax-only -verify -pedantic -Wextra -std=c++11
+
+void a() {
+  char *f = (char*)0;
+  f = (char*)((char*)0 - f); // expected-warning {{performing pointer arithmetic on a null pointer has undefined behavior}}
+  f = (char*)(f - (char*)0); // expected-warning {{performing pointer arithmetic on a null pointer has undefined behavior}}
+  f = (char*)((char*)0 - (char*)0); // valid in C++
+}
Index: clang/test/Sema/pointer-addition.c
===
--- clang/test/Sema/pointer-addition.c
+++ clang/test/Sema/pointer-addition.c
@@ -29,4 +29,7 @@
   // Cases that don't match the GNU inttoptr idiom get a different warning.
   f = (char*)0 - i; // expected-warning {{performing pointer arithmetic on a null pointer has undefined behavior}}
   int *g = (int*)0 + i; // expected-warning {{performing pointer arithmetic on a null pointer has undefined behavior}}
+  f = (char*)((char*)0 - f); // expected-warning {{performing pointer arithmetic on a null pointer has undefined behavior}}
+  f = (char*)(f - (char*)0); // expected-warning {{performing pointer arithmetic on a null pointer has undefined behavior}}
+  f = (char*)((char*)0 - (char*)0); // expected-warning {{performing pointer arithmetic on a null pointer has undefined behavior}} expected-warning {{performing pointer arithmetic on a null pointer has undefined behavior}}
 }
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp

[PATCH] D98798: Produce warning for performing pointer arithmetic on a null pointer.

[Eli Friedman via Phabricator via cfe-commits]

efriedma accepted this revision.
efriedma added a comment.
This revision is now accepted and ready to land.

LGTM


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98798/new/

https://reviews.llvm.org/D98798

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D98798: Produce warning for performing pointer arithmetic on a null pointer.

[Jamie Schmeiser via Phabricator via cfe-commits]

jamieschmeiser updated this revision to Diff 343483.
jamieschmeiser added a comment.

Respond to review comments: add C++ test.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98798/new/

https://reviews.llvm.org/D98798

Files:
  clang/lib/Sema/SemaExpr.cpp
  clang/test/Sema/pointer-addition.c
  clang/test/Sema/pointer-addition.cpp


Index: clang/test/Sema/pointer-addition.cpp
===
--- /dev/null
+++ clang/test/Sema/pointer-addition.cpp
@@ -0,0 +1,8 @@
+// RUN: %clang_cc1 %s -fsyntax-only -verify -pedantic -Wextra -std=c++11
+
+void a() {
+  char *f = (char*)0;
+  f = (char*)((char*)0 - f); // expected-warning {{performing pointer 
arithmetic on a null pointer has undefined behavior}}
+  f = (char*)(f - (char*)0); // expected-warning {{performing pointer 
arithmetic on a null pointer has undefined behavior}}
+  f = (char*)((char*)0 - (char*)0); // valid in C++
+}
Index: clang/test/Sema/pointer-addition.c
===
--- clang/test/Sema/pointer-addition.c
+++ clang/test/Sema/pointer-addition.c
@@ -29,4 +29,7 @@
   // Cases that don't match the GNU inttoptr idiom get a different warning.
   f = (char*)0 - i; // expected-warning {{performing pointer arithmetic on a 
null pointer has undefined behavior}}
   int *g = (int*)0 + i; // expected-warning {{performing pointer arithmetic on 
a null pointer has undefined behavior}}
+  f = (char*)((char*)0 - f); // expected-warning {{performing pointer 
arithmetic on a null pointer has undefined behavior}}
+  f = (char*)(f - (char*)0); // expected-warning {{performing pointer 
arithmetic on a null pointer has undefined behavior}}
+  f = (char*)((char*)0 - (char*)0); // expected-warning {{performing pointer 
arithmetic on a null pointer has undefined behavior}} expected-warning 
{{performing pointer arithmetic on a null pointer has undefined behavior}}
 }
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -10814,7 +10814,17 @@
LHS.get(), RHS.get()))
 return QualType();
 
-  // FIXME: Add warnings for nullptr - ptr.
+  bool LHSIsNullPtr = LHS.get()->IgnoreParenCasts()->isNullPointerConstant(
+  Context, Expr::NPC_ValueDependentIsNotNull);
+  bool RHSIsNullPtr = RHS.get()->IgnoreParenCasts()->isNullPointerConstant(
+  Context, Expr::NPC_ValueDependentIsNotNull);
+
+  // Subtracting nullptr or from nullptr should produce
+  // a warning expect nullptr - nullptr is valid in C++ [expr.add]p7
+  if (LHSIsNullPtr && (!getLangOpts().CPlusPlus || !RHSIsNullPtr))
+diagnoseArithmeticOnNullPointer(*this, Loc, LHS.get(), false);
+  if (RHSIsNullPtr && (!getLangOpts().CPlusPlus || !LHSIsNullPtr))
+diagnoseArithmeticOnNullPointer(*this, Loc, RHS.get(), false);
 
   // The pointee type may have zero size.  As an extension, a structure or
   // union may have zero size or an array may have zero length.  In this


Index: clang/test/Sema/pointer-addition.cpp
===
--- /dev/null
+++ clang/test/Sema/pointer-addition.cpp
@@ -0,0 +1,8 @@
+// RUN: %clang_cc1 %s -fsyntax-only -verify -pedantic -Wextra -std=c++11
+
+void a() {
+  char *f = (char*)0;
+  f = (char*)((char*)0 - f); // expected-warning {{performing pointer arithmetic on a null pointer has undefined behavior}}
+  f = (char*)(f - (char*)0); // expected-warning {{performing pointer arithmetic on a null pointer has undefined behavior}}
+  f = (char*)((char*)0 - (char*)0); // valid in C++
+}
Index: clang/test/Sema/pointer-addition.c
===
--- clang/test/Sema/pointer-addition.c
+++ clang/test/Sema/pointer-addition.c
@@ -29,4 +29,7 @@
   // Cases that don't match the GNU inttoptr idiom get a different warning.
   f = (char*)0 - i; // expected-warning {{performing pointer arithmetic on a null pointer has undefined behavior}}
   int *g = (int*)0 + i; // expected-warning {{performing pointer arithmetic on a null pointer has undefined behavior}}
+  f = (char*)((char*)0 - f); // expected-warning {{performing pointer arithmetic on a null pointer has undefined behavior}}
+  f = (char*)(f - (char*)0); // expected-warning {{performing pointer arithmetic on a null pointer has undefined behavior}}
+  f = (char*)((char*)0 - (char*)0); // expected-warning {{performing pointer arithmetic on a null pointer has undefined behavior}} expected-warning {{performing pointer arithmetic on a null pointer has undefined behavior}}
 }
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -10814,7 +10814,17 @@
LHS.get(), 

[PATCH] D98798: Produce warning for performing pointer arithmetic on a null pointer.

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:10723-10733
+  bool LHSIsNullPtr = LHS.get()->IgnoreParenCasts()->isNullPointerConstant(
+  Context, Expr::NPC_ValueDependentIsNotNull);
+  bool RHSIsNullPtr = RHS.get()->IgnoreParenCasts()->isNullPointerConstant(
+  Context, Expr::NPC_ValueDependentIsNotNull);
+
+  // Subtracting nullptr or from nullptr should produce
+  // a warning expect nullptr - nullptr is valid in C++ [expr.add]p7

nickdesaulniers wrote:
> Might it be better to move the C++ check to the top; have all of this within 
> a `if (!getLangOpts().CPlusPlus) {` block? Perhaps I'm misunderstanding what 
> the `||` is doing?
The getLangOpts().CPlusPlus check is specifically so we don't warn on 
null-null, which the C++ standard allows.



Comment at: clang/test/Sema/pointer-addition.c:34
+  f = (char*)(f - (char*)0); // expected-warning {{performing pointer 
arithmetic on a null pointer has undefined behavior}}
+  f = (char*)((char*)0 - (char*)0); // expected-warning {{performing pointer 
arithmetic on a null pointer has undefined behavior}} expected-warning 
{{performing pointer arithmetic on a null pointer has undefined behavior}}
 }

jamieschmeiser wrote:
> jamieschmeiser wrote:
> > efriedma wrote:
> > > This is what I was afraid would happen.
> > > 
> > > C++ has a specific carveout in the standard that "null-null" isn't 
> > > undefined. C doesn't, but I'm not sure warning is practically useful 
> > > there.
> > > 
> > > In any case, printing the same warning twice isn't useful.
> > Yes, I see that [expr.add] p 7 says nullptr-nullptr is defined to be 0.  I 
> > will suppress the warning for this.
> I disagree with the comment that the two warnings are not useful.  While it 
> is the same warning, the section of code indicated by each warning is 
> different and both need to be corrected to clear the code of warnings.  A 
> single warning would likely be more confusing in that a user would see the 
> same warning and not notice that a different section of code is indicated 
> after fixing the indicated problem.  I would expect the typical response to 
> be the user assuming that they had made a mistake and removing the initial 
> fix and fixing the second, only to again receive a same warning again with a 
> different section of code.  The different code sections indicated would 
> likely be overlooked, leading to frustration whereas two warnings clearly 
> indicates there are 2 problems.
I see what you mean about the two warnings; printing twice is probably okay.

In terms of whether we actually want the null-null warning in C, I could go 
either way.  I usually like to avoid differences between C and C++ where 
possible.  But the standards are actually written differently here.  And it's 
unlikely that explicitly written null-null shows up in practice, so probably 
nobody will notice either way.

Needs a testcase for the C++ behavior.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98798/new/

https://reviews.llvm.org/D98798

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D98798: Produce warning for performing pointer arithmetic on a null pointer.

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added a comment.

I didn't see any instances in quick testing on the Linux kernel: x86_64 
defconfig, aarch64 defconfig, arm defconfig, x86_64 allmodconfig.  So I guess 
that's a good thing (for Linux)!




Comment at: clang/lib/Sema/SemaExpr.cpp:10723-10733
+  bool LHSIsNullPtr = LHS.get()->IgnoreParenCasts()->isNullPointerConstant(
+  Context, Expr::NPC_ValueDependentIsNotNull);
+  bool RHSIsNullPtr = RHS.get()->IgnoreParenCasts()->isNullPointerConstant(
+  Context, Expr::NPC_ValueDependentIsNotNull);
+
+  // Subtracting nullptr or from nullptr should produce
+  // a warning expect nullptr - nullptr is valid in C++ [expr.add]p7

Might it be better to move the C++ check to the top; have all of this within a 
`if (!getLangOpts().CPlusPlus) {` block? Perhaps I'm misunderstanding what the 
`||` is doing?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98798/new/

https://reviews.llvm.org/D98798

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D98798: Produce warning for performing pointer arithmetic on a null pointer.

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added a comment.

In D98798#2734598 , @xbolva00 wrote:

> Maybe Nick could try this patch with linux kernel?

Yes! Thank you of thinking of me for this; we're currently sorting issues 
identified by this warning (as implemented today, for addition only IIUC) in:
https://lore.kernel.org/lkml/20210430111641.1911207-1-schne...@linux.ibm.com/

I ran out of time today, but I'll give this a shot tomorrow and see if this 
catches anything that looks like a false positive.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98798/new/

https://reviews.llvm.org/D98798

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D98798: Produce warning for performing pointer arithmetic on a null pointer.

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

Maybe Nick could try this patch with linux kernel?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98798/new/

https://reviews.llvm.org/D98798

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D98798: Produce warning for performing pointer arithmetic on a null pointer.

[Jamie Schmeiser via Phabricator via cfe-commits]

jamieschmeiser added a comment.

ping


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98798/new/

https://reviews.llvm.org/D98798

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D98798: Produce warning for performing pointer arithmetic on a null pointer.

[Jamie Schmeiser via Phabricator via cfe-commits]

jamieschmeiser added a comment.

ping


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98798/new/

https://reviews.llvm.org/D98798

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D98798: Produce warning for performing pointer arithmetic on a null pointer.

[Jamie Schmeiser via Phabricator via cfe-commits]

jamieschmeiser updated this revision to Diff 336828.
jamieschmeiser added a comment.

Fix indenting.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98798/new/

https://reviews.llvm.org/D98798

Files:
  clang/lib/Sema/SemaExpr.cpp
  clang/test/Sema/pointer-addition.c


Index: clang/test/Sema/pointer-addition.c
===
--- clang/test/Sema/pointer-addition.c
+++ clang/test/Sema/pointer-addition.c
@@ -29,4 +29,7 @@
   // Cases that don't match the GNU inttoptr idiom get a different warning.
   f = (char*)0 - i; // expected-warning {{performing pointer arithmetic on a 
null pointer has undefined behavior}}
   int *g = (int*)0 + i; // expected-warning {{performing pointer arithmetic on 
a null pointer has undefined behavior}}
+  f = (char*)((char*)0 - f); // expected-warning {{performing pointer 
arithmetic on a null pointer has undefined behavior}}
+  f = (char*)(f - (char*)0); // expected-warning {{performing pointer 
arithmetic on a null pointer has undefined behavior}}
+  f = (char*)((char*)0 - (char*)0); // expected-warning {{performing pointer 
arithmetic on a null pointer has undefined behavior}} expected-warning 
{{performing pointer arithmetic on a null pointer has undefined behavior}}
 }
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -10720,7 +10720,17 @@
LHS.get(), RHS.get()))
 return QualType();
 
-  // FIXME: Add warnings for nullptr - ptr.
+  bool LHSIsNullPtr = LHS.get()->IgnoreParenCasts()->isNullPointerConstant(
+  Context, Expr::NPC_ValueDependentIsNotNull);
+  bool RHSIsNullPtr = RHS.get()->IgnoreParenCasts()->isNullPointerConstant(
+  Context, Expr::NPC_ValueDependentIsNotNull);
+
+  // Subtracting nullptr or from nullptr should produce
+  // a warning expect nullptr - nullptr is valid in C++ [expr.add]p7
+  if (LHSIsNullPtr && (!getLangOpts().CPlusPlus || !RHSIsNullPtr))
+diagnoseArithmeticOnNullPointer(*this, Loc, LHS.get(), false);
+  if (RHSIsNullPtr && (!getLangOpts().CPlusPlus || !LHSIsNullPtr))
+diagnoseArithmeticOnNullPointer(*this, Loc, RHS.get(), false);
 
   // The pointee type may have zero size.  As an extension, a structure or
   // union may have zero size or an array may have zero length.  In this


Index: clang/test/Sema/pointer-addition.c
===
--- clang/test/Sema/pointer-addition.c
+++ clang/test/Sema/pointer-addition.c
@@ -29,4 +29,7 @@
   // Cases that don't match the GNU inttoptr idiom get a different warning.
   f = (char*)0 - i; // expected-warning {{performing pointer arithmetic on a null pointer has undefined behavior}}
   int *g = (int*)0 + i; // expected-warning {{performing pointer arithmetic on a null pointer has undefined behavior}}
+  f = (char*)((char*)0 - f); // expected-warning {{performing pointer arithmetic on a null pointer has undefined behavior}}
+  f = (char*)(f - (char*)0); // expected-warning {{performing pointer arithmetic on a null pointer has undefined behavior}}
+  f = (char*)((char*)0 - (char*)0); // expected-warning {{performing pointer arithmetic on a null pointer has undefined behavior}} expected-warning {{performing pointer arithmetic on a null pointer has undefined behavior}}
 }
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -10720,7 +10720,17 @@
LHS.get(), RHS.get()))
 return QualType();
 
-  // FIXME: Add warnings for nullptr - ptr.
+  bool LHSIsNullPtr = LHS.get()->IgnoreParenCasts()->isNullPointerConstant(
+  Context, Expr::NPC_ValueDependentIsNotNull);
+  bool RHSIsNullPtr = RHS.get()->IgnoreParenCasts()->isNullPointerConstant(
+  Context, Expr::NPC_ValueDependentIsNotNull);
+
+  // Subtracting nullptr or from nullptr should produce
+  // a warning expect nullptr - nullptr is valid in C++ [expr.add]p7
+  if (LHSIsNullPtr && (!getLangOpts().CPlusPlus || !RHSIsNullPtr))
+diagnoseArithmeticOnNullPointer(*this, Loc, LHS.get(), false);
+  if (RHSIsNullPtr && (!getLangOpts().CPlusPlus || !LHSIsNullPtr))
+diagnoseArithmeticOnNullPointer(*this, Loc, RHS.get(), false);
 
   // The pointee type may have zero size.  As an extension, a structure or
   // union may have zero size or an array may have zero length.  In this
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D98798: Produce warning for performing pointer arithmetic on a null pointer.

[Jamie Schmeiser via Phabricator via cfe-commits]

jamieschmeiser updated this revision to Diff 336567.
jamieschmeiser added a comment.

Respond to review comments:  Do not issue warning for nullptr - nullptr in C++.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98798/new/

https://reviews.llvm.org/D98798

Files:
  clang/lib/Sema/SemaExpr.cpp
  clang/test/Sema/pointer-addition.c


Index: clang/test/Sema/pointer-addition.c
===
--- clang/test/Sema/pointer-addition.c
+++ clang/test/Sema/pointer-addition.c
@@ -29,4 +29,7 @@
   // Cases that don't match the GNU inttoptr idiom get a different warning.
   f = (char*)0 - i; // expected-warning {{performing pointer arithmetic on a 
null pointer has undefined behavior}}
   int *g = (int*)0 + i; // expected-warning {{performing pointer arithmetic on 
a null pointer has undefined behavior}}
+  f = (char*)((char*)0 - f); // expected-warning {{performing pointer 
arithmetic on a null pointer has undefined behavior}}
+  f = (char*)(f - (char*)0); // expected-warning {{performing pointer 
arithmetic on a null pointer has undefined behavior}}
+  f = (char*)((char*)0 - (char*)0); // expected-warning {{performing pointer 
arithmetic on a null pointer has undefined behavior}} expected-warning 
{{performing pointer arithmetic on a null pointer has undefined behavior}}
 }
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -10720,7 +10720,17 @@
LHS.get(), RHS.get()))
 return QualType();
 
-  // FIXME: Add warnings for nullptr - ptr.
+  bool LHSIsNullPtr = LHS.get()->IgnoreParenCasts()->isNullPointerConstant(
+  Context, Expr::NPC_ValueDependentIsNotNull);
+  bool RHSIsNullPtr = RHS.get()->IgnoreParenCasts()->isNullPointerConstant(
+  Context, Expr::NPC_ValueDependentIsNotNull);
+
+  // Subtracting nullptr or from nullptr should produce
+  // a warning expect nullptr - nullptr is valid in C++ [expr.add]p7
+  if (LHSIsNullPtr && (!getLangOpts().CPlusPlus || !RHSIsNullPtr))
+diagnoseArithmeticOnNullPointer(*this, Loc, LHS.get(), false);
+  if (RHSIsNullPtr && (!getLangOpts().CPlusPlus || !LHSIsNullPtr))
+  diagnoseArithmeticOnNullPointer(*this, Loc, RHS.get(), false);
 
   // The pointee type may have zero size.  As an extension, a structure or
   // union may have zero size or an array may have zero length.  In this


Index: clang/test/Sema/pointer-addition.c
===
--- clang/test/Sema/pointer-addition.c
+++ clang/test/Sema/pointer-addition.c
@@ -29,4 +29,7 @@
   // Cases that don't match the GNU inttoptr idiom get a different warning.
   f = (char*)0 - i; // expected-warning {{performing pointer arithmetic on a null pointer has undefined behavior}}
   int *g = (int*)0 + i; // expected-warning {{performing pointer arithmetic on a null pointer has undefined behavior}}
+  f = (char*)((char*)0 - f); // expected-warning {{performing pointer arithmetic on a null pointer has undefined behavior}}
+  f = (char*)(f - (char*)0); // expected-warning {{performing pointer arithmetic on a null pointer has undefined behavior}}
+  f = (char*)((char*)0 - (char*)0); // expected-warning {{performing pointer arithmetic on a null pointer has undefined behavior}} expected-warning {{performing pointer arithmetic on a null pointer has undefined behavior}}
 }
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -10720,7 +10720,17 @@
LHS.get(), RHS.get()))
 return QualType();
 
-  // FIXME: Add warnings for nullptr - ptr.
+  bool LHSIsNullPtr = LHS.get()->IgnoreParenCasts()->isNullPointerConstant(
+  Context, Expr::NPC_ValueDependentIsNotNull);
+  bool RHSIsNullPtr = RHS.get()->IgnoreParenCasts()->isNullPointerConstant(
+  Context, Expr::NPC_ValueDependentIsNotNull);
+
+  // Subtracting nullptr or from nullptr should produce
+  // a warning expect nullptr - nullptr is valid in C++ [expr.add]p7
+  if (LHSIsNullPtr && (!getLangOpts().CPlusPlus || !RHSIsNullPtr))
+diagnoseArithmeticOnNullPointer(*this, Loc, LHS.get(), false);
+  if (RHSIsNullPtr && (!getLangOpts().CPlusPlus || !LHSIsNullPtr))
+  diagnoseArithmeticOnNullPointer(*this, Loc, RHS.get(), false);
 
   // The pointee type may have zero size.  As an extension, a structure or
   // union may have zero size or an array may have zero length.  In this
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D98798: Produce warning for performing pointer arithmetic on a null pointer.

[Jamie Schmeiser via Phabricator via cfe-commits]

jamieschmeiser added inline comments.



Comment at: clang/test/Sema/pointer-addition.c:34
+  f = (char*)(f - (char*)0); // expected-warning {{performing pointer 
arithmetic on a null pointer has undefined behavior}}
+  f = (char*)((char*)0 - (char*)0); // expected-warning {{performing pointer 
arithmetic on a null pointer has undefined behavior}} expected-warning 
{{performing pointer arithmetic on a null pointer has undefined behavior}}
 }

jamieschmeiser wrote:
> efriedma wrote:
> > This is what I was afraid would happen.
> > 
> > C++ has a specific carveout in the standard that "null-null" isn't 
> > undefined. C doesn't, but I'm not sure warning is practically useful there.
> > 
> > In any case, printing the same warning twice isn't useful.
> Yes, I see that [expr.add] p 7 says nullptr-nullptr is defined to be 0.  I 
> will suppress the warning for this.
I disagree with the comment that the two warnings are not useful.  While it is 
the same warning, the section of code indicated by each warning is different 
and both need to be corrected to clear the code of warnings.  A single warning 
would likely be more confusing in that a user would see the same warning and 
not notice that a different section of code is indicated after fixing the 
indicated problem.  I would expect the typical response to be the user assuming 
that they had made a mistake and removing the initial fix and fixing the 
second, only to again receive a same warning again with a different section of 
code.  The different code sections indicated would likely be overlooked, 
leading to frustration whereas two warnings clearly indicates there are 2 
problems.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98798/new/

https://reviews.llvm.org/D98798

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D98798: Produce warning for performing pointer arithmetic on a null pointer.

[Jamie Schmeiser via Phabricator via cfe-commits]

jamieschmeiser added inline comments.



Comment at: clang/test/Sema/pointer-addition.c:34
+  f = (char*)(f - (char*)0); // expected-warning {{performing pointer 
arithmetic on a null pointer has undefined behavior}}
+  f = (char*)((char*)0 - (char*)0); // expected-warning {{performing pointer 
arithmetic on a null pointer has undefined behavior}} expected-warning 
{{performing pointer arithmetic on a null pointer has undefined behavior}}
 }

efriedma wrote:
> This is what I was afraid would happen.
> 
> C++ has a specific carveout in the standard that "null-null" isn't undefined. 
> C doesn't, but I'm not sure warning is practically useful there.
> 
> In any case, printing the same warning twice isn't useful.
Yes, I see that [expr.add] p 7 says nullptr-nullptr is defined to be 0.  I will 
suppress the warning for this.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98798/new/

https://reviews.llvm.org/D98798

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D98798: Produce warning for performing pointer arithmetic on a null pointer.

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:10724
+  // Subtracting from a null pointer should produce a warning.
+  if (LHS.get()->IgnoreParenCasts()->isNullPointerConstant(
+  Context, Expr::NPC_ValueDependentIsNotNull))

IgnoreParenCasts() seems a little weird here; e.g. this code thinks 
`(char*)(char)256` isn't null?  Not sure how much it practically matters, 
though, given this is just a warning.



Comment at: clang/test/Sema/pointer-addition.c:34
+  f = (char*)(f - (char*)0); // expected-warning {{performing pointer 
arithmetic on a null pointer has undefined behavior}}
+  f = (char*)((char*)0 - (char*)0); // expected-warning {{performing pointer 
arithmetic on a null pointer has undefined behavior}} expected-warning 
{{performing pointer arithmetic on a null pointer has undefined behavior}}
 }

This is what I was afraid would happen.

C++ has a specific carveout in the standard that "null-null" isn't undefined. C 
doesn't, but I'm not sure warning is practically useful there.

In any case, printing the same warning twice isn't useful.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98798/new/

https://reviews.llvm.org/D98798

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D98798: Produce warning for performing pointer arithmetic on a null pointer.

[Jamie Schmeiser via Phabricator via cfe-commits]

jamieschmeiser updated this revision to Diff 336500.
jamieschmeiser added a comment.

Respond to review comments: add requested test.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98798/new/

https://reviews.llvm.org/D98798

Files:
  clang/lib/Sema/SemaExpr.cpp
  clang/test/Sema/pointer-addition.c


Index: clang/test/Sema/pointer-addition.c
===
--- clang/test/Sema/pointer-addition.c
+++ clang/test/Sema/pointer-addition.c
@@ -29,4 +29,7 @@
   // Cases that don't match the GNU inttoptr idiom get a different warning.
   f = (char*)0 - i; // expected-warning {{performing pointer arithmetic on a 
null pointer has undefined behavior}}
   int *g = (int*)0 + i; // expected-warning {{performing pointer arithmetic on 
a null pointer has undefined behavior}}
+  f = (char*)((char*)0 - f); // expected-warning {{performing pointer 
arithmetic on a null pointer has undefined behavior}}
+  f = (char*)(f - (char*)0); // expected-warning {{performing pointer 
arithmetic on a null pointer has undefined behavior}}
+  f = (char*)((char*)0 - (char*)0); // expected-warning {{performing pointer 
arithmetic on a null pointer has undefined behavior}} expected-warning 
{{performing pointer arithmetic on a null pointer has undefined behavior}}
 }
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -10720,7 +10720,15 @@
LHS.get(), RHS.get()))
 return QualType();
 
-  // FIXME: Add warnings for nullptr - ptr.
+  // Subtracting from a null pointer should produce a warning.
+  if (LHS.get()->IgnoreParenCasts()->isNullPointerConstant(
+  Context, Expr::NPC_ValueDependentIsNotNull))
+diagnoseArithmeticOnNullPointer(*this, Loc, LHS.get(), false);
+
+  // Subtracting a null pointer should produce a warning.
+  if (RHS.get()->IgnoreParenCasts()->isNullPointerConstant(
+  Context, Expr::NPC_ValueDependentIsNotNull))
+diagnoseArithmeticOnNullPointer(*this, Loc, RHS.get(), false);
 
   // The pointee type may have zero size.  As an extension, a structure or
   // union may have zero size or an array may have zero length.  In this


Index: clang/test/Sema/pointer-addition.c
===
--- clang/test/Sema/pointer-addition.c
+++ clang/test/Sema/pointer-addition.c
@@ -29,4 +29,7 @@
   // Cases that don't match the GNU inttoptr idiom get a different warning.
   f = (char*)0 - i; // expected-warning {{performing pointer arithmetic on a null pointer has undefined behavior}}
   int *g = (int*)0 + i; // expected-warning {{performing pointer arithmetic on a null pointer has undefined behavior}}
+  f = (char*)((char*)0 - f); // expected-warning {{performing pointer arithmetic on a null pointer has undefined behavior}}
+  f = (char*)(f - (char*)0); // expected-warning {{performing pointer arithmetic on a null pointer has undefined behavior}}
+  f = (char*)((char*)0 - (char*)0); // expected-warning {{performing pointer arithmetic on a null pointer has undefined behavior}} expected-warning {{performing pointer arithmetic on a null pointer has undefined behavior}}
 }
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -10720,7 +10720,15 @@
LHS.get(), RHS.get()))
 return QualType();
 
-  // FIXME: Add warnings for nullptr - ptr.
+  // Subtracting from a null pointer should produce a warning.
+  if (LHS.get()->IgnoreParenCasts()->isNullPointerConstant(
+  Context, Expr::NPC_ValueDependentIsNotNull))
+diagnoseArithmeticOnNullPointer(*this, Loc, LHS.get(), false);
+
+  // Subtracting a null pointer should produce a warning.
+  if (RHS.get()->IgnoreParenCasts()->isNullPointerConstant(
+  Context, Expr::NPC_ValueDependentIsNotNull))
+diagnoseArithmeticOnNullPointer(*this, Loc, RHS.get(), false);
 
   // The pointee type may have zero size.  As an extension, a structure or
   // union may have zero size or an array may have zero length.  In this
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D98798: Produce warning for performing pointer arithmetic on a null pointer.

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

Please add a test for `(char*)0-(char*)0`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98798/new/

https://reviews.llvm.org/D98798

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D98798: Produce warning for performing pointer arithmetic on a null pointer.

[Jamie Schmeiser via Phabricator via cfe-commits]

jamieschmeiser updated this revision to Diff 336148.
jamieschmeiser added a comment.

Reformat to satisfy clang-format


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98798/new/

https://reviews.llvm.org/D98798

Files:
  clang/lib/Sema/SemaExpr.cpp
  clang/test/Sema/pointer-addition.c


Index: clang/test/Sema/pointer-addition.c
===
--- clang/test/Sema/pointer-addition.c
+++ clang/test/Sema/pointer-addition.c
@@ -29,4 +29,6 @@
   // Cases that don't match the GNU inttoptr idiom get a different warning.
   f = (char*)0 - i; // expected-warning {{performing pointer arithmetic on a 
null pointer has undefined behavior}}
   int *g = (int*)0 + i; // expected-warning {{performing pointer arithmetic on 
a null pointer has undefined behavior}}
+  f = (char*)((char*)0 - f); // expected-warning {{performing pointer 
arithmetic on a null pointer has undefined behavior}}
+  f = (char*)(f - (char*)0); // expected-warning {{performing pointer 
arithmetic on a null pointer has undefined behavior}}
 }
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -10720,7 +10720,15 @@
LHS.get(), RHS.get()))
 return QualType();
 
-  // FIXME: Add warnings for nullptr - ptr.
+  // Subtracting from a null pointer should produce a warning.
+  if (LHS.get()->IgnoreParenCasts()->isNullPointerConstant(
+  Context, Expr::NPC_ValueDependentIsNotNull))
+diagnoseArithmeticOnNullPointer(*this, Loc, LHS.get(), false);
+
+  // Subtracting a null pointer should produce a warning.
+  if (RHS.get()->IgnoreParenCasts()->isNullPointerConstant(
+  Context, Expr::NPC_ValueDependentIsNotNull))
+diagnoseArithmeticOnNullPointer(*this, Loc, RHS.get(), false);
 
   // The pointee type may have zero size.  As an extension, a structure or
   // union may have zero size or an array may have zero length.  In this


Index: clang/test/Sema/pointer-addition.c
===
--- clang/test/Sema/pointer-addition.c
+++ clang/test/Sema/pointer-addition.c
@@ -29,4 +29,6 @@
   // Cases that don't match the GNU inttoptr idiom get a different warning.
   f = (char*)0 - i; // expected-warning {{performing pointer arithmetic on a null pointer has undefined behavior}}
   int *g = (int*)0 + i; // expected-warning {{performing pointer arithmetic on a null pointer has undefined behavior}}
+  f = (char*)((char*)0 - f); // expected-warning {{performing pointer arithmetic on a null pointer has undefined behavior}}
+  f = (char*)(f - (char*)0); // expected-warning {{performing pointer arithmetic on a null pointer has undefined behavior}}
 }
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -10720,7 +10720,15 @@
LHS.get(), RHS.get()))
 return QualType();
 
-  // FIXME: Add warnings for nullptr - ptr.
+  // Subtracting from a null pointer should produce a warning.
+  if (LHS.get()->IgnoreParenCasts()->isNullPointerConstant(
+  Context, Expr::NPC_ValueDependentIsNotNull))
+diagnoseArithmeticOnNullPointer(*this, Loc, LHS.get(), false);
+
+  // Subtracting a null pointer should produce a warning.
+  if (RHS.get()->IgnoreParenCasts()->isNullPointerConstant(
+  Context, Expr::NPC_ValueDependentIsNotNull))
+diagnoseArithmeticOnNullPointer(*this, Loc, RHS.get(), false);
 
   // The pointee type may have zero size.  As an extension, a structure or
   // union may have zero size or an array may have zero length.  In this
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits