fix -fmax-errors & notes

Hi,
Jonathan & I were chatting at the cauldron about how -fmax-errors kills any 
notes about the final error.  That's because we bail out just after emitting the 
final error.  This patch fixes the problem by bailing out just before emitting 
the error or warning after that.

Sure, we'll do slightly more compilation than asked for, but this is the fatal 
error path, so who cares how long it takes.  Better to get notes to the user.

I augmented the fmax-errors testcase so that the final emitted error has a 
trailing note (which we now emit), and is followed by a warning (which causes us 
to bail).

The same logic applies to -Wfatal-errors handling.

ok?

nathan

On Tue, 2016-10-11 at 06:34 -0400, Nathan Sidwell wrote:
> Hi,
> Jonathan & I were chatting at the cauldron about how -fmax-errors
> kills any 
> notes about the final error.  That's because we bail out just after
> emitting the 
> final error.  This patch fixes the problem by bailing out just before
> emitting 
> the error or warning after that.
> 
> Sure, we'll do slightly more compilation than asked for, but this is
> the fatal 
> error path, so who cares how long it takes.  Better to get notes to
> the user.
> 
> I augmented the fmax-errors testcase so that the final emitted error
> has a 
> trailing note (which we now emit), and is followed by a warning
> (which causes us 
> to bail).
> 
> The same logic applies to -Wfatal-errors handling.
> 
> ok?

Thanks.  Various comments inline below.

> 2016-10-11  Nathan Sidwell  <nathan@acm.org>
> 
> 	* diagnostic.c (diagnostic_action_after_output): Remove fatal
> 	and max error handling here ....
> 	(diagnostic_report_diagnostic): ... do it here instead.
> 
> 	testsuite/
> 	* c-c++-common/fmax-errors.c: Add error with note.
> 
> Index: diagnostic.c
> ===================================================================
> --- diagnostic.c	(revision 240920)
> +++ diagnostic.c	(working copy)
> @@ -464,24 +464,6 @@ diagnostic_action_after_output (diagnost
>      case DK_SORRY:
>        if (context->abort_on_error)
>  	real_abort ();
> -      if (context->fatal_errors)
> -	{
> -	  fnotice (stderr, "compilation terminated due to -Wfatal
-errors.\n");
> -	  diagnostic_finish (context);
> -	  exit (FATAL_EXIT_CODE);
> -	}
> -      if (context->max_errors != 0
> -	  && ((unsigned) (diagnostic_kind_count (context, DK_ERROR)
> -			  + diagnostic_kind_count (context,
DK_SORRY)
> -			  + diagnostic_kind_count (context,
DK_WERROR))
> -	      >= context->max_errors))
> -	{
> -	  fnotice (stderr,
> -		   "compilation terminated due to -fmax
-errors=%u.\n",
> -		   context->max_errors);
> -	  diagnostic_finish (context);
> -	  exit (FATAL_EXIT_CODE);
> -	}
>        break;
>  
>      case DK_ICE:
> @@ -890,6 +872,25 @@ diagnostic_report_diagnostic (diagnostic
>  	return false;
>      }
>  
> +  if (diagnostic->kind != DK_NOTE
> +      && (unsigned)(diagnostic_kind_count (context, DK_ERROR)
> +		    + diagnostic_kind_count (context, DK_SORRY)
> +		    + diagnostic_kind_count (context, DK_WERROR))
> +      >= (context->fatal_errors ? 1
> +	  : context->max_errors ? context->max_errors : ~0u))

Please simplify this conditional logic by breaking if up into multiple
nested if statements:

   if (diagnostic->kind != DK_NOTE)
     {
        int count =
(diagnostic_kind_count (context, DK_ERROR)
              	     +
diagnostic_kind_count (context, DK_SORRY)
                     +
diagnostic_kind_count (context, DK_WERROR);
or somesuch...

> +      >= (context->fatal_errors ? 1
> +	  : context->max_errors ? context->max_errors : ~0u))
> +    {
> +      /* Check before emitting the diagnostic that would exceed the
> +	 limit.  This way we will emit notes relevant to the final
> +	 emitted error.  */
> +      fnotice (stderr,
> +	       context->fatal_errors
> +	       ? "compilation terminated due to -Wfatal-errors.\n"
> +	       : "compilation terminated due to -fmax-errors=%u.\n",
> +	       context->max_errors);
> +      diagnostic_finish (context);
> +      exit (FATAL_EXIT_CODE);
> +    }

This logic is running when the next diagnostic is about to be emitted.
But what if the user has selected -Wfatal-errors and there's a single
error and no further diagnostics?  Could this change the observable
behavior?  (I'm trying to think of a case here, but failing).


>    context->lock++;
>  
>    if (diagnostic->kind == DK_ICE || diagnostic->kind == DK_ICE_NOBT)
> Index: testsuite/c-c++-common/fmax-errors.c
> ===================================================================
> --- testsuite/c-c++-common/fmax-errors.c	(revision 240920)
> +++ testsuite/c-c++-common/fmax-errors.c	(working copy)
> @@ -1,11 +1,13 @@
>  /* PR c/44782 */
>  /* { dg-do compile } */
> -/* { dg-options "-fmax-errors=3" } */
> +/* { dg-options "-fmax-errors=3 -Wall" } */
>  
>  void foo (unsigned int i, unsigned int j)
>  {
>    (i) ();			/* { dg-error "" } */
>    (j) ();			/* { dg-error "" } */
> -  (i+j) ();			/* { dg-error "" } */
> +  (k) ();			/* { dg-error "" } */
> +  /* { dg-message "identifier" "" { target c } 9 } */
> +  i + j; /* no warning.  */
>    (i*j) ();			/* no error here due to -fmax
-errors */
>  } /* { dg-prune-output "compilation terminated" } */

Please can you add a comment about the dg-message directive, explaining
the intent.  ("Verify that with -fmax-errors that a note associated
with the final error is still emitted", or somesuch").

Please can you add a testcase for -Wfatal-errors.

Thanks
Dave

On 10/11/16 16:07, David Malcolm wrote:

> This logic is running when the next diagnostic is about to be emitted.
> But what if the user has selected -Wfatal-errors and there's a single
> error and no further diagnostics?  Could this change the observable
> behavior?  (I'm trying to think of a case here, but failing).

good question.

I think both -Wfatal-errors and -fmax-errors=1 would now result in not emitting 
the 'compilation terminated' message (and similarly for any -fmax-errors=N with 
exactly N errors.   Perhaps this is worth worrying about for -Wfatal-errors, but 
not for -fmax-errors.  If one wanted notes but only one error one could use 
-fmax-errors=1.  (not sure of the use case of -Wfatal-errors given 
-fmax-errors=1 seems to be nearly the same?)

Perhaps the -Wfatal-errors logic should remain unchanged?

nathan