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

[Paul Richard Thomas <paul dot richard dot thomas at gmail dot com>]

Hi Andre,

The delta patch is OK for trunk and eventual backport to 5.2.

Thanks for all the hard work

Paul

On 14 April 2015 at 19:00, Andre Vehreschild <vehre@gmx.de> wrote:
> Hi all,
>
> during further testing of a big Fortran software I encounter two bugs with
> class arrays, that are somehow connected to pr60322. I therefore propose an
> extended patch for pr60322. Because Paul has already reviewed most the extended
> patch, I give you two patches:
>
> 1. a full patch, fixing all the issues connected to pr60322, and
> 2. a delta patch to get from the reviewed patch to the latest version.
>
> With the second patch I hope to get a faster review, because it is
> significantly shorter.
>
> Now what was the issue? To be precise there were two issues:
>
> i. a pointer to a class array (CLASS_DATA(sym).attr.class_pointer == 1) was
> dereferenced, which lead to an ICE (the patch for this in the delta is chunk 5
> in gfc_conv_expr_descriptor, and
>
> ii. (and this was a severe brain cracker) in chains of references consisting of
> more then one class-(array)-ref always the _vptr of the first symbol was taken
> and not the _vptr of the currently dereferenced class object. This occurred
> when fortran code similiar to this was executed:
>
> type innerT
>   integer, allocatable :: arr(:)
> end type
>
> type T
>   class(innerT) :: mat(:,:)
> end type
>
> class(T) :: o
>
> allocate(o%mat(2,2))
> allocate(o%mat(:,:)%arr(10)) ! This is obviously pseudo code,
>         ! but I think you get what is meant.
>
> o%mat(1,1)%arr(1) = 1
>
> In the last line the address to get to arr(1) was computed using the
> _vptr->size of o and not of o%mat(1,1). To fix this gfc_component_ref () now
> computes the class' _vptr-ref whenever it does a _data-ref (chunk 1 of
> trans-expr.c in the delta patch). The _vptr-ref is stored in gfc_se, where I
> added the new member class_vptr. The gfc_se->class_vptr is then used in
> array-refs (chunk 2 of trans.c) to get the size of the array elements of the
> correct level.
>
> The other chunks of the delta patch are:
> - parameter passing fixes, and
> - documentation fixes as requested for the version 5 of the pr60322 patch.
>
> I hope this helps in getting the patch reviewed quickly.
>
> Bootstraps and regtests ok on x86_64-linux-gnu/F21.
>
> Ok for trunk -> 6.0?
> Ok, for backport to 5.2, once available?
>
> Note, the patches may apply with shifts, as I forgot to update before taking
> the diffs.
>
> Regards,
>         Andre
>
> On Thu, 9 Apr 2015 14:37:09 +0200
> Andre Vehreschild <vehre@gmx.de> wrote:
>
>> 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



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

Groucho Marx