Patchwork [Fortran,OOP] PR 46448: [4.6 Regression] symbol `__copy_...' is already defined

login
register
mail settings
Submitter Janus Weil
Date Jan. 3, 2011, 10:48 p.m.
Message ID <AANLkTinty78SSUAn_x=Ds4cO0DQJB3Ym57sFW_dhkXT+@mail.gmail.com>
Download mbox | patch
Permalink /patch/77352/
State New
Headers show

Comments

Janus Weil - Jan. 3, 2011, 10:48 p.m.
Ok, since no one has dared yet to criticize my patch, I'll start with
some self-criticism:

The patch as posted does fix the test case in comment #0 of the PR,
but only if it comes in a single file (as in class_34.f90). Otherwise,
if I put the modules/main program into separate files and compile them
via

gfortran-4.6 m0.f90 m1.f90 m2.f90 p.f90

I still get:

/tmp/ccWGdkZv.o: In function `__copy_m0_t_':
m2.f90:(.text+0x0): multiple definition of `__copy_m0_t_'
/tmp/cc6cc0qj.o:m1.f90:(.text+0x6d): first defined here

Fortunately there is a workaround for this: It is cured by using
-fwhole-program. However, we probably do not want OOP codes to rely on
non-default switches like -fwhole-program.

One simple alternative that just came to my mind: The whole problem
would go away if the __copy_ routine would undergo the same name
mangling as all standard module procedures, i.e. getting a prefix like
"__m1_" or "__m2_". When looking for the reason, I came up with the
attached two-line patch, which seems to fix the issue for both the
single-file as well as multi-file setup, and is much simpler than the
previous approach (not regtested yet).

Cheers,
Janus




2011/1/3 Janus Weil <janus@gcc.gnu.org>:
> Hi all,
>
> here is a patch to fix a recent OOP regression. It avoids duplication
> of the '__copy_...' routines (which are used for polymorphic
> allocation with source) by adding global symbols for those routines.
>
> The patch also introduces a new function called 'gfc_add_gsymbol',
> which is used to add symbols to gfortran's list of global symbols
> (gfc_gsym_root). It replaces various other functions like
> 'add_global_entry', 'add_global_procedure' and 'add_global_program',
> which all did basically the same thing. The new function also includes
> an 'ns' argument. This is needed for the __copy_ routines, since these
> do go into the gfc_current_ns namespace.
>
> The patch was regtested on x86_64-unknown-linux-gnu. Ok for trunk?
>
> Cheers,
> Janus
>
>
> 2011-01-03  Janus Weil  <janus@gcc.gnu.org>
>
>        PR fortran/46448
>        * gfortran.h (gfc_add_gsymbol): New prototype.
>        * class.c (gfc_find_derived_vtab): Add a global symbol for the copying
>        routine to avoid duplication.
>        * decl.c (add_global_entry): Removed.
>        (gfc_match_entry): Use 'gfc_add_gsymbol' instead of 'add_global_entry'.
>        * parse.c (add_global_procedure,add_global_program): Removed.
>        (parse_block_data,parse_module,gfc_parse_file): Use 'gfc_add_gsymbol'.
>        * symbol.c (gfc_add_gsymbol): New function for adding global symbols,
>        replacing 'add_global_entry', 'add_global_procedure' and
>        'add_global_program'.
>
>
> 2011-01-03  Janus Weil  <janus@gcc.gnu.org>
>
>        PR fortran/46448
>        * gfortran.dg/class_34.f90: New.
>
Thomas Koenig - Jan. 4, 2011, 8:27 a.m.
Hi Janus,

> One simple alternative that just came to my mind: The whole problem
> would go away if the __copy_ routine would undergo the same name
> mangling as all standard module procedures, i.e. getting a prefix like
> "__m1_" or "__m2_". When looking for the reason, I came up with the
> attached two-line patch, which seems to fix the issue for both the
> single-file as well as multi-file setup, and is much simpler than the
> previous approach (not regtested yet).

this approach looks very good.

OK if it passes regtesting.

Thanks for the patch!

	Thomas
Janus Weil - Jan. 4, 2011, 10:44 a.m.
>> One simple alternative that just came to my mind: The whole problem
>> would go away if the __copy_ routine would undergo the same name
>> mangling as all standard module procedures, i.e. getting a prefix like
>> "__m1_" or "__m2_". When looking for the reason, I came up with the
>> attached two-line patch, which seems to fix the issue for both the
>> single-file as well as multi-file setup, and is much simpler than the
>> previous approach (not regtested yet).
>
> this approach looks very good.
>
> OK if it passes regtesting.

Thanks for the review, Thomas. Regtest passed without failures.

I will wait for feedback from Salvatore (who promised to try the patch
on his code) before committing.

Cheers,
Janus
Janus Weil - Jan. 4, 2011, 1:12 p.m.
>>> One simple alternative that just came to my mind: The whole problem
>>> would go away if the __copy_ routine would undergo the same name
>>> mangling as all standard module procedures, i.e. getting a prefix like
>>> "__m1_" or "__m2_". When looking for the reason, I came up with the
>>> attached two-line patch, which seems to fix the issue for both the
>>> single-file as well as multi-file setup, and is much simpler than the
>>> previous approach (not regtested yet).
>>
>> this approach looks very good.
>>
>> OK if it passes regtesting.
>
> Thanks for the review, Thomas. Regtest passed without failures.
>
> I will wait for feedback from Salvatore (who promised to try the patch
> on his code) before committing.

As this was apparently successful, I just committed the patch as r168464.

As a consequence, the OOP regression count has basically dropped to
zero (expect for one, which is not a real regression*).

Cheers,
Janus


* PR46262: Technically it *is* a regression, since it gives an ICE
which was not there for 4.5. However, 4.5 generated wrong code for the
test case, which means it was never *really* working.
Jerry DeLisle - Jan. 4, 2011, 2:36 p.m.
On 01/04/2011 05:12 AM, Janus Weil wrote:
>>>> One simple alternative that just came to my mind: The whole problem
>>>> would go away if the __copy_ routine would undergo the same name
>>>> mangling as all standard module procedures, i.e. getting a prefix like
>>>> "__m1_" or "__m2_". When looking for the reason, I came up with the
>>>> attached two-line patch, which seems to fix the issue for both the
>>>> single-file as well as multi-file setup, and is much simpler than the
>>>> previous approach (not regtested yet).
>>>
>>> this approach looks very good.
>>>
>>> OK if it passes regtesting.
>>
>> Thanks for the review, Thomas. Regtest passed without failures.
>>
>> I will wait for feedback from Salvatore (who promised to try the patch
>> on his code) before committing.
>
> As this was apparently successful, I just committed the patch as r168464.
>
> As a consequence, the OOP regression count has basically dropped to
> zero (expect for one, which is not a real regression*).
>

Thanks Janus!

Keep up the good work!

Thanks to everyone for keeping at it!

Jerry

Patch

Index: gcc/fortran/class.c
===================================================================
--- gcc/fortran/class.c	(revision 168424)
+++ gcc/fortran/class.c	(working copy)
@@ -528,6 +528,8 @@  gfc_find_derived_vtab (gfc_symbol *derived)
 		  sub_ns->proc_name = copy;
 		  copy->attr.flavor = FL_PROCEDURE;
 		  copy->attr.if_source = IFSRC_DECL;
+		  if (ns->proc_name->attr.flavor == FL_MODULE)
+		    copy->module = ns->proc_name->name;
 		  gfc_set_sym_referenced (copy);
 		  /* Set up formal arguments.  */
 		  gfc_get_symbol ("src", sub_ns, &src);