Patchwork [Fortran] PR 54107: [4.8 Regression] Memory hog with abstract interface

login
register
mail settings
Submitter Mikael Morin
Date Jan. 27, 2013, 8:16 p.m.
Message ID <51058B05.8030904@sfr.fr>
Download mbox | patch
Permalink /patch/216061/
State New
Headers show

Comments

Mikael Morin - Jan. 27, 2013, 8:16 p.m.
Hi Janus,

Le 27/01/2013 19:49, Janus Weil a écrit :
>
>   subroutine sub (arg)
>      procedure(sub) :: arg
>    end subroutine
>
You forgot to precise that this case (which is basically comment #4 in 
the PR) is *not* fixed by the patch, as it fails later on at translation 
stage.
I have made up my mind that it's not possible for the middle-end to 
build such a recursive type.  So `arg' will have to have a variadic 
function type.  No patch yet, sorry; I have just figured it out.


> Anyway, should we bump the mod version with this patch, or should we
> rather avoid it?
>
I forgot the reason why we are so reluctant to do it.  Module versions 
are not a rare resource.  I'm in favor of bumping (and any time we 
change module format).


About the patch, one nit:


The comment should probably be removed as well.


 > The patch was regtested on x86_64-unknown-linux-gnu. Ok for trunk?
 >
OK from my side;  you may or may not need someone else's ack as I'm the 
coauthor.
Or maybe wait for the fix for comment #4?

Mikael
Thomas Koenig - Jan. 27, 2013, 8:38 p.m.
Am 27.01.2013 21:16, schrieb Mikael Morin:

> Or maybe wait for the fix for comment #4?

I would rather commit the fixes now, just on general principles.
If any regression should occur, it would be easier to pinpoint
the reason.

	Thomas
Janus Weil - Jan. 27, 2013, 9:03 p.m.
Hi,

>>   subroutine sub (arg)
>>      procedure(sub) :: arg
>>    end subroutine
>>
> You forgot to precise that this case (which is basically comment #4 in the
> PR) is *not* fixed by the patch, as it fails later on at translation stage.

yes, I just silently ignored that fact.


> I have made up my mind that it's not possible for the middle-end to build
> such a recursive type.  So `arg' will have to have a variadic function type.
> No patch yet, sorry; I have just figured it out.

Don't worry. I agree with Thomas that this should preferably be done
in a follow-up patch. (The hard part is probably to detect all the
cases of 'recursive' definitions, also the indirect ones.)


>> Anyway, should we bump the mod version with this patch, or should we
>> rather avoid it?
>>
> I forgot the reason why we are so reluctant to do it.  Module versions are
> not a rare resource.  I'm in favor of bumping (and any time we change module
> format).

I'm also ok with a bump, but of course it would be preferable to have
a stable ABI and module format at some point. As a matter of fact, the
module format has changed in all recent releases, though (that's why
we introduced the module versioning, after all).



> About the patch, one nit:
>
> Index: gcc/fortran/gfortran.h
> ===================================================================
> --- gcc/fortran/gfortran.h      (revision 195493)
> +++ gcc/fortran/gfortran.h      (working copy)
> @@ -974,8 +974,6 @@ typedef struct gfc_component
>    struct gfc_component *next;
>
>    /* Needed for procedure pointer components.  */
> -  struct gfc_formal_arglist *formal;
> -  struct gfc_namespace *formal_ns;
>    struct gfc_typebound_proc *tb;
>  }
>  gfc_component;
>
> The comment should probably be removed as well.

Well, it still applies to 'tb' ...



>> The patch was regtested on x86_64-unknown-linux-gnu. Ok for trunk?
>>
> OK from my side;  you may or may not need someone else's ack as I'm the
> coauthor.

Certainly a second review/opinion could not hurt.

I'm attaching an updated patch, which includes the module-version bump now.


> Or maybe wait for the fix for comment #4?

Rather not (technically it's a separate issue, I guess).

Cheers,
Janus
Thomas Koenig - Jan. 28, 2013, 9:28 p.m.
Hi Janus,

>> Or maybe wait for the fix for comment #4?
> Rather not (technically it's a separate issue, I guess).

While the patch is rather large, I think it is OK.

One request:  Could you add a comment to gfc_sym_get_dummy_args
explaining what the function does and under which conditions sym->formal
is NULL, while sym->ts.interface->formal isn't?

Regards

	Thomas
Janus Weil - Jan. 29, 2013, 9:43 p.m.
Hi Thomas,

>>> Or maybe wait for the fix for comment #4?
>>
>> Rather not (technically it's a separate issue, I guess).
>
>
> While the patch is rather large, I think it is OK.
>
> One request:  Could you add a comment to gfc_sym_get_dummy_args
> explaining what the function does and under which conditions sym->formal
> is NULL, while sym->ts.interface->formal isn't?

thanks for the review. I have added the comment to
gfc_sym_get_dummy_args that you requested, and committed the patch as
r195562.

Cheers,
Janus

Patch

Index: gcc/fortran/gfortran.h
===================================================================
--- gcc/fortran/gfortran.h	(revision 195493)
+++ gcc/fortran/gfortran.h	(working copy)
@@ -974,8 +974,6 @@  typedef struct gfc_component
    struct gfc_component *next;

    /* Needed for procedure pointer components.  */
-  struct gfc_formal_arglist *formal;
-  struct gfc_namespace *formal_ns;
    struct gfc_typebound_proc *tb;
  }
  gfc_component;