[RFC,diagnostics/fortran] Move gfc_warning (buffered) to the common diagnostics machinery

This is just a RFC so no Changelog yet. It bootstraps and passes the
testsuite. I have three major questions:

* Dodji: Do the common diagnostics part look reasonable? I tried to be
as least invasive as possible. If you have comments or suggestions
they are very welcome.

* Fortran devs: Is this approach acceptable? The main idea is to have
an output_buffer called pp_warning_buffer with the flush_p bit unset
if we are buffering. When printing buffered warnings, use this
output_buffer in the global_dc->printer instead of the (unbuffered
one) used by the *_now variants. In principle this could support
several buffered diagnostics, but Fortran only seems to buffer at most
one.

The ugliest part is how to handle warningcount and werrorcount. I
could handle this in the common machinery in a better way by storing
DK_WERROR in the diagnostic->kind and checking it after printing. This
way we can first decrease both counters, then increase back the one
not changed and store the kind somewhere. then if the diagnostic is
canceled nothing is done, if it is flushed, then increase the
appropriate counter (perhaps calling into the common part for any post
action due to Wfatal-errors or -fmax-errors=).

I can also hide the output_buffer switching inside two helper
functions, but the helper function would need to use either a static
variable or a global one to save and restore the tmp_buffer. I'm not
sure that is better or worse (the current code uses a global pointer
&cur_error_buffer, so perhaps I should have used a similar approach).

* Fortran devs #2: The testsuite is testing that the warning is
eventually printed. However, I'm not sure it is testing when the
warning is buffered and then discarded, is it? If not, how can I
produce such a test?

Thanks,

Manuel.

On Thu, Nov 27, 2014 at 06:25:01PM +0100, Manuel López-Ibáñez wrote:
> 
> The ugliest part is how to handle warningcount and werrorcount. I
> could handle this in the common machinery in a better way by storing
> DK_WERROR in the diagnostic->kind and checking it after printing. This
> way we can first decrease both counters, then increase back the one
> not changed and store the kind somewhere. then if the diagnostic is
> canceled nothing is done, if it is flushed, then increase the
> appropriate counter (perhaps calling into the common part for any post
> action due to Wfatal-errors or -fmax-errors=).
> 
> I can also hide the output_buffer switching inside two helper
> functions, but the helper function would need to use either a static
> variable or a global one to save and restore the tmp_buffer. I'm not
> sure that is better or worse (the current code uses a global pointer
> &cur_error_buffer, so perhaps I should have used a similar approach).
> 

Hi Manuel,

First, thanks for all your work on the error/warning code in gfortran.

Second, I'm dredging through my memories from a decade or so ago.  Others
are encouraged to jump in and correct me.

The design of gfortran's warning/error code is largely unchanged from 
when g95 was originally imported into the tree.  There have been, of
course, some changes but the design was set.  My understanding of how
it all works(ed) is that gfortran's parsing runs a series of matchers
over the input.  A matcher may generate an error/warning, which is
written into a buffer.  But, instead of immediately reporting the issue
gfortran may continue to run a series of matchers to see if the input
is in fact validate.  gfortran will report the last buffered error/warning
message only after the last matcher fails.  In running the series of
matchers, there are times when a matcher may hit an error/warning
condition and it is, in fact, a problem and gfortran wants to report
the problem NOW.  This is the origin of the *_now variants.

I would need to go dredge through ChangeLogs, but I believe I may be
responsible for the counting of error messages. I know that I at least
set the default value for -fmax-error.  The origins to the counting
is that once gfortran encountered and reported an error, she would
discard the problematic statement and continue parsing the input.  This
often leads to a series of spurious run-on error messages, which are
all caused by the first error.  Thus, if one does -fmax-error=1 and
fixes the problem, then all the other problems disappear.

Finally, gfortran's error/warning mechanism isn't immediately available
at the start of execution.  However, errors can occur before the mechanism
is initialized.  This is one reason why one finds fatal_error() spinkled
throughout the code.

>
> * Fortran devs #2: The testsuite is testing that the warning is
> eventually printed. However, I'm not sure it is testing when the
> warning is buffered and then discarded, is it? If not, how can I
> produce such a test?
> 

I think that you can't check for a discarded buffered message.  In
fact, why bother?

Manuel López-Ibáñez wrote:
> * Fortran devs: Is this approach acceptable? The main idea is to have
> an output_buffer called pp_warning_buffer with the flush_p bit unset
> if we are buffering. When printing buffered warnings, use this
> output_buffer in the global_dc->printer instead of the (unbuffered
> one) used by the *_now variants. In principle this could support
> several buffered diagnostics, but Fortran only seems to buffer at most
> one.

I think the approach is fine. As the _now version overrides the buffer, 
one might even do with a single buffer by clearing it, setting flush_p 
temporarily to true and printing the message. It only might collide with 
buffered warnings (for error_now) and errors (for warning_now), but I 
don't see whether that's an issue. For warnings/error_now it probably 
isn't, for errors/warning_now, it might. Thus, having two buffers is 
probably better.

> The ugliest part is how to handle warningcount and werrorcount. I
> could handle this in the common machinery in a better way by storing
> DK_WERROR in the diagnostic->kind and checking it after printing.

I'm missing the "but". Does it require more modifications in common code 
and thus you shy away? Or is there another reason?

> I can also hide the output_buffer switching inside two helper
> functions, but the helper function would need to use either a static
> variable or a global one to save and restore the tmp_buffer. I'm not
> sure that is better or worse (the current code uses a global pointer
> &cur_error_buffer, so perhaps I should have used a similar approach).

Me neither. The current approach is rather localized in error.c; thus, 
it doesn't really matter.

> * Fortran devs #2: The testsuite is testing that the warning is
> eventually printed. However, I'm not sure it is testing when the
> warning is buffered and then discarded, is it? If not, how can I
> produce such a test?

Well, for nearly every Fortran program, at least for one line multiple 
attempts have to be done by the parser. Thus, if you set a break point 
in gfc_error, you will see "syntax error" messages which never make it 
to the screen. As the test suite checks for excess errors, nearly every 
test-suite file tests this.

* * *

With the patch in place, plus its gfc_error cousin, and after the 
follow-up conversion work, only the following would remain:

* gfc_warning: a single occurence of two locations in the same error 
string (of ~120 calls)
* gfc_error: approx. 34 times two locations (of ~1630 calls)
* gfc_error_now_1: 11 times two locations

* In scanner.c:
- gfc_warning_now_1: 6 calls in scanner.c
- gfc_warning: 3
- fprintf: 2
- gfc_error: 0
Some of those can probably be simply converted, others either need to 
remain, or one has to setup a proper location (currently, using 
gfc_warning_now would ICE), or one creates a work around and constructs 
manually the error string (at least for the fprintf cases).

* * *

Regarding the use of libcpp: Currently, it is only used with explicit 
preprocessing ("-cpp", file extension) and it is used to output into a 
temporary file which is then read back. I experimented with
   token = cpp_get_token (cpp_in);
   const char *str = cpp_token_as_text (cpp_in, token);
which kind of works okay.

But I do not understand how one gets linebreaks and spacing correctly. 
For linebreaks, I can use "flags & BOL" or a callback. But I'm still 
trying to understand how one can get the number of spacings. "flags & 
PREV_WHITE" seems to record only a single one and seems to convert all 
of them (\t, \n and ' ') into a single type.

For fixed-form Fortran, spaces are essential. On punch cards,* only the 
first 72 characters were read; as (some?) punch cards had additional 8 
characters, those were used for comments (e.g. to enumerate the cards). 
Hence, there are codes out there, which assumes that everything beyond 
72 characters is ignored. Thus, the number of spaces is crucial – at 
least with -fpreprocessed, if one wants to always goes through libcpp.

Looking at the current code, it seems to use a line-break callback with 
SOURCE_COLUMN to reconstruct the indentation. I think saving the token 
src_column and the length of the token string and subtracting it from 
the new source location, will also work for mid-line spaces. However, 
that's really a kludge – and it still doesn't permit one to distiniguish 
between spaces and tabs.

Finally, the token handling seems to get in trouble with
   print *, "Hello &
       &world!"
where a single string extends over two lines.

Taking everything together, I think using libcpp for reading Fortran 
files is a topic for GCC 6. Any suggestion how to properly handle the 
spacing?

For the lexing itself, one probably needs a Fortran mode, which can 
recognize Fortran comments, continuation lines and some special 
properties about Fortran string. When implemented, one could then also 
turn of the traditional mode.

Tobias

* Disclaimer: I started with Fortran 95 and a rather object-oriented 
code. Hence, I might have gotten the fine print of the Fortran history 
wrong.

On 27 November 2014 at 20:28, Tobias Burnus <burnus@net-b.de> wrote:
> I think the approach is fine. As the _now version overrides the buffer, one
> might even do with a single buffer by clearing it, setting flush_p
> temporarily to true and printing the message. It only might collide with
> buffered warnings (for error_now) and errors (for warning_now), but I don't
> see whether that's an issue. For warnings/error_now it probably isn't, for
> errors/warning_now, it might. Thus, having two buffers is probably better.

Oh, I didn't notice that the _now versions override the buffered
messages. Where do you see that? It seems that gfc_warning_1 saves and
restores buffer_flag but does not touch the buffers (it prints
directly to the stream).

However, as you say, I may still need two buffers, one for errors and
other for warnings. (If this were not necessary, it would be great! Is
it really?).

In fact, we will need more buffers, since there is gfc_push/pop_error.
This is why I didn't start with gfc_error, it is a bit more complex
than gfc_warning.

>> The ugliest part is how to handle warningcount and werrorcount. I
>> could handle this in the common machinery in a better way by storing
>> DK_WERROR in the diagnostic->kind and checking it after printing.
>
>
> I'm missing the "but". Does it require more modifications in common code and
> thus you shy away? Or is there another reason?

I was going to write "Exactly" and a long explanation. But now I
realize that in this particular case I can simply check DK_ERROR,
since within gfc_warning that can only come from a warning converted
to an error. That simplifies the function a bit:

+/* Issue a warning.  */
+
+bool
+gfc_warning_1 (int opt, const char *gmsgid, ...)
+{
+  va_list argp;
+  diagnostic_info diagnostic;
+  bool fatal_errors = global_dc->fatal_errors;
+  pretty_printer *pp = global_dc->printer;
+  output_buffer *tmp_buffer = pp->buffer;
+
+  if (!pp_warning_buffer.flush_p)
+    {
+      /* To prevent -fmax-errors= triggering.  */
+      --werrorcount;
+      pp->buffer = &pp_warning_buffer;
+      global_dc->fatal_errors = false;
+    }
+
+  va_start (argp, gmsgid);
+  diagnostic_set_info (&diagnostic, gmsgid, &argp, UNKNOWN_LOCATION,
+                      DK_WARNING);
+  diagnostic.option_index = opt;
+  bool ret = report_diagnostic (&diagnostic);
+
+  if (ret && !pp_warning_buffer.flush_p)
+    {
+      warningcount_buffered = 0;
+      werrorcount_buffered = 0;
+      /* Undo the above --werrorcount if not Werror, otherwise werrorcount is
+        correct already.  */
+      diagnostic.kind == DK_ERROR
+       ? (++werrorcount_buffered)
+       : (++werrorcount, --warningcount, ++warningcount_buffered);
+
+      pp->buffer = tmp_buffer;
+      global_dc->fatal_errors = fatal_errors;
+    }
+
+  va_end (argp);
+  return ret;
+}


> Well, for nearly every Fortran program, at least for one line multiple
> attempts have to be done by the parser. Thus, if you set a break point in
> gfc_error, you will see "syntax error" messages which never make it to the
> screen. As the test suite checks for excess errors, nearly every test-suite
> file tests this.

Sure, but I would like to test specifically triggering and discarding
the gfc_warning that I converted (or another single one that you
suggest), if this were possible.

> Some of those can probably be simply converted, others either need to
> remain, or one has to setup a proper location (currently, using
> gfc_warning_now would ICE), or one creates a work around and constructs
> manually the error string (at least for the fprintf cases).

Why does it ICE? At worst it should give a wrong location, but the
computation of the offset is fairly similar to what Fortran already
does. Could  you give me a patch+testcase that ICEs?

> Taking everything together, I think using libcpp for reading Fortran files
> is a topic for GCC 6. Any suggestion how to properly handle the spacing?

No idea. My knowledge of cpp is limited. I think you need to ask
Dodji, Tom or Joseph (in IRC or a new thread). I was hoping it was
going to be easier to lex Fortran with cpp since cpp can already
preprocess it.

Thanks for the comment. They are very helpful.

Cheers,

Manuel.

On 27 November 2014 at 22:33, Manuel López-Ibáñez <lopezibanez@gmail.com> wrote:
> On 27 November 2014 at 20:28, Tobias Burnus <burnus@net-b.de> wrote:
>> I think the approach is fine. As the _now version overrides the buffer, one
>> might even do with a single buffer by clearing it, setting flush_p
>> temporarily to true and printing the message. It only might collide with
>> buffered warnings (for error_now) and errors (for warning_now), but I don't
>> see whether that's an issue. For warnings/error_now it probably isn't, for
>> errors/warning_now, it might. Thus, having two buffers is probably better.
>
> Oh, I didn't notice that the _now versions override the buffered
> messages. Where do you see that? It seems that gfc_warning_1 saves and
> restores buffer_flag but does not touch the buffers (it prints
> directly to the stream).

I meant gfc_warning_now_1. However, gfc_error_now_1 does reset
error_buffer (and sets it as curr_buffer, why if without buffering it
is not used?) . Hum, if this is really necessary, I'm afraid I just
introduced a subtle bug, since the new gfc_error_now does not reset
error_buffer, thus, potentially it may give an error that should have
been discarded. Tobias, what do you think?

Manuel López-Ibáñez <lopezibanez@gmail.com> writes:

> * Dodji: Do the common diagnostics part look reasonable?

Yes they do.

I just have one minor comment nit:

[...]

> Index: gcc/pretty-print.h

[...]

> +
> +  /* Nonzero means that text should be flushed when
> +     appropriate. Otherwise, text is buffered until either
> +     pp_pp_really_flush or pp_clear_output_area are called. */

I think you wanted to say pp_really_flush, not pp_pp_realy_flush.

> +  bool flush_p;
>  };

[...]

Thanks!