From 0453aaa3f0909e46a964917cb3913d0b8e89a522 Mon Sep 17 00:00:00 2001
From: David Malcolm <dmalcolm@redhat.com>
Date: Thu, 25 Feb 2016 14:01:34 -0500
Subject: [PATCH 7/8] FIXME: PR c/6906: WIP

---
 gcc/Makefile.in                                    |   2 +-
 gcc/c-family/c-macro.c                             | 352 +++++++++++++++++++++
 gcc/c-family/c-macro.h                             |  26 ++
 gcc/c-family/c.opt                                 |   4 +
 gcc/c/c-decl.c                                     |   3 +
 gcc/cp/semantics.c                                 |   3 +
 gcc/doc/invoke.texi                                |  11 +
 .../c-c++-common/Wside-effect-in-assert-1.c        |  74 +++++
 .../c-c++-common/Wside-effect-in-assert-2.c        |  44 +++
 gcc/testsuite/g++.dg/Wside-effect-in-assert-3.C    |  29 ++
 10 files changed, 547 insertions(+), 1 deletion(-)
 create mode 100644 gcc/c-family/c-macro.c
 create mode 100644 gcc/c-family/c-macro.h
 create mode 100644 gcc/testsuite/c-c++-common/Wside-effect-in-assert-1.c
 create mode 100644 gcc/testsuite/c-c++-common/Wside-effect-in-assert-2.c
 create mode 100644 gcc/testsuite/g++.dg/Wside-effect-in-assert-3.C

diff --git a/gcc/Makefile.in b/gcc/Makefile.in
index 3d6f0e6..6b99c66 100644
--- a/gcc/Makefile.in
+++ b/gcc/Makefile.in
@@ -1169,7 +1169,7 @@ C_COMMON_OBJS = c-family/c-common.o c-family/c-cppbuiltin.o c-family/c-dump.o \
   c-family/c-lex.o c-family/c-omp.o c-family/c-opts.o c-family/c-pch.o \
   c-family/c-ppoutput.o c-family/c-pragma.o c-family/c-pretty-print.o \
   c-family/c-semantics.o c-family/c-ada-spec.o \
-  c-family/c-cilkplus.o \
+  c-family/c-cilkplus.o c-family/c-macro.o \
   c-family/array-notation-common.o c-family/cilk.o c-family/c-ubsan.o
 
 # Language-independent object files.
diff --git a/gcc/c-family/c-macro.c b/gcc/c-family/c-macro.c
new file mode 100644
index 0000000..5330ed4
--- /dev/null
+++ b/gcc/c-family/c-macro.c
@@ -0,0 +1,352 @@
+/* Implementation of warnings relating to macros.
+   Copyright (C) 2016 Free Software Foundation, Inc.
+
+This file is part of GCC.
+
+GCC is free software; you can redistribute it and/or modify it under
+the terms of the GNU General Public License as published by the Free
+Software Foundation; either version 3, or (at your option) any later
+version.
+
+GCC is distributed in the hope that it will be useful, but WITHOUT ANY
+WARRANTY; without even the implied warranty of MERCHANTABILITY or
+FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+for more details.
+
+You should have received a copy of the GNU General Public License
+along with GCC; see the file COPYING3.  If not see
+<http://www.gnu.org/licenses/>.  */
+
+#include "config.h"
+#include "system.h"
+#include "coretypes.h"
+#include "tm.h"
+#include "c-common.h"
+#include "c-macro.h"
+#include "calls.h"
+
+
+
+/* To avoid warning repeatedly, we track the macro arguments that
+   we've already warned for.  We want to only warn once per
+   macro expansion/argument, as there could be multiple side-effects
+   within the argument.  The easiest way to do this is to
+   use the virtual location of the argument with the macro line_map,
+   and only warn once per virtual location.  */
+
+class idempotent_warning
+{
+ public:
+  bool warned_about_p (location_t loc)
+  {
+    if (!m_hash_set)
+      m_hash_set = new location_hash_set ();
+    if (m_hash_set->contains (loc))
+      /* We've already warned about this virtual location.  */
+      return true;
+    else
+      return false;
+  }
+
+  void set_warned_about (location_t loc)
+  {
+    gcc_assert (m_hash_set);
+    m_hash_set->add (loc);
+  }
+
+ private:
+  typedef hash_set<location_t, int_hash<location_t, 0, 0xffffffff> >
+    location_hash_set;
+  location_hash_set *m_hash_set;
+};
+
+
+
+/* Implementation of -Wside-effect-in-assert.  */
+
+static idempotent_warning side_effects_in_assert;
+
+/* Return whether NAME contains "assert" or "ASSERT".  */
+
+static bool name_contains_assert_p (const char *name)
+{
+  gcc_assert (name);
+
+  /* FIXME: glibc has strcasestr as a non-standard extension.  */
+
+  if (strstr (name, "assert"))
+    return true;
+  if (strstr (name, "ASSERT"))
+    return true;
+
+  return false;
+}
+
+/* Return whether a macro with name MACRO_NAME should be regarded as
+   an assertion macro.  Currently any macro with "assert" or "ASSERT"
+   within the name is treated as an assertion macro.
+
+   TODO: make this case-insensitive.  */
+
+static bool macro_name_is_assertion_p (const char *macro_name)
+{
+  if (name_contains_assert_p (macro_name))
+    return true;
+
+  /* TODO: other macro names that we could warn about:
+     - the Linux kernel has BUG_ON
+     - Qt has "Q_CHECK_PTR".  */
+
+  return false;
+}
+
+/* Return whether location LOC is within the expansion of an assertion
+   macro (by looking for at the names of macros, using
+   macro_name_is_assertion_p).
+
+   If it returns true, *OUT_MACRO is written to with the macro map
+   relating to the assertion macro.  */
+
+static bool
+within_assert_expansion_p (location_t loc, const line_map_macro **out_expansion)
+{
+  const line_map *map = linemap_lookup (line_table, loc);
+
+  while (linemap_macro_expansion_map_p (map))
+    {
+      const line_map_macro *macro_map = linemap_check_macro (map);
+      const char* macro_name = linemap_map_get_macro_name (macro_map);
+      if (macro_name_is_assertion_p (macro_name))
+	{
+	  *out_expansion = macro_map;
+	  return true;
+	}
+      loc = linemap_unwind_toward_expansion (line_table, loc, &map);
+    }
+
+  return false;
+}
+
+/* Return whether CALL_EXPR is a call to an assertion-failure handler.
+   For now we just look for calls to functions with "assert" or "ASSERT"
+   in the name.
+   This covers:
+   - glibc ("__assert_fail")
+   - glib ("g_assertion_message_expr")
+   - Qt ("qt_assert").  */
+
+static bool
+fndecl_is_assertion_failure_handler_p (tree fndecl)
+{
+  tree identifier = DECL_NAME (fndecl);
+  if (!identifier)
+    return false;
+  gcc_assert (TREE_CODE (identifier) == IDENTIFIER_NODE);
+
+  if (name_contains_assert_p (IDENTIFIER_POINTER (identifier)))
+    return true;
+
+  return false;
+}
+
+/* Is LOC within the expansion of MACRO_MAP, or any of the
+   expansions that led to it?  */
+
+bool loc_within_expansion_p (location_t loc, const line_map_macro *macro_map)
+{
+  const line_map *map = linemap_lookup (line_table, loc);
+  gcc_assert (map);
+
+  while (1)
+    {
+      if (!linemap_macro_expansion_map_p (map))
+	return false;
+
+      const line_map_macro *loc_macro_map = linemap_check_macro (map);
+      gcc_assert (loc_macro_map);
+
+      if (loc >= macro_map->start_location
+	  && loc < macro_map->start_location + (2 * macro_map->n_tokens))
+	return true;
+
+      /* Iterate.  */
+      loc = linemap_unwind_toward_expansion (line_table, loc, &map);
+    }
+}
+
+/* Return true iff FNDECL is known to have an empty body.  */
+
+static bool
+is_known_empty_function_p (tree fndecl)
+{
+  gcc_assert (fndecl);
+  gcc_assert (TREE_CODE (fndecl) == FUNCTION_DECL);
+
+  tree body = DECL_SAVED_TREE (fndecl);
+  if (!body)
+    return false;
+
+  gcc_assert (TREE_CODE (body) == STATEMENT_LIST);
+
+  if (NULL == STATEMENT_LIST_HEAD (body))
+    /* We have a known empty function.  */
+    return true;
+
+  /* Otherwise it's non-empty.  For now we don't attempt to
+     analyze it for side-effects.  */
+  return false;
+}
+
+/* Return whether T could be considered a "side effect" if it occurs
+   within the given expansion of an assertion macro.
+   FIXME: ASSERT_EXPANSION (can it be NULL).
+*/
+
+static bool
+assertion_side_effect_at_p (tree t, const line_map_macro *assert_expansion)
+{
+  gcc_assert (t);
+
+  switch (TREE_CODE (t))
+    {
+      case MODIFY_EXPR:
+	{
+	  tree target = TREE_OPERAND (t, 0);
+	  /* Within the implementation of glib's assertion macro
+	     is code that expands to:
+		__extension__ ({
+		  int _g_boolean_var_;
+		  if (COND)
+		    _g_boolean_var_ = 1;
+		  else
+		    _g_boolean_var_ = 0;
+		  _g_boolean_var_;
+		})
+	     We don't want to warn about modifications to variables
+	     like "_g_boolean_var_" above that are declared within
+	     the macro expansion.  */
+	  if (TREE_CODE (target) == VAR_DECL)
+	    {
+	      /* Don't warn about compiler-generated temporaries
+		 e.g. in the above, there's a compiler-generated
+		 tmpvar = last_value_in_stmt_expression;  */
+	      if (NULL == DECL_NAME (target))
+		return false;
+
+	      /* Don't warn about variables declared within the
+		 "assert" expansion.  */
+	      location_t decl_loc = DECL_SOURCE_LOCATION (target);
+	      //inform (decl_loc, "decl_loc: %x", decl_loc);
+	      if (assert_expansion)
+		if (loc_within_expansion_p (decl_loc, assert_expansion))
+		  return false;
+	    }
+	  return true;
+	}
+      case PREDECREMENT_EXPR:
+      case PREINCREMENT_EXPR:
+      case POSTDECREMENT_EXPR:
+      case POSTINCREMENT_EXPR:
+	return true;
+
+      case CALL_EXPR:
+	{
+	  /* Is it marked with "noreturn" attribute?  */
+	  if (call_expr_flags (t) & ECF_NORETURN)
+	    /* If so, assume it's the assertion failure handler, and
+	       thus it's not a side-effect for our purposes.  */
+	    return false;
+
+	  tree fndecl = get_callee_fndecl (t);
+	  /* If we can't determine which function is called,
+	     assume the call has side-effects.  */
+	  if (!fndecl)
+	    return true;
+
+	  /* Ignore calls to "__assert_fail" and the like.  */
+	  if (fndecl_is_assertion_failure_handler_p (fndecl))
+	    return false;
+
+	  if (DECL_FUNCTION_CODE (fndecl) == BUILT_IN_EXPECT)
+	    return false;
+
+	  /* Is it known to have an empty body?  (for example Qt's
+	     Q_ASSERT() macro calls qt_noop() if the expression
+	     is true, which is:
+	       inline void qt_noop(void) {}
+	     and thus not a side-effect.  */
+	  if (is_known_empty_function_p (fndecl))
+	    return false;
+
+	  /* Otherwise, assume it's a side-effect.  */
+	  return true;
+	}
+
+      default:
+	return false;
+    }
+}
+
+/* Implementation of -Wside-effect-in-assert, considering tree node T
+   itself (rather than its descendants).
+   Issue a warning if T is within the expansion of an assertion macro,
+   and could be considered a "side effect".  */
+
+static void
+check_for_side_effects_in_assert (tree t)
+{
+  if (!CAN_HAVE_LOCATION_P (t))
+    return;
+
+  location_t loc = EXPR_LOCATION (t);
+  const line_map_macro *assert_expansion;
+  if (!within_assert_expansion_p (loc, &assert_expansion))
+    return;
+  gcc_assert (assert_expansion);
+
+  if (!assertion_side_effect_at_p (t, assert_expansion))
+    return;
+
+  /* Only warn once per location.  */
+  if (side_effects_in_assert.warned_about_p (loc))
+    return;
+
+#if 0
+  /* We probably want to suppress this warning if it occurs in a system header,
+     but note that <assert.h> is itself a system header, so need to be
+     careful here.  */
+  if (!linemap_location_in_system_header_p (struct line_maps *,
+					 source_location);
+#endif
+
+  if (warning_at (loc, OPT_Wside_effect_in_assert,
+		  "statement with side-effects within assertion macro"))
+    {
+      side_effects_in_assert.set_warned_about (loc);
+
+      /* TODO: would it be even better to tell the user what the
+	 macro expanded to? */
+    }
+}
+
+/* Tree-walking callback for use by warn_for_macro_side_effects.  */
+
+static tree
+check_for_side_effects_in_assert_r (tree *tp, int */*walk_subtrees*/,
+				    void */*data*/)
+{
+  check_for_side_effects_in_assert (*tp);
+  return NULL_TREE;
+}
+
+/* Top-level implementation of -Wside-effect-in-assert.  */
+
+void
+warn_for_macro_side_effects (tree t)
+{
+  /* Early reject for when warning is disabled.  */
+  if (!warn_side_effect_in_assert)
+    return;
+  // TODO: do we have to walk every statement within t?
+  walk_tree(&t, check_for_side_effects_in_assert_r, NULL, NULL);
+}
diff --git a/gcc/c-family/c-macro.h b/gcc/c-family/c-macro.h
new file mode 100644
index 0000000..6208c93
--- /dev/null
+++ b/gcc/c-family/c-macro.h
@@ -0,0 +1,26 @@
+/* Declarations for c-macro.c.
+   Copyright (C) 2016 Free Software Foundation, Inc.
+
+This file is part of GCC.
+
+GCC is free software; you can redistribute it and/or modify it under
+the terms of the GNU General Public License as published by the Free
+Software Foundation; either version 3, or (at your option) any later
+version.
+
+GCC is distributed in the hope that it will be useful, but WITHOUT ANY
+WARRANTY; without even the implied warranty of MERCHANTABILITY or
+FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+for more details.
+
+You should have received a copy of the GNU General Public License
+along with GCC; see the file COPYING3.  If not see
+<http://www.gnu.org/licenses/>.  */
+
+#ifndef GCC_C_MACRO_H
+#define GCC_C_MACRO_H
+
+extern void
+warn_for_macro_side_effects (tree);
+
+#endif  /* ! GCC_C_MACRO_H  */
diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index 7c5f6c7..6c7c987 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -585,6 +585,10 @@ Wnamespaces
 C++ ObjC++ Var(warn_namespaces) Warning
 Warn on namespace definition.
 
+Wside-effect-in-assert
+C C++ Common Var(warn_side_effect_in_assert) Warning
+Warn when the condition within an \"assert\" macro has side-effects.
+
 Wsized-deallocation
 C++ ObjC++ Var(warn_sized_deallocation) Warning EnabledBy(Wextra)
 Warn about missing sized deallocation functions.
diff --git a/gcc/c/c-decl.c b/gcc/c/c-decl.c
index bab4715..7c0ee5b 100644
--- a/gcc/c/c-decl.c
+++ b/gcc/c/c-decl.c
@@ -51,6 +51,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "c-family/c-ada-spec.h"
 #include "cilk.h"
 #include "builtins.h"
+#include "c-family/c-macro.h"
 
 /* In grokdeclarator, distinguish syntactic contexts of declarators.  */
 enum decl_context
@@ -617,6 +618,8 @@ add_stmt (tree t)
   if (code == LABEL_EXPR || code == CASE_LABEL_EXPR)
     STATEMENT_LIST_HAS_LABEL (cur_stmt_list) = 1;
 
+  warn_for_macro_side_effects (t);
+
   /* Add T to the statement-tree.  Non-side-effect statements need to be
      recorded during statement expressions.  */
   if (!building_stmt_list_p ())
diff --git a/gcc/cp/semantics.c b/gcc/cp/semantics.c
index fd83c46..a3c0fe9 100644
--- a/gcc/cp/semantics.c
+++ b/gcc/cp/semantics.c
@@ -41,6 +41,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "omp-low.h"
 #include "convert.h"
 #include "gomp-constants.h"
+#include "c-family/c-macro.h"
 
 /* There routines provide a modular interface to perform many parsing
    operations.  They may therefore be used during actual parsing, or
@@ -380,6 +381,8 @@ add_stmt (tree t)
   if (code == LABEL_EXPR || code == CASE_LABEL_EXPR)
     STATEMENT_LIST_HAS_LABEL (cur_stmt_list) = 1;
 
+  warn_for_macro_side_effects (t);
+
   /* Add T to the statement-tree.  Non-side-effect statements need to be
      recorded during statement expressions.  */
   gcc_checking_assert (!stmt_list_stack->is_empty ());
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 99ac11b..f6f840a 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -288,6 +288,7 @@ Objective-C and Objective-C++ Dialects}.
 -Wreturn-type  -Wsequence-point  -Wshadow  -Wno-shadow-ivar @gol
 -Wshift-overflow -Wshift-overflow=@var{n} @gol
 -Wshift-count-negative -Wshift-count-overflow -Wshift-negative-value @gol
+-Wside-effect-in-assert @gol
 -Wsign-compare  -Wsign-conversion -Wfloat-conversion @gol
 -Wno-scalar-storage-order @gol
 -Wsizeof-pointer-memaccess  -Wsizeof-array-argument @gol
@@ -5183,6 +5184,16 @@ error in any case.
 @option{-Wjump-misses-init} is included in @option{-Wc++-compat}.  It
 can be disabled with the @option{-Wno-jump-misses-init} option.
 
+@item -Wside-effect-in-assert @r{(C and C++ only)}
+@opindex Wside-effect-in-assert
+@opindex Wno-side-effect-in-assert
+@cindex warning for side-effects in assertions
+Warn when the expansion of an @code{assert} macro contains an expression
+with a side-effect, to help track down undesired differences between
+debug vs release builds.
+For the purposes of this warning, any macro with ``assert'' or ``ASSERT''
+in its name is treated as an assertion.
+
 @item -Wsign-compare
 @opindex Wsign-compare
 @opindex Wno-sign-compare
diff --git a/gcc/testsuite/c-c++-common/Wside-effect-in-assert-1.c b/gcc/testsuite/c-c++-common/Wside-effect-in-assert-1.c
new file mode 100644
index 0000000..5ad8d71
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/Wside-effect-in-assert-1.c
@@ -0,0 +1,74 @@
+/* Verify "-Wside-effect-in-assert" (PR c/6906).  */
+
+/* { dg-options "-Wside-effect-in-assert" } */
+
+#include <stdlib.h>
+
+/* Reduced form of glibc's <assert.h>.  */
+
+#if defined __cplusplus
+# define __ASSERT_VOID_CAST static_cast<void>
+#else
+# define __ASSERT_VOID_CAST (void)
+#endif
+
+# define assert(expr)							\
+  ((expr)								\
+   ? __ASSERT_VOID_CAST (0)						\
+   : __assert_fail (__STRING(expr), __FILE__, __LINE__, __PRETTY_FUNCTION__))
+
+extern void __assert_fail (const char *__assertion, const char *__file,
+      unsigned int __line, const char *__function)
+     __attribute__ ((__nothrow__ , __leaf__)) __attribute__ ((__noreturn__));
+
+void *ptr;
+extern int f(void);
+
+#define HARMLESS_MACRO(X) \
+  do { X; } while (0)
+
+/* A macro that embeds an assert.  */
+#define WRAPPER_MACRO(X) \
+  do { assert (X); } while (0)
+
+void test_1 (void)
+{
+  ptr = malloc (64);
+}
+
+void test_2 (void)
+{
+  HARMLESS_MACRO (ptr = malloc (64));
+}
+
+void test_3 (void)
+{
+  assert(ptr = malloc(64)); /* { dg-warning "statement with side-effects within assertion macro" } */
+}
+
+/* A side-effect within a macro that embeds "assert";
+   we ought to warn.  */
+
+void test_4 (void)
+{
+  WRAPPER_MACRO (ptr = malloc (64)); /* { dg-warning "statement with side-effects within assertion macro" } */
+}
+
+/* Various assertions that contain side-effects.
+   We ought to warn about these.  */
+
+void test_5 (const char *p)
+{
+  assert(*p++);  /* { dg-warning "statement with side-effects within assertion macro" } */
+}
+
+void test_6 (void)
+{
+  assert(f());  /* { dg-warning "statement with side-effects within assertion macro" } */
+}
+
+int i;
+void test_7 (void)
+{
+  assert(i = 1); /* { dg-warning "statement with side-effects within assertion macro" } */
+}
diff --git a/gcc/testsuite/c-c++-common/Wside-effect-in-assert-2.c b/gcc/testsuite/c-c++-common/Wside-effect-in-assert-2.c
new file mode 100644
index 0000000..3211126
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/Wside-effect-in-assert-2.c
@@ -0,0 +1,44 @@
+/* Verify "-Wside-effect-in-assert" (PR c/6906).  */
+
+/* { dg-options "-Wside-effect-in-assert" } */
+
+/* Example adapted from glib.  */
+
+#  define G_GNUC_EXTENSION __extension__
+
+#define _G_BOOLEAN_EXPR(expr)                   \
+ G_GNUC_EXTENSION ({                            \
+   int _g_boolean_var_;                         \
+   if (expr)                                    \
+      _g_boolean_var_ = 1;                      \
+   else                                         \
+      _g_boolean_var_ = 0;                      \
+   _g_boolean_var_;                             \
+})
+
+#define G_LIKELY(expr) (__builtin_expect (_G_BOOLEAN_EXPR(expr), 1))
+
+#define G_GNUC_NORETURN                         \
+  __attribute__((__noreturn__))
+
+extern
+void    g_assertion_message_expr        (const char     *domain,
+                                         const char     *file,
+                                         int             line,
+                                         const char     *func,
+                                         const char     *expr) G_GNUC_NORETURN;
+
+typedef char   gchar;
+#define G_LOG_DOMAIN    ((gchar*) 0)
+#define G_STRFUNC     ((const char*) (__FUNCTION__))
+
+#define g_assert(expr) \
+  do { if G_LIKELY (expr) ; else					\
+      g_assertion_message_expr (G_LOG_DOMAIN, __FILE__, __LINE__, G_STRFUNC, \
+				#expr);					\
+  } while (0)
+
+void test_1 (const char *p)
+{
+  g_assert (*p++); /* { dg-warning "statement with side-effects within assertion macro" } */
+}
diff --git a/gcc/testsuite/g++.dg/Wside-effect-in-assert-3.C b/gcc/testsuite/g++.dg/Wside-effect-in-assert-3.C
new file mode 100644
index 0000000..b9053ac
--- /dev/null
+++ b/gcc/testsuite/g++.dg/Wside-effect-in-assert-3.C
@@ -0,0 +1,29 @@
+/* Verify "-Wside-effect-in-assert" (PR c/6906).  */
+
+/* { dg-options "-Wside-effect-in-assert" } */
+
+/* Example adapted from Qt. */
+
+extern void qt_assert(const char *assertion, const char *file, int line);
+inline void qt_noop(void) {}
+#define Q_ASSERT(cond) ((!(cond)) ? qt_assert(#cond,__FILE__,__LINE__) : qt_noop())
+
+/* No side effect: we shouldn't warn.  */
+
+unsigned test_1 (unsigned a, unsigned b)
+{
+  Q_ASSERT(a < b);
+  return b - a;
+}
+
+/* Side effect: we should warn.  */
+
+char *test_2 (int size)
+{
+    char *ptr;
+    Q_ASSERT(ptr = new char[size]); // { dg-warning "statement with side-effects within assertion macro" }
+    return ptr;
+}
+
+/* There's also a Q_CHECK_PTR, but the name doesn't match "assert" so we
+   don't warn; should we? */
-- 
1.8.5.3

