From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 57683 invoked by alias); 5 Apr 2015 09:13:20 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 57658 invoked by uid 89); 5 Apr 2015 09:13:18 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=0.0 required=5.0 tests=AWL,BAYES_20,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 X-Spam-User: qpsmtpd, 2 recipients X-HELO: mail-wi0-f177.google.com Received: from mail-wi0-f177.google.com (HELO mail-wi0-f177.google.com) (209.85.212.177) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Sun, 05 Apr 2015 09:13:08 +0000 Received: by wizk4 with SMTP id k4so7019387wiz.1; Sun, 05 Apr 2015 02:13:05 -0700 (PDT) MIME-Version: 1.0 X-Received: by 10.194.95.4 with SMTP id dg4mr20433538wjb.81.1428225185482; Sun, 05 Apr 2015 02:13:05 -0700 (PDT) Received: by 10.180.77.5 with HTTP; Sun, 5 Apr 2015 02:13:05 -0700 (PDT) In-Reply-To: References: <20150226181717.480e282c@vepi2> <551006FF.1080704@sfr.fr> <20150323134357.6af740d1@vepi2> <20150324180620.3c72960e@vepi2> Date: Sun, 05 Apr 2015 09:13:00 -0000 Message-ID: Subject: Re: [Patch, Fortran, pr60322] was: [Patch 1/2, Fortran, pr60322] [OOP] Incorrect bounds on polymorphic dummy array From: Paul Richard Thomas To: Andre Vehreschild Cc: Mikael Morin , GCC-Fortran-ML , GCC-Patches-ML , Antony Lewis , Dominique Dhumieres Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable X-SW-Source: 2015-04/txt/msg00163.txt.bz2 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, bec= ause + 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 =3D 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 !=3D NULL && decl !=3D sym->backend_decl) + DECL_ARTIFICIAL (sym->backend_decl) =3D 1; + sym->backend_decl =3D 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 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 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, becau= se >> keeping both in sync was no benefit but only tedious and did not prove t= o 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 down= load. >> 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. Give= n 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 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 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 =E9crit : >>> >> > 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 =3D gfc_find_symtree (sym->ns->sym_root, sym->nam= e); >>> >> > >>> >> > /* It will always be a full array. */ >>> >> > - lval->rank =3D sym->as ? sym->as->rank : 0; >>> >> > + as =3D sym->as; >>> >> > + lval->rank =3D as ? as->rank : 0; >>> >> > if (lval->rank) >>> >> > - gfc_add_full_array_ref (lval, sym->ts.type =3D=3D 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 !=3D >>> >> 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 =3D=3D BT_CLASS or not. Sorry for the mista= ke. >>> > >>> >> > 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 * s= ym, >>> >> > 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 =3D=3D AS_ASSUMED_RANK)) >>> >> > + /* Use the array as and attr. */ >>> >> > + as =3D sym->as; >>> >> > + array_attr =3D &sym->attr; >>> >> > + >>> >> > + /* The pointer attribute is always set on a _data component, th= erefore >>> >> > check >>> >> > + the sym's attribute only. */ >>> >> > + if (sym->attr.pointer || array_attr->allocatable >>> >> > + || (as && as->type =3D=3D 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 int= ended, >>> > 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 >>> > To: Andre Vehreschild , Dominique Dhumieres >>> > 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=3D and so I substituted= SOURCE=3D. >>> > >>> > 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 =3D 99.0) >>> > allocate (a(2), source =3D [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=3D"memcpy") result(re= s) >>> > 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=3De) >>> > else >>> > allocate(tmp(size(a)),source=3Da) >>> > deallocate(a) >>> > allocate(a(size(tmp)+1),source=3De) ! mold gives a segfault >>> > dummy =3D memcpy(loc(a(1)),loc(tmp),sizeof(tmp)) >>> > dummy =3D 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 --=20 Outside of a dog, a book is a man's best friend. Inside of a dog it's too dark to read. Groucho Marx