Patchwork Fix PR middle-end/46674

login
register
mail settings
Submitter Jie Zhang
Date Nov. 30, 2010, 11:57 p.m.
Message ID <4CF58F86.1080305@codesourcery.com>
Download mbox | patch
Permalink /patch/73682/
State New
Headers show

Comments

Jie Zhang - Nov. 30, 2010, 11:57 p.m.
PR 46674 is same as PR 46221 indeed. It happens when one alias has a 
user set name.

extern void jelly (void) __asm__ ("jelly2") __attribute__ ((alias 
("dessert"), weak));
extern void wobbly (void) __attribute__ ((alias ("jelly2"), weak));

When jelly was added to the visible point set, the identifier was 
"*jelly2". But later GCC looked for identifier "jelly2" when deciding if 
alias wobbly could be removed or not. Since "jelly2" was not in the 
visible point set, alias wobbly would be removed.

With this patch, the identifier added to the visible point set has the 
prefix '*' stripped if there is one.

Bootstrapped and regression tested on x86_64-linux-gnu with c,c++,lto.

OK?

Regards,
Richard Guenther - Dec. 1, 2010, 11:50 a.m.
On Wed, Dec 1, 2010 at 12:57 AM, Jie Zhang <jie@codesourcery.com> wrote:
> PR 46674 is same as PR 46221 indeed. It happens when one alias has a user
> set name.
>
> extern void jelly (void) __asm__ ("jelly2") __attribute__ ((alias
> ("dessert"), weak));
> extern void wobbly (void) __attribute__ ((alias ("jelly2"), weak));
>
> When jelly was added to the visible point set, the identifier was "*jelly2".
> But later GCC looked for identifier "jelly2" when deciding if alias wobbly
> could be removed or not. Since "jelly2" was not in the visible point set,
> alias wobbly would be removed.
>
> With this patch, the identifier added to the visible point set has the
> prefix '*' stripped if there is one.
>
> Bootstrapped and regression tested on x86_64-linux-gnu with c,c++,lto.
>
> OK?

Hm.  If a target prefixes an underscore does the alias attribute argument
need to include that underscore?  I'm somewhat worried about
consistency on those weird targets, but the patch is clearly a
progression.

It would be really nice if DECL_ASSEMBLER_NAME would already
contain all target mangling from the start (also for LTO which has
similar problems when doing symbol name merging).

Thus, ok.

Thanks,
Richard.

> Regards,
> --
> Jie Zhang
>
>
>
Jie Zhang - Dec. 2, 2010, 4:15 a.m.
On 12/01/2010 07:50 PM, Richard Guenther wrote:
> On Wed, Dec 1, 2010 at 12:57 AM, Jie Zhang<jie@codesourcery.com>  wrote:
>> PR 46674 is same as PR 46221 indeed. It happens when one alias has a user
>> set name.
>>
>> extern void jelly (void) __asm__ ("jelly2") __attribute__ ((alias
>> ("dessert"), weak));
>> extern void wobbly (void) __attribute__ ((alias ("jelly2"), weak));
>>
>> When jelly was added to the visible point set, the identifier was "*jelly2".
>> But later GCC looked for identifier "jelly2" when deciding if alias wobbly
>> could be removed or not. Since "jelly2" was not in the visible point set,
>> alias wobbly would be removed.
>>
>> With this patch, the identifier added to the visible point set has the
>> prefix '*' stripped if there is one.
>>
>> Bootstrapped and regression tested on x86_64-linux-gnu with c,c++,lto.
>>
>> OK?
>
> Hm.  If a target prefixes an underscore does the alias attribute argument
> need to include that underscore?  I'm somewhat worried about
> consistency on those weird targets, but the patch is clearly a
> progression.
>
> It would be really nice if DECL_ASSEMBLER_NAME would already
> contain all target mangling from the start (also for LTO which has
> similar problems when doing symbol name merging).
>
Yeah. At least it's not a good to use a prefix '*' to mark its name is 
set by user. I think it will be better to use a tree field to record 
this but keep the symbol name unchanged.

> Thus, ok.
>
Thanks. Committed on trunk.
Dave Korn - Dec. 5, 2010, 1:50 p.m.
On 01/12/2010 11:50, Richard Guenther wrote:

> Hm.  If a target prefixes an underscore does the alias attribute argument
> need to include that underscore?  

FAIL: gcc.dg/pr46674.c (test for excess errors)

  That answer your question?  ;-)  How about the attached?  It fixes the test
on i686-pc-cygwin, checked with both variants of f{no-,}leading-underscore
(and none).

gcc/testsuite/ChangeLog:

	* gcc.dg/pr46674.c (LABEL3): New macro definition.
	(LABEL2): Likewise.
	(LABEL): Likewise.
	(jelly): Account for user label prefix in asm name.

> It would be really nice if DECL_ASSEMBLER_NAME would already
> contain all target mangling from the start (also for LTO which has
> similar problems when doing symbol name merging).

  We're working towards that, as you'll have seen from the "Enable
-fuse-linker-plugin by default when possible, take 2" thread. :)


    cheers,
      DaveK
Richard Guenther - Dec. 5, 2010, 8:47 p.m.
On Sun, Dec 5, 2010 at 2:50 PM, Dave Korn <dave.korn.cygwin@gmail.com> wrote:
> On 01/12/2010 11:50, Richard Guenther wrote:
>
>> Hm.  If a target prefixes an underscore does the alias attribute argument
>> need to include that underscore?
>
> FAIL: gcc.dg/pr46674.c (test for excess errors)
>
>  That answer your question?  ;-)  How about the attached?  It fixes the test
> on i686-pc-cygwin, checked with both variants of f{no-,}leading-underscore
> (and none).

Ok ;)

Thanks,
Richard.

> gcc/testsuite/ChangeLog:
>
>        * gcc.dg/pr46674.c (LABEL3): New macro definition.
>        (LABEL2): Likewise.
>        (LABEL): Likewise.
>        (jelly): Account for user label prefix in asm name.
>
>> It would be really nice if DECL_ASSEMBLER_NAME would already
>> contain all target mangling from the start (also for LTO which has
>> similar problems when doing symbol name merging).
>
>  We're working towards that, as you'll have seen from the "Enable
> -fuse-linker-plugin by default when possible, take 2" thread. :)
>
>
>    cheers,
>      DaveK
>
Dave Korn - Dec. 6, 2010, 1:38 a.m.
On 05/12/2010 20:47, Richard Guenther wrote:
> On Sun, Dec 5, 2010 at 2:50 PM, Dave Korn <dave.korn.cygwin@gmail.com> wrote:
>> On 01/12/2010 11:50, Richard Guenther wrote:
>>
>>> Hm.  If a target prefixes an underscore does the alias attribute argument
>>> need to include that underscore?
>> FAIL: gcc.dg/pr46674.c (test for excess errors)
>>
>>  That answer your question?  ;-)  How about the attached?  It fixes the test
>> on i686-pc-cygwin, checked with both variants of f{no-,}leading-underscore
>> (and none).
> 
> Ok ;)

  Committed revision 167483.

    cheers,
      DaveK

Patch


	PR middle-end/46674
	* varasm.c (compute_visible_aliases): Handle user set
	assembler name.

	testsuite/
	PR middle-end/46674
	* gcc.dg/pr46674.c: New test.

Index: testsuite/gcc.dg/pr46674.c
===================================================================
--- testsuite/gcc.dg/pr46674.c	(revision 0)
+++ testsuite/gcc.dg/pr46674.c	(revision 0)
@@ -0,0 +1,11 @@ 
+/* { dg-do compile } */
+/* { dg-require-alias "" } */
+/* { dg-options "-O2" } */
+
+int yum;
+void dessert (void) { ++yum; }
+extern void jelly (void) __asm__ ("jelly2") __attribute__ ((alias ("dessert"), weak));
+extern void wobbly (void) __attribute__ ((alias ("jelly2"), weak));
+
+/* { dg-final { scan-assembler "wobbly" } } */
+/* { dg-final { scan-assembler "jelly2" } } */
Index: varasm.c
===================================================================
--- varasm.c	(revision 167217)
+++ varasm.c	(working copy)
@@ -5526,12 +5526,21 @@  compute_visible_aliases (void)
 	{
 	  struct cgraph_node *fnode = NULL;
 	  struct varpool_node *vnode = NULL;
+	  tree asmname = DECL_ASSEMBLER_NAME (p->decl);
+	  const char *str = IDENTIFIER_POINTER (asmname);
+
+	  if (str[0] == '*')
+	    {
+	      str ++;
+	      asmname = get_identifier (str);
+	    }
+
 	  fnode = cgraph_node_for_asm (p->target);
 	  vnode = (fnode == NULL) ? varpool_node_for_asm (p->target) : NULL;
 	  if ((fnode
 	       || vnode
 	       || pointer_set_contains (visible, p->target))
-	      && !pointer_set_insert (visible, DECL_ASSEMBLER_NAME (p->decl)))
+	      && !pointer_set_insert (visible, asmname))
 	    changed = true;
 	}
     }