analyzer: don't warn for deref-before-check for checks in macros [PR108745]

Integration testing shows this patch fixes all 9 known false positives
from -Wanalyzer-deref-before-check within ImageMagick-7.1.0-57, and
eliminates 34 further as-yet unassessed such diagnostics, without
eliminating the 1 known true positive.

This improves the rate of true positives for the warning from
1.56% to 4.76% of the total:

-Wanalyzer-deref-before-check: 1.56% -> 4.76% (GOOD: 1 BAD: 63->20)
    TRUE:  1
   FALSE: 15 ->  6 (-9)
             ImageMagick-7.1.0-57:  9 ->  0 (-9)
    TODO: 48 -> 14 (-34)
             ImageMagick-7.1.0-57: 21 ->  1 (-20)
                       qemu-7.2.0: 25 -> 11 (-14)

gcc/analyzer/ChangeLog:
	PR analyzer/108745
	* sm-malloc.cc (deref_before_check::emit): Reject the warning if
	the check occurs within a macro defintion.

gcc/testsuite/ChangeLog:
	PR analyzer/108745
	* gcc.dg/analyzer/deref-before-check-macro-pr108745.c: New test.
	* gcc.dg/analyzer/deref-before-check-macro.c: New test.

Signed-off-by: David Malcolm <dmalcolm@redhat.com>

3 files changed, 116 insertions, 0 deletions

diff --git a/gcc/analyzer/sm-malloc.cc b/gcc/analyzer/sm-malloc.cc
index 9aee810..c24fe73 100644
--- a/gcc/analyzer/sm-malloc.cc
+++ b/gcc/analyzer/sm-malloc.cc
@@ -1519,6 +1519,43 @@ public:
 	!= &m_check_enode->get_point ().get_call_string ())
       return false;
 
+    /* Reject the warning if the check occurs within a macro defintion.
+       This avoids false positives for such code as:
+
+	#define throw_error \
+	   do {             \
+	     if (p)         \
+	       cleanup (p); \
+	     return;        \
+	   } while (0)
+
+	if (p->idx >= n)
+	  throw_error ();
+
+       where the usage of "throw_error" implicitly adds a check
+       on 'p'.
+
+       We do warn when the check is in a macro expansion if we can get
+       at the location of the condition and it is't part of the
+       definition, so that we warn for checks such as:
+	   if (words[0][0] == '@')
+	     return;
+	   g_assert(words[0] != NULL); <--- here
+       Unfortunately we don't have locations for individual gimple
+       arguments, so in:
+	   g_assert (ptr);
+       we merely have a gimple_cond
+	   if (p_2(D) == 0B)
+       with no way of getting at the location of the condition separately
+       from that of the gimple_cond (where the "if" is within the macro
+       definition).  We reject the warning for such cases.
+
+       We do warn when the *deref* occurs in a macro, since this can be
+       a source of real bugs; see e.g. PR 77425.  */
+    location_t check_loc = m_check_enode->get_point ().get_location ();
+    if (linemap_location_from_macro_definition_p (line_table, check_loc))
+      return false;
+
     /* Reject the warning if the deref's BB doesn't dominate that
        of the check, so that we don't warn e.g. for shared cleanup
        code that checks a pointer for NULL, when that code is sometimes
diff --git a/gcc/testsuite/gcc.dg/analyzer/deref-before-check-macro-pr108745.c b/gcc/testsuite/gcc.dg/analyzer/deref-before-check-macro-pr108745.c
new file mode 100644
index 0000000..92f5a02
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/analyzer/deref-before-check-macro-pr108745.c
@@ -0,0 +1,54 @@
+/* Reduced from ImageMagick-7.1.0-57.  */
+
+#define NULL ((void *)0)
+
+typedef __builtin_va_list va_list;
+typedef __SIZE_TYPE__ size_t;
+
+typedef struct _ExceptionInfo ExceptionInfo;
+
+void
+ThrowMagickException(ExceptionInfo*,
+		     const char*,
+		     const char*,
+		     ...) __attribute__((__format__(__printf__, 3, 4)));
+
+typedef struct _Image
+{
+  /* [...snip...] */
+  size_t columns, rows, depth, colors;
+  /* [...snip...] */
+} Image;
+
+typedef struct _ImageInfo
+{
+  /* [...snip...] */
+  char filename[4096];
+  /* [...snip...] */
+} ImageInfo;
+
+extern Image *AcquireImage(const ImageInfo*, ExceptionInfo*);
+extern void CloseBlob(Image*);
+extern Image *DestroyImageList(Image*);
+
+#define ThrowReaderException(tag) \
+{ \
+  (void) ThrowMagickException(exception, tag, \
+    "`%s'",image_info->filename); \
+  if ((image) != (Image *) NULL) \
+    { \
+      (void) CloseBlob(image); \
+      image=DestroyImageList(image); \
+    } \
+  return((Image *) NULL); \
+}
+
+Image*
+ReadMAPImage(const ImageInfo* image_info, ExceptionInfo* exception)
+{
+  Image* image;
+  image = AcquireImage(image_info, exception);
+  if ((image->columns == 0) || (image->rows == 0))
+    ThrowReaderException("MustSpecifyImageSize");
+  return image;
+}
diff --git a/gcc/testsuite/gcc.dg/analyzer/deref-before-check-macro.c b/gcc/testsuite/gcc.dg/analyzer/deref-before-check-macro.c
new file mode 100644
index 0000000..5146129
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/analyzer/deref-before-check-macro.c
@@ -0,0 +1,25 @@
+#define NULL ((void*)0)
+
+#define MY_ASSERT(COND)				\
+  do {						\
+    if (!(COND)) { __builtin_abort(); }		\
+  } while (0)
+
+int test_1 (int *p)
+{
+  int result = *p;
+  MY_ASSERT (p); /* { dg-warning "check of 'p' for NULL after already dereferencing it" "" { xfail *-*-* } } */
+  /* Due to lack of locations for gimple arguments we can't get
+     at the location of the condition separately from the
+     gimple_cond stmt, and thus can't distinguish if it's in the
+     macro definition or in the supplied params; we defer to
+     rejecting the diagnostic.  */
+  return result;
+}
+
+int test_2 (int *p)
+{
+  int result = *p;
+  MY_ASSERT (p != NULL); /* { dg-warning "check of 'p' for NULL after already dereferencing it" "" { xfail *-*-* } } */
+  return result;
+}