| Message ID | 328b94cd7d15c4d2113a676a3a70ce640b37333a.1536144068.git.ams@codesourcery.com |
|---|---|
| State | New |
| Headers | show |
| Series | AMD GCN Port | expand |
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
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
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
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
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
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
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
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
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
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. */