[PATCH] C FE: improvements to ranges of bad return values

[PATCH] C FE: improvements to ranges of bad return values

[David Malcolm]

In the C FE, c_parser_statement_after_labels passes "xloc" to
c_finish_return, which is the location of the first token
within the returned expression.

Hence we don't get a full underline for the following:

diagnostic-range-bad-return.c:34:10: warning: function returns address of local variable [-Wreturn-local-addr]
   return &some_local;
          ^

This feels like a bug; this patch fixes it to use the location of
the expr if available, and to fall back to xloc otherwise, giving
us underlining of the full expression:

diagnostic-range-bad-return.c:34:10: warning: function returns address of local variable [-Wreturn-local-addr]
   return &some_local;
          ^~~~~~~~~~~

The testcase also adds some coverage for underlining the
"return" token for the cases where we're warning about th
erroneous presence/absence of a return value.

As an additional tweak, it struck me that we could be more
user-friendly for these latter diagnostics by issuing a note
about where the function was declared, so this patch also adds
an inform for these cases:

diagnostic-range-bad-return.c: In function 'missing_return_value':
diagnostic-range-bad-return.c:31:3: warning: 'return' with no value, in function returning non-void
   return; /* { dg-warning "'return' with no value, in function returning non-void" } */
   ^~~~~~

diagnostic-range-bad-return.c:29:5: note: declared here
 int missing_return_value (void)
     ^~~~~~~~~~~~~~~~~~~~

(ideally we'd put the underline on the return type, but that location
isn't captured)

This latter part of the patch is an enhancement rather than a
bugfix, though FWIW, and I'm not sure I can argue this with a
straight face, the tweak was posted as part of:
  "[PATCH 16/22] C/C++ frontend: use tree ranges in various diagnostics"
in https://gcc.gnu.org/ml/gcc-patches/2015-09/msg00745.html
during stage 1.  Hopefully low risk, and a small usability improvement;
but if this is pushing it, it'd be simple to split this up and only
do the bug fix.

Successfully bootstrapped&regrtested on x86_64-pc-linux-gnu;
adds 12 PASS results to gcc.sum.

OK for trunk for stage 3?

gcc/c/ChangeLog:
	* c-parser.c (c_parser_statement_after_labels): When calling
	c_finish_return, Use the return expression's location if it has
	one, falling back to the location of the first token within it.
	* c-typeck.c (c_finish_return): When issuing warnings about
	the incorrect presence/absence of a return value, issue a note
	showing the declaration of the function.

gcc/testsuite/ChangeLog:
	* gcc.dg/diagnostic-range-bad-return.c: New test case.
---
 gcc/c/c-parser.c                                   |  3 +-
 gcc/c/c-typeck.c                                   | 28 ++++++++----
 gcc/testsuite/gcc.dg/diagnostic-range-bad-return.c | 52 ++++++++++++++++++++++
 3 files changed, 74 insertions(+), 9 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/diagnostic-range-bad-return.c

diff --git a/gcc/c/c-parser.c b/gcc/c/c-parser.c
index e149e19..933a938 100644
--- a/gcc/c/c-parser.c
+++ b/gcc/c/c-parser.c
@@ -5108,7 +5108,8 @@ c_parser_statement_after_labels (c_parser *parser, vec<tree> *chain)
 	      location_t xloc = c_parser_peek_token (parser)->location;
 	      struct c_expr expr = c_parser_expression_conv (parser);
 	      mark_exp_read (expr.value);
-	      stmt = c_finish_return (xloc, expr.value, expr.original_type);
+	      stmt = c_finish_return (EXPR_LOC_OR_LOC (expr.value, xloc),
+				      expr.value, expr.original_type);
 	      goto expect_semicolon;
 	    }
 	  break;
diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c
index b93da07..c314f08 100644
--- a/gcc/c/c-typeck.c
+++ b/gcc/c/c-typeck.c
@@ -9546,24 +9546,36 @@ c_finish_return (location_t loc, tree retval, tree origtype)
       if ((warn_return_type || flag_isoc99)
 	  && valtype != 0 && TREE_CODE (valtype) != VOID_TYPE)
 	{
+	  bool warned_here;
 	  if (flag_isoc99)
-	    pedwarn (loc, 0, "%<return%> with no value, in "
-		     "function returning non-void");
+	    warned_here = pedwarn
+	      (loc, 0,
+	       "%<return%> with no value, in function returning non-void");
 	  else
-	    warning_at (loc, OPT_Wreturn_type, "%<return%> with no value, "
-			"in function returning non-void");
+	    warned_here = warning_at
+	      (loc, OPT_Wreturn_type,
+	       "%<return%> with no value, in function returning non-void");
 	  no_warning = true;
+	  if (warned_here)
+	    inform (DECL_SOURCE_LOCATION (current_function_decl),
+		    "declared here");
 	}
     }
   else if (valtype == 0 || TREE_CODE (valtype) == VOID_TYPE)
     {
       current_function_returns_null = 1;
+      bool warned_here;
       if (TREE_CODE (TREE_TYPE (retval)) != VOID_TYPE)
-	pedwarn (xloc, 0,
-		 "%<return%> with a value, in function returning void");
+	warned_here = pedwarn
+	  (xloc, 0,
+	   "%<return%> with a value, in function returning void");
       else
-	pedwarn (xloc, OPT_Wpedantic, "ISO C forbids "
-		 "%<return%> with expression, in function returning void");
+	warned_here = pedwarn
+	  (xloc, OPT_Wpedantic, "ISO C forbids "
+	   "%<return%> with expression, in function returning void");
+      if (warned_here)
+	inform (DECL_SOURCE_LOCATION (current_function_decl),
+		"declared here");
     }
   else
     {
diff --git a/gcc/testsuite/gcc.dg/diagnostic-range-bad-return.c b/gcc/testsuite/gcc.dg/diagnostic-range-bad-return.c
new file mode 100644
index 0000000..063fdf1
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/diagnostic-range-bad-return.c
@@ -0,0 +1,52 @@
+/* { dg-options "-fdiagnostics-show-caret -Wreturn-local-addr" } */
+
+int *address_of_local (void)
+{
+  int some_local;
+  return &some_local; /* { dg-warning "function returns address of local variable" } */
+/* { dg-begin-multiline-output "" }
+   return &some_local;
+          ^~~~~~~~~~~
+   { dg-end-multiline-output "" } */
+}
+
+void surplus_return_when_void_1 (void)
+{
+  return 500; /* { dg-warning "'return' with a value, in function returning void" } */
+/* { dg-begin-multiline-output "" }
+   return 500;
+          ^~~
+   { dg-end-multiline-output "" } */
+/* { dg-begin-multiline-output "" }
+ void surplus_return_when_void_1 (void)
+      ^~~~~~~~~~~~~~~~~~~~~~~~~~
+   { dg-end-multiline-output "" } */
+}
+
+void surplus_return_when_void_2 (int i, int j)
+{
+  return i * j; /* { dg-warning "'return' with a value, in function returning void" } */
+/* { dg-begin-multiline-output "" }
+   return i * j;
+          ~~^~~
+   { dg-end-multiline-output "" } */
+/* { dg-begin-multiline-output "" }
+ void surplus_return_when_void_2 (int i, int j)
+      ^~~~~~~~~~~~~~~~~~~~~~~~~~
+   { dg-end-multiline-output "" } */
+}
+
+int missing_return_value (void)
+{
+  return; /* { dg-warning "'return' with no value, in function returning non-void" } */
+/* { dg-begin-multiline-output "" }
+   return;
+   ^~~~~~
+   { dg-end-multiline-output "" } */
+/* { dg-begin-multiline-output "" }
+ int missing_return_value (void)
+     ^~~~~~~~~~~~~~~~~~~~
+   { dg-end-multiline-output "" } */
+/* TODO: ideally we'd underline the return type i.e. "int", but that
+   location isn't captured.  */
+}
-- 
1.8.5.3

Re: [PATCH] C FE: improvements to ranges of bad return values

[Jeff Law]

On 12/16/2015 07:19 PM, David Malcolm wrote:
> In the C FE, c_parser_statement_after_labels passes "xloc" to
> c_finish_return, which is the location of the first token
> within the returned expression.
>
> Hence we don't get a full underline for the following:
>
> diagnostic-range-bad-return.c:34:10: warning: function returns address of local variable [-Wreturn-local-addr]
>     return &some_local;
>            ^
>
> This feels like a bug; this patch fixes it to use the location of
> the expr if available, and to fall back to xloc otherwise, giving
> us underlining of the full expression:
>
> diagnostic-range-bad-return.c:34:10: warning: function returns address of local variable [-Wreturn-local-addr]
>     return &some_local;
>            ^~~~~~~~~~~
>
> The testcase also adds some coverage for underlining the
> "return" token for the cases where we're warning about th
> erroneous presence/absence of a return value.
>
> As an additional tweak, it struck me that we could be more
> user-friendly for these latter diagnostics by issuing a note
> about where the function was declared, so this patch also adds
> an inform for these cases:
>
> diagnostic-range-bad-return.c: In function 'missing_return_value':
> diagnostic-range-bad-return.c:31:3: warning: 'return' with no value, in function returning non-void
>     return; /* { dg-warning "'return' with no value, in function returning non-void" } */
>     ^~~~~~
>
> diagnostic-range-bad-return.c:29:5: note: declared here
>   int missing_return_value (void)
>       ^~~~~~~~~~~~~~~~~~~~
>
> (ideally we'd put the underline on the return type, but that location
> isn't captured)
>
> This latter part of the patch is an enhancement rather than a
> bugfix, though FWIW, and I'm not sure I can argue this with a
> straight face, the tweak was posted as part of:
>    "[PATCH 16/22] C/C++ frontend: use tree ranges in various diagnostics"
> in https://gcc.gnu.org/ml/gcc-patches/2015-09/msg00745.html
> during stage 1.  Hopefully low risk, and a small usability improvement;
> but if this is pushing it, it'd be simple to split this up and only
> do the bug fix.
>
> Successfully bootstrapped&regrtested on x86_64-pc-linux-gnu;
> adds 12 PASS results to gcc.sum.
>
> OK for trunk for stage 3?
>
> gcc/c/ChangeLog:
> 	* c-parser.c (c_parser_statement_after_labels): When calling
> 	c_finish_return, Use the return expression's location if it has
> 	one, falling back to the location of the first token within it.
> 	* c-typeck.c (c_finish_return): When issuing warnings about
> 	the incorrect presence/absence of a return value, issue a note
> 	showing the declaration of the function.
This is fine.  I think the first is pretty easy to justify.  THe second 
is harder.  Again I think it's very low risk and has user-visible benfits.

Jeff