[03/25] Improve TARGET_MANGLE_DECL_ASSEMBLER_NAME.

Message ID 328b94cd7d15c4d2113a676a3a70ce640b37333a.1536144068.git.ams@codesourcery.com
State New
Headers show
Series
  • AMD GCN Port
Related show

Commit Message

Andrew Stubbs Sept. 5, 2018, 11:48 a.m.
The HSA GPU drivers can't cope with binaries that have the same symbol defined
multiple times, even though the names are not exported.  This happens whenever
there are file-scope static variables with matching names.  I believe it's also
an issue with switch tables.

This is a bug, but outside our control, so we must work around it when multiple
translation units have the same symbol defined.

Therefore, we've implemented name mangling via
TARGET_MANGLE_DECL_ASSEMBLER_NAME, but found some places where the middle-end
assumes that the decl name matches the name in the source.

This patch fixes up those cases by falling back to comparing the unmangled
name, when a lookup fails.

2018-09-05  Julian Brown  <julian@codesourcery.com>

	gcc/
	* cgraphunit.c (handle_alias_pairs): Scan for aliases by DECL_NAME if
	decl assembler name doesn't match.

	gcc/c-family/
	* c-pragma.c (maye_apply_pending_pragma_weaks): Scan for aliases with
	DECL_NAME if decl assembler name doesn't match.
---
 gcc/c-family/c-pragma.c | 14 ++++++++++++++
 gcc/cgraphunit.c        | 15 +++++++++++++++
 2 files changed, 29 insertions(+)

Comments

Jeff Law Sept. 11, 2018, 10:56 p.m. | #1
On 9/5/18 5:48 AM, ams@codesourcery.com wrote:
> 

> The HSA GPU drivers can't cope with binaries that have the same symbol defined

> multiple times, even though the names are not exported.  This happens whenever

> there are file-scope static variables with matching names.  I believe it's also

> an issue with switch tables.

> 

> This is a bug, but outside our control, so we must work around it when multiple

> translation units have the same symbol defined.

> 

> Therefore, we've implemented name mangling via

> TARGET_MANGLE_DECL_ASSEMBLER_NAME, but found some places where the middle-end

> assumes that the decl name matches the name in the source.

> 

> This patch fixes up those cases by falling back to comparing the unmangled

> name, when a lookup fails.

> 

> 2018-09-05  Julian Brown  <julian@codesourcery.com>

> 

> 	gcc/

> 	* cgraphunit.c (handle_alias_pairs): Scan for aliases by DECL_NAME if

> 	decl assembler name doesn't match.

> 

> 	gcc/c-family/

> 	* c-pragma.c (maye_apply_pending_pragma_weaks): Scan for aliases with

> 	DECL_NAME if decl assembler name doesn't match.

This should be fine.  But please verify there's no regressions on the
x86_64 linux target, particularly for the multi-versioning tests  (mv*.c
mv*.C

Jeff
Richard Biener Sept. 12, 2018, 2:42 p.m. | #2
On Wed, Sep 12, 2018 at 12:56 AM Jeff Law <law@redhat.com> wrote:
>

> On 9/5/18 5:48 AM, ams@codesourcery.com wrote:

> >

> > The HSA GPU drivers can't cope with binaries that have the same symbol defined

> > multiple times, even though the names are not exported.  This happens whenever

> > there are file-scope static variables with matching names.  I believe it's also

> > an issue with switch tables.

> >

> > This is a bug, but outside our control, so we must work around it when multiple

> > translation units have the same symbol defined.

> >

> > Therefore, we've implemented name mangling via

> > TARGET_MANGLE_DECL_ASSEMBLER_NAME, but found some places where the middle-end

> > assumes that the decl name matches the name in the source.

> >

> > This patch fixes up those cases by falling back to comparing the unmangled

> > name, when a lookup fails.

> >

> > 2018-09-05  Julian Brown  <julian@codesourcery.com>

> >

> >       gcc/

> >       * cgraphunit.c (handle_alias_pairs): Scan for aliases by DECL_NAME if

> >       decl assembler name doesn't match.

> >

> >       gcc/c-family/

> >       * c-pragma.c (maye_apply_pending_pragma_weaks): Scan for aliases with

> >       DECL_NAME if decl assembler name doesn't match.

> This should be fine.  But please verify there's no regressions on the

> x86_64 linux target, particularly for the multi-versioning tests  (mv*.c

> mv*.C


Err - the patch clearly introduces quadraticness into a path which
isn't acceptable.
get_for_asmname works through a hashtable.

It also looks like !target can readily happen so I wonder what happens if an
assembler name does not match but a DECL_NAME one does by accident?

I fear you have to fix this one in a different way... (and I hope
Honza agrees with me).

Thanks,
Richard.


> Jeff
Jeff Law Sept. 12, 2018, 3:07 p.m. | #3
On 9/12/18 8:42 AM, Richard Biener wrote:
> On Wed, Sep 12, 2018 at 12:56 AM Jeff Law <law@redhat.com> wrote:

>>

>> On 9/5/18 5:48 AM, ams@codesourcery.com wrote:

>>>

>>> The HSA GPU drivers can't cope with binaries that have the same symbol defined

>>> multiple times, even though the names are not exported.  This happens whenever

>>> there are file-scope static variables with matching names.  I believe it's also

>>> an issue with switch tables.

>>>

>>> This is a bug, but outside our control, so we must work around it when multiple

>>> translation units have the same symbol defined.

>>>

>>> Therefore, we've implemented name mangling via

>>> TARGET_MANGLE_DECL_ASSEMBLER_NAME, but found some places where the middle-end

>>> assumes that the decl name matches the name in the source.

>>>

>>> This patch fixes up those cases by falling back to comparing the unmangled

>>> name, when a lookup fails.

>>>

>>> 2018-09-05  Julian Brown  <julian@codesourcery.com>

>>>

>>>       gcc/

>>>       * cgraphunit.c (handle_alias_pairs): Scan for aliases by DECL_NAME if

>>>       decl assembler name doesn't match.

>>>

>>>       gcc/c-family/

>>>       * c-pragma.c (maye_apply_pending_pragma_weaks): Scan for aliases with

>>>       DECL_NAME if decl assembler name doesn't match.

>> This should be fine.  But please verify there's no regressions on the

>> x86_64 linux target, particularly for the multi-versioning tests  (mv*.c

>> mv*.C

> 

> Err - the patch clearly introduces quadraticness into a path which

> isn't acceptable.

> get_for_asmname works through a hashtable.

But isn't this only being rused when we aren't able to find the symbol?
 My impression was that should be rare, except for the GCN target.

> 

> It also looks like !target can readily happen so I wonder what happens if an

> assembler name does not match but a DECL_NAME one does by accident?

> 

> I fear you have to fix this one in a different way... (and I hope

> Honza agrees with me).

Honza certainly knows the code better than I.  If  he thinks there's a
performance issue and this needs to be resolved a better way, then I'll
go along with that.

Jeff
Richard Biener Sept. 12, 2018, 3:16 p.m. | #4
On Wed, Sep 12, 2018 at 5:07 PM Jeff Law <law@redhat.com> wrote:
>

> On 9/12/18 8:42 AM, Richard Biener wrote:

> > On Wed, Sep 12, 2018 at 12:56 AM Jeff Law <law@redhat.com> wrote:

> >>

> >> On 9/5/18 5:48 AM, ams@codesourcery.com wrote:

> >>>

> >>> The HSA GPU drivers can't cope with binaries that have the same symbol defined

> >>> multiple times, even though the names are not exported.  This happens whenever

> >>> there are file-scope static variables with matching names.  I believe it's also

> >>> an issue with switch tables.

> >>>

> >>> This is a bug, but outside our control, so we must work around it when multiple

> >>> translation units have the same symbol defined.

> >>>

> >>> Therefore, we've implemented name mangling via

> >>> TARGET_MANGLE_DECL_ASSEMBLER_NAME, but found some places where the middle-end

> >>> assumes that the decl name matches the name in the source.

> >>>

> >>> This patch fixes up those cases by falling back to comparing the unmangled

> >>> name, when a lookup fails.

> >>>

> >>> 2018-09-05  Julian Brown  <julian@codesourcery.com>

> >>>

> >>>       gcc/

> >>>       * cgraphunit.c (handle_alias_pairs): Scan for aliases by DECL_NAME if

> >>>       decl assembler name doesn't match.

> >>>

> >>>       gcc/c-family/

> >>>       * c-pragma.c (maye_apply_pending_pragma_weaks): Scan for aliases with

> >>>       DECL_NAME if decl assembler name doesn't match.

> >> This should be fine.  But please verify there's no regressions on the

> >> x86_64 linux target, particularly for the multi-versioning tests  (mv*.c

> >> mv*.C

> >

> > Err - the patch clearly introduces quadraticness into a path which

> > isn't acceptable.

> > get_for_asmname works through a hashtable.

> But isn't this only being rused when we aren't able to find the symbol?


This case seems to happen though.

>  My impression was that should be rare, except for the GCN target.


Still even for the GCN target it looks like a hack given the linear search.

I think it is required to track down the "invalid" uses of DECL_NAME vs.
"mangled" name instead.

> >

> > It also looks like !target can readily happen so I wonder what happens if an

> > assembler name does not match but a DECL_NAME one does by accident?

> >

> > I fear you have to fix this one in a different way... (and I hope

> > Honza agrees with me).

> Honza certainly knows the code better than I.  If  he thinks there's a

> performance issue and this needs to be resolved a better way, then I'll

> go along with that.


I think the symptom GCN sees needs to be better understood - like wheter
it is generally OK to mangle things arbitrarily.

Note that TARGET_MANGLE_DECL_ASSEMBLER_NAME might not be
a general symbol mangling hook but may be restricted to symbols with
specific visibility.

Richard.

>

> Jeff
Andrew Stubbs Sept. 12, 2018, 4:31 p.m. | #5
On 12/09/18 16:16, Richard Biener wrote:
> I think the symptom GCN sees needs to be better understood - like wheter

> it is generally OK to mangle things arbitrarily.


The name mangling is a horrible workaround for a bug in the HSA runtime 
code (which we do not own, cannot fix, and would want to support old 
versions of anyway).  Basically it refuses to load any binary that has 
the same symbol as another, already loaded, binary, regardless of the 
symbol's linkage.  Worse, it also rejects any binary that has duplicate 
symbols within it, despite the fact that it already linked just fine.

Adding the extra lookups is enough to build GCN binaries, with mangled 
names, whereas the existing name mangling support was either more 
specialized or bit rotten (I don't know which).

It may well be that there's a better way to solve the problem, or at 
least to do the lookups.

It may also be that there are some unintended consequences, such as 
false name matches, but I don't know of any at present.

Julian, can you comment, please?

Andrew
Julian Brown Sept. 12, 2018, 5:34 p.m. | #6
On Wed, 12 Sep 2018 17:31:58 +0100
Andrew Stubbs <ams@codesourcery.com> wrote:

> On 12/09/18 16:16, Richard Biener wrote:

> > I think the symptom GCN sees needs to be better understood - like

> > wheter it is generally OK to mangle things arbitrarily.  

> 

> The name mangling is a horrible workaround for a bug in the HSA

> runtime code (which we do not own, cannot fix, and would want to

> support old versions of anyway).  Basically it refuses to load any

> binary that has the same symbol as another, already loaded, binary,

> regardless of the symbol's linkage.  Worse, it also rejects any

> binary that has duplicate symbols within it, despite the fact that it

> already linked just fine.

> 

> Adding the extra lookups is enough to build GCN binaries, with

> mangled names, whereas the existing name mangling support was either

> more specialized or bit rotten (I don't know which).

> 

> It may well be that there's a better way to solve the problem, or at 

> least to do the lookups.

> 

> It may also be that there are some unintended consequences, such as 

> false name matches, but I don't know of any at present.

> 

> Julian, can you comment, please?


I did the local-symbol name mangling in two places:

- The ASM_FORMAT_PRIVATE_NAME macro (good for local statics)
- The TARGET_MANGLE_DECL_ASSEMBLER_NAME hook (for file-scope
  local/statics)

Possibly, this was an abuse of these hooks, but it's arguably wrong that
that e.g. handle_alias_pairs has the "assembler name" leak through into
the user's source code -- if it's expected that the hook could make
arbitrary transformations to the string. (The latter hook is only used
by PE code for x86 at present, by the look of it, and the default
handles only special-purpose mangling indicated by placing a '*' at the
front of the symbol.)

I couldn't find an existing place where the DECL_NAMEs for symbols were
indexed in a hash table, equivalent to the table for assembler names.
Aliases are made via pragmas, so it's not 100% clear to me what the
scoping/lookup rules are supposed to be for those anyway, nor what the
possibility or consequences might be of false matches.

(The "!target" case in maybe_apply_pending_pragma_weaks, if it doesn't
somehow make a false match, just slows down reporting of an error a
little, I think. Similarly in handle_alias_pairs.)

If we had a symtab_node::get_for_name () using a suitable hash table, I
think it'd probably be right to use that. Can that be done (easily), or
is there some equivalent way? Introducing a new hash table everywhere
for a bug workaround for a relatively obscure feature on a single
target seems unfortunate.

Thanks,

Julian
Julian Brown Sept. 15, 2018, 2:49 a.m. | #7
On Wed, 12 Sep 2018 13:34:06 -0400
Julian Brown <julian@codesourcery.com> wrote:

> On Wed, 12 Sep 2018 17:31:58 +0100

> Andrew Stubbs <ams@codesourcery.com> wrote:

> 

> > On 12/09/18 16:16, Richard Biener wrote:  

> > > I think the symptom GCN sees needs to be better understood - like

> > > wheter it is generally OK to mangle things arbitrarily.    

> > 

> > The name mangling is a horrible workaround for a bug in the HSA

> > runtime code (which we do not own, cannot fix, and would want to

> > support old versions of anyway).  Basically it refuses to load any

> > binary that has the same symbol as another, already loaded, binary,

> > regardless of the symbol's linkage.  Worse, it also rejects any

> > binary that has duplicate symbols within it, despite the fact that

> > it already linked just fine.

> > 

> > Adding the extra lookups is enough to build GCN binaries, with

> > mangled names, whereas the existing name mangling support was either

> > more specialized or bit rotten (I don't know which).

> > 

> > It may well be that there's a better way to solve the problem, or

> > at least to do the lookups.

> > 

> > It may also be that there are some unintended consequences, such as 

> > false name matches, but I don't know of any at present.

> > 

> > Julian, can you comment, please?  

> 

> I did the local-symbol name mangling in two places:

> 

> - The ASM_FORMAT_PRIVATE_NAME macro (good for local statics)

> - The TARGET_MANGLE_DECL_ASSEMBLER_NAME hook (for file-scope

>   local/statics)

> 

> Possibly, this was an abuse of these hooks, but it's arguably wrong

> that that e.g. handle_alias_pairs has the "assembler name" leak

> through into the user's source code -- if it's expected that the hook

> could make arbitrary transformations to the string. (The latter hook

> is only used by PE code for x86 at present, by the look of it, and

> the default handles only special-purpose mangling indicated by

> placing a '*' at the front of the symbol.)


One possibility might be to allow
symbol_table::decl_assembler_name_hash and
symbol_table::assembler_names_equal_p to be overridden by a target
hook, and define them for GCN to ignore the symbol "localisation"
magic. At the moment they will just ignore "*" at the start of a
symbol, and a (fixed) user label prefix, no matter what
TARGET_MANGLE_DECL_ASSEMBLER_NAME does.

Another way would be to do some appropriate mangling for local symbols
in the assembler, rather than the compiler (though we're using the LLVM
assembler, and so far have got away with not making any invasive
changes to that).

> If we had a symtab_node::get_for_name () using a suitable hash table,

> I think it'd probably be right to use that. Can that be done

> (easily), or is there some equivalent way? Introducing a new hash

> table everywhere for a bug workaround for a relatively obscure

> feature on a single target seems unfortunate.


An "obvious" solution of calling targetm.mangle_decl_assembler_name
before looking up in symtab_node::get_for_asmname, something like:

static void
handle_alias_pairs (void)
{
  alias_pair *p;
  unsigned i;

  for (i = 0; alias_pairs && alias_pairs->iterate (i, &p);)
    {
      tree asmname = targetm.mangle_decl_assembler_name (p->decl, p->target);
      symtab_node *target_node = symtab_node::get_for_asmname (asmname);
      [...]

seems like it could possibly work for handle_alias_pairs, but not so
much for c-pragma.c:maybe_apply_pending_pragma_weaks, where there is no
decl available to pass as the first argument to the target hook.

Julian
Julian Brown Sept. 19, 2018, 3:11 p.m. | #8
On Fri, 14 Sep 2018 22:49:35 -0400
Julian Brown <julian@codesourcery.com> wrote:

> > > On 12/09/18 16:16, Richard Biener wrote:    

> > > It may well be that there's a better way to solve the problem, or

> > > at least to do the lookups.

> > > 

> > > It may also be that there are some unintended consequences, such

> > > as false name matches, but I don't know of any at present.


> > Possibly, this was an abuse of these hooks, but it's arguably wrong

> > that that e.g. handle_alias_pairs has the "assembler name" leak

> > through into the user's source code -- if it's expected that the

> > hook could make arbitrary transformations to the string. (The

> > latter hook is only used by PE code for x86 at present, by the look

> > of it, and the default handles only special-purpose mangling

> > indicated by placing a '*' at the front of the symbol.)  


Two places I've found that currently expose the underlying symbol name
in the user's source code: one (documented!) is C++, where one must
write the mangled symbol name as the alias target:

int foo (int c) { ... }
int bar (int) __attribute__((alias("_Z3fooi")));

another (perhaps obscure) is x86/PE with "fastcall":

__attribute__((fastcall)) void foo(void) { ... }
void bar(void) __attribute__((alias("@foo@0")));

both of which probably suggest that using the decl name, rather than
demangling the assembler name (or using some completely different
solution) was the wrong thing to do.

I'll keep thinking about this...

Julian
Richard Biener Sept. 20, 2018, 12:36 p.m. | #9
On Wed, Sep 19, 2018 at 5:11 PM Julian Brown <julian@codesourcery.com> wrote:
>

> On Fri, 14 Sep 2018 22:49:35 -0400

> Julian Brown <julian@codesourcery.com> wrote:

>

> > > > On 12/09/18 16:16, Richard Biener wrote:

> > > > It may well be that there's a better way to solve the problem, or

> > > > at least to do the lookups.

> > > >

> > > > It may also be that there are some unintended consequences, such

> > > > as false name matches, but I don't know of any at present.

>

> > > Possibly, this was an abuse of these hooks, but it's arguably wrong

> > > that that e.g. handle_alias_pairs has the "assembler name" leak

> > > through into the user's source code -- if it's expected that the

> > > hook could make arbitrary transformations to the string. (The

> > > latter hook is only used by PE code for x86 at present, by the look

> > > of it, and the default handles only special-purpose mangling

> > > indicated by placing a '*' at the front of the symbol.)

>

> Two places I've found that currently expose the underlying symbol name

> in the user's source code: one (documented!) is C++, where one must

> write the mangled symbol name as the alias target:

>

> int foo (int c) { ... }

> int bar (int) __attribute__((alias("_Z3fooi")));

>

> another (perhaps obscure) is x86/PE with "fastcall":

>

> __attribute__((fastcall)) void foo(void) { ... }

> void bar(void) __attribute__((alias("@foo@0")));

>

> both of which probably suggest that using the decl name, rather than

> demangling the assembler name (or using some completely different

> solution) was the wrong thing to do.

>

> I'll keep thinking about this...


Thanks, IIRC we already have some targets with quite complex renaming
where I wonder iff uses like above work correctly.

Btw, if you don't "fix" the handle_alias_paris code but keep your mangling
what does break for you in practice (apart from maybe some testcases)?

Richard.

> Julian

Patch

diff --git a/gcc/c-family/c-pragma.c b/gcc/c-family/c-pragma.c
index 84e4341..1c0be0c 100644
--- a/gcc/c-family/c-pragma.c
+++ b/gcc/c-family/c-pragma.c
@@ -323,6 +323,20 @@  maybe_apply_pending_pragma_weaks (void)
 	continue;
 
       target = symtab_node::get_for_asmname (id);
+
+      /* Try again if ID didn't match an assembler name by looking through
+	 decl names.  */
+      if (!target)
+	{
+	  symtab_node *node;
+	  FOR_EACH_SYMBOL (node)
+	    if (strcmp (IDENTIFIER_POINTER (id), node->name ()) == 0)
+	      {
+	        target = node;
+		break;
+	      }
+	}
+
       decl = build_decl (UNKNOWN_LOCATION,
 			 target ? TREE_CODE (target->decl) : FUNCTION_DECL,
 			 alias_id, default_function_type);
diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c
index ec490d7..fc3f34e 100644
--- a/gcc/cgraphunit.c
+++ b/gcc/cgraphunit.c
@@ -1393,6 +1393,21 @@  handle_alias_pairs (void)
     {
       symtab_node *target_node = symtab_node::get_for_asmname (p->target);
 
+      /* If the alias target didn't match a symbol's assembler name (e.g.
+	 because it has been mangled by TARGET_MANGLE_DECL_ASSEMBLER_NAME),
+	 try again with the unmangled decl name.  */
+      if (!target_node)
+	{
+	  symtab_node *node;
+	  FOR_EACH_SYMBOL (node)
+	    if (strcmp (IDENTIFIER_POINTER (p->target),
+			node->name ()) == 0)
+	      {
+		target_node = node;
+		break;
+	      }
+	}
+
       /* Weakrefs with target not defined in current unit are easy to handle:
 	 they behave just as external variables except we need to note the
 	 alias flag to later output the weakref pseudo op into asm file.  */