diff mbox

Fix aliases on AIX

Message ID 20140707200928.GC12716@kam.mff.cuni.cz
State New
Headers show

Commit Message

Jan Hubicka July 7, 2014, 8:09 p.m. UTC 
Hi,
AIX as' .set pseudoop has somewhat unexpected behaviour.  It seems to be
implemented by syntactically replacin all appereances of the alias by
its target (that can be an expressoin) but it is still possible to globalize
the alias by .globl operation.

This breaks the assumption we make about aliases; for example when alias is
declared static but it is alias of global/weak symbol, we assume it is static
symbol, but in AIX implementation it is handled as global.  This breaks
horrible way for local aliases.

Another unexpected behaviour is that .set output before definition of alias
target is silently compiled into something else (NULL?). We are not fully
consistent about outputing aliases after targets.

This patch avoids use of .set for aliases. Instead
rs6000_xcoff_declare_object_name and rs6000_xcoff_declare_function_name simply
walk and outputs all associated aliases at the time they output function.
I.e.

var:
  <var data>
.set var, aliasvar

Is now done as:
var:
aliasvar:
  <var data>

This avoids the surprises we was hitting on AIX and makes aliases to work
consistently.  The way it is hooked into output machinery is not the most
beautiful, but I do not see better way except for redesigning the output
interface (adding new macro for this type of aliases) that seems to be unnecesay
unless we find more targets that would benefit from aliases output this way.

The testcases localalias/globalalias was added to check sanity of alias
implementation and this patch makes them to work.

Bootstrapped/regtested rs6000-aix OK?

Honza

	* rs6000/rs6000-protos.h (rs6000_xcoff_declare_object_name): Declare.
	* rs6000/rs6000.c: Inline output of .set instruction.
	(declare_alias_data): New struct.
	(rs6000_declare_alias): New function.
	(rs6000_xcoff_declare_function_name): Use it.
	(rs6000_xcoff_declare_object_name): New function.
	* config/rs6000/xcoff.h: Define ASM_DECLARE_OBJECT_NAME.
	(ASM_OUTPUT_DEF): Turn to empty definition.
	* ipa-visibility (function_and_variable_visibility): Remove
	temporary AIX workaround.

	* gcc.dg/globalalias.c: Remove XFAIL
	* gcc.dg/localalias.c: Remove XFAIL

Comments

David Edelsohn July 8, 2014, 5:55 p.m. UTC | #1 
On Mon, Jul 7, 2014 at 4:09 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
> Hi,
> AIX as' .set pseudoop has somewhat unexpected behaviour.  It seems to be
> implemented by syntactically replacin all appereances of the alias by
> its target (that can be an expressoin) but it is still possible to globalize
> the alias by .globl operation.
>
> This breaks the assumption we make about aliases; for example when alias is
> declared static but it is alias of global/weak symbol, we assume it is static
> symbol, but in AIX implementation it is handled as global.  This breaks
> horrible way for local aliases.
>
> Another unexpected behaviour is that .set output before definition of alias
> target is silently compiled into something else (NULL?). We are not fully
> consistent about outputing aliases after targets.
>
> This patch avoids use of .set for aliases. Instead
> rs6000_xcoff_declare_object_name and rs6000_xcoff_declare_function_name simply
> walk and outputs all associated aliases at the time they output function.
> I.e.
>
> var:
>   <var data>
> .set var, aliasvar
>
> Is now done as:
> var:
> aliasvar:
>   <var data>
>
> This avoids the surprises we was hitting on AIX and makes aliases to work
> consistently.  The way it is hooked into output machinery is not the most
> beautiful, but I do not see better way except for redesigning the output
> interface (adding new macro for this type of aliases) that seems to be unnecesay
> unless we find more targets that would benefit from aliases output this way.
>
> The testcases localalias/globalalias was added to check sanity of alias
> implementation and this patch makes them to work.
>
> Bootstrapped/regtested rs6000-aix OK?

Hi, Honza

This looks really good and mostly is working.

With the patch, GCC on AIX now responds that "ifunc" is supported.
The C and C++ attr-ifunc testcases now run and fail.

g++.dg/ipa/devirt-10 and devirt-15 now fail.

Otherwise, the results look very good.

Thanks, David
David Edelsohn July 8, 2014, 10:52 p.m. UTC | #2 
The problem with devirt-10 and devirt-15 is the excellent kludge for
aliases on AIX can produce multiple symbols.

ipa-prop: Discovered a virtual call to a known target (void
wxBufferedDC::InitCommon(wxDCBase*)/3 -> virtual void
wxDCBase::_ZN8wxDCBase18SetLayoutDirectionEi.localalias.6(int)/36),
for stmt OBJ_TYPE_REF(_6;_7->1) (_7, _11);
Speculative call turned into direct call.
ipa-prop: Discovered a virtual call to a known target (void
wxBufferedDC::InitCommon(wxDCBase*)/3 -> virtual int
wxDCBase::_ZNK8wxDCBase18GetLayoutDirectionEv.localalias.5()
const/35), for stmt _11 = OBJ_TYPE_REF(_9;dc_2(D)->0) (dc_2(D));


The testcase expects exactly 1 occurrence, but the new code produces 2 for AIX.

What is the best way to adjust the testcases?

Thanks, David
Jan Hubicka July 9, 2014, 4:25 a.m. UTC | #3 
> The problem with devirt-10 and devirt-15 is the excellent kludge for
> aliases on AIX can produce multiple symbols.
> 
> ipa-prop: Discovered a virtual call to a known target (void
> wxBufferedDC::InitCommon(wxDCBase*)/3 -> virtual void
> wxDCBase::_ZN8wxDCBase18SetLayoutDirectionEi.localalias.6(int)/36),
> for stmt OBJ_TYPE_REF(_6;_7->1) (_7, _11);
> Speculative call turned into direct call.
> ipa-prop: Discovered a virtual call to a known target (void
> wxBufferedDC::InitCommon(wxDCBase*)/3 -> virtual int
> wxDCBase::_ZNK8wxDCBase18GetLayoutDirectionEv.localalias.5()
> const/35), for stmt _11 = OBJ_TYPE_REF(_9;dc_2(D)->0) (dc_2(D));
> 
> 
> The testcase expects exactly 1 occurrence, but the new code produces 2 for AIX.
> 
> What is the best way to adjust the testcases?

I guess it actually difference in inlining - those aliases are not of same
symbols so we devirtualize more than the testcase's template expect.  It may
just go away with enabling COMDAT but I need to look into making those two
testcases more robust.

Definitely not a wrong code issue though, just fragile testcase.
Honza
> 
> Thanks, David
Jan Hubicka July 9, 2014, 4:50 a.m. UTC | #4 
> With the patch, GCC on AIX now responds that "ifunc" is supported.
> The C and C++ attr-ifunc testcases now run and fail.

This is unexpected.  I am testing version of the patch with

  if (lookup_attribute ("ifunc", DECL_ATTRIBUTES (n->decl)))
    return false;

added to beggining of rs6000_declare_alias.  The problem here is that ifunc is
dealt with as an alias and becuase we output aliases ourselves, we skip the
sanity checking done in do_assemble_alias.  I think that checking is way too
late, will look into moving it earlier.

I also checked that we won't skip other useful sanity checking there. (other one is
weakref that we already know to not output in the new way)

Honza
> 
> g++.dg/ipa/devirt-10 and devirt-15 now fail.
> 
> Otherwise, the results look very good.
> 
> Thanks, David
David Edelsohn July 9, 2014, 1:35 p.m. UTC | #5 
On Wed, Jul 9, 2014 at 12:50 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> With the patch, GCC on AIX now responds that "ifunc" is supported.
>> The C and C++ attr-ifunc testcases now run and fail.
>
> This is unexpected.  I am testing version of the patch with
>
>   if (lookup_attribute ("ifunc", DECL_ATTRIBUTES (n->decl)))
>     return false;
>
> added to beggining of rs6000_declare_alias.  The problem here is that ifunc is
> dealt with as an alias and becuase we output aliases ourselves, we skip the
> sanity checking done in do_assemble_alias.  I think that checking is way too
> late, will look into moving it earlier.
>
> I also checked that we won't skip other useful sanity checking there. (other one is
> weakref that we already know to not output in the new way)
>
> Honza
>>
>> g++.dg/ipa/devirt-10 and devirt-15 now fail.
>>
>> Otherwise, the results look very good.

Hi, Honza

Other than those issues, the results generally look good.  If you can
correct the patch so that it does not avoid relevant sanity checks, it
should be ready to merge.

Thanks, David
diff mbox

Patch

Index: config/rs6000/rs6000-protos.h
===================================================================
--- config/rs6000/rs6000-protos.h	(revision 212279)
+++ config/rs6000/rs6000-protos.h	(working copy)
@@ -165,6 +165,7 @@ 
 extern int function_ok_for_sibcall (tree);
 extern int rs6000_reg_parm_stack_space (tree, bool);
 extern void rs6000_xcoff_declare_function_name (FILE *, const char *, tree);
+extern void rs6000_xcoff_declare_object_name (FILE *, const char *, tree);
 extern void rs6000_elf_declare_function_name (FILE *, const char *, tree);
 extern bool rs6000_elf_in_small_data_p (const_tree);
 #ifdef ARGS_SIZE_RTX
Index: config/rs6000/rs6000.c
===================================================================
--- config/rs6000/rs6000.c	(revision 212279)
+++ config/rs6000/rs6000.c	(working copy)
@@ -29156,7 +29156,11 @@ 
 
   sprintf (buffer, "$ + " HOST_WIDE_INT_PRINT_DEC,
 	   SYMBOL_REF_BLOCK_OFFSET (symbol));
-  ASM_OUTPUT_DEF (asm_out_file, XSTR (symbol, 0), buffer);
+  fprintf (asm_out_file, "%s", SET_ASM_OP);
+  RS6000_OUTPUT_BASENAME (asm_out_file, XSTR (symbol, 0));
+  fprintf (asm_out_file, ",");
+  RS6000_OUTPUT_BASENAME (asm_out_file, buffer);
+  fprintf (asm_out_file, "\n");
 }
 
 static void
@@ -29453,6 +29457,82 @@ 
 	 asm_out_file);
 }
 
+struct declare_alias_data
+{
+  FILE *file;
+  bool function_descriptor;
+};
+
+/* Declare alias N.  A helper function for for_node_and_aliases.  */
+
+static bool
+rs6000_declare_alias (struct symtab_node *n, void *d)
+{
+  struct declare_alias_data *data = (struct declare_alias_data *)d;
+  /* Main symbol is output specially, because varasm machinery does part of
+     the job for us - we do not need to declare .globl/lglobs and such.  */
+  if (!n->alias || n->weakref)
+    return false;
+
+  /* Prevent assemble_alias from trying to use .set pseudo operation
+     that does not behave as expected by the middle-end.  */
+  TREE_ASM_WRITTEN (n->decl) = true;
+
+  const char *name = IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (n->decl));
+  char *buffer = (char *) alloca (strlen (name) + 2);
+  char *p;
+  int dollar_inside = 0;
+
+  strcpy (buffer, name);
+  p = strchr (buffer, '$');
+  while (p) {
+    *p = '_';
+    dollar_inside++;
+    p = strchr (p + 1, '$');
+  }
+  if (TREE_PUBLIC (n->decl))
+    {
+      if (!RS6000_WEAK || !DECL_WEAK (n->decl))
+	{
+          if (dollar_inside) {
+	      if (data->function_descriptor)
+                fprintf(data->file, "\t.rename .%s,\".%s\"\n", buffer, name);
+	      else
+                fprintf(data->file, "\t.rename %s,\"%s\"\n", buffer, name);
+	    }
+	  if (data->function_descriptor)
+	    fputs ("\t.globl .", data->file);
+	  else
+	    fputs ("\t.globl ", data->file);
+	  RS6000_OUTPUT_BASENAME (data->file, buffer);
+	  putc ('\n', data->file);
+	}
+      else if (DECL_WEAK (n->decl) && !data->function_descriptor)
+	ASM_WEAKEN_DECL (data->file, n->decl, name, NULL);
+    }
+  else
+    {
+      if (dollar_inside)
+	{
+	  if (data->function_descriptor)
+            fprintf(data->file, "\t.rename %s,\"%s\"\n", buffer, name);
+	  else
+            fprintf(data->file, "\t.rename .%s,\".%s\"\n", buffer, name);
+	}
+      if (data->function_descriptor)
+	fputs ("\t.lglobl .", data->file);
+      else
+	fputs ("\t.lglobl ", data->file);
+      RS6000_OUTPUT_BASENAME (data->file, buffer);
+      putc ('\n', data->file);
+    }
+  if (data->function_descriptor)
+    fputs (".", data->file);
+  RS6000_OUTPUT_BASENAME (data->file, buffer);
+  fputs (":\n", data->file);
+  return false;
+}
+
 /* This macro produces the initial definition of a function name.
    On the RS/6000, we need to place an extra '.' in the function name and
    output the function descriptor.
@@ -29462,14 +29542,19 @@ 
    text_section was selected.  We do have to go back to that csect, however.
 
    The third and fourth parameters to the .function pseudo-op (16 and 044)
-   are placeholders which no longer have any use.  */
+   are placeholders which no longer have any use.
 
+   Because AIX assembler's .set command has unexpected semantics, we output
+   all aliases as alternative labels in front of the definition.  */
+
 void
 rs6000_xcoff_declare_function_name (FILE *file, const char *name, tree decl)
 {
   char *buffer = (char *) alloca (strlen (name) + 1);
   char *p;
   int dollar_inside = 0;
+  struct declare_alias_data data = {file, false};
+
   strcpy (buffer, name);
   p = strchr (buffer, '$');
   while (p) {
@@ -29505,6 +29590,7 @@ 
   fputs (TARGET_32BIT ? "[DS]\n" : "[DS],3\n", file);
   RS6000_OUTPUT_BASENAME (file, buffer);
   fputs (":\n", file);
+  symtab_for_node_and_aliases (symtab_get_node (decl), rs6000_declare_alias, &data, true);
   fputs (TARGET_32BIT ? "\t.long ." : "\t.llong .", file);
   RS6000_OUTPUT_BASENAME (file, buffer);
   fputs (", TOC[tc0], 0\n", file);
@@ -29513,11 +29599,26 @@ 
   putc ('.', file);
   RS6000_OUTPUT_BASENAME (file, buffer);
   fputs (":\n", file);
+  data.function_descriptor = true;
+  symtab_for_node_and_aliases (symtab_get_node (decl), rs6000_declare_alias, &data, true);
   if (write_symbols != NO_DEBUG && !DECL_IGNORED_P (decl))
     xcoffout_declare_function (file, decl, buffer);
   return;
 }
 
+/* This macro produces the initial definition of a object (variable) name.
+   Because AIX assembler's .set command has unexpected semantics, we output
+   all aliases as alternative labels in front of the definition.  */
+
+void
+rs6000_xcoff_declare_object_name (FILE *file, const char *name, tree decl)
+{
+  struct declare_alias_data data = {file, false};
+  RS6000_OUTPUT_BASENAME (file, name);
+  fputs (":\n", file);
+  symtab_for_node_and_aliases (symtab_get_node (decl), rs6000_declare_alias, &data, true);
+}
+
 #ifdef HAVE_AS_TLS
 static void
 rs6000_xcoff_encode_section_info (tree decl, rtx rtl, int first)
Index: config/rs6000/xcoff.h
===================================================================
--- config/rs6000/xcoff.h	(revision 212279)
+++ config/rs6000/xcoff.h	(working copy)
@@ -139,6 +139,9 @@ 
 #undef ASM_DECLARE_FUNCTION_NAME
 #define ASM_DECLARE_FUNCTION_NAME(FILE, NAME, DECL)			\
   rs6000_xcoff_declare_function_name ((FILE), (NAME), (DECL))
+#undef ASM_DECLARE_OBJECT_NAME
+#define ASM_DECLARE_OBJECT_NAME(FILE, NAME, DECL)			\
+  rs6000_xcoff_declare_object_name ((FILE), (NAME), (DECL))
 
 /* Output a reference to SYM on FILE.  */
 
@@ -280,16 +283,18 @@ 
 /* This is how we tell the assembler that two symbols have the same value.  */
 #define SET_ASM_OP "\t.set "
 
-/* This is how we tell the assembler to equate two values.  */
-#define ASM_OUTPUT_DEF(FILE,LABEL1,LABEL2)				\
- do {	fprintf ((FILE), "%s", SET_ASM_OP);				\
-	RS6000_OUTPUT_BASENAME (FILE, LABEL1);				\
-	fprintf (FILE, ",");						\
-	RS6000_OUTPUT_BASENAME (FILE, LABEL2);				\
-	fprintf (FILE, "\n");						\
-  } while (0)
+/* This is how we tell the assembler to equate two values. 
+   The semantic of AIX assembler's .set do not correspond to middle-end expectations.
+   We output aliases as alternative symbols in the front of the definition
+   via DECLARE_FUNCTION_NAME and DECLARE_OBJECT_NAME.
+   We still need to define this macro to let middle-end know that aliases are
+   supported.
+ */
+#define ASM_OUTPUT_DEF(FILE,LABEL1,LABEL2) do { } while (0)
 
 /* Used by rs6000_assemble_integer, among others.  */
+
+/* Used by rs6000_assemble_integer, among others.  */
 #define DOUBLE_INT_ASM_OP "\t.llong\t"
 
 /* Output before instructions.  */
Index: ipa-visibility.c
===================================================================
--- ipa-visibility.c	(revision 212279)
+++ ipa-visibility.c	(working copy)
@@ -567,9 +567,6 @@ 
 
 	 TODO: We can also update virtual tables.  */
       if (node->callers 
-          /* FIXME: currently this optimization breaks on AIX.  Disable it for targets
-             without comdat support for now.  */
-	  && SUPPORTS_ONE_ONLY
 	  && can_replace_by_local_alias (node))
 	{
 	  struct cgraph_node *alias = cgraph (symtab_nonoverwritable_alias (node));
@@ -672,10 +669,7 @@ 
 
       /* Update virtual tables to point to local aliases where possible.  */
       if (DECL_VIRTUAL_P (vnode->decl)
-	  && !DECL_EXTERNAL (vnode->decl)
-	  /* FIXME: currently this optimization breaks on AIX.  Disable it for targets
-	     without comdat support for now.  */
-	  && SUPPORTS_ONE_ONLY)
+	  && !DECL_EXTERNAL (vnode->decl))
 	{
 	  int i;
 	  struct ipa_ref *ref;
Index: testsuite/gcc.dg/globalalias.c
===================================================================
--- testsuite/gcc.dg/globalalias.c	(revision 212279)
+++ testsuite/gcc.dg/globalalias.c	(working copy)
@@ -8,7 +8,6 @@ 
 /* { dg-do run }
    { dg-options "-O2" } 
    { dg-require-alias "" }
-   { dg-xfail-if "" { powerpc-ibm-aix* } { "*" } { "" } } 
    { dg-additional-sources "globalalias-2.c" } */
 extern int test2count;
 extern void abort (void);
Index: testsuite/gcc.dg/localalias.c
===================================================================
--- testsuite/gcc.dg/localalias.c	(revision 212279)
+++ testsuite/gcc.dg/localalias.c	(working copy)
@@ -9,7 +9,6 @@ 
 /* { dg-do run }
    { dg-options "-Wstrict-aliasing=2 -fstrict-aliasing" } 
    { dg-require-alias "" }
-   { dg-xfail-if "" { powerpc-ibm-aix* } { "*" } { "" } } 
    { dg-additional-sources "localalias-2.c" } */
 extern void abort (void);
 extern int test2count;