Re: [Patch, Fortran, pr60322] was: [Patch 1/2, Fortran, pr60322] [OOP] Incorrect bounds on polymorphic dummy array

[Paul Richard Thomas <paul.richard.thomas@gmail.com>]

Dear Andre,

Well, time passed and it didn't get done. Too much going on at the moment!

As you say, the patch bootstraps and regtests on x86_64, FC21 in my case.

I am now very reluctant to mess around with the gcc-5 release. Thus, I
think that this patch must be committed to 5.2 and 6.0, when the are
open for business.

A few trivial comments:

+  /* The dummy is returned for pointer, allocatable or assumed rank arrays.
+     The check for pointerness needs to be repeated here (it is done in
+     IS_CLASS_ARRAY (), too), because for class arrays that are pointers, as
+     is the one of the sym, which is incorrect here.  */

What does this mean, please?

+      /* Returning the descriptor for dummy class arrays is hazardous, because
+     some caller is expecting an expression to apply the component refs to.
+     Therefore the descriptor is only created and stored in
+     sym->backend_decl's GFC_DECL_SAVED_DESCRIPTOR.  The caller is then
+     responsible to extract it from there, when the descriptor is
+     desired.  */
+      if (IS_CLASS_ARRAY (sym)
+      && (!DECL_LANG_SPECIFIC (sym->backend_decl)
+          || !GFC_DECL_SAVED_DESCRIPTOR (sym->backend_decl)))
+    {
+      decl = gfc_build_dummy_array_decl (sym, sym->backend_decl);
+      /* Prevent the dummy from being detected as unused if it is copied.  */
+      if (sym->backend_decl != NULL && decl != sym->backend_decl)
+        DECL_ARTIFICIAL (sym->backend_decl) = 1;
+      sym->backend_decl = decl;
+    }

The comments, such as the above are often going well beyond column 72,
into the 80's. I know that much of the existing code violates this
style requirement but there is no need to do so if clarity is not
reduced thereby.

In trans-stmt.c s/standart/standard/

Don't forget to put the PR numbers in the ChangeLogs.

For this submission, I would have appreciated some a description of
what each chunk in the patch is doing, just because there is so much
of it. I suppose that it was good for my imortal soul to sort it out
for myself but it took a little while :-)

Cheers and many thanks for the patch.

Paul

On 27 March 2015 at 13:48, Paul Richard Thomas
<paul.richard.thomas@gmail.com> wrote:
> Dear Andre,
>
> I am in the UK as of last night. Before leaving, I bootstrapped and
> regtested your patch and all was well. I must drive to Cambridge this
> afternoon to see my mother and will try to get to it either this
> evening or tomorrow morning. There is so much of it and it touches
> many places; so I must give it a very careful looking over before
> giving the green light. Bear with me please.
>
> Great work though!
>
> Paul
>
> On 24 March 2015 at 18:06, Andre Vehreschild <vehre@gmx.de> wrote:
>> Hi all,
>>
>> I have worked on the comments Mikael gave me. I am now checking for
>> class_pointer in the way he pointed out.
>>
>> Furthermore did I *join the two parts* of the patch into this one, because
>> keeping both in sync was no benefit but only tedious and did not prove to be
>> reviewed faster.
>>
>> Paul, Dominique: I have addressed the LOC issue that came up lately. Or rather
>> the patch addressed it already. I feel like this is not tested very well, not
>> the loc() call nor the sizeof() call as given in the 57305 second's download.
>> Unfortunately, is that download not runable. I would love to see a test similar
>> to that download, but couldn't come up with one, that satisfied me. Given that
>> the patch's review will last some days, I still have enough time to come up
>> with something beautiful which I will add then.
>>
>> Bootstraps and regtests ok on x86_64-linux-gnu/F20.
>>
>> Regards,
>>         Andre
>>
>>
>> On Tue, 24 Mar 2015 11:13:27 +0100
>> Paul Richard Thomas <paul.richard.thomas@gmail.com> wrote:
>>
>>> Dear Andre,
>>>
>>> Dominique pointed out to me that the 'loc' patch causes a ICE in the
>>> testsuite. It seems that 'loc' should provide the address of the class
>>> container in some places and the address of the data in others. I will
>>> put my thinking cap on tonight :-)
>>>
>>> Cheers
>>>
>>> Paul
>>>
>>> On 23 March 2015 at 13:43, Andre Vehreschild <vehre@gmx.de> wrote:
>>> > Hi Mikael,
>>> >
>>> > thanks for looking at the patch. Please note, that Paul has sent an
>>> > addendum to the patches for 60322, which I deliberately have attached.
>>> >
>>> >>  26/02/2015 18:17, Andre Vehreschild a écrit :
>>> >> > This first patch is only preparatory and does not change any of the
>>> >> > semantics of gfortran at all.
>>> >> Sure?
>>> >
>>> > With the counterexample you found below, this of course is a wrong
>>> > statement.
>>> >
>>> >> > diff --git a/gcc/fortran/expr.c b/gcc/fortran/expr.c
>>> >> > index ab6f7a5..d28cf77 100644
>>> >> > --- a/gcc/fortran/expr.c
>>> >> > +++ b/gcc/fortran/expr.c
>>> >> > @@ -4059,10 +4060,10 @@ gfc_lval_expr_from_sym (gfc_symbol *sym)
>>> >> >    lval->symtree = gfc_find_symtree (sym->ns->sym_root, sym->name);
>>> >> >
>>> >> >    /* It will always be a full array.  */
>>> >> > -  lval->rank = sym->as ? sym->as->rank : 0;
>>> >> > +  as = sym->as;
>>> >> > +  lval->rank = as ? as->rank : 0;
>>> >> >    if (lval->rank)
>>> >> > -    gfc_add_full_array_ref (lval, sym->ts.type == BT_CLASS ?
>>> >> > -                       CLASS_DATA (sym)->as : sym->as);
>>> >> > +    gfc_add_full_array_ref (lval, as);
>>> >>
>>> >> This is a change of semantics.  Or do you know that sym->ts.type !=
>>> >> BT_CLASS?
>>> >
>>> > You are completely right. I have made a mistake here. I have to tell the
>>> > truth, I never ran a regtest with only part 1 of the patches applied. The
>>> > second part of the patch will correct this, by setting the variable as
>>> > depending on whether type == BT_CLASS or not. Sorry for the mistake.
>>> >
>>> >> > diff --git a/gcc/fortran/trans-decl.c b/gcc/fortran/trans-decl.c
>>> >> > index 3664824..e571a17 100644
>>> >> > --- a/gcc/fortran/trans-decl.c
>>> >> > +++ b/gcc/fortran/trans-decl.c
>>> >> > @@ -1013,16 +1017,24 @@ gfc_build_dummy_array_decl (gfc_symbol * sym,
>>> >> > tree dummy) tree decl;
>>> >> >    tree type;
>>> >> >    gfc_array_spec *as;
>>> >> > +  symbol_attribute *array_attr;
>>> >> >    char *name;
>>> >> >    gfc_packed packed;
>>> >> >    int n;
>>> >> >    bool known_size;
>>> >> >
>>> >> > -  if (sym->attr.pointer || sym->attr.allocatable
>>> >> > -      || (sym->as && sym->as->type == AS_ASSUMED_RANK))
>>> >> > +  /* Use the array as and attr.  */
>>> >> > +  as = sym->as;
>>> >> > +  array_attr = &sym->attr;
>>> >> > +
>>> >> > +  /* The pointer attribute is always set on a _data component, therefore
>>> >> > check
>>> >> > +     the sym's attribute only.  */
>>> >> > +  if (sym->attr.pointer || array_attr->allocatable
>>> >> > +      || (as && as->type == AS_ASSUMED_RANK))
>>> >> >      return dummy;
>>> >> >
>>> >> Any reason to sometimes use array_attr, sometimes not, like here?
>>> >> By the way, the comment is misleading: for classes, there is the
>>> >> class_pointer attribute (and it is a pain, I know).
>>> >
>>> > Yes, and a good one. Array_attr is sometimes sym->attr and sometimes
>>> > CLASS_DATA(sym)->attr aka sym->ts.u.derived->components->attr. In the later
>>> > case .pointer is always set to 1 in the _data component's attr. I.e., the
>>> > above if, would always yield true for a class_array, which is not intended,
>>> > but rather destructive. I know about the class_pointer attribute, but I
>>> > figured, that it is not relevant here. Any idea how to formulate the
>>> > comment better, to reflect what I just explained?
>>> >
>>> > Regards,
>>> >         Andre
>>> > --
>>> > Andre Vehreschild * Email: vehre ad gmx dot de
>>> >
>>> >
>>> > ---------- Forwarded message ----------
>>> > From: Paul Richard Thomas <paul.richard.thomas@gmail.com>
>>> > To: Andre Vehreschild <vehre@gmx.de>, Dominique Dhumieres
>>> > <dominiq@lps.ens.fr> Cc:
>>> > Date: Sun, 22 Mar 2015 21:20:20 +0100
>>> > Subject: Bug in intrinsic LOC for scalar class objects
>>> > Dear Andre and Dominique,
>>> >
>>> > I have found that LOC is returning the address of the class container
>>> > rather than the _data component for class scalars. See the source
>>> > below, which you will recognise! A fix is attached.
>>> >
>>> > Note that the scalar allocate fails with MOLD= and so I substituted SOURCE=.
>>> >
>>> > Cheers
>>> >
>>> > Paul
>>> >
>>> >     class(*), allocatable :: a(:), e ! Change 'e' to an array and
>>> > second memcpy works correctly
>>> >                                      ! Problem is with loc(e), which
>>> > returns the address of the
>>> >                                      ! class container.
>>> >     allocate (e, source = 99.0)
>>> >     allocate (a(2), source = [1.0, 2.0])
>>> >     call add_element_poly (a,e)
>>> >     select type (a)
>>> >       type is (real)
>>> >         print *, a
>>> >     end select
>>> >
>>> > contains
>>> >
>>> >     subroutine add_element_poly(a,e)
>>> >       use iso_c_binding
>>> >       class(*),allocatable,intent(inout),target :: a(:)
>>> >       class(*),intent(in),target :: e
>>> >       class(*),allocatable,target :: tmp(:)
>>> >       type(c_ptr) :: dummy
>>> >
>>> >       interface
>>> >         function memcpy(dest,src,n) bind(C,name="memcpy") result(res)
>>> >           import
>>> >           type(c_ptr) :: res
>>> >           integer(c_intptr_t),value :: dest
>>> >           integer(c_intptr_t),value :: src
>>> >           integer(c_size_t),value :: n
>>> >         end function
>>> >       end interface
>>> >
>>> >       if (.not.allocated(a)) then
>>> >         allocate(a(1), source=e)
>>> >       else
>>> >         allocate(tmp(size(a)),source=a)
>>> >         deallocate(a)
>>> >         allocate(a(size(tmp)+1),source=e) ! mold gives a segfault
>>> >         dummy = memcpy(loc(a(1)),loc(tmp),sizeof(tmp))
>>> >         dummy = memcpy(loc(a(size(tmp)+1)),loc(e),sizeof(e))
>>> >       end if
>>> >     end subroutine
>>> > end
>>> >
>>>
>>>
>>>
>>
>>
>> --
>> Andre Vehreschild * Email: vehre ad gmx dot de
>
>
>
> --
> Outside of a dog, a book is a man's best friend. Inside of a dog it's
> too dark to read.
>
> Groucho Marx



-- 
Outside of a dog, a book is a man's best friend. Inside of a dog it's
too dark to read.

Groucho Marx