diff mbox

Fix source locations of bad enum values (PR c/71610 and PR c/71613)

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

Commit Message

David Malcolm June 23, 2016, 2:52 a.m. UTC 
PR c/71613 identifies a problem where we fail to report this enum:

  enum { e1 = LLONG_MIN };

with -pedantic, due to LLONG_MIN being inside a system header.

This patch updates the C and C++ frontends to use the location of the
name as the primary location in the diagnostic, supplying the location
of the value as a secondary location, fixing the issue.

Before:
  $ gcc -c /tmp/test.c -Wpedantic
  /tmp/test.c: In function 'main':
  /tmp/test.c:3:14: warning: ISO C restricts enumerator values to range of 'int' [-Wpedantic]
     enum { c = -3000000000 };
                ^

After:
  $ ./xgcc -B. -c /tmp/test.c -Wpedantic
  /tmp/test.c: In function 'main':
  /tmp/test.c:3:10: warning: ISO C restricts enumerator values to range of 'int' [-Wpedantic]
     enum { c = -3000000000 };
            ^   ~~~~~~~~~~~

Successfully bootstrapped&regretested on x86_64-pc-linux-gnu;
adds 13 PASS results to gcc.sum and 9 PASS results to g++.sum.

OK for trunk?

gcc/c/ChangeLog:
	PR c/71610
	PR c/71613
	* c-decl.c (build_enumerator): Fix description of LOC in comment.
	Update diagnostics to use a rich_location at decl_loc, rather than
	at loc, adding loc as a secondary range if available.
	* c-parser.c (c_parser_enum_specifier): Use the full location of
	the expression for value_loc, rather than just the first token.

gcc/cp/ChangeLog:
	PR c/71610
	PR c/71613
	* cp-tree.h (build_enumerator): Add location_t param.
	* decl.c (build_enumerator): Add "value_loc" param.  Update
	"not an integer constant" diagnostic to use "loc" rather than
	input_location, and to add "value_loc" as a secondary range if
	available.
	* parser.c (cp_parser_enumerator_definition): Extract the
	location of the value from the cp_expr for the constant
	expression, if any, and pass it to build_enumerator.
	* pt.c (tsubst_enum): Extract EXPR_LOCATION of the value,
	and pass it to build_enumerator.

gcc/ChangeLog:
	PR c/71610
	PR c/71613
	* diagnostic-core.h (pedwarn_at_rich_loc): New prototype.
	* diagnostic.c (pedwarn_at_rich_loc): New function.

gcc/testsuite/ChangeLog:
	PR c/71610
	PR c/71613
	* c-c++-common/pr71610.c: New test case.
	* gcc.dg/c90-const-expr-8.c: Update expected column of diagnostic.
	* gcc.dg/pr71610-2.c: New test case.
	* gcc.dg/pr71613.c: New test case.
---
 gcc/c/c-decl.c                          | 32 +++++++++++++++++++++-----------
 gcc/c/c-parser.c                        | 10 ++++++++--
 gcc/cp/cp-tree.h                        |  2 +-
 gcc/cp/decl.c                           | 11 ++++++++---
 gcc/cp/parser.c                         |  7 +++++--
 gcc/cp/pt.c                             |  3 ++-
 gcc/diagnostic-core.h                   |  2 ++
 gcc/diagnostic.c                        | 12 ++++++++++++
 gcc/testsuite/c-c++-common/pr71610.c    | 11 +++++++++++
 gcc/testsuite/gcc.dg/c90-const-expr-8.c |  2 +-
 gcc/testsuite/gcc.dg/pr71610-2.c        | 17 +++++++++++++++++
 gcc/testsuite/gcc.dg/pr71613.c          | 19 +++++++++++++++++++
 12 files changed, 107 insertions(+), 21 deletions(-)
 create mode 100644 gcc/testsuite/c-c++-common/pr71610.c
 create mode 100644 gcc/testsuite/gcc.dg/pr71610-2.c
 create mode 100644 gcc/testsuite/gcc.dg/pr71613.c

Comments

Joseph Myers June 29, 2016, 10:05 p.m. UTC | #1 
On Wed, 22 Jun 2016, David Malcolm wrote:

> PR c/71613 identifies a problem where we fail to report this enum:
> 
>   enum { e1 = LLONG_MIN };
> 
> with -pedantic, due to LLONG_MIN being inside a system header.
> 
> This patch updates the C and C++ frontends to use the location of the
> name as the primary location in the diagnostic, supplying the location
> of the value as a secondary location, fixing the issue.
> 
> Before:
>   $ gcc -c /tmp/test.c -Wpedantic
>   /tmp/test.c: In function 'main':
>   /tmp/test.c:3:14: warning: ISO C restricts enumerator values to range of 'int' [-Wpedantic]
>      enum { c = -3000000000 };
>                 ^
> 
> After:
>   $ ./xgcc -B. -c /tmp/test.c -Wpedantic
>   /tmp/test.c: In function 'main':
>   /tmp/test.c:3:10: warning: ISO C restricts enumerator values to range of 'int' [-Wpedantic]
>      enum { c = -3000000000 };
>             ^   ~~~~~~~~~~~
> 
> Successfully bootstrapped&regretested on x86_64-pc-linux-gnu;
> adds 13 PASS results to gcc.sum and 9 PASS results to g++.sum.
> 
> OK for trunk?

The C front-end changes are OK for the diagnostic improvement, although I 
think piecemeal fixes make no sense for the issue of missing diagnostics - 
in the absence of a clear strategy to identify which cases should or 
should get diagnostics when a macro from a system header is expanded 
outside a system header, we need to make the default to give the 
diagnostics if the expansion location is outside a system header.  (This 
applies both to the general diagnostic disabling outside system headers, 
and to specific tests for being in system headers in front ends - those 
specific tests may or may not be relics left over from before the general 
disabling was added.)
Jeff Law July 20, 2016, 8:11 p.m. UTC | #2 
On 06/22/2016 08:52 PM, David Malcolm wrote:
> PR c/71613 identifies a problem where we fail to report this enum:
>
>   enum { e1 = LLONG_MIN };
>
> with -pedantic, due to LLONG_MIN being inside a system header.
>
> This patch updates the C and C++ frontends to use the location of the
> name as the primary location in the diagnostic, supplying the location
> of the value as a secondary location, fixing the issue.
>
> Before:
>   $ gcc -c /tmp/test.c -Wpedantic
>   /tmp/test.c: In function 'main':
>   /tmp/test.c:3:14: warning: ISO C restricts enumerator values to range of 'int' [-Wpedantic]
>      enum { c = -3000000000 };
>                 ^
>
> After:
>   $ ./xgcc -B. -c /tmp/test.c -Wpedantic
>   /tmp/test.c: In function 'main':
>   /tmp/test.c:3:10: warning: ISO C restricts enumerator values to range of 'int' [-Wpedantic]
>      enum { c = -3000000000 };
>             ^   ~~~~~~~~~~~
>
> Successfully bootstrapped&regretested on x86_64-pc-linux-gnu;
> adds 13 PASS results to gcc.sum and 9 PASS results to g++.sum.
>
> OK for trunk?
>
> gcc/c/ChangeLog:
> 	PR c/71610
> 	PR c/71613
> 	* c-decl.c (build_enumerator): Fix description of LOC in comment.
> 	Update diagnostics to use a rich_location at decl_loc, rather than
> 	at loc, adding loc as a secondary range if available.
> 	* c-parser.c (c_parser_enum_specifier): Use the full location of
> 	the expression for value_loc, rather than just the first token.
>
> gcc/cp/ChangeLog:
> 	PR c/71610
> 	PR c/71613
> 	* cp-tree.h (build_enumerator): Add location_t param.
> 	* decl.c (build_enumerator): Add "value_loc" param.  Update
> 	"not an integer constant" diagnostic to use "loc" rather than
> 	input_location, and to add "value_loc" as a secondary range if
> 	available.
> 	* parser.c (cp_parser_enumerator_definition): Extract the
> 	location of the value from the cp_expr for the constant
> 	expression, if any, and pass it to build_enumerator.
> 	* pt.c (tsubst_enum): Extract EXPR_LOCATION of the value,
> 	and pass it to build_enumerator.
>
> gcc/ChangeLog:
> 	PR c/71610
> 	PR c/71613
> 	* diagnostic-core.h (pedwarn_at_rich_loc): New prototype.
> 	* diagnostic.c (pedwarn_at_rich_loc): New function.
>
> gcc/testsuite/ChangeLog:
> 	PR c/71610
> 	PR c/71613
> 	* c-c++-common/pr71610.c: New test case.
> 	* gcc.dg/c90-const-expr-8.c: Update expected column of diagnostic.
> 	* gcc.dg/pr71610-2.c: New test case.
> 	* gcc.dg/pr71613.c: New test case.
OK.

jeff
Jason Merrill July 20, 2016, 8:16 p.m. UTC | #3 
On Thu, Jun 30, 2016 at 1:49 PM, Jason Merrill <jason@redhat.com> wrote:
> This needs a template testcase.

Did you get this reply before?  It bounced from the mailing list, but
I thought you would have gotten it directly.

Jason
David Malcolm July 21, 2016, 12:25 a.m. UTC | #4 
On Wed, 2016-07-20 at 16:16 -0400, Jason Merrill wrote:
> On Thu, Jun 30, 2016 at 1:49 PM, Jason Merrill <jason@redhat.com>
> wrote:
> > This needs a template testcase.
> 
> Did you get this reply before?  It bounced from the mailing list, but
> I thought you would have gotten it directly.

I did; sorry for not responding - I have quite a few projects underway
for gcc 7 and this one managed to slip off my radar.  I'll try to
resubmit with extra test coverage tomorrow.
diff mbox

Patch

diff --git a/gcc/c/c-decl.c b/gcc/c/c-decl.c
index 5c08c59..0d081ff 100644
--- a/gcc/c/c-decl.c
+++ b/gcc/c/c-decl.c
@@ -8159,7 +8159,7 @@  finish_enum (tree enumtype, tree values, tree attributes)
 /* Build and install a CONST_DECL for one value of the
    current enumeration type (one that was begun with start_enum).
    DECL_LOC is the location of the enumerator.
-   LOC is the location of the '=' operator if any, DECL_LOC otherwise.
+   LOC is the location of the value if any, DECL_LOC otherwise.
    Return a tree-list containing the CONST_DECL and its value.
    Assignment of sequential values by default is handled here.  */
 
@@ -8169,6 +8169,10 @@  build_enumerator (location_t decl_loc, location_t loc,
 {
   tree decl, type;
 
+  rich_location richloc (line_table, decl_loc);
+  if (loc && loc != decl_loc)
+    richloc.add_range (loc, false);
+
   /* Validate and default VALUE.  */
 
   if (value != 0)
@@ -8179,8 +8183,10 @@  build_enumerator (location_t decl_loc, location_t loc,
 	value = 0;
       else if (!INTEGRAL_TYPE_P (TREE_TYPE (value)))
 	{
-	  error_at (loc, "enumerator value for %qE is not an integer constant",
-		    name);
+	  error_at_rich_loc
+	    (&richloc,
+	     "enumerator value for %qE is not an integer constant",
+	     name);
 	  value = 0;
 	}
       else
@@ -8189,14 +8195,17 @@  build_enumerator (location_t decl_loc, location_t loc,
 	    {
 	      value = c_fully_fold (value, false, NULL);
 	      if (TREE_CODE (value) == INTEGER_CST)
-		pedwarn (loc, OPT_Wpedantic,
-			 "enumerator value for %qE is not an integer "
-			 "constant expression", name);
+		pedwarn_at_rich_loc
+		  (&richloc, OPT_Wpedantic,
+		   "enumerator value for %qE is not an integer "
+		   "constant expression", name);
 	    }
 	  if (TREE_CODE (value) != INTEGER_CST)
 	    {
-	      error ("enumerator value for %qE is not an integer constant",
-		     name);
+	      error_at_rich_loc
+		(&richloc,
+		 "enumerator value for %qE is not an integer constant",
+		 name);
 	      value = 0;
 	    }
 	  else
@@ -8214,15 +8223,16 @@  build_enumerator (location_t decl_loc, location_t loc,
     {
       value = the_enum->enum_next_value;
       if (the_enum->enum_overflow)
-	error_at (loc, "overflow in enumeration values");
+	error_at_rich_loc (&richloc, "overflow in enumeration values");
     }
   /* Even though the underlying type of an enum is unspecified, the
      type of enumeration constants is explicitly defined as int
      (6.4.4.3/2 in the C99 Standard).  GCC allows any integer type as
      an extension.  */
   else if (!int_fits_type_p (value, integer_type_node))
-    pedwarn (loc, OPT_Wpedantic,
-	     "ISO C restricts enumerator values to range of %<int%>");
+    pedwarn_at_rich_loc
+      (&richloc, OPT_Wpedantic,
+       "ISO C restricts enumerator values to range of %<int%>");
 
   /* The ISO C Standard mandates enumerators to have type int, even
      though the underlying type of an enum type is unspecified.
diff --git a/gcc/c/c-parser.c b/gcc/c/c-parser.c
index 78bf68e..3e9458c 100644
--- a/gcc/c/c-parser.c
+++ b/gcc/c/c-parser.c
@@ -2720,8 +2720,14 @@  c_parser_enum_specifier (c_parser *parser)
 	  if (c_parser_next_token_is (parser, CPP_EQ))
 	    {
 	      c_parser_consume_token (parser);
-	      value_loc = c_parser_peek_token (parser)->location;
-	      enum_value = c_parser_expr_no_commas (parser, NULL).value;
+	      c_expr value = c_parser_expr_no_commas (parser, NULL);
+	      enum_value = value.value;
+	      if (EXPR_HAS_LOCATION (enum_value))
+		value_loc = EXPR_LOCATION (enum_value);
+	      else
+		value_loc
+		  = make_location (value.get_start (), value.get_start (),
+				   value.get_finish ());
 	    }
 	  else
 	    enum_value = NULL_TREE;
diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index 5b87bb3..9ce0ee5 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -5792,7 +5792,7 @@  extern void xref_basetypes			(tree, tree);
 extern tree start_enum				(tree, tree, tree, tree, bool, bool *);
 extern void finish_enum_value_list		(tree);
 extern void finish_enum				(tree);
-extern void build_enumerator			(tree, tree, tree, tree, location_t);
+extern void build_enumerator			(tree, tree, tree, tree, location_t, location_t);
 extern tree lookup_enumerator			(tree, tree);
 extern bool start_preparsed_function		(tree, tree, int);
 extern bool start_function			(cp_decl_specifier_seq *,
diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
index c86a131..3db8317 100644
--- a/gcc/cp/decl.c
+++ b/gcc/cp/decl.c
@@ -13542,7 +13542,7 @@  finish_enum (tree enumtype)
 
 void
 build_enumerator (tree name, tree value, tree enumtype, tree attributes,
-		  location_t loc)
+		  location_t loc, location_t value_loc)
 {
   tree decl;
   tree context;
@@ -13590,8 +13590,13 @@  build_enumerator (tree name, tree value, tree enumtype, tree attributes,
 	      if (TREE_CODE (value) != INTEGER_CST
 		  || ! INTEGRAL_OR_ENUMERATION_TYPE_P (TREE_TYPE (value)))
 		{
-		  error ("enumerator value for %qD is not an integer constant",
-			 name);
+		  rich_location richloc (line_table, loc);
+		  if (value_loc)
+		    richloc.add_range (value_loc, false);
+		  error_at_rich_loc
+		    (&richloc,
+		     "enumerator value for %qD is not an integer constant",
+		     name);
 		  value = NULL_TREE;
 		}
 	    }
diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index d1f06fd..306b1dc 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -17398,6 +17398,7 @@  cp_parser_enumerator_definition (cp_parser* parser, tree type)
   tree identifier;
   tree value;
   location_t loc;
+  location_t value_loc = UNKNOWN_LOCATION;
 
   /* Save the input location because we are interested in the location
      of the identifier and not the location of the explicit value.  */
@@ -17417,7 +17418,9 @@  cp_parser_enumerator_definition (cp_parser* parser, tree type)
       /* Consume the `=' token.  */
       cp_lexer_consume_token (parser->lexer);
       /* Parse the value.  */
-      value = cp_parser_constant_expression (parser);
+      cp_expr value_expr = cp_parser_constant_expression (parser);
+      value = value_expr;
+      value_loc = value_expr.get_location ();
     }
   else
     value = NULL_TREE;
@@ -17428,7 +17431,7 @@  cp_parser_enumerator_definition (cp_parser* parser, tree type)
     value = error_mark_node;
 
   /* Create the enumerator.  */
-  build_enumerator (identifier, value, type, attrs, loc);
+  build_enumerator (identifier, value, type, attrs, loc, value_loc);
 }
 
 /* Parse a namespace-name.
diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
index 11b5d82..4e062c5 100644
--- a/gcc/cp/pt.c
+++ b/gcc/cp/pt.c
@@ -22373,7 +22373,8 @@  tsubst_enum (tree tag, tree newtag, tree args)
       /* Actually build the enumerator itself.  Here we're assuming that
 	 enumerators can't have dependent attributes.  */
       build_enumerator (DECL_NAME (decl), value, newtag,
-			DECL_ATTRIBUTES (decl), DECL_SOURCE_LOCATION (decl));
+			DECL_ATTRIBUTES (decl), DECL_SOURCE_LOCATION (decl),
+			EXPR_LOCATION (value));
     }
 
   if (SCOPED_ENUM_P (newtag))
diff --git a/gcc/diagnostic-core.h b/gcc/diagnostic-core.h
index f783761..51df150 100644
--- a/gcc/diagnostic-core.h
+++ b/gcc/diagnostic-core.h
@@ -76,6 +76,8 @@  extern void fatal_error (location_t, const char *, ...) ATTRIBUTE_GCC_DIAG(2,3)
 /* Pass one of the OPT_W* from options.h as the second parameter.  */
 extern bool pedwarn (location_t, int, const char *, ...)
      ATTRIBUTE_GCC_DIAG(3,4);
+extern bool pedwarn_at_rich_loc (rich_location *, int, const char *, ...)
+     ATTRIBUTE_GCC_DIAG(3,4);
 extern bool permerror (location_t, const char *, ...) ATTRIBUTE_GCC_DIAG(2,3);
 extern bool permerror_at_rich_loc (rich_location *, const char *,
 				   ...) ATTRIBUTE_GCC_DIAG(2,3);
diff --git a/gcc/diagnostic.c b/gcc/diagnostic.c
index 8467aaa..62ba5b4 100644
--- a/gcc/diagnostic.c
+++ b/gcc/diagnostic.c
@@ -1089,6 +1089,18 @@  pedwarn (location_t location, int opt, const char *gmsgid, ...)
   return ret;
 }
 
+/* Same as "pedwarn", but at RICHLOC.  */
+
+bool
+pedwarn_at_rich_loc (rich_location *richloc, int opt, const char *gmsgid, ...)
+{
+  va_list ap;
+  va_start (ap, gmsgid);
+  bool ret = diagnostic_impl (richloc, opt, gmsgid, &ap, DK_PEDWARN);
+  va_end (ap);
+  return ret;
+}
+
 /* A "permissive" error at LOCATION: issues an error unless
    -fpermissive was given on the command line, in which case it issues
    a warning.  Use this for things that really should be errors but we
diff --git a/gcc/testsuite/c-c++-common/pr71610.c b/gcc/testsuite/c-c++-common/pr71610.c
new file mode 100644
index 0000000..b62a2e1
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/pr71610.c
@@ -0,0 +1,11 @@ 
+/* { dg-options "-fdiagnostics-show-caret" } */
+void test (void)
+{
+  enum { c1 = "not int", /* { dg-error "10: enumerator value for .c1. is not an integer constant" } */
+/* { dg-begin-multiline-output "" }
+   enum { c1 = "not int",
+          ^~   ~~~~~~~~~
+   { dg-end-multiline-output "" } */
+
+	 c_end };
+}
diff --git a/gcc/testsuite/gcc.dg/c90-const-expr-8.c b/gcc/testsuite/gcc.dg/c90-const-expr-8.c
index b6aba7b..ba2b029 100644
--- a/gcc/testsuite/gcc.dg/c90-const-expr-8.c
+++ b/gcc/testsuite/gcc.dg/c90-const-expr-8.c
@@ -22,7 +22,7 @@  enum e {
   E5 = 0 * -INT_MIN, /* { dg-warning "12:integer overflow in expression" } */
   /* { dg-error "3:overflow in constant expression" "constant" { target *-*-* } 22 } */
   E6 = 0 * !-INT_MIN, /* { dg-warning "13:integer overflow in expression" } */
-  /* { dg-error "8:not an integer constant" "constant" { target *-*-* } 24 } */
+  /* { dg-error "3:not an integer constant" "constant" { target *-*-* } 24 } */
   E7 = INT_MIN % -1 /* { dg-warning "16:integer overflow in expression" } */
   /* { dg-error "1:overflow in constant expression" "constant" { target *-*-* } 28 } */
 };
diff --git a/gcc/testsuite/gcc.dg/pr71610-2.c b/gcc/testsuite/gcc.dg/pr71610-2.c
new file mode 100644
index 0000000..36bed6e
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr71610-2.c
@@ -0,0 +1,17 @@ 
+/* { dg-options "-pedantic -fdiagnostics-show-caret" } */
+void test (void)
+{
+  enum { c1 = -3000000000, /* { dg-warning "10: ISO C restricts enumerator values to range of .int." } */
+/* { dg-begin-multiline-output "" }
+   enum { c1 = -3000000000,
+          ^~   ~~~~~~~~~~~
+   { dg-end-multiline-output "" } */
+
+          c2 = "not int", /* { dg-error "11: enumerator value for .c2. is not an integer constant" } */
+/* { dg-begin-multiline-output "" }
+           c2 = "not int",
+           ^~   ~~~~~~~~~
+   { dg-end-multiline-output "" } */
+
+	 c_end };
+}
diff --git a/gcc/testsuite/gcc.dg/pr71613.c b/gcc/testsuite/gcc.dg/pr71613.c
new file mode 100644
index 0000000..fa1bba2
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr71613.c
@@ -0,0 +1,19 @@ 
+/* { dg-options "-pedantic -fdiagnostics-show-caret" } */
+
+#include <limits.h>
+
+void test (void)
+{
+  enum { e1 = LLONG_MIN };  /* { dg-warning "ISO C restricts enumerator values to range of .int." } */
+/* { dg-begin-multiline-output "" }
+   enum { e1 = LLONG_MIN };
+          ^~
+   { dg-end-multiline-output "" } */
+
+  enum { e2 = +LLONG_MIN }; /* { dg-warning "ISO C restricts enumerator values to range of .int." } */
+/* { dg-begin-multiline-output "" }
+   enum { e2 = +LLONG_MIN };
+          ^~
+   { dg-end-multiline-output "" } */
+
+}