diff mbox

C and C++ FE: better source ranges for binary ops

Message ID 1450289044-35720-1-git-send-email-dmalcolm@redhat.com
State New
Headers show

Commit Message

David Malcolm Dec. 16, 2015, 6:04 p.m. UTC 
Currently trunk emits range information for most bad binary operations
in the C++ frontend; but not in the C frontend.

The helper function binary_op_error shared by C and C++ takes a
location_t.  In the C++ frontend, a location_t containing the range
has already been built, so we get the underline when it later validates
the expression: e.g. this example from:
   https://gcc.gnu.org/wiki/ClangDiagnosticsComparison

t.cc:9:19: error: no match for ‘operator+’ (operand types are ‘int’ and ‘foo’)
   return P->bar() + *P;
          ~~~~~~~~~^~~~

In the C frontend, the "full" location_t is only built
after type-checking, so if type-checking fails, we get a caret/range
covering just the operator so e.g. for:

   return (some_function ()
    + some_other_function ());

we get just:

gcc.dg/bad-binary-ops.c:29:4: error: invalid operands to binary + (have 'struct s' and 'struct t')
    + some_other_function ());
    ^

The following patch updates binary_op_error to accept a rich_location *.
For the C++ frontend we populate it with just the location_t as before,
but for the C frontend we can add locations for the operands, giving
this underlining for the example above:

bad-binary-ops.c:29:4: error: invalid operands to binary + (have 'struct s' and 'struct t')
   return (some_function ()
           ~~~~~~~~~~~~~~~~
    + some_other_function ());
    ^ ~~~~~~~~~~~~~~~~~~~~~~

Additionally, in the C++ frontend, cp_build_binary_op has an "error"
call, which implicitly uses input_location, giving this reduced
information for another test case from
https://gcc.gnu.org/wiki/ClangDiagnosticsComparison:

bad-binary-ops.C:10:14: error: invalid operands of types '__m128 {aka float}' and 'const int*' to binary 'operator/'
   myvec[1] / ptr;
              ^~~

The patch updates it to use error_at with the location_t provided,
fixing the above to become:

bad-binary-ops.C:10:12: error: invalid operands of types '__m128 {aka float}' and 'const int*' to binary 'operator/'
   myvec[1] / ptr;
   ~~~~~~~~~^~~~~

Finally, since this patch adds a usage of
gcc_rich_location::maybe_add_expr to cc1, we can drop the hacked-in
copy of gcc_rich_location::add_expr from
gcc.dg/plugin/diagnostic_plugin_show_trees.c.

Successfully bootstrapped&regrtested on x86_64-pc-linux-gnu;
adds 15 new PASS results in g++.sum and 7 new PASS results in gcc.sum

OK for trunk in stage 3?

gcc/c-family/ChangeLog:
	* c-common.c (binary_op_error): Convert first param from
	location_t to rich_location * and use it when emitting an error.
	* c-common.h (binary_op_error): Convert first param from
	location_t to rich_location *.

gcc/c/ChangeLog:
	* c-typeck.c: Include "gcc-rich-location.h".
	(build_binary_op): In the two places that call binary_op_error,
	create a gcc_rich_location and populate it with the location of
	the binary op and its two operands.

gcc/cp/ChangeLog:
	* typeck.c (cp_build_binary_op): Update for change in signature
	of build_binary_op.  Use error_at to replace an implicit use
	of input_location with param "location" in "invalid operands"
	error.
	(cp_build_binary_op): Replace an error with an error_at, using
	"location", rather than implicitly using input_location.

gcc/testsuite/ChangeLog:
	* g++.dg/diagnostic/bad-binary-ops.C: New test case.
	* gcc.dg/bad-binary-ops.c: New test case.
	gcc.dg/plugin/diagnostic_plugin_show_trees.c (get_range_for_expr):
	Remove material copied from gcc-rich-location.c
	(gcc_rich_location::add_expr): Likewise.
---
 gcc/c-family/c-common.c                            | 21 +++++++---
 gcc/c-family/c-common.h                            |  2 +-
 gcc/c/c-typeck.c                                   | 11 ++++-
 gcc/cp/typeck.c                                    | 13 ++++--
 gcc/testsuite/g++.dg/diagnostic/bad-binary-ops.C   | 44 ++++++++++++++++++++
 gcc/testsuite/gcc.dg/bad-binary-ops.c              | 48 ++++++++++++++++++++++
 .../gcc.dg/plugin/diagnostic_plugin_show_trees.c   | 44 --------------------
 7 files changed, 128 insertions(+), 55 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/diagnostic/bad-binary-ops.C
 create mode 100644 gcc/testsuite/gcc.dg/bad-binary-ops.c

Comments

Jeff Law Dec. 16, 2015, 9:15 p.m. UTC | #1 
On 12/16/2015 11:04 AM, David Malcolm wrote:
> Currently trunk emits range information for most bad binary operations
> in the C++ frontend; but not in the C frontend.
>
> The helper function binary_op_error shared by C and C++ takes a
> location_t.  In the C++ frontend, a location_t containing the range
> has already been built, so we get the underline when it later validates
> the expression: e.g. this example from:
>     https://gcc.gnu.org/wiki/ClangDiagnosticsComparison
>
> t.cc:9:19: error: no match for ‘operator+’ (operand types are ‘int’ and ‘foo’)
>     return P->bar() + *P;
>            ~~~~~~~~~^~~~
>
> In the C frontend, the "full" location_t is only built
> after type-checking, so if type-checking fails, we get a caret/range
> covering just the operator so e.g. for:
>
>     return (some_function ()
>      + some_other_function ());
>
> we get just:
>
> gcc.dg/bad-binary-ops.c:29:4: error: invalid operands to binary + (have 'struct s' and 'struct t')
>      + some_other_function ());
>      ^
>
> The following patch updates binary_op_error to accept a rich_location *.
> For the C++ frontend we populate it with just the location_t as before,
> but for the C frontend we can add locations for the operands, giving
> this underlining for the example above:
>
> bad-binary-ops.c:29:4: error: invalid operands to binary + (have 'struct s' and 'struct t')
>     return (some_function ()
>             ~~~~~~~~~~~~~~~~
>      + some_other_function ());
>      ^ ~~~~~~~~~~~~~~~~~~~~~~
>
> Additionally, in the C++ frontend, cp_build_binary_op has an "error"
> call, which implicitly uses input_location, giving this reduced
> information for another test case from
> https://gcc.gnu.org/wiki/ClangDiagnosticsComparison:
>
> bad-binary-ops.C:10:14: error: invalid operands of types '__m128 {aka float}' and 'const int*' to binary 'operator/'
>     myvec[1] / ptr;
>                ^~~
>
> The patch updates it to use error_at with the location_t provided,
> fixing the above to become:
>
> bad-binary-ops.C:10:12: error: invalid operands of types '__m128 {aka float}' and 'const int*' to binary 'operator/'
>     myvec[1] / ptr;
>     ~~~~~~~~~^~~~~
>
> Finally, since this patch adds a usage of
> gcc_rich_location::maybe_add_expr to cc1, we can drop the hacked-in
> copy of gcc_rich_location::add_expr from
> gcc.dg/plugin/diagnostic_plugin_show_trees.c.
>
> Successfully bootstrapped&regrtested on x86_64-pc-linux-gnu;
> adds 15 new PASS results in g++.sum and 7 new PASS results in gcc.sum
>
> OK for trunk in stage 3?
>
> gcc/c-family/ChangeLog:
> 	* c-common.c (binary_op_error): Convert first param from
> 	location_t to rich_location * and use it when emitting an error.
> 	* c-common.h (binary_op_error): Convert first param from
> 	location_t to rich_location *.
>
> gcc/c/ChangeLog:
> 	* c-typeck.c: Include "gcc-rich-location.h".
> 	(build_binary_op): In the two places that call binary_op_error,
> 	create a gcc_rich_location and populate it with the location of
> 	the binary op and its two operands.
>
> gcc/cp/ChangeLog:
> 	* typeck.c (cp_build_binary_op): Update for change in signature
> 	of build_binary_op.  Use error_at to replace an implicit use
> 	of input_location with param "location" in "invalid operands"
> 	error.
> 	(cp_build_binary_op): Replace an error with an error_at, using
> 	"location", rather than implicitly using input_location.
>
> gcc/testsuite/ChangeLog:
> 	* g++.dg/diagnostic/bad-binary-ops.C: New test case.
> 	* gcc.dg/bad-binary-ops.c: New test case.
> 	gcc.dg/plugin/diagnostic_plugin_show_trees.c (get_range_for_expr):
> 	Remove material copied from gcc-rich-location.c
> 	(gcc_rich_location::add_expr): Likewise.
So I'm of a mixed mind here.

We're well into stage3 and I can't see a reasonable way to call this a 
bugfix, but I see the value here at the end user level and it looks to 
be quite safe.

I'll tentatively approve -- give other maintainers 48hrs to object on 
the grounds this isn't a bugfix before committing.

Jeff
diff mbox

Patch

diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index 4250cdf..653d1dc 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -3795,10 +3795,21 @@  c_register_builtin_type (tree type, const char* name)
 
 /* Print an error message for invalid operands to arith operation
    CODE with TYPE0 for operand 0, and TYPE1 for operand 1.
-   LOCATION is the location of the message.  */
+   RICHLOC is a rich location for the message, containing either
+   three separate locations for each of the operator and operands
+
+      lhs op rhs
+      ~~~ ^~ ~~~
+
+   (C FE), or one location ranging over all over them
+
+      lhs op rhs
+      ~~~~^~~~~~
+
+   (C++ FE).  */
 
 void
-binary_op_error (location_t location, enum tree_code code,
+binary_op_error (rich_location *richloc, enum tree_code code,
 		 tree type0, tree type1)
 {
   const char *opname;
@@ -3850,9 +3861,9 @@  binary_op_error (location_t location, enum tree_code code,
     default:
       gcc_unreachable ();
     }
-  error_at (location,
-	    "invalid operands to binary %s (have %qT and %qT)", opname,
-	    type0, type1);
+  error_at_rich_loc (richloc,
+		     "invalid operands to binary %s (have %qT and %qT)",
+		     opname, type0, type1);
 }
 
 /* Given an expression as a tree, return its original type.  Do this
diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h
index ef64e6b..2d434b2 100644
--- a/gcc/c-family/c-common.h
+++ b/gcc/c-family/c-common.h
@@ -817,7 +817,7 @@  extern tree c_sizeof_or_alignof_type (location_t, tree, bool, bool, int);
 extern tree c_alignof_expr (location_t, tree);
 /* Print an error message for invalid operands to arith operation CODE.
    NOP_EXPR is used as a special case (see truthvalue_conversion).  */
-extern void binary_op_error (location_t, enum tree_code, tree, tree);
+extern void binary_op_error (rich_location *, enum tree_code, tree, tree);
 extern tree fix_string_type (tree);
 extern void constant_expression_warning (tree);
 extern void constant_expression_error (tree);
diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c
index 9d6c604..b93da07 100644
--- a/gcc/c/c-typeck.c
+++ b/gcc/c/c-typeck.c
@@ -48,6 +48,7 @@  along with GCC; see the file COPYING3.  If not see
 #include "cilk.h"
 #include "gomp-constants.h"
 #include "spellcheck.h"
+#include "gcc-rich-location.h"
 
 /* Possible cases of implicit bad conversions.  Used to select
    diagnostic messages in convert_for_assignment.  */
@@ -11190,7 +11191,10 @@  build_binary_op (location_t location, enum tree_code code,
       && (!tree_int_cst_equal (TYPE_SIZE (type0), TYPE_SIZE (type1))
 	  || !vector_types_compatible_elements_p (type0, type1)))
     {
-      binary_op_error (location, code, type0, type1);
+      gcc_rich_location richloc (location);
+      richloc.maybe_add_expr (orig_op0);
+      richloc.maybe_add_expr (orig_op1);
+      binary_op_error (&richloc, code, type0, type1);
       return error_mark_node;
     }
 
@@ -11429,7 +11433,10 @@  build_binary_op (location_t location, enum tree_code code,
 
   if (!result_type)
     {
-      binary_op_error (location, code, TREE_TYPE (op0), TREE_TYPE (op1));
+      gcc_rich_location richloc (location);
+      richloc.maybe_add_expr (orig_op0);
+      richloc.maybe_add_expr (orig_op1);
+      binary_op_error (&richloc, code, TREE_TYPE (op0), TREE_TYPE (op1));
       return error_mark_node;
     }
 
diff --git a/gcc/cp/typeck.c b/gcc/cp/typeck.c
index 39c1af2..ef9ae9a 100644
--- a/gcc/cp/typeck.c
+++ b/gcc/cp/typeck.c
@@ -4908,7 +4908,13 @@  cp_build_binary_op (location_t location,
 	      || !vector_types_compatible_elements_p (type0, type1))
 	    {
 	      if (complain & tf_error)
-		binary_op_error (location, code, type0, type1);
+		{
+		  /* "location" already embeds the locations of the
+		     operands, so we don't need to add them separately
+		     to richloc.  */
+		  rich_location richloc (line_table, location);
+		  binary_op_error (&richloc, code, type0, type1);
+		}
 	      return error_mark_node;
 	    }
 	  arithmetic_types_p = 1;
@@ -4931,8 +4937,9 @@  cp_build_binary_op (location_t location,
   if (!result_type)
     {
       if (complain & tf_error)
-	error ("invalid operands of types %qT and %qT to binary %qO",
-	       TREE_TYPE (orig_op0), TREE_TYPE (orig_op1), code);
+	error_at (location,
+		  "invalid operands of types %qT and %qT to binary %qO",
+		  TREE_TYPE (orig_op0), TREE_TYPE (orig_op1), code);
       return error_mark_node;
     }
 
diff --git a/gcc/testsuite/g++.dg/diagnostic/bad-binary-ops.C b/gcc/testsuite/g++.dg/diagnostic/bad-binary-ops.C
new file mode 100644
index 0000000..4ab7656
--- /dev/null
+++ b/gcc/testsuite/g++.dg/diagnostic/bad-binary-ops.C
@@ -0,0 +1,44 @@ 
+// { dg-options "-fdiagnostics-show-caret" }
+
+// Adapted from https://gcc.gnu.org/wiki/ClangDiagnosticsComparison
+
+typedef float __m128;
+void test_1 ()
+{
+  __m128 myvec[2];
+  int const *ptr;
+  myvec[1] / ptr; // { dg-error "invalid operands" }
+
+/* { dg-begin-multiline-output "" }
+   myvec[1] / ptr;
+   ~~~~~~~~~^~~~~
+   { dg-end-multiline-output "" } */
+}
+
+struct s {};
+struct t {};
+extern struct s some_function (void);
+extern struct t some_other_function (void);
+
+int test_2 (void)
+{
+  return (some_function ()
+	  + some_other_function ()); // { dg-error "no match for .operator" }
+
+/* { dg-begin-multiline-output "" }
+   return (some_function ()
+           ~~~~~~~~~~~~~~~~
+    + some_other_function ());
+    ^~~~~~~~~~~~~~~~~~~~~~~~
+   { dg-end-multiline-output "" } */
+}
+
+int test_3 (struct s param_s, struct t param_t)
+{
+  return param_s && param_t; // { dg-error "no match for .operator" }
+
+/* { dg-begin-multiline-output "" }
+   return param_s && param_t;
+          ~~~~~~~~^~~~~~~~~~
+   { dg-end-multiline-output "" } */
+}
diff --git a/gcc/testsuite/gcc.dg/bad-binary-ops.c b/gcc/testsuite/gcc.dg/bad-binary-ops.c
new file mode 100644
index 0000000..e1da4d6
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/bad-binary-ops.c
@@ -0,0 +1,48 @@ 
+/* { dg-options "-fdiagnostics-show-caret" } */
+
+/* Adapted from https://gcc.gnu.org/wiki/ClangDiagnosticsComparison */
+
+typedef float __m128;
+void test_1 ()
+{
+  __m128 myvec[2];
+  int const *ptr;
+  myvec[1]/ptr; /* { dg-error "invalid operands to binary /" } */
+
+/* TODO: ideally we'd underline "ptr" as well.
+{ dg-begin-multiline-output "" }
+   myvec[1]/ptr;
+   ~~~~~~~~^
+{ dg-end-multiline-output "" } */
+
+
+}
+
+struct s {};
+struct t {};
+extern struct s some_function (void);
+extern struct t some_other_function (void);
+
+int test_2 (void)
+{
+  return (some_function ()
+	  + some_other_function ()); /* { dg-error "invalid operands to binary \+" } */
+
+/* { dg-begin-multiline-output "" }
+   return (some_function ()
+           ~~~~~~~~~~~~~~~~
+    + some_other_function ());
+    ^ ~~~~~~~~~~~~~~~~~~~~~~
+   { dg-end-multiline-output "" } */
+}
+
+int test_3 (struct s param_s, struct t param_t)
+{
+  return param_s + param_t; // { dg-error "invalid operands to binary \+" }
+
+/* { dg-begin-multiline-output "" }
+   return param_s + param_t;
+                  ^
+   { dg-end-multiline-output "" } */
+/* TODO: ideally we'd underline both params here.  */
+}
diff --git a/gcc/testsuite/gcc.dg/plugin/diagnostic_plugin_show_trees.c b/gcc/testsuite/gcc.dg/plugin/diagnostic_plugin_show_trees.c
index 5a911c1..c98034f 100644
--- a/gcc/testsuite/gcc.dg/plugin/diagnostic_plugin_show_trees.c
+++ b/gcc/testsuite/gcc.dg/plugin/diagnostic_plugin_show_trees.c
@@ -32,50 +32,6 @@ 
 #include "gcc-rich-location.h"
 #include "print-tree.h"
 
-/*
-  Hack: fails with linker error:
-./diagnostic_plugin_show_trees.so: undefined symbol: _ZN17gcc_rich_location8add_exprEP9tree_node
-  since nothing in the tree is using gcc_rich_location::add_expr yet.
-
-  I've tried various workarounds (adding DEBUG_FUNCTION to the
-  method, taking its address), but can't seem to fix it that way.
-  So as a nasty workaround, the following material is copied&pasted
-  from gcc-rich-location.c: */
-
-static bool
-get_range_for_expr (tree expr, location_range *r)
-{
-  if (EXPR_HAS_RANGE (expr))
-    {
-      source_range sr = EXPR_LOCATION_RANGE (expr);
-
-      /* Do we have meaningful data?  */
-      if (sr.m_start && sr.m_finish)
-	{
-	  r->m_start = expand_location (sr.m_start);
-	  r->m_finish = expand_location (sr.m_finish);
-	  return true;
-	}
-    }
-
-  return false;
-}
-
-/* Add a range to the rich_location, covering expression EXPR. */
-
-void
-gcc_rich_location::add_expr (tree expr)
-{
-  gcc_assert (expr);
-
-  location_range r;
-  r.m_show_caret_p = false;
-  if (get_range_for_expr (expr, &r))
-    add_range (&r);
-}
-
-/* FIXME: end of material taken from gcc-rich-location.c */
-
 int plugin_is_GPL_compatible;
 
 static void