| Message ID | CAKwh3qi8rsBrgUTEGGDzbWAdC3S+8VpdySb8Qp6kF7szbgWt-A@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
Dear Janus, On Mon, Nov 7, 2011 at 12:14 AM, Janus Weil <janus@gcc.gnu.org> wrote: > The patch actually consists of two parts: > 1) The resolve.c part prevents the conversion to a PPC call via the > _vptr (for functions and subroutines). This is obviously OK > 2) The class.c parts prevents adding the non-overridable TBP to the vtable. > > As noted by Tobias, the second part breaks the ABI, so we might > consider deferring it until other ABI-breaking features will be > implemented (cf. http://gcc.gnu.org/wiki/LibgfortranAbiCleanup). On > the other hand, one could argue that the OOP ABI is still quite young > and hasn't really stabilized yet (it was broken already from 4.5 to > 4.6), so we might as well break it again. I know that there are a > couple of real-world codes out there, which make use of gfortran's OOP > features already, but I have a hard time estimating how many such > projects exists, or how problematic an ABI breaking would be for them > (user input welcome). Do we need to exclude it from the _vtable? I have to confess that, although I tried, I could not think of any reason to exclude it. On the other hand, I could not see any great harm in retaining a pointer, albeit a redundant one. > > So, the question is: Should I commit both parts, or only the resolve.c > one for now? The patch was regtested on x86_64-unknown-linux-gnu. In spite of the above remark, I think that you should commit both parts. Perhaps, until just before 4.7 release, a warning should be triggered that says that pre-existing code containing non-overridable procedures, should be recompiled before linking? OK for trunk. Thanks for the patch Paul
Hi Paul, >> The patch actually consists of two parts: >> 1) The resolve.c part prevents the conversion to a PPC call via the >> _vptr (for functions and subroutines). > > This is obviously OK thanks for the review. >> 2) The class.c parts prevents adding the non-overridable TBP to the vtable. >> >> As noted by Tobias, the second part breaks the ABI, so we might >> consider deferring it until other ABI-breaking features will be >> implemented (cf. http://gcc.gnu.org/wiki/LibgfortranAbiCleanup). On >> the other hand, one could argue that the OOP ABI is still quite young >> and hasn't really stabilized yet (it was broken already from 4.5 to >> 4.6), so we might as well break it again. I know that there are a >> couple of real-world codes out there, which make use of gfortran's OOP >> features already, but I have a hard time estimating how many such >> projects exists, or how problematic an ABI breaking would be for them >> (user input welcome). > > Do we need to exclude it from the _vtable? I have to confess that, > although I tried, I could not think of any reason to exclude it. On > the other hand, I could not see any great harm in retaining a pointer, > albeit a redundant one. Well, no, there is no hard reason why we *need* to exclude it. The point is that, given we call these non-overriable procedure in a "non-vptr" way, there is no need to have them in the vtable either ;) .... except for "historical" / ABI-compatibility reasons. >> So, the question is: Should I commit both parts, or only the resolve.c >> one for now? The patch was regtested on x86_64-unknown-linux-gnu. > > In spite of the above remark, I think that you should commit both > parts. I would in principle also prefer to commit both, but we need to be aware of the risks. In fact I'm not sure whether it is so easy to produce wrong behavior at all, combining code parts that were compiled with different gfortran versions (4.6/4.7). Since the vtabs are supposed to be 'global', usually there should be only one version of them generated in the main program (at least that is how it's supposed to be, but probably that does not work in *every* situation yet). I will try to see if I can find with a failing code example. (The issue is of course that if the vtabs change their contents between 4.6 and 4.7, a type-bound call could end up calling the wrong procedure). Moreover, I have the feeling that NON_OVERRIDABLE is not so widely used "in the wild" (but that is just a feeling, of course). > Perhaps, until just before 4.7 release, a warning should be > triggered that says that pre-existing code containing non-overridable > procedures, should be recompiled before linking? Do you mean the warning should be given on the trunk now, and later taken out of the release? Why not the other way around? > OK for trunk. > > Thanks for the patch Thanks. I will wait a bit before committing to give others a chance to comment. Cheers, Janus
On Mon, Nov 7, 2011 at 10:50, Janus Weil <janus@gcc.gnu.org> wrote: >>> 2) The class.c parts prevents adding the non-overridable TBP to the vtable. >>> >>> As noted by Tobias, the second part breaks the ABI, so we might >>> consider deferring it until other ABI-breaking features will be >>> implemented (cf. http://gcc.gnu.org/wiki/LibgfortranAbiCleanup). On >>> the other hand, one could argue that the OOP ABI is still quite young >>> and hasn't really stabilized yet (it was broken already from 4.5 to >>> 4.6), so we might as well break it again. I know that there are a >>> couple of real-world codes out there, which make use of gfortran's OOP >>> features already, but I have a hard time estimating how many such >>> projects exists, or how problematic an ABI breaking would be for them >>> (user input welcome). >> >> Do we need to exclude it from the _vtable? I have to confess that, >> although I tried, I could not think of any reason to exclude it. On >> the other hand, I could not see any great harm in retaining a pointer, >> albeit a redundant one. > > Well, no, there is no hard reason why we *need* to exclude it. The > point is that, given we call these non-overriable procedure in a > "non-vptr" way, there is no need to have them in the vtable either ;) > .... except for "historical" / ABI-compatibility reasons. > > >>> So, the question is: Should I commit both parts, or only the resolve.c >>> one for now? The patch was regtested on x86_64-unknown-linux-gnu. >> >> In spite of the above remark, I think that you should commit both >> parts. > > I would in principle also prefer to commit both, but we need to be > aware of the risks. > > In fact I'm not sure whether it is so easy to produce wrong behavior > at all, combining code parts that were compiled with different > gfortran versions (4.6/4.7). Since the vtabs are supposed to be > 'global', usually there should be only one version of them generated > in the main program (at least that is how it's supposed to be, but > probably that does not work in *every* situation yet). I will try to > see if I can find with a failing code example. (The issue is of course > that if the vtabs change their contents between 4.6 and 4.7, a > type-bound call could end up calling the wrong procedure). > > Moreover, I have the feeling that NON_OVERRIDABLE is not so widely > used "in the wild" (but that is just a feeling, of course). Well, we have no stable module ABI either, so I wouldn't worry about this. And, as you say, OOP is still a quite new feature (compared to modules, at least!).
On 11/07/2011 09:50 AM, Janus Weil wrote: >>> As noted by Tobias, the second part breaks the ABI ... On >>> the other hand, one could argue that the OOP ABI is still quite young >>> and hasn't really stabilized yet (it was broken already from 4.5 to >>> 4.6), so we might as well break it again. I know that there are a >>> couple of real-world codes out there, which make use of gfortran's OOP >>> features already, but I have a hard time estimating how many such >>> projects exists, or how problematic an ABI breaking would be for them >>> (user input welcome). >> Do we need to exclude it from the _vtable? I have to confess that, >> although I tried, I could not think of any reason to exclude it. On >> the other hand, I could not see any great harm in retaining a pointer, >> albeit a redundant one. > Well, no, there is no hard reason why we *need* to exclude it. The > point is that, given we call these non-overriable procedure in a > "non-vptr" way, there is no need to have them in the vtable either ;) > .... except for "historical" / ABI-compatibility reasons. [...] > In fact I'm not sure whether it is so easy to produce wrong behavior > at all, combining code parts that were compiled with different > gfortran versions (4.6/4.7). Since the vtabs are supposed to be > 'global', usually there should be only one version of them generated > in the main program (at least that is how it's supposed to be, but > probably that does not work in *every* situation yet). I will try to > see if I can find with a failing code example. (The issue is of course > that if the vtabs change their contents between 4.6 and 4.7, a > type-bound call could end up calling the wrong procedure). > > Moreover, I have the feeling that NON_OVERRIDABLE is not so widely > used "in the wild" (but that is just a feeling, of course). I concur that it is not that simple to produce a failing example. The only solid example where it really matters is a closed-source library which comes with a .f90 file which contains the module information, which is needed to to produce the .mod files. All other ideas are based on some careful recompilation and relinking, which avoids touching the .mod files, which is possible but either nontrivial or unlikely. Example: Recompiling the library and using it with an old program - for instance as shared library or by mere relinking. Note that the issue won't go away soon. We will have for years installations which have 4.6 and some which have 4.7 or later (e.g. with some OS update). Thus, one should be careful with the argument that there are not so many non_overridable programs in the wild as that might change in the next years. Though, it might take a while longer until the users realize that non_overridable provides a performance advantage. (By the way, PSBLAS does not include non_overridable.) -- Additionally, I *do* see a difference between GCC 4.5->4.6 and 4.6->4.7. There were many issues with OOP in 4.5, which made it difficult to use it. By contrast, OOP in 4.6 is usable - there are some issues (operators, polymorphic arrays, constructors, final) but they are not preventing its usage. Unfortunately, having a different vtable will not produce compile- or link-time failures but will often cause that the wrong TB procedure is started (possibly with all kind of argument-value/presence issues) or might segfault immediately. Thus, I believe it is rather difficult to run into the ABI issue in the real world (due to technical but also due to usage reasons), but it is possible. Hence, I wanted to point out that there is an ABI issue, but I also don't oppose the committal as it is difficult to encounter the issue - both in practise but also with an constructed example. Tobias
> Thus, I believe it is rather difficult to run into the ABI issue in the real > world (due to technical but also due to usage reasons), but it is possible. > Hence, I wanted to point out that there is an ABI issue, but I also don't > oppose the committal as it is difficult to encounter the issue - both in > practise but also with an constructed example. Thanks for the feedback, everyone. Since we all seem to agree more or less that the whole patch is ok for trunk, I'll commit tonight (if no further protests appear). Cheers, Janus
2011/11/7 Janus Weil <janus@gcc.gnu.org>: >> Thus, I believe it is rather difficult to run into the ABI issue in the real >> world (due to technical but also due to usage reasons), but it is possible. >> Hence, I wanted to point out that there is an ABI issue, but I also don't >> oppose the committal as it is difficult to encounter the issue - both in >> practise but also with an constructed example. > > Thanks for the feedback, everyone. Since we all seem to agree more or > less that the whole patch is ok for trunk, I'll commit tonight (if no > further protests appear). Committed as r181107. Cheers, Janus
Index: gcc/fortran/class.c =================================================================== --- gcc/fortran/class.c (revision 181043) +++ gcc/fortran/class.c (working copy) @@ -288,6 +288,10 @@ static void add_proc_comp (gfc_symbol *vtype, const char *name, gfc_typebound_proc *tb) { gfc_component *c; + + if (tb->non_overridable) + return; + c = gfc_find_component (vtype, name, true, true); if (c == NULL) Index: gcc/fortran/resolve.c =================================================================== --- gcc/fortran/resolve.c (revision 181044) +++ gcc/fortran/resolve.c (working copy) @@ -5868,11 +5868,13 @@ resolve_typebound_function (gfc_expr* e) const char *name; gfc_typespec ts; gfc_expr *expr; + bool overridable; st = e->symtree; /* Deal with typebound operators for CLASS objects. */ expr = e->value.compcall.base_object; + overridable = !e->value.compcall.tbp->non_overridable; if (expr && expr->ts.type == BT_CLASS && e->value.compcall.name) { /* Since the typebound operators are generic, we have to ensure @@ -5923,22 +5925,26 @@ resolve_typebound_function (gfc_expr* e) return FAILURE; ts = e->ts; - /* Then convert the expression to a procedure pointer component call. */ - e->value.function.esym = NULL; - e->symtree = st; + if (overridable) + { + /* Convert the expression to a procedure pointer component call. */ + e->value.function.esym = NULL; + e->symtree = st; - if (new_ref) - e->ref = new_ref; + if (new_ref) + e->ref = new_ref; - /* '_vptr' points to the vtab, which contains the procedure pointers. */ - gfc_add_vptr_component (e); - gfc_add_component_ref (e, name); + /* '_vptr' points to the vtab, which contains the procedure pointers. */ + gfc_add_vptr_component (e); + gfc_add_component_ref (e, name); - /* Recover the typespec for the expression. This is really only - necessary for generic procedures, where the additional call - to gfc_add_component_ref seems to throw the collection of the - correct typespec. */ - e->ts = ts; + /* Recover the typespec for the expression. This is really only + necessary for generic procedures, where the additional call + to gfc_add_component_ref seems to throw the collection of the + correct typespec. */ + e->ts = ts; + } + return SUCCESS; } @@ -5957,11 +5963,13 @@ resolve_typebound_subroutine (gfc_code *code) const char *name; gfc_typespec ts; gfc_expr *expr; + bool overridable; st = code->expr1->symtree; /* Deal with typebound operators for CLASS objects. */ expr = code->expr1->value.compcall.base_object; + overridable = !code->expr1->value.compcall.tbp->non_overridable; if (expr && expr->ts.type == BT_CLASS && code->expr1->value.compcall.name) { /* Since the typebound operators are generic, we have to ensure @@ -6006,22 +6014,26 @@ resolve_typebound_subroutine (gfc_code *code) return FAILURE; ts = code->expr1->ts; - /* Then convert the expression to a procedure pointer component call. */ - code->expr1->value.function.esym = NULL; - code->expr1->symtree = st; + if (overridable) + { + /* Convert the expression to a procedure pointer component call. */ + code->expr1->value.function.esym = NULL; + code->expr1->symtree = st; - if (new_ref) - code->expr1->ref = new_ref; + if (new_ref) + code->expr1->ref = new_ref; - /* '_vptr' points to the vtab, which contains the procedure pointers. */ - gfc_add_vptr_component (code->expr1); - gfc_add_component_ref (code->expr1, name); + /* '_vptr' points to the vtab, which contains the procedure pointers. */ + gfc_add_vptr_component (code->expr1); + gfc_add_component_ref (code->expr1, name); - /* Recover the typespec for the expression. This is really only - necessary for generic procedures, where the additional call - to gfc_add_component_ref seems to throw the collection of the - correct typespec. */ - code->expr1->ts = ts; + /* Recover the typespec for the expression. This is really only + necessary for generic procedures, where the additional call + to gfc_add_component_ref seems to throw the collection of the + correct typespec. */ + code->expr1->ts = ts; + } + return SUCCESS; }