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

[Andre Vehreschild <vehre at gmx dot de>]

Hi Paul, hi all,

Paul, thanks for the review. Answers to your questions are inline below:

On Sun, 5 Apr 2015 11:13:05 +0200
Paul Richard Thomas <paul.richard.thomas@gmail.com> wrote:
<snip>
> +  /* 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?

The first sentence is about regular arrays and should be unchanged from the
original source. Then I have to check for class (arrays) that are pointers,
i.e., independent of whether the sym is a class array or a regular pointer to a
class object. (The latter shouldn't make it into the routine anyway.)
IS_CLASS_ARRAY () returns false for too many reasons to be of use here. I have
to apologize and confess that the comment was a mere note to myself to not
return to use is_classarray in the if below. Let me rephrase the comment to be:

/* The dummy is returned for pointer, allocatable or assumed rank arrays.
   For class arrays the information if sym is an allocatable or pointer
   object needs to be checked explicitly (IS_CLASS_ARRAY can be false for
   too many reasons to be of use here).  */

> +      /* 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.

Er, the document at 

https://gcc.gnu.org/codingconventions.html#C_Formatting 

says that line length is 80, or is there another convention, that I am not
aware of?

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

Fixed.

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

I won't anymore, already got told off :-)

> 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 :-)

I initially tried to split the submission in two parts to make it more
manageable. One part with the brain-dead substitutions of as and array_attr and
one with the new code. Albeit I failed to get the brain-dead part right and
made some mistakes there already, which Mikael pointed out. I therefore went
for the big submission. 

Now doing a description of what each "chunk" does is quite tedious. I really
would like to spend my time more productive. Would you be satisfied, when I
write a story about the patch, referring to some parts more explicitly, like

"Chunk 4 of file trans-stmt.c is the heart of the patch and does this and that.
The remaining chunks are more or less putting the data together."

(This is not correct for this patch of course. Just an example.) More elaborate
of course, but just to give an idea.

Thanks again. I will commit as soon as 5.2/6.0 commit window is open.

Regards,
	Andre

> 
> 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
> 
> 
> 


-- 
Andre Vehreschild * Email: vehre ad gmx dot de