From 6667ea91f47cb3e5a6f97e31cab76052799cdf18 Mon Sep 17 00:00:00 2001
From: David Malcolm <dmalcolm@redhat.com>
Date: Fri, 11 Mar 2016 17:01:11 -0500
Subject: [PATCH 07/10] FIXME: WIP at -Wunsafe-macro-expansion

---
 gcc/c-family/c-macro.c                             | 210 +++++++++++++++++++++
 gcc/c-family/c-macro.h                             |   3 +
 gcc/c-family/c.opt                                 |   4 +
 gcc/c/c-decl.c                                     |   1 +
 .../c-c++-common/Wunsafe-macro-expansion-2.c       |  46 +++++
 .../c-c++-common/Wunsafe-macro-expansion-3.c       |  39 ++++
 .../c-c++-common/Wunsafe-macro-expansion.c         |  39 ++++
 libcpp/include/line-map.h                          |  22 +++
 8 files changed, 364 insertions(+)
 create mode 100644 gcc/testsuite/c-c++-common/Wunsafe-macro-expansion-2.c
 create mode 100644 gcc/testsuite/c-c++-common/Wunsafe-macro-expansion-3.c
 create mode 100644 gcc/testsuite/c-c++-common/Wunsafe-macro-expansion.c

diff --git a/gcc/c-family/c-macro.c b/gcc/c-family/c-macro.c
index 9d422ef..e0530e7 100644
--- a/gcc/c-family/c-macro.c
+++ b/gcc/c-family/c-macro.c
@@ -23,6 +23,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "tm.h"
 #include "c-common.h"
 #include "c-macro.h"
+#include "cpp-id-data.h"
 
 // FIXME:
 #include "print-tree.h" // for debug_tree
@@ -193,3 +194,212 @@ warn_for_macro_side_effects (tree t)
   // TODO: do we have to walk every statement within t?
   walk_tree(&t, check_for_side_effects_in_assert_r, NULL, NULL);
 }
+
+
+
+/* Implementation of -Wunsafe-macro-expansion.  */
+
+/* We will warn about macro expansions containting side-effects,
+   where the location of the side effect is from an argument
+   that is expanded more than once.  */
+
+/* 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.  */
+
+typedef hash_set<location_t, int_hash<location_t, 0, 0xffffffff> >
+  location_hash_set;
+static location_hash_set *locations_warned_about;
+
+/* Given MACRO_MAP and the index EXPANSION_TOKEN_IDX of a token
+   within the expanded tokens, return the index of the token
+   within the definition that the expanded token came from.
+
+   We do this by looking at changes in the y_loc within the macro map:
+   runs of equal value correspond to multiple tokens coming from the
+   same argument.  */
+
+static int
+calc_index_within_definition (const line_map_macro *macro_map,
+			      unsigned expansion_token_idx)
+{
+  gcc_assert (macro_map);
+  gcc_assert (expansion_token_idx < macro_map->n_tokens);
+
+  location_t last_y_loc = macro_map->get_y_loc (0);
+  unsigned definition_token_idx = 0;
+  for (unsigned i = 1; i <= expansion_token_idx; i++)
+    {
+      const location_t y_loc = macro_map->get_y_loc (i);
+      gcc_assert (y_loc != UNKNOWN_LOCATION);
+      if (y_loc != last_y_loc)
+	definition_token_idx++;
+      last_y_loc = y_loc;
+    }
+  return definition_token_idx;
+}
+
+/* Implementation of -Wunsafe-macro-expansion, considering tree node T
+   itself (rather than its descendants).
+   Issue a warning if:
+   - T is within the first expansion of a macro argument
+     - where the argument is expanded more than once by the macro
+       (and is thus unsafe), and
+   - where T could be considered a "side effect".  */
+
+static void
+check_for_unsafe_macro_expansion (tree t)
+{
+  if (!CAN_HAVE_LOCATION_P (t))
+    return;
+
+  // FIXME: need a weaker condition, I think
+  if (!assertion_side_effect_p (t))
+    return;
+
+  /* Only warn within macro expansions.  */
+  location_t loc = EXPR_LOCATION (t);
+  const line_map *map = linemap_lookup (line_table, loc);
+  gcc_assert (map);
+  if (!linemap_macro_expansion_map_p (map))
+    return;
+
+  const line_map_macro *macro_map = linemap_check_macro (map);
+  const cpp_macro *macro = macro_map->macro->value.macro;
+
+  if (macro->traditional)
+    return;
+
+  /* Locate the token within the macro expansion.  */
+  if (IS_ADHOC_LOC (loc))
+    loc = get_location_from_adhoc_loc (line_table, loc);
+
+  gcc_assert (loc >= macro_map->start_location);
+  unsigned expansion_token_idx = loc - MAP_START_LOCATION (macro_map);
+  gcc_assert (expansion_token_idx < MACRO_MAP_NUM_MACRO_TOKENS (macro_map));
+
+  /* We require a token in the expansion which came from an argument.
+     Perform a relatively cheap test to reject tokens
+     that don't come from arguments.
+     Arguments have x_loc != y_loc, so reject other tokens.  */
+  const location_t x_loc = macro_map->get_x_loc (expansion_token_idx);
+  const location_t y_loc = macro_map->get_y_loc (expansion_token_idx);
+  if (x_loc == y_loc)
+    return;
+
+  /* Now locate the corresponding token within the
+     macro *definition*.  */
+  unsigned definition_token_idx
+    = calc_index_within_definition (macro_map, expansion_token_idx);
+  gcc_assert (definition_token_idx < macro->count);
+
+  const cpp_token *token = &macro->exp.tokens[definition_token_idx];
+  gcc_assert (token->type == CPP_MACRO_ARG);
+
+  unsigned arg_no = token->val.macro_arg.arg_no;
+  gcc_assert (arg_no > 0);
+  unsigned arg_idx = arg_no - 1;
+
+  const cpp_hashnode *param = macro->params[arg_idx];
+  gcc_assert (param);
+
+  /* We care about expansions where the given token is one that is expanded
+     more than once.
+     Populate given_arg_indices with the indices of those tokens in the macro
+     expansion that are copies of the given token.
+     TODO: optimize this?  only do it once per macro?  */
+  auto_vec <unsigned> given_arg_indices;
+  for (unsigned i = 0; i < macro_map->n_tokens; i++)
+    {
+      gcc_assert (!macro->traditional);
+      cpp_token *other_tok = &macro->exp.tokens[i];
+      if (other_tok->type == CPP_MACRO_ARG)
+	if (other_tok->val.macro_arg.arg_no == arg_no)
+	  given_arg_indices.safe_push (i);
+    }
+
+  /* Given that we saw the token, we must have at least one mention in the
+     vec.  */
+  gcc_assert (given_arg_indices.length () > 0);
+
+  /* We care if there's *more* than one.  */
+  if (given_arg_indices.length () == 1)
+    return;
+
+  /* OK: we have an expression with side-effects that came from an
+     argument to a macro, where the argument is expanded more than once
+     by that macro.  */
+
+  /* Only complain about the first expansion seen. */
+  if (definition_token_idx != given_arg_indices[0])
+    return;
+
+  /* We want to only warn once per macro expansion/argument.
+     We can do this by only warning once per virtual location.  */
+  if (!locations_warned_about)
+    locations_warned_about = new location_hash_set ();
+  if (locations_warned_about->contains (loc))
+    /* We've already warned about this virtual location.  */
+    return;
+  // TODO: what about nested expansions?
+
+  if (warning_at (loc, OPT_Wunsafe_macro_expansion,
+		  "repeated side-effects within expansion of unsafe macro %qs",
+		  linemap_map_get_macro_name (macro_map)))
+    {
+      locations_warned_about->add (loc);
+
+      /* Show the macro definition, highlighting the locations where the
+	 given parameter is expanded.
+
+	 Build a rich_location, starting with the line containing the
+	 macro definition as the primary location.  Ideally we would use
+	 the location of the given param, but that doesn't seem to be
+	 available.  */
+      rich_location richloc (line_table, macro->line);
+
+      /* Add locations showing each place in the macro definition
+	 where the given parameter is used.  */
+      for (unsigned i = 0; i < given_arg_indices.length (); i++)
+	{
+	  cpp_token *tok = &macro->exp.tokens[given_arg_indices[i]];
+	  richloc.add_range (tok->src_loc, true);
+	}
+
+      /* Emit the note.  */
+      inform_at_rich_loc
+	(&richloc,
+	 "parameter %i (%qs) is expanded %i times by macro %qs",
+	 arg_no,
+	 param->ident.str,
+	 given_arg_indices.length (),
+	 linemap_map_get_macro_name (macro_map));
+    }
+}
+
+/* Tree-walking callback for use by warn_for_macro_side_effects.  */
+
+static tree
+check_for_unsafe_macro_expansion_r (tree *tp, int */*walk_subtrees*/,
+				    void */*data*/)
+{
+  check_for_unsafe_macro_expansion (*tp);
+  return NULL_TREE;
+}
+
+/* Top-level implementation of -Wunsafe-macro-expansion.  */
+
+void
+warn_for_unsafe_macro_expansion (tree t)
+{
+  /* Early reject for when warning is disabled.  */
+  if (!warn_unsafe_macro_expansion)
+    return;
+  walk_tree(&t, check_for_unsafe_macro_expansion_r, NULL, NULL);
+}
+
+/* FIXME: should the tree-walking be combined for the two macro-based
+   warnings?  */
diff --git a/gcc/c-family/c-macro.h b/gcc/c-family/c-macro.h
index 6208c93..17c4e6c 100644
--- a/gcc/c-family/c-macro.h
+++ b/gcc/c-family/c-macro.h
@@ -23,4 +23,7 @@ along with GCC; see the file COPYING3.  If not see
 extern void
 warn_for_macro_side_effects (tree);
 
+extern void
+warn_for_unsafe_macro_expansion (tree);
+
 #endif  /* ! GCC_C_MACRO_H  */
diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index 6c7c987..a6d10be 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -928,6 +928,10 @@ Wunknown-pragmas
 C ObjC C++ ObjC++ Warning Var(warn_unknown_pragmas) LangEnabledBy(C ObjC C++ ObjC++,Wall, 1, 0)
 Warn about unrecognized pragmas.
 
+Wunsafe-macro-expansion
+C C++ Common Var(warn_unsafe_macro_expansion) Warning
+Warn when an unsafe macro is expanded with arguments that have side-effects.
+
 Wunsuffixed-float-constants
 C ObjC Var(warn_unsuffixed_float_constants) Warning
 Warn about unsuffixed float constants.
diff --git a/gcc/c/c-decl.c b/gcc/c/c-decl.c
index 7c0ee5b..aadc511 100644
--- a/gcc/c/c-decl.c
+++ b/gcc/c/c-decl.c
@@ -619,6 +619,7 @@ add_stmt (tree t)
     STATEMENT_LIST_HAS_LABEL (cur_stmt_list) = 1;
 
   warn_for_macro_side_effects (t);
+  warn_for_unsafe_macro_expansion (t);
 
   /* Add T to the statement-tree.  Non-side-effect statements need to be
      recorded during statement expressions.  */
diff --git a/gcc/testsuite/c-c++-common/Wunsafe-macro-expansion-2.c b/gcc/testsuite/c-c++-common/Wunsafe-macro-expansion-2.c
new file mode 100644
index 0000000..6d95b62
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/Wunsafe-macro-expansion-2.c
@@ -0,0 +1,46 @@
+/* Reduced reproducer for the issue described in
+   https://dmalcolm.fedorapeople.org/presentations/PyCon-US-2013/PyCon-US-2013-dmalcolm-StaticAnalysis.html#(38)
+*/
+
+/* { dg-options "-Wunsafe-macro-expansion -fdiagnostics-show-caret" } */
+
+#define NULL ((void *)0)
+typedef struct _object { int ob_refcnt; } PyObject;
+#define Py_DECREF(op)                                   \
+    do {                                                \
+        if (--((PyObject*)(op))->ob_refcnt == 0)        \
+          _Py_Dealloc((PyObject *)(op));                \
+    } while (0)
+#define Py_XDECREF(op) do { if ((op) == NULL) ; else Py_DECREF(op); } while (0)
+/* { dg-message "parameter 1 ..op.. is expanded 2 times by macro .Py_XDECREF." "" { target *-*-* } 14 } */
+#define _Py_Dealloc(op) /* no-op for simplicity */
+
+extern PyObject *
+PyObject_CallObject (PyObject *callable_object, PyObject *args);
+
+void
+test_1 (PyObject *callable, PyObject *args)
+{
+  Py_XDECREF(PyObject_CallObject(callable, args)); /* { dg-warning "repeated side-effects within expansion of unsafe macro .Py_XDECREF." } */
+
+  /* { dg-begin-multiline-output "" }
+   Py_XDECREF(PyObject_CallObject(callable, args));
+              ^
+     { dg-end-multiline-output "" } */
+  /* { dg-begin-multiline-output "" }
+ #define Py_XDECREF(op) do { if ((op) == NULL) ; else Py_DECREF(op); } while (0)
+                                  ^~
+     { dg-end-multiline-output "" } */
+  /* { dg-begin-multiline-output "" }
+ #define Py_XDECREF(op) do { if ((op) == NULL) ; else Py_DECREF(op); } while (0)
+                                  ^~                            ^~
+     { dg-end-multiline-output "" } */
+}
+
+/* This invocation doesn't have side-effects; we shouldn't warn.  */
+
+void
+test_2 (PyObject *obj)
+{
+  Py_XDECREF (obj);
+}
diff --git a/gcc/testsuite/c-c++-common/Wunsafe-macro-expansion-3.c b/gcc/testsuite/c-c++-common/Wunsafe-macro-expansion-3.c
new file mode 100644
index 0000000..2fe9bb5
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/Wunsafe-macro-expansion-3.c
@@ -0,0 +1,39 @@
+/* { dg-options "-Wunsafe-macro-expansion -fdiagnostics-show-caret" } */
+
+/* Example taken from our docs for "typeof".  */
+
+#define min(A, B)  ((A) < (B) ? (A) : (B))
+
+/* { dg-message "parameter 2 ..B.. is expanded 2 times by macro .min." "" { target *-*-* } 5 } */
+
+int x, y, z, next;
+extern int foo (int);
+
+/* This invocation has side-effects; we should warn.  */
+
+void
+test_1 (void)
+{
+  next = min (x + y, foo (z)); /* { dg-warning "repeated side-effects within expansion of unsafe macro .min." }
+
+  /* { dg-begin-multiline-output "" }
+   next = min (x + y, foo (z));
+                      ^
+     { dg-end-multiline-output "" } */
+  /* { dg-begin-multiline-output "" }
+ #define min(A, B)  ((A) < (B) ? (A) : (B))
+                            ^
+     { dg-end-multiline-output "" } */
+  /* { dg-begin-multiline-output "" }
+ #define min(A, B)  ((A) < (B) ? (A) : (B))
+                            ^           ^
+     { dg-end-multiline-output "" } */
+}
+
+/* This invocation doesn't have side-effects; we shouldn't warn.  */
+
+int
+test_2 (int p, int q)
+{
+  return min (p, q);
+}
diff --git a/gcc/testsuite/c-c++-common/Wunsafe-macro-expansion.c b/gcc/testsuite/c-c++-common/Wunsafe-macro-expansion.c
new file mode 100644
index 0000000..f8b4f85
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/Wunsafe-macro-expansion.c
@@ -0,0 +1,39 @@
+/* { dg-options "-Wunsafe-macro-expansion -fdiagnostics-show-caret" } */
+
+/* The macro invocation:
+     ABS (++n)
+   expands to:
+     m = (((++n) < 0) ? -(++n) : (++n));
+   which is probably not what was desired.  */
+
+#define ABS(ARG) (((ARG) < 0) ? -(ARG) : (ARG))
+  /* { dg-message "parameter 1 ..ARG.. is expanded 3 times by macro .ABS." "" { target *-*-* } 9 } */
+
+int
+test_1 (int n)
+{
+  int m;
+  m = ABS (++n); /* { dg-warning "repeated side-effects within expansion of unsafe macro .ABS." } */
+  return m;
+
+  /* { dg-begin-multiline-output "" }
+   m = ABS (++n);
+            ^
+     { dg-end-multiline-output "" } */
+  /* { dg-begin-multiline-output "" }
+ #define ABS(ARG) (((ARG) < 0) ? -(ARG) : (ARG))
+                     ^~~
+     { dg-end-multiline-output "" } */
+  /* { dg-begin-multiline-output "" }
+ #define ABS(ARG) (((ARG) < 0) ? -(ARG) : (ARG))
+                     ^~~           ^~~     ^~~
+     { dg-end-multiline-output "" } */
+}
+
+/* This invocation doesn't have side-effects; we shouldn't warn.  */
+
+int
+test_2 (int n, int m)
+{
+  return ABS (n * m);
+}
diff --git a/libcpp/include/line-map.h b/libcpp/include/line-map.h
index 6ead57c..bf26496 100644
--- a/libcpp/include/line-map.h
+++ b/libcpp/include/line-map.h
@@ -389,6 +389,10 @@ struct cpp_hashnode;
    The offset from START_LOCATION is used to index into
    MACRO_LOCATIONS; this holds the original location of the token.  */
 struct GTY((tag ("2"))) line_map_macro : public line_map {
+
+  source_location get_x_loc (unsigned token_idx) const;
+  source_location get_y_loc (unsigned token_idx) const;
+
   /* The cpp macro which expansion gave birth to this macro map.  */
   struct cpp_hashnode * GTY ((nested_ptr (union tree_node,
 				   "%h ? CPP_HASHNODE (GCC_IDENT_TO_HT_IDENT (%h)) : NULL",
@@ -486,6 +490,24 @@ struct GTY((tag ("2"))) line_map_macro : public line_map {
 #define linemap_assert_fails(EXPR) (! (EXPR))
 #endif
 
+/* FIXME.  */
+
+inline source_location
+line_map_macro::get_x_loc (unsigned token_idx) const
+{
+  linemap_assert (token_idx < n_tokens);
+  return macro_locations[token_idx * 2];
+}
+
+/* FIXME.  */
+
+inline source_location
+line_map_macro::get_y_loc (unsigned token_idx) const
+{
+  linemap_assert (token_idx < n_tokens);
+  return macro_locations[(token_idx * 2) + 1];
+}
+
 /* Return TRUE if MAP encodes locations coming from a macro
    replacement-list at macro expansion point.  */
 bool
-- 
1.8.5.3

