Fortran: fix CLASS attribute handling [PR106856]

  Dear all,

the attached patch fixes a long-standing issue with CLASS attributes
when a declaration is scattered over multiple statements.

The major part ("draft") of the patch is by Tobias, which I took up
before it started to bit-rot too much, see PR.  It is mainly about
a proper updating and book-keeping of symbol attributes.

While debugging the draft patch, I fixed a few disturbing memleaks
in class.cc that showed up when looking at intermediate fallout.

This patch also addresses issues reported in a few other PRs:
pr53951, pr101101, pr104229, pr107380.  These are mostly
duplicates at some level.

Regtested on x86_64-pc-linux-gnu.  OK for mainline?

Thanks,
Harald
  

On Thu, Mar 02, 2023 at 11:03:48PM +0100, Harald Anlauf via Fortran wrote:
> -  if (attr->class_ok)
> -    /* Class container has already been built.  */
> +  /* Class container has already been built with same name.  */
> +  if (attr->class_ok
> +      && ts->u.derived->components->attr.dimension >= attr->dimension
> +      && ts->u.derived->components->attr.codimension >= attr->codimension
> +      && ts->u.derived->components->attr.class_pointer >= attr->pointer
> +      && ts->u.derived->components->attr.allocatable >= attr->allocatable)

I suppose I'm a bit confused here.  dimension, codimension, 
pointer and allocatable are 1-bit bitfields in the attr
struct.  These can have the values 0 and 1, so the above
conditionals are always true.

The rest of the patch looks reasonable.  If Tobias has no 
objections or comments, it's ok to commit once the above
is explained.
  

Hi Steve,

Am 03.03.23 um 20:57 schrieb Steve Kargl via Gcc-patches:
> On Thu, Mar 02, 2023 at 11:03:48PM +0100, Harald Anlauf via Fortran wrote:
>> -  if (attr->class_ok)
>> -    /* Class container has already been built.  */
>> +  /* Class container has already been built with same name.  */
>> +  if (attr->class_ok
>> +      && ts->u.derived->components->attr.dimension >= attr->dimension
>> +      && ts->u.derived->components->attr.codimension >= attr->codimension
>> +      && ts->u.derived->components->attr.class_pointer >= attr->pointer
>> +      && ts->u.derived->components->attr.allocatable >= attr->allocatable)
> 
> I suppose I'm a bit confused here.  dimension, codimension,
> pointer and allocatable are 1-bit bitfields in the attr
> struct.  These can have the values 0 and 1, so the above
> conditionals are always true.

thanks for looking into it.

The above part is from the original draft.  I thought I could
generate testcases that allow to exercise this part, and found
a new case that is not covered by the patch and still ICEs:

subroutine bar (x)
   class(*)    :: x
   dimension   :: x(:)
   allocatable :: x
end

:-(

We'll need to revisit the logic...

> The rest of the patch looks reasonable.  If Tobias has no
> objections or comments, it's ok to commit once the above
> is explained.
> 

Thanks,
Harald
  

Hello,

Le 03/03/2023 à 20:57, Steve Kargl via Fortran a écrit :
> On Thu, Mar 02, 2023 at 11:03:48PM +0100, Harald Anlauf via Fortran wrote:
>> -  if (attr->class_ok)
>> -    /* Class container has already been built.  */
>> +  /* Class container has already been built with same name.  */
>> +  if (attr->class_ok
>> +      && ts->u.derived->components->attr.dimension >= attr->dimension
>> +      && ts->u.derived->components->attr.codimension >= attr->codimension
>> +      && ts->u.derived->components->attr.class_pointer >= attr->pointer
>> +      && ts->u.derived->components->attr.allocatable >= attr->allocatable)
> 
> I suppose I'm a bit confused here.  dimension, codimension,
> pointer and allocatable are 1-bit bitfields in the attr
> struct.  These can have the values 0 and 1, so the above
> conditionals are always true.
> 
as I understand it, they aren't if attr has attributes that aren't 
already set in the class container's first component.
a >= b == !(a < b) and if a and b are boolean-valued, a < b == !a && b.
Admittedly, I haven't tested the logic like Harald has.

> The rest of the patch looks reasonable.  If Tobias has no
> objections or comments, it's ok to commit once the above
> is explained.
> 

I have two comments, one about the handling of as and sym->as, which I 
quite don't understand, but I haven't had time to write something about it.
The other is about this:
> +  else if (sym->ts.type == BT_CLASS
> +	   && sym->ts.u.derived->attr.is_class
> +	   && sym->old_symbol && sym->old_symbol->as == CLASS_DATA (sym)->as)
> +    sym->old_symbol->as = NULL;
Can this be avoided?  The management of symbol versions should not need 
any manual change.  In principle, either the modified symbols are 
committed, or (in case of error) the previous symbols are restored, but 
there shouldn't be any need for restoring a modified previous symbol.

I guess it's a matter of memory management, because 
gfc_build_class_symbol copies the AS pointer to the class descriptor, 
but I think using gfc_copy_array_spec there or adding the condition 
above to free_old_symbol would be preferable.
  

On Fri, Mar 03, 2023 at 10:24:07PM +0100, Mikael Morin wrote:
> Hello,
> 
> Le 03/03/2023 à 20:57, Steve Kargl via Fortran a écrit :
> > On Thu, Mar 02, 2023 at 11:03:48PM +0100, Harald Anlauf via Fortran wrote:
> > > -  if (attr->class_ok)
> > > -    /* Class container has already been built.  */
> > > +  /* Class container has already been built with same name.  */
> > > +  if (attr->class_ok
> > > +      && ts->u.derived->components->attr.dimension >= attr->dimension
> > > +      && ts->u.derived->components->attr.codimension >= attr->codimension
> > > +      && ts->u.derived->components->attr.class_pointer >= attr->pointer
> > > +      && ts->u.derived->components->attr.allocatable >= attr->allocatable)
> > 
> > I suppose I'm a bit confused here.  dimension, codimension,
> > pointer and allocatable are 1-bit bitfields in the attr
> > struct.  These can have the values 0 and 1, so the above
> > conditionals are always true.
> > 
> as I understand it, they aren't if attr has attributes that aren't already
> set in the class container's first component.
> a >= b == !(a < b) and if a and b are boolean-valued, a < b == !a && b.
> Admittedly, I haven't tested the logic like Harald has.
> 

Mikael, thanks for smacking me with the clue stick.  I had to do a quick
test to see the trees.

% cc -o z a.c && ./z
a.i = 0,  b.i = 0, a.i >= b.i = 1
a.i = 1,  b.i = 0, a.i >= b.i = 1
a.i = 1,  b.i = 1, a.i >= b.i = 1
a.i = 0,  b.i = 1, a.i >= b.i = 0

I was overlooking the last case.  So, the above is an all
or nothing test.
  

Le 03/03/2023 à 22:24, Mikael Morin a écrit :
> 
> I have two comments, one about the handling of as and sym->as, which I 
> quite don't understand, but I haven't had time to write something about it.
I have found the time finally.  It's not as bad as it seemed.  See below.

> diff --git a/gcc/fortran/decl.cc b/gcc/fortran/decl.cc
> index eec0314cf4c..72d8c6f1c14 100644
> --- a/gcc/fortran/decl.cc
> +++ b/gcc/fortran/decl.cc
> @@ -8740,45 +8740,23 @@ attr_decl1 (void)
>  	}
>      }
> 
> -  /* Update symbol table.  DIMENSION attribute is set in
> -     gfc_set_array_spec().  For CLASS variables, this must be applied
> -     to the first component, or '_data' field.  */
>    if (sym->ts.type == BT_CLASS
>        && sym->ts.u.derived
>        && sym->ts.u.derived->attr.is_class)
>      {
> -      /* gfc_set_array_spec sets sym->attr not CLASS_DATA(sym)->attr.  Check
> -	 for duplicate attribute here.  */
> -      if (CLASS_DATA(sym)->attr.dimension == 1 && as)
> -	{
> -	  gfc_error ("Duplicate DIMENSION attribute at %C");
> -	  m = MATCH_ERROR;
> -	  goto cleanup;
> -	}
> -
> -      if (!gfc_copy_attr (&CLASS_DATA(sym)->attr, &current_attr, &var_locus))
> -	{
> -	  m = MATCH_ERROR;
> -	  goto cleanup;
> -	}
> +      sym->attr.pointer = CLASS_DATA(sym)->attr.class_pointer;
> +      sym->attr.allocatable = CLASS_DATA(sym)->attr.allocatable;
> +      sym->attr.dimension = CLASS_DATA(sym)->attr.dimension;
> +      sym->attr.codimension = CLASS_DATA(sym)->attr.codimension;
> +      if (as && CLASS_DATA (sym)->as)
> +	sym->as = gfc_copy_array_spec (CLASS_DATA (sym)->as);

Here the condition on 'CLASS_DATA(sym)->as' makes obviously sense, but I 
don't see why there is also a condition on 'as'.

For example, if the array spec has been previously set on the class 
container's first component, and there is no array spec information in 
the current statement (i.e. as == NULL), sym->as will remain NULL, and a 
non-array class container will be built in gfc_build_class_symbol below.

>      }
> -  else
> -    {
> -      if (current_attr.dimension == 0 && current_attr.codimension == 0
> -	  && !gfc_copy_attr (&sym->attr, &current_attr, &var_locus))
> -	{
> -	  m = MATCH_ERROR;
> -	  goto cleanup;
> -	}
> -    }
> -
> -  if (sym->ts.type == BT_CLASS
> -      && !gfc_build_class_symbol (&sym->ts, &sym->attr, &sym->as))
> +  if (current_attr.dimension == 0 && current_attr.codimension == 0
> +      && !gfc_copy_attr (&sym->attr, &current_attr, &var_locus))
>      {
>        m = MATCH_ERROR;
>        goto cleanup;
>      }
> -
>    if (!gfc_set_array_spec (sym, as, &var_locus))
>      {
>        m = MATCH_ERROR;
> @@ -8807,6 +8785,27 @@ attr_decl1 (void)
>        goto cleanup;
>      }
> 
> +  if (sym->ts.type == BT_CLASS && sym->ts.u.derived->attr.is_class
> +      && !as && !current_attr.pointer && !current_attr.allocatable
> +      && !current_attr.external)
> +    {
> +      sym->attr.pointer = 0;
> +      sym->attr.allocatable = 0;
> +      sym->attr.dimension = 0;
> +      sym->attr.codimension = 0;

> +      gfc_free_array_spec (sym->as);
sym->as should probably be reset to NULL here.
Maybe both calls to gfc_free_array_spec here and to gfc_copy_array_spec 
above can be avoided by doing a simple pointer copy?

> +    }
> +  else if (sym->ts.type == BT_CLASS
> +      && !gfc_build_class_symbol (&sym->ts, &sym->attr, &sym->as))
> +    {
> +      m = MATCH_ERROR;
> +      goto cleanup;
> +    }
> +  else if (sym->ts.type == BT_CLASS
> +	   && sym->ts.u.derived->attr.is_class
> +	   && sym->old_symbol && sym->old_symbol->as == CLASS_DATA (sym)->as)
> +    sym->old_symbol->as = NULL;
> +
>    add_hidden_procptr_result (sym);
> 
>    return MATCH_YES;
  

Hi Mikael!

Am 04.03.23 um 14:56 schrieb Mikael Morin:
> I have found the time finally.  It's not as bad as it seemed.  See below.
> 
>> diff --git a/gcc/fortran/decl.cc b/gcc/fortran/decl.cc
>> index eec0314cf4c..72d8c6f1c14 100644
>> --- a/gcc/fortran/decl.cc
>> +++ b/gcc/fortran/decl.cc

>> +      sym->attr.pointer = CLASS_DATA(sym)->attr.class_pointer;
>> +      sym->attr.allocatable = CLASS_DATA(sym)->attr.allocatable;
>> +      sym->attr.dimension = CLASS_DATA(sym)->attr.dimension;
>> +      sym->attr.codimension = CLASS_DATA(sym)->attr.codimension;
>> +      if (as && CLASS_DATA (sym)->as)
>> +    sym->as = gfc_copy_array_spec (CLASS_DATA (sym)->as);
> 
> Here the condition on 'CLASS_DATA(sym)->as' makes obviously sense, but I 
> don't see why there is also a condition on 'as'.
> 
> For example, if the array spec has been previously set on the class 
> container's first component, and there is no array spec information in 
> the current statement (i.e. as == NULL), sym->as will remain NULL, and a 
> non-array class container will be built in gfc_build_class_symbol below.

Very good catch!  Indeed, this fixes the testcase variations.


>> @@ -8807,6 +8785,27 @@ attr_decl1 (void)
>>        goto cleanup;
>>      }
>>
>> +  if (sym->ts.type == BT_CLASS && sym->ts.u.derived->attr.is_class
>> +      && !as && !current_attr.pointer && !current_attr.allocatable
>> +      && !current_attr.external)
>> +    {
>> +      sym->attr.pointer = 0;
>> +      sym->attr.allocatable = 0;
>> +      sym->attr.dimension = 0;
>> +      sym->attr.codimension = 0;
> 
>> +      gfc_free_array_spec (sym->as);
> sym->as should probably be reset to NULL here.

Done.

> Maybe both calls to gfc_free_array_spec here and to gfc_copy_array_spec 
> above can be avoided by doing a simple pointer copy?

I tried that, but this produced a crash with a double-free.

The attached revised version uses the above proven changes,
and extends the new testcase class_74.f90 by variations of
the failures remaining with version 1 so that different
codepaths are tested.

Regtested again on x86_64-pc-linux-gnu.

Any further comments?

Thanks for your very helpful review!

Harald
  

Hi Mikael!

Am 04.03.23 um 14:56 schrieb Mikael Morin:
> I have found the time finally.  It's not as bad as it seemed.  See below.
>
>> diff --git a/gcc/fortran/decl.cc b/gcc/fortran/decl.cc
>> index eec0314cf4c..72d8c6f1c14 100644
>> --- a/gcc/fortran/decl.cc
>> +++ b/gcc/fortran/decl.cc

>> +      sym->attr.pointer = CLASS_DATA(sym)->attr.class_pointer;
>> +      sym->attr.allocatable = CLASS_DATA(sym)->attr.allocatable;
>> +      sym->attr.dimension = CLASS_DATA(sym)->attr.dimension;
>> +      sym->attr.codimension = CLASS_DATA(sym)->attr.codimension;
>> +      if (as && CLASS_DATA (sym)->as)
>> +    sym->as = gfc_copy_array_spec (CLASS_DATA (sym)->as);
>
> Here the condition on 'CLASS_DATA(sym)->as' makes obviously sense, but I
> don't see why there is also a condition on 'as'.
>
> For example, if the array spec has been previously set on the class
> container's first component, and there is no array spec information in
> the current statement (i.e. as == NULL), sym->as will remain NULL, and a
> non-array class container will be built in gfc_build_class_symbol below.

Very good catch!  Indeed, this fixes the testcase variations.


>> @@ -8807,6 +8785,27 @@ attr_decl1 (void)
>>        goto cleanup;
>>      }
>>
>> +  if (sym->ts.type == BT_CLASS && sym->ts.u.derived->attr.is_class
>> +      && !as && !current_attr.pointer && !current_attr.allocatable
>> +      && !current_attr.external)
>> +    {
>> +      sym->attr.pointer = 0;
>> +      sym->attr.allocatable = 0;
>> +      sym->attr.dimension = 0;
>> +      sym->attr.codimension = 0;
>
>> +      gfc_free_array_spec (sym->as);
> sym->as should probably be reset to NULL here.

Done.

> Maybe both calls to gfc_free_array_spec here and to gfc_copy_array_spec
> above can be avoided by doing a simple pointer copy?

I tried that, but this produced a crash with a double-free.

The attached revised version uses the above proven changes,
and extends the new testcase class_74.f90 by variations of
the failures remaining with version 1 so that different
codepaths are tested.

Regtested again on x86_64-pc-linux-gnu.

Any further comments?

Thanks for your very helpful review!

Harald
  

Sorry, attached the wrong patch.

Here's the correct one.

Harald

Am 04.03.23 um 17:02 schrieb Harald Anlauf via Gcc-patches:

> The attached revised version uses the above proven changes,
> and extends the new testcase class_74.f90 by variations of
> the failures remaining with version 1 so that different
> codepaths are tested.
>
> Regtested again on x86_64-pc-linux-gnu.
>
> Any further comments?
>
> Thanks for your very helpful review!
>
> Harald
  

Le 04/03/2023 à 17:06, Harald Anlauf a écrit :
> Sorry, attached the wrong patch.
> 
> Here's the correct one.
> 
> Harald
> 
> Am 04.03.23 um 17:02 schrieb Harald Anlauf via Gcc-patches:
> 
>> The attached revised version uses the above proven changes,
>> and extends the new testcase class_74.f90 by variations of
>> the failures remaining with version 1 so that different
>> codepaths are tested.
>>
>> Regtested again on x86_64-pc-linux-gnu.
>>
>> Any further comments?
>>
There was a comment about the old_symbol thing at the end of my previous 
message:
https://gcc.gnu.org/pipermail/gcc-patches/2023-March/613354.html
  

Hi Mikael,

Am 04.03.23 um 18:09 schrieb Mikael Morin:
> There was a comment about the old_symbol thing at the end of my previous 
> message:
> https://gcc.gnu.org/pipermail/gcc-patches/2023-March/613354.html

I think Tobias might be the better person to answer this.
But when playing with variations of that else-branch,
I always hit an issue with class_74.f90, where the class
variables are not dummy arguments but local variables.

E.g. take the following reduced testcase:

subroutine foo
   class(*)  :: y
   dimension :: y(:,:)
   pointer   :: y
end subroutine foo

So when we see the dimension but haven't seen the
pointer (or allocatable) declaration, we appear to
generate an error with bad consequences (ICE).

If this is a resolution issue, maybe it can be fixed
differently, but likely needs digging deeper.  With
the patch as-is at least I do not see a memory leak
in that context.

Cheers,
Harald
  

Le 04/03/2023 à 22:20, Harald Anlauf a écrit :
> Hi Mikael,
> 
> Am 04.03.23 um 18:09 schrieb Mikael Morin:
>> There was a comment about the old_symbol thing at the end of my previous
>> message:
>> https://gcc.gnu.org/pipermail/gcc-patches/2023-March/613354.html
> 
> I think Tobias might be the better person to answer this.
> But when playing with variations of that else-branch,
> I always hit an issue with class_74.f90, where the class
> variables are not dummy arguments but local variables.
> 
> E.g. take the following reduced testcase:
> 
> subroutine foo
>    class(*)  :: y
>    dimension :: y(:,:)
>    pointer   :: y
> end subroutine foo
> 
> So when we see the dimension but haven't seen the
> pointer (or allocatable) declaration, we appear to
> generate an error with bad consequences (ICE).
> 
> If this is a resolution issue, maybe it can be fixed
> differently, but likely needs digging deeper.  With
> the patch as-is at least I do not see a memory leak
> in that context.
> 
One of my suggestions was to fix it as attached.
It is probably more clear with an actual patch to look at.
It seems to work on your example and class_74 as well.
It seems to also fix some valgrind errors on this example:
    subroutine foo
      pointer   :: y
      dimension :: y(:,:)
      class(*)  :: y
    end subroutine foo
I'm fine with that fix if it works for you.

I suggest waiting for next stage 1, but it's your call, you have the 
green light from Steve anyway.

Thanks for your work.