Fix PR middle-end/46674

Submitted by Jie Zhang on Nov. 30, 2010, 11:57 p.m.

Details

Message ID 4CF58F86.1080305@codesourcery.com
State New
Headers show

Commit Message

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,

Comments

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 hide | download patch | download mbox


	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;
 	}
     }