[pushed] analyzer: don't warn for deref-before-check for checks in macros [PR108745]
Checks
Commit Message
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)
Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.
Pushed to trunk as r13-5811-gaa601e30758581.
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>
---
gcc/analyzer/sm-malloc.cc | 37 +++++++++++++
.../deref-before-check-macro-pr108745.c | 54 +++++++++++++++++++
.../analyzer/deref-before-check-macro.c | 25 +++++++++
3 files changed, 116 insertions(+)
create mode 100644 gcc/testsuite/gcc.dg/analyzer/deref-before-check-macro-pr108745.c
create mode 100644 gcc/testsuite/gcc.dg/analyzer/deref-before-check-macro.c
@@ -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
new file mode 100644
@@ -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;
+}
new file mode 100644
@@ -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;
+}