[prev in list] [next in list] [prev in thread] [next in thread]
List: gcc-fortran
Subject: Re: [Patch, Fortran, pr60322] was: [Patch 1/2, Fortran, pr60322] [OOP] Incorrect bounds on polymorph
From: Paul Richard Thomas <paul.richard.thomas () gmail ! com>
Date: 2015-03-27 12:48:32
Message-ID: CAGkQGiL+rYEdDzw8RFEr4Abyu6HQV78x8XFCu5-xViMvZrmfFg () mail ! gmail ! com
[Download RAW message or body]
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
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic