RFD: improved diagnostics when reading module files

RFD: improved diagnostics when reading module files

[Harald Anlauf]

Dear all,

while chasing down an ICE that occurred when reading inconsistent
modules files, I found that it might be helpful to change an assert
in module.c:5164 into some error message.  Suggestion:

Index: gcc/fortran/module.c
===================================================================
--- gcc/fortran/module.c        (revision 259152)
+++ gcc/fortran/module.c        (working copy)
@@ -5161,7 +5161,13 @@
              if (p->u.pointer == NULL)
                associate_integer_pointer (p, c);
              mio_pool_string (&comp_name);
-             gcc_assert (comp_name == c->name);
+             if (comp_name != c->name)
+               {
+                 gfc_error_now ("While reading module files: mismatch in "
+                                "derived type components: %qs != %qs",
+                                comp_name, c->name);
+               }
+             /* gcc_assert (comp_name == c->name); */
              skip_list (1); /* component end.  */
            }
          mio_rparen (); /* component list closing.  */


Is this something worth considering?

Harald

Re: RFD: improved diagnostics when reading module files

[Steve Kargl]

On Fri, Apr 06, 2018 at 09:35:31PM +0200, Harald Anlauf wrote:
> 
> while chasing down an ICE that occurred when reading inconsistent
> modules files, I found that it might be helpful to change an assert
> in module.c:5164 into some error message.  Suggestion:
> 
> Index: gcc/fortran/module.c
> ===================================================================
> --- gcc/fortran/module.c        (revision 259152)
> +++ gcc/fortran/module.c        (working copy)
> @@ -5161,7 +5161,13 @@
>               if (p->u.pointer == NULL)
>                 associate_integer_pointer (p, c);
>               mio_pool_string (&comp_name);
> -             gcc_assert (comp_name == c->name);
> +             if (comp_name != c->name)
> +               {
> +                 gfc_error_now ("While reading module files: mismatch in "
> +                                "derived type components: %qs != %qs",
> +                                comp_name, c->name);

I would remove the "While reading module files:" and simply
start the error message with "Mismatch".  I haven't looked at
module.c, but if the module name or derived type is available
you might considering added that to the message.   If you
try to reference a locus with "at %C", do you get a caret
pointing at a "USE XXXX" statement?  This might also be helpful.

> +               }
> +             /* gcc_assert (comp_name == c->name); */

Delete the above line.


>               skip_list (1); /* component end.  */
>             }
>           mio_rparen (); /* component list closing.  */
> 
> 
> Is this something worth considering?
> 

IMHO, yes.  Anything that helps a user with her code is valuable.

-- 
Steve

Re: RFD: improved diagnostics when reading module files

[Harald Anlauf]

Steve,

I've tried to incorporate your suggestions.

On 04/06/18 21:58, Steve Kargl wrote:
> I would remove the "While reading module files:" and simply
> start the error message with "Mismatch".  I haven't looked at
> module.c, but if the module name or derived type is available
> you might considering added that to the message.   If you
> try to reference a locus with "at %C", do you get a caret
> pointing at a "USE XXXX" statement?  This might also be helpful.
>> +               }
>> +             /* gcc_assert (comp_name == c->name); */
> 
> Delete the above line.

With

Index: gcc/fortran/module.c
===================================================================
--- gcc/fortran/module.c        (revision 259189)
+++ gcc/fortran/module.c        (working copy)
@@ -5161,7 +5161,12 @@
              if (p->u.pointer == NULL)
                associate_integer_pointer (p, c);
              mio_pool_string (&comp_name);
-             gcc_assert (comp_name == c->name);
+             if (comp_name != c->name)
+               {
+                 gfc_error_now ("Mismatch in derived type components of "
+                                "%qs from %qs at %C: %qs != %qs",
+                                sym->name, sym->module, c->name,
comp_name);
+               }
              skip_list (1); /* component end.  */
            }
          mio_rparen (); /* component list closing.  */


I get some improved output.  I'm not really overwhelmed, but it is
definitely better from my point of view.  OTOH, removing the %C
makes the error message definitely less useful.

I haven't been able to produce a simple but sufficiently contorted
testcase.  But for the case I was debugging, where there was some
outdated module file holding the outdated layout of a derived type,
I was able to zgrep back to the culprit.

With the change above, I now get:

../../../../analysis/mo_ir_emis.f90:61:6:

   use mo_dec_matrix,  only: t_vector       ! decomposed vector
      1
Error: Mismatch in derived type components of 't_par_grib2' from
'mo_grib12_dwd' at (1): 'modnum' != 'shortname'
../../../../analysis/mo_ir_emis.f90:61:6: Error: Mismatch in derived
type components of 't_par_grib2' from 'mo_grib12_dwd' at (1):
'shortname' != 'longname'
f951: Fatal Error: Reading module 'mo_dec_matrix' at line 3072 column
53: Expected left parenthesis
compilation terminated.


That was useful enough for me, and I will continue using it.
If you think others may profit, please consider applying it.

Thanks,
Harald

Re: RFD: improved diagnostics when reading module files

[Steve Kargl]

On Fri, Apr 06, 2018 at 11:17:58PM +0200, Harald Anlauf wrote:
> On 04/06/18 21:58, Steve Kargl wrote:
> > I would remove the "While reading module files:" and simply
> > start the error message with "Mismatch".  I haven't looked at
> > module.c, but if the module name or derived type is available
> > you might considering added that to the message.   If you
> > try to reference a locus with "at %C", do you get a caret
> > pointing at a "USE XXXX" statement?  This might also be helpful.
> >> +               }
> >> +             /* gcc_assert (comp_name == c->name); */
> > 
> > Delete the above line.
> 
> With
> 
> Index: gcc/fortran/module.c
> ===================================================================
> --- gcc/fortran/module.c        (revision 259189)
> +++ gcc/fortran/module.c        (working copy)
> @@ -5161,7 +5161,12 @@
>               if (p->u.pointer == NULL)
>                 associate_integer_pointer (p, c);
>               mio_pool_string (&comp_name);
> -             gcc_assert (comp_name == c->name);
> +             if (comp_name != c->name)
> +               {
> +                 gfc_error_now ("Mismatch in derived type components of "
> +                                "%qs from %qs at %C: %qs != %qs",
> +                                sym->name, sym->module, c->name,
> comp_name);
> +               }
>               skip_list (1); /* component end.  */
>             }
>           mio_rparen (); /* component list closing.  */
> 
> 
> I get some improved output.  I'm not really overwhelmed, but it is
> definitely better from my point of view.  OTOH, removing the %C
> makes the error message definitely less useful.
> 
> I haven't been able to produce a simple but sufficiently contorted
> testcase.  But for the case I was debugging, where there was some
> outdated module file holding the outdated layout of a derived type,
> I was able to zgrep back to the culprit.
> 
> With the change above, I now get:
> 
> ../../../../analysis/mo_ir_emis.f90:61:6:
> 
>    use mo_dec_matrix,  only: t_vector       ! decomposed vector
>       1
> Error: Mismatch in derived type components of 't_par_grib2' from
> 'mo_grib12_dwd' at (1): 'modnum' != 'shortname'

Yes, I think it is useful.  An assert() gives an ICE, which is
not useful in this situation.  A slight better error message 
might be

Error: Mismatch in components of derived type 't_par_grib2' from
module 'mo_grib12_dwd' at (1) (expecting 'modnum', but got 'shortname')

You may want to let others way.

BTW, do you have commit privileges for the subversion repository?

-- 
Steve

Re: RFD: improved diagnostics when reading module files

[Harald Anlauf]

Steve,

On 04/07/18 01:59, Steve Kargl wrote:
> On Fri, Apr 06, 2018 at 11:17:58PM +0200, Harald Anlauf wrote:
>> With the change above, I now get:
>>
>> ../../../../analysis/mo_ir_emis.f90:61:6:
>>
>>    use mo_dec_matrix,  only: t_vector       ! decomposed vector
>>       1
>> Error: Mismatch in derived type components of 't_par_grib2' from
>> 'mo_grib12_dwd' at (1): 'modnum' != 'shortname'
> 
> Yes, I think it is useful.  An assert() gives an ICE, which is
> not useful in this situation.  A slight better error message 
> might be
> 
> Error: Mismatch in components of derived type 't_par_grib2' from
> module 'mo_grib12_dwd' at (1) (expecting 'modnum', but got 'shortname')
> 
> You may want to let others way.

I'm not sure what you meant here, but here's an improved version that
prints only one error on the first mismatch.  It uses gfc_fatal_error
instead of gfc_error_now and is probably less confusing:

Index: gcc/fortran/module.c
===================================================================
--- gcc/fortran/module.c        (revision 259189)
+++ gcc/fortran/module.c        (working copy)
@@ -5161,7 +5161,12 @@
              if (p->u.pointer == NULL)
                associate_integer_pointer (p, c);
              mio_pool_string (&comp_name);
-             gcc_assert (comp_name == c->name);
+             if (comp_name != c->name)
+               {
+                 gfc_fatal_error ("Mismatch in components of derived
type %qs "
+                                  "from %qs at %C: expecting %qs, but
got %qs",
+                                  sym->name, sym->module, c->name,
comp_name);
+               }
              skip_list (1); /* component end.  */
            }
          mio_rparen (); /* component list closing.  */


By playing bad with some sample modules, this produces now:

mod3.f90:3:6:

   use mod2
      1
Fatal Error: Mismatch in components of derived type 'x' from 'mod1' at
(1): expecting 'z', but got 'q'
compilation terminated.

which I think is probably good enough.

> BTW, do you have commit privileges for the subversion repository?

No.  But if you like, you could commit it e.g. with the following changelog:

2018-04-07  Harald Anlauf  <anlauf@gmx.de>,
	Steven G. Kargl  <kargl@gcc.gnu.org>

	* module.c (read_module): Replace assert by error message on
	mismatch of components of derived type.


Thanks,
Harald

Re: RFD: improved diagnostics when reading module files

[Steve Kargl]

On Sat, Apr 07, 2018 at 06:15:41PM +0200, Harald Anlauf wrote:
> Steve,
> 
> On 04/07/18 01:59, Steve Kargl wrote:
> > On Fri, Apr 06, 2018 at 11:17:58PM +0200, Harald Anlauf wrote:
> >> With the change above, I now get:
> >>
> >> ../../../../analysis/mo_ir_emis.f90:61:6:
> >>
> >>    use mo_dec_matrix,  only: t_vector       ! decomposed vector
> >>       1
> >> Error: Mismatch in derived type components of 't_par_grib2' from
> >> 'mo_grib12_dwd' at (1): 'modnum' != 'shortname'
> > 
> > Yes, I think it is useful.  An assert() gives an ICE, which is
> > not useful in this situation.  A slight better error message 
> > might be
> > 
> > Error: Mismatch in components of derived type 't_par_grib2' from
> > module 'mo_grib12_dwd' at (1) (expecting 'modnum', but got 'shortname')
> > 
> > You may want to let others way.

Whoops.  Multitasking at work.

"Let others weigh in."

> I'm not sure what you meant here, but here's an improved version that
> prints only one error on the first mismatch.  It uses gfc_fatal_error
> instead of gfc_error_now and is probably less confusing:
> 

(patch looks good to me)

>    use mod2
>       1
> Fatal Error: Mismatch in components of derived type 'x' from 'mod1' at
> (1): expecting 'z', but got 'q'
> compilation terminated.
> 
> which I think is probably good enough.

Agreed.  A user should be able to find to problem now.

> 
> > BTW, do you have commit privileges for the subversion repository?
> 
> No.  But if you like, you could commit it e.g. with the following changelog:
> 
> 2018-04-07  Harald Anlauf  <anlauf@gmx.de>,
> 	Steven G. Kargl  <kargl@gcc.gnu.org>
> 
> 	* module.c (read_module): Replace assert by error message on
> 	mismatch of components of derived type.

I'll commit it.  But, would like the ability to directly commit
your fixes?  (My not so subtle hint that we could use additional
help. :-)

-- 
Steve

Re: RFD: improved diagnostics when reading module files

[Harald Anlauf]

Since the deadline for gcc-8 is approaching, I've attached the
patch to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=85407
so that it doesn't get lost.

On 04/07/18 18:45, Steve Kargl wrote:
> On Sat, Apr 07, 2018 at 06:15:41PM +0200, Harald Anlauf wrote:
>> Steve,
>>
>> On 04/07/18 01:59, Steve Kargl wrote:
>>> On Fri, Apr 06, 2018 at 11:17:58PM +0200, Harald Anlauf wrote:
>>>> With the change above, I now get:
>>>>
>>>> ../../../../analysis/mo_ir_emis.f90:61:6:
>>>>
>>>>    use mo_dec_matrix,  only: t_vector       ! decomposed vector
>>>>       1
>>>> Error: Mismatch in derived type components of 't_par_grib2' from
>>>> 'mo_grib12_dwd' at (1): 'modnum' != 'shortname'
>>>
>>> Yes, I think it is useful.  An assert() gives an ICE, which is
>>> not useful in this situation.  A slight better error message 
>>> might be
>>>
>>> Error: Mismatch in components of derived type 't_par_grib2' from
>>> module 'mo_grib12_dwd' at (1) (expecting 'modnum', but got 'shortname')
>>>
>>> You may want to let others way.
> 
> Whoops.  Multitasking at work.
> 
> "Let others weigh in."
> 
>> I'm not sure what you meant here, but here's an improved version that
>> prints only one error on the first mismatch.  It uses gfc_fatal_error
>> instead of gfc_error_now and is probably less confusing:
>>
> 
> (patch looks good to me)
> 
>>    use mod2
>>       1
>> Fatal Error: Mismatch in components of derived type 'x' from 'mod1' at
>> (1): expecting 'z', but got 'q'
>> compilation terminated.
>>
>> which I think is probably good enough.
> 
> Agreed.  A user should be able to find to problem now.
> 
>>
>>> BTW, do you have commit privileges for the subversion repository?
>>
>> No.  But if you like, you could commit it e.g. with the following changelog:
>>
>> 2018-04-07  Harald Anlauf  <anlauf@gmx.de>,
>> 	Steven G. Kargl  <kargl@gcc.gnu.org>
>>
>> 	* module.c (read_module): Replace assert by error message on
>> 	mismatch of components of derived type.
> 
> I'll commit it.  But, would like the ability to directly commit
> your fixes?  (My not so subtle hint that we could use additional
> help. :-)
>