This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: 4.1/4.2 PATCH: PR middle-end/20218: Can't use __attribute__ ((visibility ("hidden"))) to hide a symbol


"H. J. Lu" <hjl@lucon.org> writes:
> I updated mips_output_external to use FILE parameter.

Thanks for doing this.  It was beyond the call of duty, and like
I said in the earlier message, I'm a little reluctant to apply this
to the release branch, but I've not got a strong opinion either way.

I tested the patch by building a mainline compiler for mips64-elf,
mips-sgi-irix6.5 and mips-linux-gnu.  I compared the before-and-after
testsuite assembly output for:

  -mabi=32 -mno-explicit-relocs
  -mabi=32 -mno-explicit-relocs -O2

The "before" side was without this patch and without your original
elfos.h patch.  The declaration changes seemed fine, but I noticed
an unexpected side-effect.  expand_builtin() has:

  /* When not optimizing, generate calls to library functions for a certain
     set of builtins.  */
  if (!optimize
      && !called_as_built_in (fndecl)
      && DECL_ASSEMBLER_NAME_SET_P (fndecl)
      && fcode != BUILT_IN_ALLOCA)
    return expand_call (exp, target, ignore);

However, let's take gcc.c-torture/compile/920617-1.c as an example:

  f(){double*xp,y;*xp++=sqrt(y);}

Even before your patch, i686 used the builtin expansion of sqrt()
when optimisation is disabled:

f:
        pushl   %ebp
        movl    %esp, %ebp
        subl    $40, %esp
        fldl    -8(%ebp)
        fsqrt
        fstpl   -24(%ebp)
        fldl    -24(%ebp)
        fucomp  %st(0)
        fnstsw  %ax
        sahf
        jp      .L4
        je      .L2
.L4:
        fldl    -8(%ebp)
        fstpl   (%esp)
        call    sqrt
        fstpl   -24(%ebp)
.L2:
        fldl    -24(%ebp)
        movl    -12(%ebp), %eax
        fstpl   (%eax)
        addl    $8, -12(%ebp)
        leave
        ret

(Note that this is the builtin expansion, not the glibc expansion.)
This is because DECL_ASSEMBLER_NAME_SET_P (fndecl) is false at this point.

However, before your patch, the builtins happened to still work on
targets that define ASM_OUTPUT_EXTERNAL.  This is because:

  rtx rtl = DECL_RTL (decl);

in assemble_output_real causes a (mem (symbol_ref "sqrt")) to
be created, and fndecl's assembler name to be set.

Your patch postpones the call to assemble_external_real until
the end of compilation for !flag_unit_at_a_time, so the code
above no longer works for ASM_OUTPUT_EXTERNAL targets either.

I don't see this as your fault, I'm just noting it for reference.
Clearly something more general is broken here.

Anyway, I tweaked the patch slightly so that it calls
default_elf_asm_output_external rather than inlining its
contents in mips_output_external.  Here's what I applied
to mainline.  Thanks again for the patch.

Richard


gcc/
2007-01-17  H.J. Lu  <hongjiu.lu@intel.com>

	* config/mips/mips-protos.h (mips_output_external): Make it
	return void.
	* config/mips/iris.h (TARGET_ASM_EXTERNAL_LIBCALL): Removed.
	* config/mips/mips.c (irix_output_external_libcall): Likewise.
	(extern_list): Likewise.
	(extern_head): Likewise.
	(TARGET_ASM_FILE_END): Likewise.
	(mips_file_end): Likewise.
	(mips_output_external): Rewritten.

Index: gcc/config/mips/iris.h
===================================================================
--- gcc/config/mips/iris.h	(revision 120822)
+++ gcc/config/mips/iris.h	(working copy)
@@ -61,10 +61,6 @@ #define ASM_DECLARE_OBJECT_NAME mips_dec
 #undef ASM_FINISH_DECLARE_OBJECT
 #define ASM_FINISH_DECLARE_OBJECT mips_finish_declare_object
 
-/* Also do this for libcalls.  */
-#undef TARGET_ASM_EXTERNAL_LIBCALL
-#define TARGET_ASM_EXTERNAL_LIBCALL irix_output_external_libcall
-
 /* The linker needs a space after "-o".  */
 #define SWITCHES_NEED_SPACES "o"
 
Index: gcc/config/mips/mips-protos.h
===================================================================
--- gcc/config/mips/mips-protos.h	(revision 120822)
+++ gcc/config/mips/mips-protos.h	(working copy)
@@ -197,7 +197,7 @@ extern HOST_WIDE_INT mips_debugger_offse
 
 extern void print_operand (FILE *, rtx, int);
 extern void print_operand_address (FILE *, rtx);
-extern int mips_output_external (FILE *, tree, const char *);
+extern void mips_output_external (FILE *, tree, const char *);
 extern void mips_output_filename (FILE *, const char *);
 extern void mips_output_ascii (FILE *, const char *, size_t, const char *);
 extern void mips_output_aligned_bss (FILE *, tree, const char *,
Index: gcc/config/mips/mips.c
===================================================================
--- gcc/config/mips/mips.c	(revision 120822)
+++ gcc/config/mips/mips.c	(working copy)
@@ -303,11 +303,7 @@ static void mips_set_tune (const struct 
 static bool mips_handle_option (size_t, const char *, int);
 static struct machine_function *mips_init_machine_status (void);
 static void print_operand_reloc (FILE *, rtx, const char **);
-#if TARGET_IRIX
-static void irix_output_external_libcall (rtx);
-#endif
 static void mips_file_start (void);
-static void mips_file_end (void);
 static bool mips_rewrite_small_data_p (rtx);
 static int mips_small_data_pattern_1 (rtx *, void *);
 static int mips_rewrite_small_data_1 (rtx *, void *);
@@ -552,19 +548,6 @@ int sdb_label_count = 0;
 /* Next label # for each statement for Silicon Graphics IRIS systems.  */
 int sym_lineno = 0;
 
-/* Linked list of all externals that are to be emitted when optimizing
-   for the global pointer if they haven't been declared by the end of
-   the program with an appropriate .comm or initialization.  */
-
-struct extern_list GTY (())
-{
-  struct extern_list *next;	/* next external */
-  const char *name;		/* name of the external */
-  int size;			/* size in bytes */
-};
-
-static GTY (()) struct extern_list *extern_head = 0;
-
 /* Name of the file containing the current function.  */
 const char *current_function_file = "";
 
@@ -1144,9 +1127,7 @@ #define TARGET_IN_SMALL_DATA_P mips_in_s
 #define TARGET_MACHINE_DEPENDENT_REORG mips_reorg
 
 #undef TARGET_ASM_FILE_START
-#undef TARGET_ASM_FILE_END
 #define TARGET_ASM_FILE_START mips_file_start
-#define TARGET_ASM_FILE_END mips_file_end
 #undef TARGET_ASM_FILE_START_FILE_DIRECTIVE
 #define TARGET_ASM_FILE_START_FILE_DIRECTIVE true
 
@@ -5786,48 +5767,38 @@ print_operand_address (FILE *file, rtx x
    the -G limit but declared by the user to be in a section other
    than .sbss or .sdata.  */
 
-int
-mips_output_external (FILE *file ATTRIBUTE_UNUSED, tree decl, const char *name)
-{
-  register struct extern_list *p;
-
-  if (!TARGET_EXPLICIT_RELOCS && mips_in_small_data_p (decl))
-    {
-      p = (struct extern_list *) ggc_alloc (sizeof (struct extern_list));
-      p->next = extern_head;
-      p->name = name;
-      p->size = int_size_in_bytes (TREE_TYPE (decl));
-      extern_head = p;
-    }
-
-  if (TARGET_IRIX && mips_abi == ABI_32 && TREE_CODE (decl) == FUNCTION_DECL)
-    {
-      p = (struct extern_list *) ggc_alloc (sizeof (struct extern_list));
-      p->next = extern_head;
-      p->name = name;
-      p->size = -1;
-      extern_head = p;
-    }
-
-  return 0;
-}
-
-#if TARGET_IRIX
-static void
-irix_output_external_libcall (rtx fun)
+void
+mips_output_external (FILE *file, tree decl, const char *name)
 {
-  register struct extern_list *p;
+  default_elf_asm_output_external (file, decl, name);
 
-  if (mips_abi == ABI_32)
-    {
-      p = (struct extern_list *) ggc_alloc (sizeof (struct extern_list));
-      p->next = extern_head;
-      p->name = XSTR (fun, 0);
-      p->size = -1;
-      extern_head = p;
+  /* We output the name if and only if TREE_SYMBOL_REFERENCED is
+     set in order to avoid putting out names that are never really
+     used. */
+  if (TREE_SYMBOL_REFERENCED (DECL_ASSEMBLER_NAME (decl)))
+    {
+      if (!TARGET_EXPLICIT_RELOCS && mips_in_small_data_p (decl))
+	{
+	  fputs ("\t.extern\t", file);
+	  assemble_name (file, name);
+	  fprintf (file, ", " HOST_WIDE_INT_PRINT_DEC "\n",
+		   int_size_in_bytes (TREE_TYPE (decl)));
+	}
+      else if (TARGET_IRIX
+	       && mips_abi == ABI_32
+	       && TREE_CODE (decl) == FUNCTION_DECL)
+	{
+	  /* In IRIX 5 or IRIX 6 for the O32 ABI, we must output a
+	     `.global name .text' directive for every used but
+	     undefined function.  If we don't, the linker may perform
+	     an optimization (skipping over the insns that set $gp)
+	     when it is unsafe.  */
+	  fputs ("\t.globl ", file);
+	  assemble_name (file, name);
+	  fputs (" .text\n", file);
+	}
     }
 }
-#endif
 
 /* Emit a new filename to a stream.  If we are smuggling stabs, try to
    put out a MIPS ECOFF file and a stab.  */
@@ -5989,50 +5960,6 @@ mips_output_aligned_bss (FILE *stream, t
 }
 #endif
 
-/* Implement TARGET_ASM_FILE_END.  When using assembler macros, emit
-   .externs for any small-data variables that turned out to be external.  */
-
-static void
-mips_file_end (void)
-{
-  tree name_tree;
-  struct extern_list *p;
-
-  if (extern_head)
-    {
-      fputs ("\n", asm_out_file);
-
-      for (p = extern_head; p != 0; p = p->next)
-	{
-	  name_tree = get_identifier (p->name);
-
-	  /* Positively ensure only one .extern for any given symbol.  */
-	  if (!TREE_ASM_WRITTEN (name_tree)
-	      && TREE_SYMBOL_REFERENCED (name_tree))
-	    {
-	      TREE_ASM_WRITTEN (name_tree) = 1;
-	      /* In IRIX 5 or IRIX 6 for the O32 ABI, we must output a
-		 `.global name .text' directive for every used but
-		 undefined function.  If we don't, the linker may perform
-		 an optimization (skipping over the insns that set $gp)
-		 when it is unsafe.  */
-	      if (TARGET_IRIX && mips_abi == ABI_32 && p->size == -1)
-		{
-		  fputs ("\t.globl ", asm_out_file);
-		  assemble_name (asm_out_file, p->name);
-		  fputs (" .text\n", asm_out_file);
-		}
-	      else
-		{
-		  fputs ("\t.extern\t", asm_out_file);
-		  assemble_name (asm_out_file, p->name);
-		  fprintf (asm_out_file, ", %d\n", p->size);
-		}
-	    }
-	}
-    }
-}
-
 /* Implement ASM_OUTPUT_ALIGNED_DECL_COMMON.  This is usually the same as the
    elfos.h version, but we also need to handle -muninit-const-in-rodata.  */
 


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]