[patch i386]: Add support of "hotfix" -feature for x64

[patch i386]: Add support of "hotfix" -feature for x64

[Kai Tietz]

[-- Attachment #1: Type: text/plain, Size: 1251 bytes --]

Hello,

this patch adds the hot-patchabel Win32 feature used by Wine also for x64 
targets. Additionally it adds the missing feature part of adding prefix 
before function as hot-patchable area, as defined by win32 ABI.

ChangeLog gcc/

2010-06-30  Kai Tietz  <kai.tietz@onevision.com>

        * config/i386/i386.c (ix86_function_ms_hook_prologue): Enable x64 
support.
        (ix86_expand_prologue): Likewise.
        (ix86_handle_fndecl_attribute): Likewise.
        (ix86_asm_declare_function_name): New function for 
ASM_DECLARE_FUNCTION_NAME.
        * config/i386/i386.h (ASM_DECLARE_FUNCTION_NAME): New macro.
        * config/i386/cygming.h (ASM_DECLARE_FUNCTION_NAME): Removed.
        (SUBTARGET_ASM_DECLARE_FUNCTION_NAME): New macro.
        * config/i386/i386-protos.h (ix86_asm_declare_function_name): New.
        * doc/extend.texi: Adjust documentation about ms_hook_prologue
        attribute.

ChangeLog gcc/testsuite

2010-06-30  Kai Tietz  <kai.tietz@onevision.com>

        * gcc.target/i386/ms_hook_prologue.c: Add x64 ms_hook_prologue 
support.
        * gcc.target/i386/i386.exp: Likewise.

Regards,
Kai

|  (\_/)  This is Bunny. Copy and paste Bunny
| (='.'=) into your signature to help him gain
| (")_(") world domination.

[-- Attachment #2: adfn_hook.diff --]
[-- Type: application/octet-stream, Size: 8953 bytes --]

Index: gcc/gcc/config/i386/cygming.h
===================================================================
--- gcc.orig/gcc/config/i386/cygming.h	2010-06-07 12:36:23.000000000 +0200
+++ gcc/gcc/config/i386/cygming.h	2010-06-30 11:02:41.618620500 +0200
@@ -269,14 +269,13 @@ do {						\
 /* Write the extra assembler code needed to declare a function
    properly.  If we are generating SDB debugging information, this
    will happen automatically, so we only need to handle other cases.  */
-#undef ASM_DECLARE_FUNCTION_NAME
-#define ASM_DECLARE_FUNCTION_NAME(FILE, NAME, DECL)			\
+#undef SUBTARGET_ASM_DECLARE_FUNCTION_NAME
+#define SUBTARGET_ASM_DECLARE_FUNCTION_NAME(FILE, NAME, DECL)		\
   do									\
     {									\
       i386_pe_maybe_record_exported_symbol (DECL, NAME, 0);		\
       if (write_symbols != SDB_DEBUG)					\
 	i386_pe_declare_function_type (FILE, NAME, TREE_PUBLIC (DECL));	\
-      ASM_OUTPUT_LABEL (FILE, NAME);					\
     }									\
   while (0)
 
Index: gcc/gcc/config/i386/i386-protos.h
===================================================================
--- gcc.orig/gcc/config/i386/i386-protos.h	2010-06-28 13:08:20.000000000 +0200
+++ gcc/gcc/config/i386/i386-protos.h	2010-06-30 10:57:07.206920900 +0200
@@ -136,6 +136,7 @@ extern enum machine_mode ix86_fp_compare
 
 extern rtx ix86_libcall_value (enum machine_mode);
 extern bool ix86_function_arg_regno_p (int);
+extern void ix86_asm_declare_function_name (FILE *, const char *, tree);
 extern int ix86_function_arg_boundary (enum machine_mode, tree);
 extern bool ix86_sol10_return_in_memory (const_tree,const_tree);
 extern rtx ix86_force_to_memory (enum machine_mode, rtx);
Index: gcc/gcc/config/i386/i386.c
===================================================================
--- gcc.orig/gcc/config/i386/i386.c	2010-06-29 17:06:23.000000000 +0200
+++ gcc/gcc/config/i386/i386.c	2010-06-30 13:41:42.325140600 +0200
@@ -5009,18 +5009,15 @@ ix86_function_type_abi (const_tree fntyp
 static bool
 ix86_function_ms_hook_prologue (const_tree fntype)
 {
-  if (!TARGET_64BIT)
+  if (lookup_attribute ("ms_hook_prologue", DECL_ATTRIBUTES (fntype)))
     {
-      if (lookup_attribute ("ms_hook_prologue", DECL_ATTRIBUTES (fntype)))
-        {
-          if (decl_function_context (fntype) != NULL_TREE)
-          {
-            error_at (DECL_SOURCE_LOCATION (fntype),
-                "ms_hook_prologue is not compatible with nested function");
-          }
+      if (decl_function_context (fntype) != NULL_TREE)
+      {
+	error_at (DECL_SOURCE_LOCATION (fntype),
+	    "ms_hook_prologue is not compatible with nested function");
+      }
 
-          return true;
-        }
+      return true;
     }
   return false;
 }
@@ -5043,6 +5040,52 @@ ix86_cfun_abi (void)
   return cfun->machine->call_abi;
 }
 
+/* Write the extra assembler code needed to declare a function properly.  */
+
+void
+ix86_asm_declare_function_name (FILE *asm_out_file, const char *fname,
+				tree decl)
+{
+  bool is_ms_hook = ((decl && ix86_function_ms_hook_prologue (decl)) ? true
+  								     : false);
+#ifdef SUBTARGET_ASM_DECLARE_FUNCTION_NAME
+  SUBTARGET_ASM_DECLARE_FUNCTION_NAME (asm_out_file, fname, decl);
+#endif
+
+  if (is_ms_hook)
+    {
+      int filler_count = (TARGET_64BIT ? 32 : 16);
+      unsigned int filler_cc = 0xcccccccc;
+
+      while (filler_count > 0)
+        {
+	  fprintf (asm_out_file, ASM_LONG " 0x%x", filler_cc);
+	  filler_count -= 4;
+	  while ((filler_count & 0xf) != 0)
+	    {
+	      fprintf (asm_out_file, ", 0x%x", filler_cc);
+	      filler_count -= 4;
+	    }
+	  fprintf (asm_out_file, "\n");
+	}
+    }
+
+  ASM_OUTPUT_LABEL (asm_out_file, fname);
+
+  /* Output magic byte marker, if hot-patch attribute is set.
+     For x86 case frame-pointer prologue will be emitted in
+     expand_prologue.  */
+  if (is_ms_hook)
+    {
+      if (TARGET_64BIT)
+	/* leaq [%rsp + 0], %rsp  */
+	asm_fprintf (asm_out_file, ASM_BYTE "0x48, 0x8d, 0xa4, 0x24, 0x00, 0x00, 0x00, 0x00\n");
+      else
+        /* movl.s %edi, %edi.  */
+	asm_fprintf (asm_out_file, ASM_BYTE "0x8b, 0xff\n");
+    }
+}
+
 /* regclass.c  */
 extern void init_regs (void);
 
@@ -8837,7 +8880,7 @@ ix86_expand_prologue (void)
 
   ix86_compute_frame_layout (&frame);
 
-  if (ix86_function_ms_hook_prologue (current_function_decl))
+  if (!TARGET_64BIT && ix86_function_ms_hook_prologue (current_function_decl))
     {
       rtx push, mov;
 
@@ -8850,8 +8893,6 @@ ix86_expand_prologue (void)
 	 functions in Microsoft Windows XP Service Pack 2 and newer.
 	 Wine uses this to enable Windows apps to hook the Win32 API
 	 functions provided by Wine.  */
-      insn = emit_insn (gen_vswapmov (gen_rtx_REG (SImode, DI_REG),
-				      gen_rtx_REG (SImode, DI_REG)));
       push = emit_insn (gen_push (hard_frame_pointer_rtx));
       mov = emit_insn (gen_vswapmov (hard_frame_pointer_rtx,
 				     stack_pointer_rtx));
@@ -26587,15 +26628,9 @@ ix86_handle_fndecl_attribute (tree *node
       return NULL_TREE;
     }
 
-  if (TARGET_64BIT)
-    {
-      warning (OPT_Wattributes, "%qE attribute only available for 32-bit",
-               name);
-      return NULL_TREE;
-    }
-
 #ifndef HAVE_AS_IX86_SWAP
-  sorry ("ms_hook_prologue attribute needs assembler swap suffix support");
+  if (!TARGET_64BIT)
+    sorry ("ms_hook_prologue attribute needs assembler swap suffix support");
 #endif
 
     return NULL_TREE;
Index: gcc/gcc/config/i386/i386.h
===================================================================
--- gcc.orig/gcc/config/i386/i386.h	2010-06-30 10:03:12.000000000 +0200
+++ gcc/gcc/config/i386/i386.h	2010-06-30 11:01:41.492850900 +0200
@@ -2122,6 +2122,14 @@ do {									\
     }
 #endif
 
+/* Write the extra assembler code needed to declare a function
+   properly.  Target can add additional code by the sub-target
+   macro SUBTARGET_ASM_DECLARE_FUNCTION_NAME.  */
+
+#undef ASM_DECLARE_FUNCTION_NAME
+#define ASM_DECLARE_FUNCTION_NAME(FILE, NAME, DECL) \
+  ix86_asm_declare_function_name (FILE, NAME, DECL)
+
 /* Under some conditions we need jump tables in the text section,
    because the assembler cannot handle label differences between
    sections.  This is the case for x86_64 on Mach-O for example.  */
Index: gcc/gcc/doc/extend.texi
===================================================================
--- gcc.orig/gcc/doc/extend.texi	2010-06-28 13:07:29.000000000 +0200
+++ gcc/gcc/doc/extend.texi	2010-06-30 13:27:08.772674900 +0200
@@ -2736,10 +2736,10 @@ the @option{-maccumulate-outgoing-args} 
 @item ms_hook_prologue
 @cindex @code{ms_hook_prologue} attribute
 
-On 32 bit i[34567]86-*-* targets, you can use this function attribute to make
-gcc generate the "hot-patching" function prologue used in Win32 API
-functions in Microsoft Windows XP Service Pack 2 and newer. This requires
-support for the swap suffix in the assembler. (GNU Binutils 2.19.51 or later)
+On 32 bit i[34567]86-*-* targets and 64 bit x86_64-*-* targets, you can use
+this function attribute to make gcc generate the "hot-patching" function
+prologue used in Win32 API functions in Microsoft Windows XP Service Pack 2
+and newer.
 
 @item naked
 @cindex function without a prologue/epilogue code
Index: gcc/gcc/testsuite/gcc.target/i386/ms_hook_prologue.c
===================================================================
--- gcc.orig/gcc/testsuite/gcc.target/i386/ms_hook_prologue.c	2009-10-27 09:30:57.000000000 +0100
+++ gcc/gcc/testsuite/gcc.target/i386/ms_hook_prologue.c	2010-06-30 13:33:32.837632900 +0200
@@ -11,7 +11,7 @@ int __attribute__ ((__ms_hook_prologue__
   /* The NOP mov must not be optimized away by optimizations.
      The push %ebp, mov %esp, %ebp must not be removed by
      -fomit-frame-pointer */
-
+#ifndef __x86_64__
   /* movl.s %edi, %edi */
   if(*ptr++ != 0x8b) return 1;
   if(*ptr++ != 0xff) return 1;
@@ -20,6 +20,15 @@ int __attribute__ ((__ms_hook_prologue__
   /* movl.s %esp, %ebp */
   if(*ptr++ != 0x8b) return 1;
   if(*ptr++ != 0xec) return 1;
+#else
+  /* leaq 0(%rsp), %rsp */
+  if (*ptr++ != 0x48) return 1;
+  if (*ptr++ != 0x8d) return 1;
+  if (*ptr++ != 0xa4) return 1;
+  if (*ptr++ != 0x24) return 1;
+  if (ptr[0] != 0 || ptr[1] != 0 || ptr[2] != 0 || ptr[3] != 0)
+    return 1;
+#endif
   return 0;
 }
 
Index: gcc/gcc/testsuite/gcc.target/i386/i386.exp
===================================================================
--- gcc.orig/gcc/testsuite/gcc.target/i386/i386.exp	2010-05-21 12:40:33.000000000 +0200
+++ gcc/gcc/testsuite/gcc.target/i386/i386.exp	2010-06-30 13:57:25.904162600 +0200
@@ -27,8 +27,7 @@ load_lib gcc-dg.exp
 
 # Return 1 if attribute ms_hook_prologue is supported.
 proc check_effective_target_ms_hook_prologue { } {
-    if { [check_effective_target_ilp32]
-	 && [check_no_compiler_messages ms_hook_prologue object {
+    if { [check_no_compiler_messages ms_hook_prologue object {
 	     void __attribute__ ((__ms_hook_prologue__)) foo ();
 	 } ""] } {
 	return 1

Re: [patch i386]: Add support of "hotfix" -feature for x64

[Richard Henderson]

On 06/30/2010 04:58 AM, Kai Tietz wrote:
> Hello,
> 
> this patch adds the hot-patchabel Win32 feature used by Wine also for x64 
> targets. Additionally it adds the missing feature part of adding prefix 
> before function as hot-patchable area, as defined by win32 ABI.

I like most of this patch.  There are a few quirks...

+      while (filler_count > 0)
+        {
+	  fprintf (asm_out_file, ASM_LONG " 0x%x", filler_cc);
+	  filler_count -= 4;
+	  while ((filler_count & 0xf) != 0)
+	    {
+	      fprintf (asm_out_file, ", 0x%x", filler_cc);
+	      filler_count -= 4;
+	    }
+	  fprintf (asm_out_file, "\n");
+	}

The double loop overcomplicates things.  I think you're better off
with just a plain for-loop.

+	/* leaq [%rsp + 0], %rsp  */
+	asm_fprintf (asm_out_file, ASM_BYTE "0x48, 0x8d, 0xa4, 0x24, 0x00, 0x00, 0x00, 0x00\n");

Needs line wrap.

 	 Wine uses this to enable Windows apps to hook the Win32 API
 	 functions provided by Wine.  */
-      insn = emit_insn (gen_vswapmov (gen_rtx_REG (SImode, DI_REG),
-				      gen_rtx_REG (SImode, DI_REG)));
       push = emit_insn (gen_push (hard_frame_pointer_rtx));

The preceeding comment needs adjusting to match the new code.  Just
mentioning that the nop-move is emitted elsewhere should be enough.



r~

Re: [patch i386]: Add support of "hotfix" -feature for x64

[Kai Tietz]

[-- Attachment #1: Type: text/plain, Size: 1796 bytes --]

2010/7/2 Richard Henderson <rth@redhat.com>:
> On 06/30/2010 04:58 AM, Kai Tietz wrote:
>> Hello,
>>
>> this patch adds the hot-patchabel Win32 feature used by Wine also for x64
>> targets. Additionally it adds the missing feature part of adding prefix
>> before function as hot-patchable area, as defined by win32 ABI.
>
> I like most of this patch.  There are a few quirks...
>
> +      while (filler_count > 0)
> +        {
> +         fprintf (asm_out_file, ASM_LONG " 0x%x", filler_cc);
> +         filler_count -= 4;
> +         while ((filler_count & 0xf) != 0)
> +           {
> +             fprintf (asm_out_file, ", 0x%x", filler_cc);
> +             filler_count -= 4;
> +           }
> +         fprintf (asm_out_file, "\n");
> +       }
>
> The double loop overcomplicates things.  I think you're better off
> with just a plain for-loop.
>
> +       /* leaq [%rsp + 0], %rsp  */
> +       asm_fprintf (asm_out_file, ASM_BYTE "0x48, 0x8d, 0xa4, 0x24, 0x00, 0x00, 0x00, 0x00\n");
>
> Needs line wrap.
>
>         Wine uses this to enable Windows apps to hook the Win32 API
>         functions provided by Wine.  */
> -      insn = emit_insn (gen_vswapmov (gen_rtx_REG (SImode, DI_REG),
> -                                     gen_rtx_REG (SImode, DI_REG)));
>       push = emit_insn (gen_push (hard_frame_pointer_rtx));
>
> The preceeding comment needs adjusting to match the new code.  Just
> mentioning that the nop-move is emitted elsewhere should be enough.
>
>
>
> r~
>

Here is the patch with suggested corrections.
Tested for i686-pc-linux and x86_64-pc-mingw32 targets. Ok for apply?

Regards,
Kai

-- 
|  (\_/) This is Bunny. Copy and paste
| (='.'=) Bunny into your signature to help
| (")_(") him gain world domination

[-- Attachment #2: adfn_hook.diff --]
[-- Type: application/octet-stream, Size: 8209 bytes --]

Index: gcc/gcc/config/i386/cygming.h
===================================================================
--- gcc.orig/gcc/config/i386/cygming.h	2010-06-07 10:58:21.000000000 +0200
+++ gcc/gcc/config/i386/cygming.h	2010-07-02 20:16:28.320809200 +0200
@@ -269,14 +269,13 @@
 /* Write the extra assembler code needed to declare a function
    properly.  If we are generating SDB debugging information, this
    will happen automatically, so we only need to handle other cases.  */
-#undef ASM_DECLARE_FUNCTION_NAME
-#define ASM_DECLARE_FUNCTION_NAME(FILE, NAME, DECL)			\
+#undef SUBTARGET_ASM_DECLARE_FUNCTION_NAME
+#define SUBTARGET_ASM_DECLARE_FUNCTION_NAME(FILE, NAME, DECL)		\
   do									\
     {									\
       i386_pe_maybe_record_exported_symbol (DECL, NAME, 0);		\
       if (write_symbols != SDB_DEBUG)					\
 	i386_pe_declare_function_type (FILE, NAME, TREE_PUBLIC (DECL));	\
-      ASM_OUTPUT_LABEL (FILE, NAME);					\
     }									\
   while (0)
 
Index: gcc/gcc/config/i386/i386-protos.h
===================================================================
--- gcc.orig/gcc/config/i386/i386-protos.h	2010-07-02 20:12:07.000000000 +0200
+++ gcc/gcc/config/i386/i386-protos.h	2010-07-02 20:16:28.323809300 +0200
@@ -136,6 +136,7 @@
 
 extern rtx ix86_libcall_value (enum machine_mode);
 extern bool ix86_function_arg_regno_p (int);
+extern void ix86_asm_declare_function_name (FILE *, const char *, tree);
 extern int ix86_function_arg_boundary (enum machine_mode, tree);
 extern bool ix86_sol10_return_in_memory (const_tree,const_tree);
 extern rtx ix86_force_to_memory (enum machine_mode, rtx);
Index: gcc/gcc/config/i386/i386.c
===================================================================
--- gcc.orig/gcc/config/i386/i386.c	2010-07-02 20:12:07.000000000 +0200
+++ gcc/gcc/config/i386/i386.c	2010-07-03 10:58:48.045641000 +0200
@@ -5009,18 +5009,15 @@
 static bool
 ix86_function_ms_hook_prologue (const_tree fntype)
 {
-  if (!TARGET_64BIT)
+  if (lookup_attribute ("ms_hook_prologue", DECL_ATTRIBUTES (fntype)))
     {
-      if (lookup_attribute ("ms_hook_prologue", DECL_ATTRIBUTES (fntype)))
-        {
-          if (decl_function_context (fntype) != NULL_TREE)
-          {
-            error_at (DECL_SOURCE_LOCATION (fntype),
-                "ms_hook_prologue is not compatible with nested function");
-          }
+      if (decl_function_context (fntype) != NULL_TREE)
+      {
+	error_at (DECL_SOURCE_LOCATION (fntype),
+	    "ms_hook_prologue is not compatible with nested function");
+      }
 
-          return true;
-        }
+      return true;
     }
   return false;
 }
@@ -5043,6 +5040,53 @@
   return cfun->machine->call_abi;
 }
 
+/* Write the extra assembler code needed to declare a function properly.  */
+
+void
+ix86_asm_declare_function_name (FILE *asm_out_file, const char *fname,
+				tree decl)
+{
+  bool is_ms_hook = ((decl && ix86_function_ms_hook_prologue (decl)) ? true
+  								     : false);
+#ifdef SUBTARGET_ASM_DECLARE_FUNCTION_NAME
+  SUBTARGET_ASM_DECLARE_FUNCTION_NAME (asm_out_file, fname, decl);
+#endif
+
+  if (is_ms_hook)
+    {
+      int filler_count = (TARGET_64BIT ? 32 : 16);
+      unsigned int filler_cc = 0xcccccccc;
+
+      for (; filler_count > 0;)
+	{
+	  if ((filler_count & 15) == 0)
+	    fprintf (asm_out_file, ASM_LONG " 0x%x", filler_cc);
+	  else
+	    fprintf (asm_out_file, ", 0x%x", filler_cc);
+	  filler_count -= 4;
+	  if ((filler_count & 15) == 0)
+	    fprintf (asm_out_file, "\n");
+	}
+    }
+
+  ASM_OUTPUT_LABEL (asm_out_file, fname);
+
+  /* Output magic byte marker, if hot-patch attribute is set.
+     For x86 case frame-pointer prologue will be emitted in
+     expand_prologue.  */
+  if (is_ms_hook)
+    {
+      if (TARGET_64BIT)
+	/* leaq [%rsp + 0], %rsp  */
+	asm_fprintf (asm_out_file, ASM_BYTE
+		     "0x48, 0x8d, 0xa4, 0x24, "
+		     "0x00, 0x00, 0x00, 0x00\n");
+      else
+        /* movl.s %edi, %edi.  */
+	asm_fprintf (asm_out_file, ASM_BYTE "0x8b, 0xff\n");
+    }
+}
+
 /* regclass.c  */
 extern void init_regs (void);
 
@@ -8688,21 +8732,24 @@
 
   ix86_compute_frame_layout (&frame);
 
-  if (ix86_function_ms_hook_prologue (current_function_decl))
+  if (!TARGET_64BIT && ix86_function_ms_hook_prologue (current_function_decl))
     {
       rtx push, mov;
 
       /* Make sure the function starts with
-	 8b ff     movl.s %edi,%edi
+	 8b ff     movl.s %edi,%edi (see below in text)
 	 55        push   %ebp
 	 8b ec     movl.s %esp,%ebp
 
 	 This matches the hookable function prologue in Win32 API
 	 functions in Microsoft Windows XP Service Pack 2 and newer.
 	 Wine uses this to enable Windows apps to hook the Win32 API
-	 functions provided by Wine.  */
-      insn = emit_insn (gen_vswapmov (gen_rtx_REG (SImode, DI_REG),
-				      gen_rtx_REG (SImode, DI_REG)));
+	 functions provided by Wine.
+	 Remark: Initial nop-move gets emitted by the function
+	 ix86_asm_declare_function_name and isn't part of this
+	 function.  The following instruction don't get hard-coded
+	 in ix86_asm_declare_function_name too, as here notes
+	 for those instructions are necessary for unwinder/debug.  */
       push = emit_insn (gen_push (hard_frame_pointer_rtx));
       mov = emit_insn (gen_vswapmov (hard_frame_pointer_rtx,
 				     stack_pointer_rtx));
@@ -26431,15 +26478,9 @@
       return NULL_TREE;
     }
 
-  if (TARGET_64BIT)
-    {
-      warning (OPT_Wattributes, "%qE attribute only available for 32-bit",
-               name);
-      return NULL_TREE;
-    }
-
 #ifndef HAVE_AS_IX86_SWAP
-  sorry ("ms_hook_prologue attribute needs assembler swap suffix support");
+  if (!TARGET_64BIT)
+    sorry ("ms_hook_prologue attribute needs assembler swap suffix support");
 #endif
 
     return NULL_TREE;
Index: gcc/gcc/config/i386/i386.h
===================================================================
--- gcc.orig/gcc/config/i386/i386.h	2010-07-02 20:12:07.000000000 +0200
+++ gcc/gcc/config/i386/i386.h	2010-07-02 20:16:28.363811600 +0200
@@ -2079,6 +2079,14 @@
     }
 #endif
 
+/* Write the extra assembler code needed to declare a function
+   properly.  Target can add additional code by the sub-target
+   macro SUBTARGET_ASM_DECLARE_FUNCTION_NAME.  */
+
+#undef ASM_DECLARE_FUNCTION_NAME
+#define ASM_DECLARE_FUNCTION_NAME(FILE, NAME, DECL) \
+  ix86_asm_declare_function_name (FILE, NAME, DECL)
+
 /* Under some conditions we need jump tables in the text section,
    because the assembler cannot handle label differences between
    sections.  This is the case for x86_64 on Mach-O for example.  */
Index: gcc/gcc/testsuite/gcc.target/i386/i386.exp
===================================================================
--- gcc.orig/gcc/testsuite/gcc.target/i386/i386.exp	2010-05-21 20:15:48.000000000 +0200
+++ gcc/gcc/testsuite/gcc.target/i386/i386.exp	2010-07-02 20:16:28.393813300 +0200
@@ -27,8 +27,7 @@
 
 # Return 1 if attribute ms_hook_prologue is supported.
 proc check_effective_target_ms_hook_prologue { } {
-    if { [check_effective_target_ilp32]
-	 && [check_no_compiler_messages ms_hook_prologue object {
+    if { [check_no_compiler_messages ms_hook_prologue object {
 	     void __attribute__ ((__ms_hook_prologue__)) foo ();
 	 } ""] } {
 	return 1
Index: gcc/gcc/testsuite/gcc.target/i386/ms_hook_prologue.c
===================================================================
--- gcc.orig/gcc/testsuite/gcc.target/i386/ms_hook_prologue.c	2010-05-08 18:05:25.000000000 +0200
+++ gcc/gcc/testsuite/gcc.target/i386/ms_hook_prologue.c	2010-07-02 20:16:28.391813200 +0200
@@ -11,7 +11,7 @@
   /* The NOP mov must not be optimized away by optimizations.
      The push %ebp, mov %esp, %ebp must not be removed by
      -fomit-frame-pointer */
-
+#ifndef __x86_64__
   /* movl.s %edi, %edi */
   if(*ptr++ != 0x8b) return 1;
   if(*ptr++ != 0xff) return 1;
@@ -20,6 +20,15 @@
   /* movl.s %esp, %ebp */
   if(*ptr++ != 0x8b) return 1;
   if(*ptr++ != 0xec) return 1;
+#else
+  /* leaq 0(%rsp), %rsp */
+  if (*ptr++ != 0x48) return 1;
+  if (*ptr++ != 0x8d) return 1;
+  if (*ptr++ != 0xa4) return 1;
+  if (*ptr++ != 0x24) return 1;
+  if (ptr[0] != 0 || ptr[1] != 0 || ptr[2] != 0 || ptr[3] != 0)
+    return 1;
+#endif
   return 0;
 }
 

Re: [patch i386]: Add support of "hotfix" -feature for x64

[Richard Henderson]

On 07/03/2010 02:05 AM, Kai Tietz wrote:
>> The double loop overcomplicates things.  I think you're better off
>> with just a plain for-loop.

+      for (; filler_count > 0;)
+	{
+	  if ((filler_count & 15) == 0)
+	    fprintf (asm_out_file, ASM_LONG " 0x%x", filler_cc);
+	  else
+	    fprintf (asm_out_file, ", 0x%x", filler_cc);
+	  filler_count -= 4;
+	  if ((filler_count & 15) == 0)
+	    fprintf (asm_out_file, "\n");
+	}

Better, but honestly I'd been thinking just to bring one
directive per line, rather than trying to block the output
onto several lines.  I.e.

  for (i = 0; i < filler_count; i += 4)
    fprintf (asm_out_file, ASM_LONG " 0x%x\n", filler_cc);


Ok with that change.


r~

Re: [patch i386]: Add support of "hotfix" -feature for x64

[H.J. Lu]

On Sat, Jul 3, 2010 at 2:05 AM, Kai Tietz <ktietz70@googlemail.com> wrote:
> 2010/7/2 Richard Henderson <rth@redhat.com>:
>> On 06/30/2010 04:58 AM, Kai Tietz wrote:
>>> Hello,
>>>
>>> this patch adds the hot-patchabel Win32 feature used by Wine also for x64
>>> targets. Additionally it adds the missing feature part of adding prefix
>>> before function as hot-patchable area, as defined by win32 ABI.
>>
>> I like most of this patch.  There are a few quirks...
>>
>> +      while (filler_count > 0)
>> +        {
>> +         fprintf (asm_out_file, ASM_LONG " 0x%x", filler_cc);
>> +         filler_count -= 4;
>> +         while ((filler_count & 0xf) != 0)
>> +           {
>> +             fprintf (asm_out_file, ", 0x%x", filler_cc);
>> +             filler_count -= 4;
>> +           }
>> +         fprintf (asm_out_file, "\n");
>> +       }
>>
>> The double loop overcomplicates things.  I think you're better off
>> with just a plain for-loop.
>>
>> +       /* leaq [%rsp + 0], %rsp  */
>> +       asm_fprintf (asm_out_file, ASM_BYTE "0x48, 0x8d, 0xa4, 0x24, 0x00, 0x00, 0x00, 0x00\n");
>>
>> Needs line wrap.
>>
>>         Wine uses this to enable Windows apps to hook the Win32 API
>>         functions provided by Wine.  */
>> -      insn = emit_insn (gen_vswapmov (gen_rtx_REG (SImode, DI_REG),
>> -                                     gen_rtx_REG (SImode, DI_REG)));
>>       push = emit_insn (gen_push (hard_frame_pointer_rtx));
>>
>> The preceeding comment needs adjusting to match the new code.  Just
>> mentioning that the nop-move is emitted elsewhere should be enough.
>>
>>
>>
>> r~
>>
>
> Here is the patch with suggested corrections.
> Tested for i686-pc-linux and x86_64-pc-mingw32 targets. Ok for apply?
>

I have a very hard time to believe this patch was tested on i686-pc-linux
since revision 161876 is bogus for Linux and caused 5000+ run-time
failures in gcc testsuite on Linux/i686.


-- 
H.J.

Re: [patch i386]: Add support of "hotfix" -feature for x64

[Kai Tietz]

[-- Attachment #1: Type: text/plain, Size: 421 bytes --]

Sorry,

This patch should fix this issue.

ChangeLog

          * config/i386.h (ASM_DECLARE_FUNCTION_NAME): Remove
          define.
          * config/cygming.h (ASM_DECLARE_FUNCTION_NAME):
          Add here instead.

Could you test this patch, if it fixes your issues?

Thanks in advance,
Kai

-- 
|  (\_/) This is Bunny. Copy and paste
| (='.'=) Bunny into your signature to help
| (")_(") him gain world domination

[-- Attachment #2: asm_decl_fct_name.diff --]
[-- Type: application/octet-stream, Size: 1617 bytes --]

Index: gcc/gcc/config/i386/cygming.h
===================================================================
--- gcc.orig/gcc/config/i386/cygming.h	2010-07-06 20:21:44.000000000 +0200
+++ gcc/gcc/config/i386/cygming.h	2010-07-07 08:22:30.300259700 +0200
@@ -212,6 +212,14 @@
   ASM_OUTPUT_LABEL ((STREAM), (NAME));			\
 } while (0)
 
+/* Write the extra assembler code needed to declare a function
+   properly.  Target can add additional code by the sub-target
+   macro SUBTARGET_ASM_DECLARE_FUNCTION_NAME.  */
+
+#undef ASM_DECLARE_FUNCTION_NAME
+#define ASM_DECLARE_FUNCTION_NAME(FILE, NAME, DECL) \
+  ix86_asm_declare_function_name (FILE, NAME, DECL)
+
 /* Output a reference to a label. Fastcall function symbols
    keep their '@' prefix, while other symbols are prefixed
    with user_label_prefix.  */
Index: gcc/gcc/config/i386/i386.h
===================================================================
--- gcc.orig/gcc/config/i386/i386.h	2010-07-06 20:21:44.000000000 +0200
+++ gcc/gcc/config/i386/i386.h	2010-07-07 08:21:21.288312400 +0200
@@ -2082,14 +2082,6 @@
     }
 #endif
 
-/* Write the extra assembler code needed to declare a function
-   properly.  Target can add additional code by the sub-target
-   macro SUBTARGET_ASM_DECLARE_FUNCTION_NAME.  */
-
-#undef ASM_DECLARE_FUNCTION_NAME
-#define ASM_DECLARE_FUNCTION_NAME(FILE, NAME, DECL) \
-  ix86_asm_declare_function_name (FILE, NAME, DECL)
-
 /* Under some conditions we need jump tables in the text section,
    because the assembler cannot handle label differences between
    sections.  This is the case for x86_64 on Mach-O for example.  */

Re: [patch i386]: Add support of "hotfix" -feature for x64

[Kai Tietz]

[-- Attachment #1: Type: text/plain, Size: 664 bytes --]

Hello,

this is the more improved patch, which handles for Wine linux the
hotfix part via ASM_DECLARE_FUNCTION_NAME, too.

ChangeLog

* config/i386.c (ix86_asm_declare_function_name): Add
xname local variable to allow its override as used in darwin.
* config/darwin.h (SUBTARGET_ASM_FUNCTION_NAME): New macro.
(ASM_FUNCTION_NAME): Use
SUBTARGET_ASM_DECLARE_FUNCTION_NAME.
* config/elfos.h: Likewise.
* config/netbsd-aout.h: Likewise.
* config/openbsd.h: Likewise.

HJ, Jack could you please check if this patch works for you?

Regards,
Kai

-- 
|  (\_/) This is Bunny. Copy and paste
| (='.'=) Bunny into your signature to help
| (")_(") him gain world domination

[-- Attachment #2: asm_decl_fct_name.diff --]
[-- Type: application/octet-stream, Size: 5667 bytes --]

Index: gcc/gcc/config/darwin.h
===================================================================
--- gcc.orig/gcc/config/darwin.h	2010-07-07 08:49:37.000000000 +0200
+++ gcc/gcc/config/darwin.h	2010-07-07 09:07:27.769546100 +0200
@@ -635,9 +635,8 @@ extern GTY(()) int darwin_ms_struct;
       assemble_zeros (1);						\
   } while (0)
 
-#define ASM_DECLARE_FUNCTION_NAME(FILE, NAME, DECL)			\
+#define SUBTARGET_ASM_DECLARE_FUNCTION_NAME(FILE, NAME, DECL)		\
   do {									\
-    const char *xname = NAME;						\
     if (GET_CODE (XEXP (DECL_RTL (DECL), 0)) != SYMBOL_REF)		\
       xname = IDENTIFIER_POINTER (DECL_NAME (DECL));			\
     if (! DECL_WEAK (DECL)						\
@@ -649,6 +648,12 @@ extern GTY(()) int darwin_ms_struct;
 	 && (!DECL_COMMON (DECL) || !TREE_PUBLIC (DECL)))		\
         || DECL_INITIAL (DECL))						\
       (* targetm.encode_section_info) (DECL, DECL_RTL (DECL), false);	\
+  } while (0)
+
+#define ASM_DECLARE_FUNCTION_NAME(FILE, NAME, DECL)			\
+  do {									\
+    const char *xname = NAME;						\
+    SUBTARGET_ASM_DECLARE_FUNCTION_NAME (FILE, xname, DECL);		\
     ASM_OUTPUT_LABEL (FILE, xname);					\
   } while (0)
 
Index: gcc/gcc/config/elfos.h
===================================================================
--- gcc.orig/gcc/config/elfos.h	2010-05-13 12:09:42.000000000 +0200
+++ gcc/gcc/config/elfos.h	2010-07-07 09:00:24.252322300 +0200
@@ -277,14 +277,20 @@ see the files COPYING3 and COPYING.RUNTI
    function's return value.  We allow for that here.  */
 
 #ifndef ASM_DECLARE_FUNCTION_NAME
-#define ASM_DECLARE_FUNCTION_NAME(FILE, NAME, DECL)		\
+#define SUBTARGET_ASM_DECLARE_FUNCTION_NAME(FILE, NAME, DECL)	\
   do								\
     {								\
       ASM_OUTPUT_TYPE_DIRECTIVE (FILE, NAME, "function");	\
       ASM_DECLARE_RESULT (FILE, DECL_RESULT (DECL));		\
-      ASM_OUTPUT_LABEL (FILE, NAME);				\
     }								\
   while (0)
+
+#define ASM_DECLARE_FUNCTION_NAME(FILE, NAME, DECL)		\
+  do {								\
+    SUBTARGET_ASM_DECLARE_FUNCTION_NAME(FILE, NAME, DECL);	\
+    ASM_OUTPUT_LABEL (FILE, NAME);				\
+  } while (0)
+
 #endif
 
 /* Write the extra assembler code needed to declare an object properly.  */
Index: gcc/gcc/config/i386/i386.c
===================================================================
--- gcc.orig/gcc/config/i386/i386.c	2010-07-06 21:49:55.000000000 +0200
+++ gcc/gcc/config/i386/i386.c	2010-07-07 08:53:23.829275500 +0200
@@ -5115,6 +5115,7 @@ void
 ix86_asm_declare_function_name (FILE *asm_out_file, const char *fname,
 				tree decl)
 {
+  const char *xname = fname;
   bool is_ms_hook = ((decl && ix86_function_ms_hook_prologue (decl)) ? true
   								     : false);
 #ifdef SUBTARGET_ASM_DECLARE_FUNCTION_NAME
@@ -5130,7 +5131,7 @@ ix86_asm_declare_function_name (FILE *as
 	fprintf (asm_out_file, ASM_LONG " %#x\n", filler_cc);
     }
 
-  ASM_OUTPUT_LABEL (asm_out_file, fname);
+  ASM_OUTPUT_LABEL (asm_out_file, xname);
 
   /* Output magic byte marker, if hot-patch attribute is set.
      For x86 case frame-pointer prologue will be emitted in
Index: gcc/gcc/config/netbsd-aout.h
===================================================================
--- gcc.orig/gcc/config/netbsd-aout.h	2010-05-08 18:17:33.000000000 +0200
+++ gcc/gcc/config/netbsd-aout.h	2010-07-07 09:03:45.956859100 +0200
@@ -135,16 +135,23 @@ along with GCC; see the file COPYING3.
    Some svr4 assemblers need to also have something extra said about the
    function's return value.  We allow for that here.  */
 
-#undef ASM_DECLARE_FUNCTION_NAME
-#define ASM_DECLARE_FUNCTION_NAME(FILE, NAME, DECL)			\
+#undef SUBTARGET_ASM_DECLARE_FUNCTION_NAME
+#define SUBTARGET_ASM_DECLARE_FUNCTION_NAME(FILE, NAME, DECL)		\
   do									\
     {									\
       ASM_OUTPUT_TYPE_DIRECTIVE (FILE, NAME, "function");		\
       ASM_DECLARE_RESULT (FILE, DECL_RESULT (DECL));			\
-      ASM_OUTPUT_LABEL(FILE, NAME);					\
     }									\
   while (0)
 
+#undef ASM_DECLARE_FUNCTION_NAME
+#define ASM_DECLARE_FUNCTION_NAME(FILE, NAME, DECL)                     \
+  do                                                                    \
+    {                                                                   \
+      SUBTARGET_ASM_DECLARE_FUNCTION_NAME(FILE, NAME, DECL);		\
+      ASM_OUTPUT_LABEL(FILE, NAME);                                     \
+    }                                                                   \
+  while (0)
 
 /* Write the extra assembler code needed to declare an object properly.  */
 
Index: gcc/gcc/config/openbsd.h
===================================================================
--- gcc.orig/gcc/config/openbsd.h	2010-05-08 18:17:34.000000000 +0200
+++ gcc/gcc/config/openbsd.h	2010-07-07 09:06:19.131620200 +0200
@@ -211,11 +211,18 @@ while (0)
 /* Extra assembler code needed to declare a function properly.
    Some assemblers may also need to also have something extra said 
    about the function's return value.  We allow for that here.  */
+
+#undef SUBTARGET_ASM_DECLARE_FUNCTION_NAME
+#define SUBTARGET_ASM_DECLARE_FUNCTION_NAME(FILE, NAME, DECL)           \
+  do {                                                                  \
+    ASM_OUTPUT_TYPE_DIRECTIVE (FILE, NAME, "function");                 \
+    ASM_DECLARE_RESULT (FILE, DECL_RESULT (DECL));                      \
+  } while (0)
+
 #undef ASM_DECLARE_FUNCTION_NAME
 #define ASM_DECLARE_FUNCTION_NAME(FILE, NAME, DECL)			\
   do {									\
-    ASM_OUTPUT_TYPE_DIRECTIVE (FILE, NAME, "function");			\
-    ASM_DECLARE_RESULT (FILE, DECL_RESULT (DECL));			\
+    SUBTARGET_ASM_DECLARE_FUNCTION_NAME(FILE, NAME, DECL);		\
     ASM_OUTPUT_LABEL(FILE, NAME);					\
   } while (0)
 #endif

Re: [patch i386]: Add support of "hotfix" -feature for x64

[Jakub Jelinek]

On Wed, Jul 07, 2010 at 09:15:45AM +0200, Kai Tietz wrote:
> this is the more improved patch, which handles for Wine linux the
> hotfix part via ASM_DECLARE_FUNCTION_NAME, too.

This is very ugly.  An OS isn't a subtarget of any kind.
If this is really the way to go, use ASM_DECLARE_OS_FUNCTION_NAME
or something similar.

BTW,

  bool is_ms_hook = ((decl && ix86_function_ms_hook_prologue (decl)) ? true
                                                                     : false);

is horribly ugly.  You should just use

  bool is_ms_hook = decl && ix86_function_ms_hook_prologue (decl);

	Jakub

Re: [patch i386]: Add support of "hotfix" -feature for x64

[Kai Tietz]

[-- Attachment #1: Type: text/plain, Size: 1599 bytes --]

Hello,

Jakub Jelinek <jakub@redhat.com> wrote on 07.07.2010 09:25:20:

> On Wed, Jul 07, 2010 at 09:15:45AM +0200, Kai Tietz wrote:
> > this is the more improved patch, which handles for Wine linux the
> > hotfix part via ASM_DECLARE_FUNCTION_NAME, too.

I renamed by this patch the SUBTARGET_ASM_DECLARE_FUNCTION_NAME to 
ASM_DECLARE_OS_FUNCTION_NAME as suggested.

> This is very ugly.  An OS isn't a subtarget of any kind.
> If this is really the way to go, use ASM_DECLARE_OS_FUNCTION_NAME
> or something similar.
> 
> BTW,
> 
>   bool is_ms_hook = ((decl && ix86_function_ms_hook_prologue (decl)) ? 
true
>                                                                     : 
false);
> 
> is horribly ugly.  You should just use

Cleaned up.

ChangeLog
2010-07-07  Kai Tietz

        PR target/44850
        * config/i386/i386.c (ix86_asm_declare_function_name): Add
        xname local variable to allow its override as used in darwin.
        * config/i386/i386.h (SUBTARGET_ASM_DECLARE_FUNCTION_NAME):
        Rename in comment to ASM_DECLARE_OS_FUNCTION_NAME.
        * config/darwin.h (ASM_DECLARE_OS_FUNCTION_NAME): New macro.
        (ASM_DECLARE_FUNCTION_NAME): Use ASM_DECLARE_OS_FUNCTION_NAME.
        * config/elfos.h: Likewise.
        * config/netbsd-aout.h: Likewise.
        * config/openbsd.h: Likewise.
        * config/i386/cygming.h (SUBTARGET_ASM_DECLARE_FUNCTION_NAME):
        Rename to ASM_DECLARE_OS_FUNCTION_NAME.


Ok for apply?

Regards,
Kai

|  (\_/)  This is Bunny. Copy and paste Bunny
| (='.'=) into your signature to help him gain
| (")_(") world domination.


[-- Attachment #2: pr44850.diff --]
[-- Type: application/octet-stream, Size: 8229 bytes --]

Index: gcc/gcc/config/darwin.h
===================================================================
--- gcc.orig/gcc/config/darwin.h	2010-06-17 10:00:51.000000000 +0200
+++ gcc/gcc/config/darwin.h	2010-07-07 10:19:06.763831800 +0200
@@ -635,21 +635,29 @@ extern GTY(()) int darwin_ms_struct;
       assemble_zeros (1);						\
   } while (0)
 
-#define ASM_DECLARE_FUNCTION_NAME(FILE, NAME, DECL)			\
-  do {									\
-    const char *xname = NAME;						\
-    if (GET_CODE (XEXP (DECL_RTL (DECL), 0)) != SYMBOL_REF)		\
-      xname = IDENTIFIER_POINTER (DECL_NAME (DECL));			\
-    if (! DECL_WEAK (DECL)						\
-        && ((TREE_STATIC (DECL)						\
+#define ASM_DECLARE_OS_FUNCTION_NAME(FILE, NAME, DECL)			\
+  do									\
+    {									\
+      if (GET_CODE (XEXP (DECL_RTL (DECL), 0)) != SYMBOL_REF)		\
+        xname = IDENTIFIER_POINTER (DECL_NAME (DECL));			\
+      if (! DECL_WEAK (DECL)						\
+          && ((TREE_STATIC (DECL)					\
 	     && (!DECL_COMMON (DECL) || !TREE_PUBLIC (DECL)))		\
-            || DECL_INITIAL (DECL)))					\
-        machopic_define_symbol (DECL_RTL (DECL));			\
-    if ((TREE_STATIC (DECL)						\
-	 && (!DECL_COMMON (DECL) || !TREE_PUBLIC (DECL)))		\
-        || DECL_INITIAL (DECL))						\
-      (* targetm.encode_section_info) (DECL, DECL_RTL (DECL), false);	\
-    ASM_OUTPUT_LABEL (FILE, xname);					\
+              || DECL_INITIAL (DECL)))					\
+          machopic_define_symbol (DECL_RTL (DECL));			\
+      if ((TREE_STATIC (DECL)						\
+	   && (!DECL_COMMON (DECL) || !TREE_PUBLIC (DECL)))		\
+          || DECL_INITIAL (DECL))					\
+        (* targetm.encode_section_info) (DECL, DECL_RTL (DECL), false);	\
+    }									\
+  while (0)
+
+
+#define ASM_DECLARE_FUNCTION_NAME(FILE, NAME, DECL)	\
+  do {							\
+    const char *xname = NAME;				\
+    ASM_DECLARE_OS_FUNCTION_NAME (FILE, xname, DECL);	\
+    ASM_OUTPUT_LABEL (FILE, xname);			\
   } while (0)
 
 #undef TARGET_ASM_DECLARE_CONSTANT_NAME
Index: gcc/gcc/config/elfos.h
===================================================================
--- gcc.orig/gcc/config/elfos.h	2010-05-12 10:01:13.000000000 +0200
+++ gcc/gcc/config/elfos.h	2010-07-07 10:16:33.538217600 +0200
@@ -277,14 +277,22 @@ see the files COPYING3 and COPYING.RUNTI
    function's return value.  We allow for that here.  */
 
 #ifndef ASM_DECLARE_FUNCTION_NAME
-#define ASM_DECLARE_FUNCTION_NAME(FILE, NAME, DECL)		\
+#define ASM_DECLARE_OS_FUNCTION_NAME(FILE, NAME, DECL)		\
   do								\
     {								\
       ASM_OUTPUT_TYPE_DIRECTIVE (FILE, NAME, "function");	\
       ASM_DECLARE_RESULT (FILE, DECL_RESULT (DECL));		\
-      ASM_OUTPUT_LABEL (FILE, NAME);				\
     }								\
   while (0)
+
+#define ASM_DECLARE_FUNCTION_NAME(FILE, NAME, DECL)	\
+  do							\
+    {							\
+      ASM_DECLARE_OS_FUNCTION_NAME(FILE, NAME, DECL);	\
+      ASM_OUTPUT_LABEL (FILE, NAME);			\
+    }							\
+  while (0)
+
 #endif
 
 /* Write the extra assembler code needed to declare an object properly.  */
Index: gcc/gcc/config/i386/cygming.h
===================================================================
--- gcc.orig/gcc/config/i386/cygming.h	2010-07-07 09:52:46.000000000 +0200
+++ gcc/gcc/config/i386/cygming.h	2010-07-07 10:02:03.402237800 +0200
@@ -269,8 +269,8 @@ do {						\
 /* Write the extra assembler code needed to declare a function
    properly.  If we are generating SDB debugging information, this
    will happen automatically, so we only need to handle other cases.  */
-#undef SUBTARGET_ASM_DECLARE_FUNCTION_NAME
-#define SUBTARGET_ASM_DECLARE_FUNCTION_NAME(FILE, NAME, DECL)		\
+#undef ASM_DECLARE_OS_FUNCTION_NAME
+#define ASM_OS_DECLARE_FUNCTION_NAME(FILE, NAME, DECL)			\
   do									\
     {									\
       i386_pe_maybe_record_exported_symbol (DECL, NAME, 0);		\
Index: gcc/gcc/config/i386/i386.c
===================================================================
--- gcc.orig/gcc/config/i386/i386.c	2010-07-07 09:52:46.000000000 +0200
+++ gcc/gcc/config/i386/i386.c	2010-07-07 10:01:11.132626900 +0200
@@ -5115,10 +5115,10 @@ void
 ix86_asm_declare_function_name (FILE *asm_out_file, const char *fname,
 				tree decl)
 {
-  bool is_ms_hook = ((decl && ix86_function_ms_hook_prologue (decl)) ? true
-  								     : false);
-#ifdef SUBTARGET_ASM_DECLARE_FUNCTION_NAME
-  SUBTARGET_ASM_DECLARE_FUNCTION_NAME (asm_out_file, fname, decl);
+  const char *xname = fname;
+  bool is_ms_hook = decl && ix86_function_ms_hook_prologue (decl);
+#ifdef ASM_DECLARE_OS_FUNCTION_NAME
+  ASM_DECLARE_OS_FUNCTION_NAME (asm_out_file, fname, decl);
 #endif
 
   if (is_ms_hook)
@@ -5130,7 +5130,7 @@ ix86_asm_declare_function_name (FILE *as
 	fprintf (asm_out_file, ASM_LONG " %#x\n", filler_cc);
     }
 
-  ASM_OUTPUT_LABEL (asm_out_file, fname);
+  ASM_OUTPUT_LABEL (asm_out_file, xname);
 
   /* Output magic byte marker, if hot-patch attribute is set.
      For x86 case frame-pointer prologue will be emitted in
Index: gcc/gcc/config/i386/i386.h
===================================================================
--- gcc.orig/gcc/config/i386/i386.h	2010-07-07 09:52:46.000000000 +0200
+++ gcc/gcc/config/i386/i386.h	2010-07-07 10:03:04.636376400 +0200
@@ -2084,7 +2084,7 @@ do {									\
 
 /* Write the extra assembler code needed to declare a function
    properly.  Target can add additional code by the sub-target
-   macro SUBTARGET_ASM_DECLARE_FUNCTION_NAME.  */
+   macro ASM_DECLARE_OS_FUNCTION_NAME.  */
 
 #undef ASM_DECLARE_FUNCTION_NAME
 #define ASM_DECLARE_FUNCTION_NAME(FILE, NAME, DECL) \
Index: gcc/gcc/config/netbsd-aout.h
===================================================================
--- gcc.orig/gcc/config/netbsd-aout.h	2008-05-08 09:55:21.000000000 +0200
+++ gcc/gcc/config/netbsd-aout.h	2010-07-07 10:10:47.283947400 +0200
@@ -135,16 +135,23 @@ along with GCC; see the file COPYING3.  
    Some svr4 assemblers need to also have something extra said about the
    function's return value.  We allow for that here.  */
 
-#undef ASM_DECLARE_FUNCTION_NAME
-#define ASM_DECLARE_FUNCTION_NAME(FILE, NAME, DECL)			\
+#undef ASM_DECLARE_OS_FUNCTION_NAME
+#define ASM_DECLARE_OS_FUNCTION_NAME(FILE, NAME, DECL)			\
   do									\
     {									\
       ASM_OUTPUT_TYPE_DIRECTIVE (FILE, NAME, "function");		\
       ASM_DECLARE_RESULT (FILE, DECL_RESULT (DECL));			\
-      ASM_OUTPUT_LABEL(FILE, NAME);					\
     }									\
   while (0)
 
+#undef ASM_DECLARE_FUNCTION_NAME
+#define ASM_DECLARE_FUNCTION_NAME(FILE, NAME, DECL)                     \
+  do                                                                    \
+    {                                                                   \
+      ASM_DECLARE_OS_FUNCTION_NAME(FILE, NAME, DECL);			\
+      ASM_OUTPUT_LABEL(FILE, NAME);                                     \
+    }                                                                   \
+  while (0)
 
 /* Write the extra assembler code needed to declare an object properly.  */
 
Index: gcc/gcc/config/openbsd.h
===================================================================
--- gcc.orig/gcc/config/openbsd.h	2009-09-28 10:42:53.000000000 +0200
+++ gcc/gcc/config/openbsd.h	2010-07-07 10:14:24.812958600 +0200
@@ -211,13 +211,25 @@ while (0)
 /* Extra assembler code needed to declare a function properly.
    Some assemblers may also need to also have something extra said 
    about the function's return value.  We allow for that here.  */
+
+#undef ASM_DECLARE_OS_FUNCTION_NAME
+#define ASM_DECLARE_OS_FUNCTION_NAME(FILE, NAME, DECL)		\
+  do								\
+    {								\
+      ASM_OUTPUT_TYPE_DIRECTIVE (FILE, NAME, "function");	\
+      ASM_DECLARE_RESULT (FILE, DECL_RESULT (DECL));		\
+    }								\
+  while (0)
+
 #undef ASM_DECLARE_FUNCTION_NAME
-#define ASM_DECLARE_FUNCTION_NAME(FILE, NAME, DECL)			\
-  do {									\
-    ASM_OUTPUT_TYPE_DIRECTIVE (FILE, NAME, "function");			\
-    ASM_DECLARE_RESULT (FILE, DECL_RESULT (DECL));			\
-    ASM_OUTPUT_LABEL(FILE, NAME);					\
-  } while (0)
+#define ASM_DECLARE_FUNCTION_NAME(FILE, NAME, DECL)	\
+  do							\
+    {							\
+      ASM_DECLARE_OS_FUNCTION_NAME(FILE, NAME, DECL);	\
+      ASM_OUTPUT_LABEL(FILE, NAME);			\
+    }							\
+  while (0)
+
 #endif
 
 #ifndef OBSD_HAS_DECLARE_FUNCTION_SIZE

Re: [patch i386]: Add support of "hotfix" -feature for x64

[Richard Guenther]

On Wed, Jul 7, 2010 at 10:27 AM, Kai Tietz <Kai.Tietz@onevision.com> wrote:
> Hello,
>
> Jakub Jelinek <jakub@redhat.com> wrote on 07.07.2010 09:25:20:
>
>> On Wed, Jul 07, 2010 at 09:15:45AM +0200, Kai Tietz wrote:
>> > this is the more improved patch, which handles for Wine linux the
>> > hotfix part via ASM_DECLARE_FUNCTION_NAME, too.
>
> I renamed by this patch the SUBTARGET_ASM_DECLARE_FUNCTION_NAME to
> ASM_DECLARE_OS_FUNCTION_NAME as suggested.
>
>> This is very ugly.  An OS isn't a subtarget of any kind.
>> If this is really the way to go, use ASM_DECLARE_OS_FUNCTION_NAME
>> or something similar.
>>
>> BTW,
>>
>>   bool is_ms_hook = ((decl && ix86_function_ms_hook_prologue (decl)) ?
> true
>>                                                                     :
> false);
>>
>> is horribly ugly.  You should just use
>
> Cleaned up.

I don't see the "cleaned up" part.

Can you please revert the original patch that caused the massive
breakage on *-linux and post a fixed patch instead (the patch below
doesn't make sense to me on its own).

Thanks,
Richard.

> ChangeLog
> 2010-07-07  Kai Tietz
>
>        PR target/44850
>        * config/i386/i386.c (ix86_asm_declare_function_name): Add
>        xname local variable to allow its override as used in darwin.
>        * config/i386/i386.h (SUBTARGET_ASM_DECLARE_FUNCTION_NAME):
>        Rename in comment to ASM_DECLARE_OS_FUNCTION_NAME.
>        * config/darwin.h (ASM_DECLARE_OS_FUNCTION_NAME): New macro.
>        (ASM_DECLARE_FUNCTION_NAME): Use ASM_DECLARE_OS_FUNCTION_NAME.
>        * config/elfos.h: Likewise.
>        * config/netbsd-aout.h: Likewise.
>        * config/openbsd.h: Likewise.
>        * config/i386/cygming.h (SUBTARGET_ASM_DECLARE_FUNCTION_NAME):
>        Rename to ASM_DECLARE_OS_FUNCTION_NAME.
>
>
> Ok for apply?
>
> Regards,
> Kai
>
> |  (\_/)  This is Bunny. Copy and paste Bunny
> | (='.'=) into your signature to help him gain
> | (")_(") world domination.
>
>

Re: [patch i386]: Add support of "hotfix" -feature for x64

[Kai Tietz]

2010/7/7 Richard Guenther <richard.guenther@gmail.com>:
> On Wed, Jul 7, 2010 at 10:27 AM, Kai Tietz <Kai.Tietz@onevision.com> wrote:
>> Hello,
>>
>> Jakub Jelinek <jakub@redhat.com> wrote on 07.07.2010 09:25:20:
>>
>>> On Wed, Jul 07, 2010 at 09:15:45AM +0200, Kai Tietz wrote:
>>> > this is the more improved patch, which handles for Wine linux the
>>> > hotfix part via ASM_DECLARE_FUNCTION_NAME, too.
>>
>> I renamed by this patch the SUBTARGET_ASM_DECLARE_FUNCTION_NAME to
>> ASM_DECLARE_OS_FUNCTION_NAME as suggested.
>>
>>> This is very ugly.  An OS isn't a subtarget of any kind.
>>> If this is really the way to go, use ASM_DECLARE_OS_FUNCTION_NAME
>>> or something similar.
>>>
>>> BTW,
>>>
>>>   bool is_ms_hook = ((decl && ix86_function_ms_hook_prologue (decl)) ?
>> true
>>>                                                                     :
>> false);
>>>
>>> is horribly ugly.  You should just use
>>
>> Cleaned up.
>
> I don't see the "cleaned up" part.

Then look into patch about ix86_asm_declare_function_name in i386.c.
This was what Jakub was refering to here AFAIU.

> Can you please revert the original patch that caused the massive
> breakage on *-linux and post a fixed patch instead (the patch below
> doesn't make sense to me on its own).
>
> Thanks,
> Richard.

Isn't the patch for fixing PR/44850 quite obvious? Caused by OS
headers, which are defining target macros (target is architecture plus
OS AFAIU), the override of ASM_DECLARE_FUNCTION_NAME needs to be
splitted here into two parts. As i386 needs to override this hook to
allow architecture depend logic, the macro in OS headers get renamed
to ASM_DECLARE_OS_FUNCTION_NAME. As other architecture don't override
the ASM_DECLARE_FUNCTION_NAME macro, it is necessary to provide the
default macro implementation of ASM_DECLARE_FUNCTION_NAME, too.

So, I'll wait for comment of rth before I revert the already approved patch.

Regards,
Kai
-- 
|  (\_/) This is Bunny. Copy and paste
| (='.'=) Bunny into your signature to help
| (")_(") him gain world domination

Re: [patch i386]: Add support of "hotfix" -feature for x64

[Jack Howarth]

On Wed, Jul 07, 2010 at 10:27:57AM +0200, Kai Tietz wrote:
> Hello,
> 
> Jakub Jelinek <jakub@redhat.com> wrote on 07.07.2010 09:25:20:
> 
> > On Wed, Jul 07, 2010 at 09:15:45AM +0200, Kai Tietz wrote:
> > > this is the more improved patch, which handles for Wine linux the
> > > hotfix part via ASM_DECLARE_FUNCTION_NAME, too.
> 
> I renamed by this patch the SUBTARGET_ASM_DECLARE_FUNCTION_NAME to 
> ASM_DECLARE_OS_FUNCTION_NAME as suggested.
> 
> > This is very ugly.  An OS isn't a subtarget of any kind.
> > If this is really the way to go, use ASM_DECLARE_OS_FUNCTION_NAME
> > or something similar.
> > 
> > BTW,
> > 
> >   bool is_ms_hook = ((decl && ix86_function_ms_hook_prologue (decl)) ? 
> true
> >                                                                     : 
> false);
> > 
> > is horribly ugly.  You should just use
> 
> Cleaned up.

Kai,
   This is still broken on darwin with your proposed changes...

gcc -c   -g -fkeep-inline-functions -DIN_GCC   -W -Wall -Wwrite-strings -Wcast-qual -Wstrict-prototypes -Wmissing-prototypes -Wmissing-format-attribute -pedantic -Wno-long-long -Wno-variadic-macros -Wno-overlength-strings -Wold-style-definition -Wc++-compat -fno-common  -DHAVE_CONFIG_H -DGENERATOR_FILE -I. -Ibuild -I../../gcc/gcc -I../../gcc/gcc/build -I../../gcc/gcc/../include -I../../gcc/gcc/../libcpp/include -I/sw/include -I/sw/include  -I../../gcc/gcc/../libdecnumber -I../../gcc/gcc/../libdecnumber/dpd -I../libdecnumber -I/sw/include  -I/sw/include -DCLOOG_PPL_BACKEND   -I/sw/include \
		-o build/genautomata.o ../../gcc/gcc/genautomata.c
In file included from ./tm.h:9,
                 from ../../gcc/gcc/gencheck.c:24:
../../gcc/gcc/config/darwin.h:656:1: warning: "ASM_DECLARE_FUNCTION_NAME" redefined
In file included from ./tm.h:8,
                 from ../../gcc/gcc/gencheck.c:24:
../../gcc/gcc/config/i386/i386.h:2090:1: warning: this is the location of the previous definition
In file included from ./tm.h:9,
                 from ../../gcc/gcc/genconditions.c:32:
../../gcc/gcc/config/darwin.h:656:1: warning: "ASM_DECLARE_FUNCTION_NAME" redefined
In file included from ./tm.h:8,
                 from ../../gcc/gcc/genconditions.c:32:
../../gcc/gcc/config/i386/i386.h:2090:1: warning: this is the location of the previous definition
In file included from ./tm.h:9,
                 from ../../gcc/gcc/genflags.c:27:
../../gcc/gcc/config/darwin.h:656:1: warning: "ASM_DECLARE_FUNCTION_NAME" redefined
In file included from ./tm.h:8,
                 from ../../gcc/gcc/genflags.c:27:
../../gcc/gcc/config/i386/i386.h:2090:1: warning: this is the location of the previous definition
In file included from ./tm.h:9,
                 from ../../gcc/gcc/genpreds.c:27:
../../gcc/gcc/config/darwin.h:656:1: warning: "ASM_DECLARE_FUNCTION_NAME" redefined
In file included from ./tm.h:8,
                 from ../../gcc/gcc/genpreds.c:27:
../../gcc/gcc/config/i386/i386.h:2090:1: warning: this is the location of the previous definition
In file included from ./tm.h:9,
                 from ../../gcc/gcc/genattr.c:26:
../../gcc/gcc/config/darwin.h:656:1: warning: "ASM_DECLARE_FUNCTION_NAME" redefined
In file included from ./tm.h:8,
                 from ../../gcc/gcc/genattr.c:26:
../../gcc/gcc/config/i386/i386.h:2090:1: warning: this is the location of the previous definition
In file included from ./tm.h:9,
                 from ../../gcc/gcc/read-rtl.c:29:
../../gcc/gcc/config/darwin.h:656:1: warning: "ASM_DECLARE_FUNCTION_NAME" redefined
In file included from ./tm.h:8,
                 from ../../gcc/gcc/read-rtl.c:29:
../../gcc/gcc/config/i386/i386.h:2090:1: warning: this is the location of the previous definition
In file included from ./tm.h:9,
                 from ../../gcc/gcc/genautomata.c:111:
../../gcc/gcc/config/darwin.h:656:1: warning: "ASM_DECLARE_FUNCTION_NAME" redefined
In file included from ./tm.h:8,
                 from ../../gcc/gcc/genautomata.c:111:
../../gcc/gcc/config/i386/i386.h:2090:1: warning: this is the location of the previous definition
In file included from ./tm.h:9,
                 from ../../gcc/gcc/gencodes.c:27:
../../gcc/gcc/config/darwin.h:656:1: warning: "ASM_DECLARE_FUNCTION_NAME" redefined
In file included from ./tm.h:8,
                 from ../../gcc/gcc/gencodes.c:27:
../../gcc/gcc/config/i386/i386.h:2090:1: warning: this is the location of the previous definition
In file included from ./tm.h:9,
                 from ../../gcc/gcc/print-rtl.c:32:
../../gcc/gcc/config/darwin.h:656:1: warning: "ASM_DECLARE_FUNCTION_NAME" redefined
In file included from ./tm.h:8,
                 from ../../gcc/gcc/print-rtl.c:32:
../../gcc/gcc/config/i386/i386.h:2090:1: warning: this is the location of the previous definition
In file included from ./tm.h:9,
                 from ../../gcc/gcc/genconfig.c:26:
../../gcc/gcc/config/darwin.h:656:1: warning: "ASM_DECLARE_FUNCTION_NAME" redefined
In file included from ./tm.h:8,
                 from ../../gcc/gcc/genconfig.c:26:
../../gcc/gcc/config/i386/i386.h:2090:1: warning: this is the location of the previous definition
In file included from ./tm.h:9,
                 from ../../gcc/gcc/gensupport.c:24:
../../gcc/gcc/config/darwin.h:656:1: warning: "ASM_DECLARE_FUNCTION_NAME" redefined
In file included from ./tm.h:8,
                 from ../../gcc/gcc/gensupport.c:24:
../../gcc/gcc/config/i386/i386.h:2090:1: warning: this is the location of the previous definition

throughout the build. Please revert r161876 as Richard requested until the patch can be
properly redesigned and regression tested. It is blocking intel darwin from testing current gcc
trunk.
          Jack
> 
> ChangeLog
> 2010-07-07  Kai Tietz
> 
>         PR target/44850
>         * config/i386/i386.c (ix86_asm_declare_function_name): Add
>         xname local variable to allow its override as used in darwin.
>         * config/i386/i386.h (SUBTARGET_ASM_DECLARE_FUNCTION_NAME):
>         Rename in comment to ASM_DECLARE_OS_FUNCTION_NAME.
>         * config/darwin.h (ASM_DECLARE_OS_FUNCTION_NAME): New macro.
>         (ASM_DECLARE_FUNCTION_NAME): Use ASM_DECLARE_OS_FUNCTION_NAME.
>         * config/elfos.h: Likewise.
>         * config/netbsd-aout.h: Likewise.
>         * config/openbsd.h: Likewise.
>         * config/i386/cygming.h (SUBTARGET_ASM_DECLARE_FUNCTION_NAME):
>         Rename to ASM_DECLARE_OS_FUNCTION_NAME.
> 
> 
> Ok for apply?
> 
> Regards,
> Kai
> 
> |  (\_/)  This is Bunny. Copy and paste Bunny
> | (='.'=) into your signature to help him gain
> | (")_(") world domination.
> 

Re: [patch i386]: Add support of "hotfix" -feature for x64

[H.J. Lu]

On Wed, Jul 7, 2010 at 3:53 AM, Kai Tietz <ktietz70@googlemail.com> wrote:
> 2010/7/7 Richard Guenther <richard.guenther@gmail.com>:
>> On Wed, Jul 7, 2010 at 10:27 AM, Kai Tietz <Kai.Tietz@onevision.com> wrote:
>>> Hello,
>>>
>>> Jakub Jelinek <jakub@redhat.com> wrote on 07.07.2010 09:25:20:
>>>
>>>> On Wed, Jul 07, 2010 at 09:15:45AM +0200, Kai Tietz wrote:
>>>> > this is the more improved patch, which handles for Wine linux the
>>>> > hotfix part via ASM_DECLARE_FUNCTION_NAME, too.
>>>
>>> I renamed by this patch the SUBTARGET_ASM_DECLARE_FUNCTION_NAME to
>>> ASM_DECLARE_OS_FUNCTION_NAME as suggested.
>>>
>>>> This is very ugly.  An OS isn't a subtarget of any kind.
>>>> If this is really the way to go, use ASM_DECLARE_OS_FUNCTION_NAME
>>>> or something similar.
>>>>
>>>> BTW,
>>>>
>>>>   bool is_ms_hook = ((decl && ix86_function_ms_hook_prologue (decl)) ?
>>> true
>>>>                                                                     :
>>> false);
>>>>
>>>> is horribly ugly.  You should just use
>>>
>>> Cleaned up.
>>
>> I don't see the "cleaned up" part.
>
> Then look into patch about ix86_asm_declare_function_name in i386.c.
> This was what Jakub was refering to here AFAIU.
>
>> Can you please revert the original patch that caused the massive
>> breakage on *-linux and post a fixed patch instead (the patch below
>> doesn't make sense to me on its own).
>>
>> Thanks,
>> Richard.
>
> Isn't the patch for fixing PR/44850 quite obvious? Caused by OS
> headers, which are defining target macros (target is architecture plus
> OS AFAIU), the override of ASM_DECLARE_FUNCTION_NAME needs to be
> splitted here into two parts. As i386 needs to override this hook to
> allow architecture depend logic, the macro in OS headers get renamed
> to ASM_DECLARE_OS_FUNCTION_NAME. As other architecture don't override
> the ASM_DECLARE_FUNCTION_NAME macro, it is necessary to provide the
> default macro implementation of ASM_DECLARE_FUNCTION_NAME, too.
>
> So, I'll wait for comment of rth before I revert the already approved patch.
>

I will revert it for now. The latest patch looks wrong to me and
it doesn't work on Darwin.


-- 
H.J.

Re: [patch i386]: Add support of "hotfix" -feature for x64

[Jack Howarth]

Kai,
   While I can test any revised patch you come up with, the changes to
gcc/config/darwin.h really needs to be approved by the Darwin maintainer
(Mike Stump) before a revised patch is reapplied.
         Jack

Re: [patch i386]: Add support of "hotfix" -feature for x64

[Kai Tietz]

[-- Attachment #1: Type: text/plain, Size: 712 bytes --]

2010/7/7 Jack Howarth <howarth@bromo.med.uc.edu>:
> Kai,
>   While I can test any revised patch you come up with, the changes to
> gcc/config/darwin.h really needs to be approved by the Darwin maintainer
> (Mike Stump) before a revised patch is reapplied.
>         Jack
>

Thanks, and sorry for the troubles.
I see that I have to guard the default define of
ASM_DECLARE_FUNCTION_NAME macro in config/ OS headers. This should fix
the issue. If you could test it for darwin?

I'll respin the complete patch (with prior i386 parts) then again.

Thanks in advance,
Kai

-- 
|  (\_/) This is Bunny. Copy and paste
| (='.'=) Bunny into your signature to help
| (")_(") him gain world domination

[-- Attachment #2: pr44850.diff --]
[-- Type: application/octet-stream, Size: 8944 bytes --]

Index: gcc/gcc/config/darwin.h
===================================================================
--- gcc.orig/gcc/config/darwin.h	2010-07-07 15:10:43.026082000 +0200
+++ gcc/gcc/config/darwin.h	2010-07-07 15:39:52.599780100 +0200
@@ -635,22 +635,32 @@ extern GTY(()) int darwin_ms_struct;
       assemble_zeros (1);						\
   } while (0)
 
-#define ASM_DECLARE_FUNCTION_NAME(FILE, NAME, DECL)			\
-  do {									\
-    const char *xname = NAME;						\
-    if (GET_CODE (XEXP (DECL_RTL (DECL), 0)) != SYMBOL_REF)		\
-      xname = IDENTIFIER_POINTER (DECL_NAME (DECL));			\
-    if (! DECL_WEAK (DECL)						\
-        && ((TREE_STATIC (DECL)						\
+#define ASM_DECLARE_OS_FUNCTION_NAME(FILE, NAME, DECL)			\
+  do									\
+    {									\
+      if (GET_CODE (XEXP (DECL_RTL (DECL), 0)) != SYMBOL_REF)		\
+        xname = IDENTIFIER_POINTER (DECL_NAME (DECL));			\
+      if (! DECL_WEAK (DECL)						\
+          && ((TREE_STATIC (DECL)					\
 	     && (!DECL_COMMON (DECL) || !TREE_PUBLIC (DECL)))		\
-            || DECL_INITIAL (DECL)))					\
-        machopic_define_symbol (DECL_RTL (DECL));			\
-    if ((TREE_STATIC (DECL)						\
-	 && (!DECL_COMMON (DECL) || !TREE_PUBLIC (DECL)))		\
-        || DECL_INITIAL (DECL))						\
-      (* targetm.encode_section_info) (DECL, DECL_RTL (DECL), false);	\
-    ASM_OUTPUT_LABEL (FILE, xname);					\
+              || DECL_INITIAL (DECL)))					\
+          machopic_define_symbol (DECL_RTL (DECL));			\
+      if ((TREE_STATIC (DECL)						\
+	   && (!DECL_COMMON (DECL) || !TREE_PUBLIC (DECL)))		\
+          || DECL_INITIAL (DECL))					\
+        (* targetm.encode_section_info) (DECL, DECL_RTL (DECL), false);	\
+    }									\
+  while (0)
+
+/* For i386 target this macro is already declared.  */
+#ifndef ASM_DECLARE_FUNCTION_NAME
+#define ASM_DECLARE_FUNCTION_NAME(FILE, NAME, DECL)	\
+  do {							\
+    const char *xname = NAME;				\
+    ASM_DECLARE_OS_FUNCTION_NAME (FILE, xname, DECL);	\
+    ASM_OUTPUT_LABEL (FILE, xname);			\
   } while (0)
+#endif
 
 #undef TARGET_ASM_DECLARE_CONSTANT_NAME
 #define TARGET_ASM_DECLARE_CONSTANT_NAME darwin_asm_declare_constant_name
Index: gcc/gcc/config/elfos.h
===================================================================
--- gcc.orig/gcc/config/elfos.h	2010-07-07 15:10:43.027082000 +0200
+++ gcc/gcc/config/elfos.h	2010-07-07 15:41:26.853967300 +0200
@@ -276,15 +276,25 @@ see the files COPYING3 and COPYING.RUNTI
    Some svr4 assemblers need to also have something extra said about the
    function's return value.  We allow for that here.  */
 
-#ifndef ASM_DECLARE_FUNCTION_NAME
-#define ASM_DECLARE_FUNCTION_NAME(FILE, NAME, DECL)		\
+#undef ASM_DECLARE_OS_FUNCTION_NAME
+#define ASM_DECLARE_OS_FUNCTION_NAME(FILE, NAME, DECL)		\
   do								\
     {								\
       ASM_OUTPUT_TYPE_DIRECTIVE (FILE, NAME, "function");	\
       ASM_DECLARE_RESULT (FILE, DECL_RESULT (DECL));		\
-      ASM_OUTPUT_LABEL (FILE, NAME);				\
     }								\
   while (0)
+
+/* For i386 target this macro is already declared.  */
+#ifndef ASM_DECLARE_FUNCTION_NAME
+#define ASM_DECLARE_FUNCTION_NAME(FILE, NAME, DECL)	\
+  do							\
+    {							\
+      ASM_DECLARE_OS_FUNCTION_NAME(FILE, NAME, DECL);	\
+      ASM_OUTPUT_LABEL (FILE, NAME);			\
+    }							\
+  while (0)
+
 #endif
 
 /* Write the extra assembler code needed to declare an object properly.  */
Index: gcc/gcc/config/i386/cygming.h
===================================================================
--- gcc.orig/gcc/config/i386/cygming.h	2010-07-07 15:10:43.028082000 +0200
+++ gcc/gcc/config/i386/cygming.h	2010-07-07 15:38:00.208435800 +0200
@@ -269,8 +269,8 @@ do {						\
 /* Write the extra assembler code needed to declare a function
    properly.  If we are generating SDB debugging information, this
    will happen automatically, so we only need to handle other cases.  */
-#undef SUBTARGET_ASM_DECLARE_FUNCTION_NAME
-#define SUBTARGET_ASM_DECLARE_FUNCTION_NAME(FILE, NAME, DECL)		\
+#undef ASM_DECLARE_OS_FUNCTION_NAME
+#define ASM_OS_DECLARE_FUNCTION_NAME(FILE, NAME, DECL)			\
   do									\
     {									\
       i386_pe_maybe_record_exported_symbol (DECL, NAME, 0);		\
Index: gcc/gcc/config/i386/i386.c
===================================================================
--- gcc.orig/gcc/config/i386/i386.c	2010-07-07 15:10:43.030082000 +0200
+++ gcc/gcc/config/i386/i386.c	2010-07-07 15:38:00.411562100 +0200
@@ -5115,10 +5115,10 @@ void
 ix86_asm_declare_function_name (FILE *asm_out_file, const char *fname,
 				tree decl)
 {
-  bool is_ms_hook = ((decl && ix86_function_ms_hook_prologue (decl)) ? true
-  								     : false);
-#ifdef SUBTARGET_ASM_DECLARE_FUNCTION_NAME
-  SUBTARGET_ASM_DECLARE_FUNCTION_NAME (asm_out_file, fname, decl);
+  const char *xname = fname;
+  bool is_ms_hook = decl && ix86_function_ms_hook_prologue (decl);
+#ifdef ASM_DECLARE_OS_FUNCTION_NAME
+  ASM_DECLARE_OS_FUNCTION_NAME (asm_out_file, fname, decl);
 #endif
 
   if (is_ms_hook)
@@ -5130,7 +5130,7 @@ ix86_asm_declare_function_name (FILE *as
 	fprintf (asm_out_file, ASM_LONG " %#x\n", filler_cc);
     }
 
-  ASM_OUTPUT_LABEL (asm_out_file, fname);
+  ASM_OUTPUT_LABEL (asm_out_file, xname);
 
   /* Output magic byte marker, if hot-patch attribute is set.
      For x86 case frame-pointer prologue will be emitted in
Index: gcc/gcc/config/i386/i386.h
===================================================================
--- gcc.orig/gcc/config/i386/i386.h	2010-07-07 15:10:43.035082000 +0200
+++ gcc/gcc/config/i386/i386.h	2010-07-07 15:38:00.474062500 +0200
@@ -2084,7 +2084,7 @@ do {									\
 
 /* Write the extra assembler code needed to declare a function
    properly.  Target can add additional code by the sub-target
-   macro SUBTARGET_ASM_DECLARE_FUNCTION_NAME.  */
+   macro ASM_DECLARE_OS_FUNCTION_NAME.  */
 
 #undef ASM_DECLARE_FUNCTION_NAME
 #define ASM_DECLARE_FUNCTION_NAME(FILE, NAME, DECL) \
Index: gcc/gcc/config/netbsd-aout.h
===================================================================
--- gcc.orig/gcc/config/netbsd-aout.h	2010-07-07 15:10:43.037082000 +0200
+++ gcc/gcc/config/netbsd-aout.h	2010-07-07 15:42:23.067523600 +0200
@@ -135,16 +135,25 @@ along with GCC; see the file COPYING3.  
    Some svr4 assemblers need to also have something extra said about the
    function's return value.  We allow for that here.  */
 
-#undef ASM_DECLARE_FUNCTION_NAME
-#define ASM_DECLARE_FUNCTION_NAME(FILE, NAME, DECL)			\
+#undef ASM_DECLARE_OS_FUNCTION_NAME
+#define ASM_DECLARE_OS_FUNCTION_NAME(FILE, NAME, DECL)			\
   do									\
     {									\
       ASM_OUTPUT_TYPE_DIRECTIVE (FILE, NAME, "function");		\
       ASM_DECLARE_RESULT (FILE, DECL_RESULT (DECL));			\
-      ASM_OUTPUT_LABEL(FILE, NAME);					\
     }									\
   while (0)
 
+/* For i386 target this macro is already declared.  */
+#ifndef ASM_DECLARE_FUNCTION_NAME
+#define ASM_DECLARE_FUNCTION_NAME(FILE, NAME, DECL)                     \
+  do                                                                    \
+    {                                                                   \
+      ASM_DECLARE_OS_FUNCTION_NAME(FILE, NAME, DECL);			\
+      ASM_OUTPUT_LABEL(FILE, NAME);                                     \
+    }                                                                   \
+  while (0)
+#endif
 
 /* Write the extra assembler code needed to declare an object properly.  */
 
Index: gcc/gcc/config/openbsd.h
===================================================================
--- gcc.orig/gcc/config/openbsd.h	2010-07-07 15:10:43.039082000 +0200
+++ gcc/gcc/config/openbsd.h	2010-07-07 15:43:50.755670500 +0200
@@ -207,17 +207,32 @@ while (0)
    entries under OpenBSD.  These macros also have to output the starting 
    labels for the relevant functions/objects.  */
 
+#ifndef ASM_DECLARE_OS_FUNCTION_NAME
+#define ASM_DECLARE_OS_FUNCTION_NAME(FILE, NAME, DECL)		\
+  do								\
+    {								\
+      ASM_OUTPUT_TYPE_DIRECTIVE (FILE, NAME, "function");	\
+      ASM_DECLARE_RESULT (FILE, DECL_RESULT (DECL));		\
+    }								\
+  while (0)
+#endif
+
 #ifndef OBSD_HAS_DECLARE_FUNCTION_NAME
 /* Extra assembler code needed to declare a function properly.
    Some assemblers may also need to also have something extra said 
    about the function's return value.  We allow for that here.  */
-#undef ASM_DECLARE_FUNCTION_NAME
-#define ASM_DECLARE_FUNCTION_NAME(FILE, NAME, DECL)			\
-  do {									\
-    ASM_OUTPUT_TYPE_DIRECTIVE (FILE, NAME, "function");			\
-    ASM_DECLARE_RESULT (FILE, DECL_RESULT (DECL));			\
-    ASM_OUTPUT_LABEL(FILE, NAME);					\
-  } while (0)
+
+/* For i386 target this macro is already declared.  */
+#ifndef ASM_DECLARE_FUNCTION_NAME
+#define ASM_DECLARE_FUNCTION_NAME(FILE, NAME, DECL)	\
+  do							\
+    {							\
+      ASM_DECLARE_OS_FUNCTION_NAME(FILE, NAME, DECL);	\
+      ASM_OUTPUT_LABEL(FILE, NAME);			\
+    }							\
+  while (0)
+#endif
+
 #endif
 
 #ifndef OBSD_HAS_DECLARE_FUNCTION_SIZE

Re: [patch i386]: Add support of "hotfix" -feature for x64

[H.J. Lu]

On Wed, Jul 7, 2010 at 6:50 AM, Kai Tietz <ktietz70@googlemail.com> wrote:
> 2010/7/7 Jack Howarth <howarth@bromo.med.uc.edu>:
>> Kai,
>>   While I can test any revised patch you come up with, the changes to
>> gcc/config/darwin.h really needs to be approved by the Darwin maintainer
>> (Mike Stump) before a revised patch is reapplied.
>>         Jack
>>
>
> Thanks, and sorry for the troubles.
> I see that I have to guard the default define of
> ASM_DECLARE_FUNCTION_NAME macro in config/ OS headers. This should fix
> the issue. If you could test it for darwin?
>
> I'll respin the complete patch (with prior i386 parts) then again.
>

The whole approach looks wrong to me. I think you should
introduce a new macro, ASM_OUTPUT_FUNCTION_LABEL,
which is default to ASM_OUTPUT_LABEL.  We replace
ASM_OUTPUT_LABEL with ASM_OUTPUT_FUNCTION_LABEL
in ASM_DECLARE_FUNCTION_NAME.  You can override
ASM_OUTPUT_FUNCTION_LABEL in x86 to generate
what you need.

-- 
H.J.

Re: [patch i386]: Add support of "hotfix" -feature for x64

[Mike Stump]

On Jul 7, 2010, at 6:27 AM, H.J. Lu wrote:
>> So, I'll wait for comment of rth before I revert the already approved patch.
> 
> I will revert it for now. The latest patch looks wrong to me and
> it doesn't work on Darwin.

I don't favor waiting...  Thanks for the reversion.

Re: [patch i386]: Add support of "hotfix" -feature for x64

[Mike Stump]

On Jul 7, 2010, at 6:42 AM, Jack Howarth wrote:
>   While I can test any revised patch you come up with, the changes to
> gcc/config/darwin.h really needs to be approved by the Darwin maintainer
> (Mike Stump) before a revised patch is reapplied.

I think the design of the change is busted.  I'd go back to the drawing board, and have someone design in a way to make the change that doesn't break everything and so that the change can be trivially audited for correctness.  If people want to rejigger how the port/ports are written to detangle and make the code more clear and prevent this type of breakage in the future, that would be stellar.

I briefly looked at it, but it seems to want to change the use of ASM_OUTPUT_LABEL by ASM_DECLARE_FUNCTION_NAME  in the x86 ports to instead use ASM_OUTPUT_HOTFIX_LABEL.  Just do that, and then define ASM_OUTPUT_HOTFIX_LABEL in i386.h.  Isn't that simpler?

So, to be concrete, the darwin change would be something like:

--- config/darwin.h.~1~	2010-07-01 07:47:13.000000000 -0700
+++ config/darwin.h	2010-07-08 13:18:10.000000000 -0700
@@ -628,7 +628,7 @@ extern GTY(()) int darwin_ms_struct;
 	 && (!DECL_COMMON (DECL) || !TREE_PUBLIC (DECL)))		\
         || DECL_INITIAL (DECL))						\
       (* targetm.encode_section_info) (DECL, DECL_RTL (DECL), false);	\
-    ASM_OUTPUT_LABEL (FILE, xname);					\
+    ASM_OUTPUT_HOTFIX_LABEL (FILE, xname, DECL);			\
     /* Darwin doesn't support zero-size objects, so give them a		\
        byte.  */							\
     if (tree_low_cst (DECL_SIZE_UNIT (DECL), 1) == 0)			\
@@ -649,7 +649,7 @@ extern GTY(()) int darwin_ms_struct;
 	 && (!DECL_COMMON (DECL) || !TREE_PUBLIC (DECL)))		\
         || DECL_INITIAL (DECL))						\
       (* targetm.encode_section_info) (DECL, DECL_RTL (DECL), false);	\
-    ASM_OUTPUT_LABEL (FILE, xname);					\
+    ASM_OUTPUT_HOTFIX_LABEL (FILE, xname, DECL);					\
   } while (0)
 
 #undef TARGET_ASM_DECLARE_CONSTANT_NAME
--------------

which, to the unaided eye, is trivial to review.  Then, ASM_OUTPUT_HOTFIX_LABEL takes the responsibility to  get the details right, which, I suspect would be easy enough to do...  ?


I'd rather have the x86 port maintainer review and approve this type of patch.  I'm fine with people using the darwin tester to figure out if it worked or not after the fact, if they want, but they do have to watch it and resolve issues.  And I'm fine with a port reviewer's decisions...

Re: [patch i386]: Add support of "hotfix" -feature for x64

[Mike Stump]

On Jul 7, 2010, at 6:50 AM, Kai Tietz wrote:
> 2010/7/7 Jack Howarth <howarth@bromo.med.uc.edu>:
>> Kai,
>>   While I can test any revised patch you come up with, the changes to
>> gcc/config/darwin.h really needs to be approved by the Darwin maintainer
>> (Mike Stump) before a revised patch is reapplied.
>>         Jack
>> 
> 
> Thanks, and sorry for the troubles.
> I see that I have to guard the default define of
> ASM_DECLARE_FUNCTION_NAME macro in config/ OS headers. This should fix
> the issue. If you could test it for darwin?

Looks way more complex than H.J. Lu's suggestion or mine...  Could you comment on those two first?

Re: [patch i386]: Add support of "hotfix" -feature for x64

[H.J. Lu]

On Tue, Jul 6, 2010 at 11:28 AM, Kai Tietz <ktietz70@googlemail.com> wrote:
> 2010/7/6 Uros Bizjak <ubizjak@gmail.com>:
>> Hello!
>>
>>> Better, but honestly I'd been thinking just to bring one
>>> directive per line, rather than trying to block the output
>>> onto several lines.  I.e.
>>>
>>>   for (i = 0; i < filler_count; i += 4)
>>>    fprintf (asm_out_file, ASM_LONG " 0x%x\n", filler_cc);
>>>
>>>
>>> Ok with that change.
>>
>> Please also use %#x instead of 0x%x.
>>
>> Thanks,
>> Uros.
>>
>
>
> Ok, changed it to a simple loop without formatting and I use %#x.
> Committed at revision 161875 & 161876

The patch is wrong since on Linux, it replaces ASM_DECLARE_FUNCTION_NAME
defined in config/elfos.h with a bogus one in config/i386/i386.h.


-- 
H.J.

Re: [patch i386]: Add support of "hotfix" -feature for x64

[H.J. Lu]

On Tue, Jul 6, 2010 at 4:26 PM, Jack Howarth <howarth@bromo.med.uc.edu> wrote:
> On Tue, Jul 06, 2010 at 08:28:48PM +0200, Kai Tietz wrote:
>> 2010/7/6 Uros Bizjak <ubizjak@gmail.com>:
>> > Hello!
>> >
>> >> Better, but honestly I'd been thinking just to bring one
>> >> directive per line, rather than trying to block the output
>> >> onto several lines.  I.e.
>> >>
>> >>   for (i = 0; i < filler_count; i += 4)
>> >>    fprintf (asm_out_file, ASM_LONG " 0x%x\n", filler_cc);
>> >>
>> >>
>> >> Ok with that change.
>> >
>> > Please also use %#x instead of 0x%x.
>> >
>> > Thanks,
>> > Uros.
>> >
>>
>>
>> Ok, changed it to a simple loop without formatting and I use %#x.
>> Committed at revision 161875 & 161876
>>
>> Thanks,
>> Kai
>
> This patch breaks the bootstrap on i386-apple-darwin* and
> x86_64-apple-darwin* since gcc/config/darwin.h defines
> ASM_DECLARE_FUNCTION_NAME(FILE, NAME, DECL). Simply changing...
>

This may also cause:

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44850

-- 
H.J.

Re: [patch i386]: Add support of "hotfix" -feature for x64

[Jack Howarth]

On Tue, Jul 06, 2010 at 08:28:48PM +0200, Kai Tietz wrote:
> 2010/7/6 Uros Bizjak <ubizjak@gmail.com>:
> > Hello!
> >
> >> Better, but honestly I'd been thinking just to bring one
> >> directive per line, rather than trying to block the output
> >> onto several lines.  I.e.
> >>
> >>   for (i = 0; i < filler_count; i += 4)
> >>    fprintf (asm_out_file, ASM_LONG " 0x%x\n", filler_cc);
> >>
> >>
> >> Ok with that change.
> >
> > Please also use %#x instead of 0x%x.
> >
> > Thanks,
> > Uros.
> >
> 
> 
> Ok, changed it to a simple loop without formatting and I use %#x.
> Committed at revision 161875 & 161876
> 
> Thanks,
> Kai

This patch breaks the bootstrap on i386-apple-darwin* and
x86_64-apple-darwin* since gcc/config/darwin.h defines
ASM_DECLARE_FUNCTION_NAME(FILE, NAME, DECL). Simply changing...

Index: gcc/config/darwin.h
===================================================================
--- gcc/config/darwin.h (revision 161889)
+++ gcc/config/darwin.h (working copy)
@@ -635,7 +635,8 @@
       assemble_zeros (1);                                              \
   } while (0)
 
-#define ASM_DECLARE_FUNCTION_NAME(FILE, NAME, DECL)                    \
+#undef SUBTARGET_ASM_DECLARE_FUNCTION_NAME
+#define SUBTARGET_ASM_DECLARE_FUNCTION_NAME(FILE, NAME, DECL)                  \
   do {                                                                 \
     const char *xname = NAME;                                          \
     if (GET_CODE (XEXP (DECL_RTL (DECL), 0)) != SYMBOL_REF)            \
@@ -649,7 +650,6 @@
         && (!DECL_COMMON (DECL) || !TREE_PUBLIC (DECL)))               \
         || DECL_INITIAL (DECL))                                                \
       (* targetm.encode_section_info) (DECL, DECL_RTL (DECL), false);  \
-    ASM_OUTPUT_LABEL (FILE, xname);                                    \
   } while (0)
 
 #undef TARGET_ASM_DECLARE_CONSTANT_NAME

is insufficient since it results in a bootstrap failure of...

/sw/src/fink.build/gcc46-4.6.0-1000/darwin_objdir/./prev-gcc/xgcc -B/sw/src/fink.build/gcc46-4.6.0-1000/darwin_objdir/./prev-gcc/ -B/sw/lib/gcc4.6/x86_64-apple-darwin10.4.0/bin/ -B/sw/lib/gcc4.6/x86_64-apple-darwin10.4.0/bin/ -B/sw/lib/gcc4.6/x86_64-apple-darwin10.4.0/lib/ -isystem /sw/lib/gcc4.6/x86_64-apple-darwin10.4.0/include -isystem /sw/lib/gcc4.6/x86_64-apple-darwin10.4.0/sys-include    -c   -g -O2 -gtoggle -DIN_GCC   -W -Wall -Wwrite-strings -Wcast-qual -Wstrict-prototypes -Wmissing-prototypes -Wmissing-format-attribute -pedantic -Wno-long-long -Wno-variadic-macros -Wno-overlength-strings -Werror -Wold-style-definition -Wc++-compat   -DHAVE_CONFIG_H -I. -I. -I../../gcc-4.6-20100706/gcc -I../../gcc-4.6-20100706/gcc/. -I../../gcc-4.6-20100706/gcc/../include -I../../gcc-4.6-20100706/gcc/../libcpp/include -I/sw/include -I/sw/include  -I../../gcc-4.6-20100706/gcc/../libdecnumber -I../../gcc-4.6-20100706/gcc/../libdecnumber/dpd -I../libdecnumber -I/sw/include  -I/sw/include -DCLOOG_PPL_BACKEND   -I/sw/include -I. -I. -I../../gcc-4.6-20100706/gcc -I../../gcc-4.6-20100706/gcc/. -I../../gcc-4.6-20100706/gcc/../include -I../../gcc-4.6-20100706/gcc/../libcpp/include -I/sw/include -I/sw/include  -I../../gcc-4.6-20100706/gcc/../libdecnumber -I../../gcc-4.6-20100706/gcc/../libdecnumber/dpd -I../libdecnumber -I/sw/include  -I/sw/include -DCLOOG_PPL_BACKEND   ../../gcc-4.6-20100706/gcc/config/host-darwin.c
../../gcc-4.6-20100706/gcc/config/i386/i386.c: In function 'ix86_asm_declare_function_name':
../../gcc-4.6-20100706/gcc/config/i386/i386.c:5121:3: error: variable 'xname' set but not used [-Werror=unused-but-set-variable]

Also any fix will have to take in account that darwin.h is shared
between powerpc-apple-darwin* and *86-apple-darwin* so the hot-fix
will either have to be extended to rs6000 or the macro definition
moved out of gcc/config/darwin.h and into gcc/config/rs6000/darwin.h
as ASM_DECLARE_FUNCTION_NAME() and into gcc/config/i386/darwin.h as
SUBTARGET_ASM_DECLARE_FUNCTION_NAME(). A mechanism to handle the use
of xname in SUBTARGET_ASM_DECLARE_FUNCTION_NAME() on darwin still is
required.
          Jack
> 
> -- 
> |  (\_/) This is Bunny. Copy and paste
> | (='.'=) Bunny into your signature to help
> | (")_(") him gain world domination

Re: [patch i386]: Add support of "hotfix" -feature for x64

[Kai Tietz]

2010/7/6 Uros Bizjak <ubizjak@gmail.com>:
> Hello!
>
>> Better, but honestly I'd been thinking just to bring one
>> directive per line, rather than trying to block the output
>> onto several lines.  I.e.
>>
>>   for (i = 0; i < filler_count; i += 4)
>>    fprintf (asm_out_file, ASM_LONG " 0x%x\n", filler_cc);
>>
>>
>> Ok with that change.
>
> Please also use %#x instead of 0x%x.
>
> Thanks,
> Uros.
>


Ok, changed it to a simple loop without formatting and I use %#x.
Committed at revision 161875 & 161876

Thanks,
Kai

-- 
|  (\_/) This is Bunny. Copy and paste
| (='.'=) Bunny into your signature to help
| (")_(") him gain world domination

Re: [patch i386]: Add support of "hotfix" -feature for x64

[Uros Bizjak]

Hello!

> Better, but honestly I'd been thinking just to bring one
> directive per line, rather than trying to block the output
> onto several lines.  I.e.
>
>   for (i = 0; i < filler_count; i += 4)
>    fprintf (asm_out_file, ASM_LONG " 0x%x\n", filler_cc);
>
>
> Ok with that change.

Please also use %#x instead of 0x%x.

Thanks,
Uros.