PR fortran/78500 -- Yet another NULL pointer dereference

Regression tested on x86_64-*-freebsd.  OK to commit?

2016-11-23  Steven G. Kargl  <kargl@gcc.gnu.org>

	PR fortran/78500
	* expr.c (gfc_check_vardef_contextm): YANPD
	* interface.c (matching_typebound_op): Ditto.

2016-11-23  Steven G. Kargl  <kargl@gcc.gnu.org>

	PR fortran/78500
	* gfortran.dg/pr78500.f90: New test.

Hi Steve,

> Regression tested on x86_64-*-freebsd.  OK to commit?

the patch is certainly ok.

Just my usual request for a meaningful test-case name: This one could
be class_result_4.f90. (I don't want to be pedantic about this, but in
a growing testsuite of 4000+ files, I really think it makes sense to
have meaningful names. It does have various advantages and, to be
honest, I don't see any disadvantage).

Thanks for the patch,
Janus



> 2016-11-23  Steven G. Kargl  <kargl@gcc.gnu.org>
>
>         PR fortran/78500
>         * expr.c (gfc_check_vardef_contextm): YANPD
>         * interface.c (matching_typebound_op): Ditto.
>
> 2016-11-23  Steven G. Kargl  <kargl@gcc.gnu.org>
>
>         PR fortran/78500
>         * gfortran.dg/pr78500.f90: New test.
>
>
> Index: gcc/fortran/expr.c
> ===================================================================
> --- gcc/fortran/expr.c  (revision 242789)
> +++ gcc/fortran/expr.c  (working copy)
> @@ -5291,7 +5291,8 @@ gfc_check_vardef_context (gfc_expr* e, b
>       component.  Note that (normal) assignment to procedure pointers is not
>       possible.  */
>    check_intentin = !own_scope;
> -  ptr_component = (sym->ts.type == BT_CLASS && CLASS_DATA (sym))
> +  ptr_component = (sym->ts.type == BT_CLASS && sym->ts.u.derived
> +                  && CLASS_DATA (sym))
>                   ? CLASS_DATA (sym)->attr.class_pointer : sym->attr.pointer;
>    for (ref = e->ref; ref && check_intentin; ref = ref->next)
>      {
> Index: gcc/fortran/interface.c
> ===================================================================
> --- gcc/fortran/interface.c     (revision 242789)
> +++ gcc/fortran/interface.c     (working copy)
> @@ -3885,7 +3885,7 @@ matching_typebound_op (gfc_expr** tb_bas
>
>         if (base->expr->ts.type == BT_CLASS)
>           {
> -           if (CLASS_DATA (base->expr) == NULL
> +           if (!base->expr->ts.u.derived || CLASS_DATA (base->expr) == NULL
>                 || !gfc_expr_attr (base->expr).class_ok)
>               continue;
>             derived = CLASS_DATA (base->expr)->ts.u.derived;
> Index: gcc/testsuite/gfortran.dg/pr78500.f90
> ===================================================================
> --- gcc/testsuite/gfortran.dg/pr78500.f90       (nonexistent)
> +++ gcc/testsuite/gfortran.dg/pr78500.f90       (working copy)
> @@ -0,0 +1,5 @@
> +! { dg-do compile }
> +class(t) function f() ! { dg-error "must be dummy, allocatable or pointer" }
> +   f = 1              ! { dg-error "variable must not be polymorphic" }
> +end
> +
>
> --
> Steve

On Thu, 24 Nov 2016 11:56:02 +0100
Janus Weil <janus@gcc.gnu.org> wrote:

> Hi Steve,
> 
> > Regression tested on x86_64-*-freebsd.  OK to commit?  
> 
> the patch is certainly ok.
> 
> Just my usual request for a meaningful test-case name: This one could
> be class_result_4.f90. (I don't want to be pedantic about this, but in
> a growing testsuite of 4000+ files, I really think it makes sense to
> have meaningful names. It does have various advantages and, to be
> honest, I don't see any disadvantage).

Seconded!

- Andre
> Thanks for the patch,
> Janus
> 
> 
> 
> > 2016-11-23  Steven G. Kargl  <kargl@gcc.gnu.org>
> >
> >         PR fortran/78500
> >         * expr.c (gfc_check_vardef_contextm): YANPD
> >         * interface.c (matching_typebound_op): Ditto.
> >
> > 2016-11-23  Steven G. Kargl  <kargl@gcc.gnu.org>
> >
> >         PR fortran/78500
> >         * gfortran.dg/pr78500.f90: New test.
> >
> >
> > Index: gcc/fortran/expr.c
> > ===================================================================
> > --- gcc/fortran/expr.c  (revision 242789)
> > +++ gcc/fortran/expr.c  (working copy)
> > @@ -5291,7 +5291,8 @@ gfc_check_vardef_context (gfc_expr* e, b
> >       component.  Note that (normal) assignment to procedure pointers is not
> >       possible.  */
> >    check_intentin = !own_scope;
> > -  ptr_component = (sym->ts.type == BT_CLASS && CLASS_DATA (sym))
> > +  ptr_component = (sym->ts.type == BT_CLASS && sym->ts.u.derived
> > +                  && CLASS_DATA (sym))
> >                   ? CLASS_DATA (sym)->attr.class_pointer :
> > sym->attr.pointer; for (ref = e->ref; ref && check_intentin; ref =
> > ref->next) {
> > Index: gcc/fortran/interface.c
> > ===================================================================
> > --- gcc/fortran/interface.c     (revision 242789)
> > +++ gcc/fortran/interface.c     (working copy)
> > @@ -3885,7 +3885,7 @@ matching_typebound_op (gfc_expr** tb_bas
> >
> >         if (base->expr->ts.type == BT_CLASS)
> >           {
> > -           if (CLASS_DATA (base->expr) == NULL
> > +           if (!base->expr->ts.u.derived || CLASS_DATA (base->expr) == NULL
> >                 || !gfc_expr_attr (base->expr).class_ok)
> >               continue;
> >             derived = CLASS_DATA (base->expr)->ts.u.derived;
> > Index: gcc/testsuite/gfortran.dg/pr78500.f90
> > ===================================================================
> > --- gcc/testsuite/gfortran.dg/pr78500.f90       (nonexistent)
> > +++ gcc/testsuite/gfortran.dg/pr78500.f90       (working copy)
> > @@ -0,0 +1,5 @@
> > +! { dg-do compile }
> > +class(t) function f() ! { dg-error "must be dummy, allocatable or
> > pointer" }
> > +   f = 1              ! { dg-error "variable must not be polymorphic" }
> > +end
> > +
> >
> > --
> > Steve

On Thu, Nov 24, 2016 at 11:56:02AM +0100, Janus Weil wrote:
> Hi Steve,
> 
> > Regression tested on x86_64-*-freebsd.  OK to commit?
> 
> the patch is certainly ok.
> 
> Just my usual request for a meaningful test-case name: This one could
> be class_result_4.f90. (I don't want to be pedantic about this, but in
> a growing testsuite of 4000+ files, I really think it makes sense to
> have meaningful names. It does have various advantages and, to be
> honest, I don't see any disadvantage).
> 

Well, the main disadvantage is thinking up a suitable file name.
I would have never arrived at class_result_4.f90 given that the
fixes are in gfc_check_vardef_context() and matching_typebound_op().
Neither suggest to me that I'm dealing with a class_result.  I 
also have no idea how to make the invalid code valid as I don't
use CLASS or any Fortran OOP features, so creating a name based on
what may have been intended is beyond me.

The only benefit that I see is that during development one can
run regression testing on a subset of the testcases via
"make RUNTESTFLAGS='dg.exp=\*class\*.f90'" where filenames matched
by the regex pattern are tested.  But, eventually one needs to run
a patch against the entire collection of tests.

Hi Steve,

>> Just my usual request for a meaningful test-case name: This one could
>> be class_result_4.f90. (I don't want to be pedantic about this, but in
>> a growing testsuite of 4000+ files, I really think it makes sense to
>> have meaningful names. It does have various advantages and, to be
>> honest, I don't see any disadvantage).
>>
>
> Well, the main disadvantage is thinking up a suitable file name.
> I would have never arrived at class_result_4.f90 given that the
> fixes are in gfc_check_vardef_context() and matching_typebound_op().
> Neither suggest to me that I'm dealing with a class_result.

well, but obviously the test case does. It defines a FUNCTION that
returns a CLASS, which led me straightforward to the name
class_result_*, but I guess anything like class_* would be fine as
well (since the problem in this specific PR was mainly due to wrong
usage of a CLASS variable, as indicated by the repeated use of
BT_CLASS and CLASS_DATA in the patch).


> The only benefit that I see is that during development one can
> run regression testing on a subset of the testcases via
> "make RUNTESTFLAGS='dg.exp=\*class\*.f90'" where filenames matched
> by the regex pattern are tested.

Yes, that is one important benefit. Another is that if you break
something, you immediately get an idea what sort of stuff you break.

In general the use of meaningful names simply helps with organization
and overview. Having a folder with 4000 files that all have arbitrary
(and very similar names) like pr12345.f90 just isn't very nice. It
also provokes typos (swapped digits etc).

If you are looking for the test case that belongs to a certain PR
number, you can still grep for that number (if it is included as a
comment in the test case) or look in bugzilla which should show the
commit and the corresponding files.

I'm not trying to force you to do this, just trying to explain why I
think meaningful names are useful. But it's not a hell of a problem
and if you insist to keep the name you proposed, please go ahead and
commit.

Cheers,
Janus