diff mbox

[x86/darwin] fix PR51784 'PIC register not correctly preserved...'

Message ID E56CCB11-39AB-4A60-AEBD-C514F5258632@codesourcery.com
State New
Headers show

Commit Message

Iain Sandoe July 19, 2013, 1:47 p.m. UTC
Hi Uros,

thanks for your reviews,

On 18 Jul 2013, at 12:39, Uros Bizjak wrote:

> On Thu, Jul 18, 2013 at 12:12 PM, Iain Sandoe <iain@codesourcery.com> wrote:
>> 
>> So, I think we have to use the define_insn_and_split, or am I still missing something?
> 
> Just a wild guess, do you also need "&& reload_completed" in the split
> condition?

good catch, thanks - this got cut erroneously from the last variant of the patch.

Fixed & re-tested on x86_64-darwin12 / x86_64-linux (both at m32 and m64) showing the expected progressions on darwin (and correct behaviour on linux for the shlib example).

OK for trunk?

Ok for open branches? (this is a wrong-code bug)

[N.B. No changes to the darwin-specific portions already approved by Mike]

thanks
Iain

gcc/

	PR target/51784
	* config/i386/i386.c (output_set_got) [TARGET_MACHO]: Adjust to emit a second label for
	nonlocal goto receivers. Don't output pic base labels unless we're producing PIC; mark
	that action unreachable().
	(ix86_save_reg): If the function contains a nonlocal label, save the PIC base reg.
	* config/darwin-protos.h (machopic_should_output_picbase_label): New.
	* gcc/config/darwin.c (emitted_pic_label_num): New GTY. 
	(update_pic_label_number_if_needed): New.
	(machopic_output_function_base_name): Adjust for nonlocal receiver case.
	(machopic_should_output_picbase_label): New.
	* config/i386/i386.md (enum unspecv): UNSPECV_NLGR: New.
	(nonlocal_goto_receiver): New insn and split.
gcc/config/darwin-protos.h |  1 +
 gcc/config/darwin.c        | 30 +++++++++++++++++++++++++-----
 gcc/config/i386/i386.c     | 23 ++++++++++++++---------
 gcc/config/i386/i386.md    | 34 +++++++++++++++++++++++++++++++++-
 4 files changed, 73 insertions(+), 15 deletions(-)

Comments

Jakub Jelinek July 19, 2013, 2:06 p.m. UTC | #1
On Fri, Jul 19, 2013 at 02:47:51PM +0100, Iain Sandoe wrote:
> +      /* Get a new pic base.  */
> +      emit_insn (gen_set_got_labelled (pic_offset_table_rtx, label_rtx));
> +      /* Correct this with the offset from the new to the old.  */
> +      xops[0] = xops[1] = pic_offset_table_rtx;
> +      label_rtx = gen_rtx_LABEL_REF (SImode, label_rtx);
> +      tmp = gen_rtx_UNSPEC (Pmode, gen_rtvec (1, label_rtx), UNSPEC_MACHOPIC_OFFSET);

Too long line, please wrap it.

> +      xops[2] = gen_rtx_CONST (Pmode, tmp);
> +      ix86_expand_binary_operator (MINUS, SImode, xops);
> +    }
> +  DONE;
> +})
> +
>  ;; Avoid redundant prefixes by splitting HImode arithmetic to SImode.
>  
>  (define_split


	Jakub
Uros Bizjak July 19, 2013, 2:56 p.m. UTC | #2
On Fri, Jul 19, 2013 at 3:47 PM, Iain Sandoe <iain@codesourcery.com> wrote:
> Hi Uros,
>
> thanks for your reviews,
>
> On 18 Jul 2013, at 12:39, Uros Bizjak wrote:
>
>> On Thu, Jul 18, 2013 at 12:12 PM, Iain Sandoe <iain@codesourcery.com> wrote:
>>>
>>> So, I think we have to use the define_insn_and_split, or am I still missing something?
>>
>> Just a wild guess, do you also need "&& reload_completed" in the split
>> condition?
>
> good catch, thanks - this got cut erroneously from the last variant of the patch.
>
> Fixed & re-tested on x86_64-darwin12 / x86_64-linux (both at m32 and m64) showing the expected progressions on darwin (and correct behaviour on linux for the shlib example).

> gcc/
>
>         PR target/51784
>         * config/i386/i386.c (output_set_got) [TARGET_MACHO]: Adjust to emit a second label for
>         nonlocal goto receivers. Don't output pic base labels unless we're producing PIC; mark
>         that action unreachable().
>         (ix86_save_reg): If the function contains a nonlocal label, save the PIC base reg.
>         * config/darwin-protos.h (machopic_should_output_picbase_label): New.
>         * gcc/config/darwin.c (emitted_pic_label_num): New GTY.
>         (update_pic_label_number_if_needed): New.
>         (machopic_output_function_base_name): Adjust for nonlocal receiver case.
>         (machopic_should_output_picbase_label): New.
>         * config/i386/i386.md (enum unspecv): UNSPECV_NLGR: New.
>         (nonlocal_goto_receiver): New insn and split.
> OK for trunk?

Assuming that Jakub is OK with the patch, it is OK for trunk.

> Ok for open branches? (this is a wrong-code bug)

OK from the maintainer POV, but also needs release manager approval.

I'd suggest a small improvement:

+++ b/gcc/config/i386/i386.c
@@ -8827,10 +8827,8 @@ output_set_got (rtx dest, rtx label ATTRIBUTE_UNUSED)
       output_asm_insn ("mov%z0\t{%2, %0|%0, %2}", xops);

 #if TARGET_MACHO
-      /* Output the Mach-O "canonical" label name ("Lxx$pb") here too.  This
-         is what will be referenced by the Mach-O PIC subsystem.  */
-      if (!label)
- ASM_OUTPUT_LABEL (asm_out_file, MACHOPIC_FUNCTION_BASE_NAME);
+      /* We don't need a pic base, we're not producing pic.  */
+      gcc_unreachable ();
 #endif

Put this part just after the opening curly brace. There is no need to
calculate xops and output asm insn for TARGET_MACHO.

Thanks,
Uros.
Jakub Jelinek July 19, 2013, 9 p.m. UTC | #3
On Fri, Jul 19, 2013 at 04:56:47PM +0200, Uros Bizjak wrote:
> > OK for trunk?
> 
> Assuming that Jakub is OK with the patch, it is OK for trunk.

With the line wrapping fix and Uros' suggested improvement this is ok for
both trunk and branches.

	Jakub
diff mbox

Patch

diff --git a/gcc/config/darwin-protos.h b/gcc/config/darwin-protos.h
index 0755e94..70b7fb0 100644
--- a/gcc/config/darwin-protos.h
+++ b/gcc/config/darwin-protos.h
@@ -25,6 +25,7 @@  extern void machopic_validate_stub_or_non_lazy_ptr (const char *);
 extern void machopic_output_function_base_name (FILE *);
 extern const char *machopic_indirection_name (rtx, bool);
 extern const char *machopic_mcount_stub_name (void);
+extern bool machopic_should_output_picbase_label (void);
 
 #ifdef RTX_CODE
 
diff --git a/gcc/config/darwin.c b/gcc/config/darwin.c
index a049a5d..e07fa4c 100644
--- a/gcc/config/darwin.c
+++ b/gcc/config/darwin.c
@@ -369,14 +369,13 @@  machopic_gen_offset (rtx orig)
 
 static GTY(()) const char * function_base_func_name;
 static GTY(()) int current_pic_label_num;
+static GTY(()) int emitted_pic_label_num;
 
-void
-machopic_output_function_base_name (FILE *file)
+static void
+update_pic_label_number_if_needed (void)
 {
   const char *current_name;
 
-  /* If dynamic-no-pic is on, we should not get here.  */
-  gcc_assert (!MACHO_DYNAMIC_NO_PIC_P);
   /* When we are generating _get_pc thunks within stubs, there is no current
      function.  */
   if (current_function_decl)
@@ -394,7 +393,28 @@  machopic_output_function_base_name (FILE *file)
       ++current_pic_label_num;
       function_base_func_name = "L_machopic_stub_dummy";
     }
-  fprintf (file, "L%011d$pb", current_pic_label_num);
+}
+
+void
+machopic_output_function_base_name (FILE *file)
+{
+  /* If dynamic-no-pic is on, we should not get here.  */
+  gcc_assert (!MACHO_DYNAMIC_NO_PIC_P);
+
+  update_pic_label_number_if_needed ();
+  fprintf (file, "L%d$pb", current_pic_label_num);
+}
+
+bool
+machopic_should_output_picbase_label (void)
+{
+  update_pic_label_number_if_needed ();
+
+  if (current_pic_label_num == emitted_pic_label_num)
+    return false;
+
+  emitted_pic_label_num = current_pic_label_num;
+  return true;
 }
 
 /* The suffix attached to non-lazy pointer symbols.  */
diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index 5df6ab7..f523c2a 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -8827,10 +8827,8 @@  output_set_got (rtx dest, rtx label ATTRIBUTE_UNUSED)
       output_asm_insn ("mov%z0\t{%2, %0|%0, %2}", xops);
 
 #if TARGET_MACHO
-      /* Output the Mach-O "canonical" label name ("Lxx$pb") here too.  This
-         is what will be referenced by the Mach-O PIC subsystem.  */
-      if (!label)
-	ASM_OUTPUT_LABEL (asm_out_file, MACHOPIC_FUNCTION_BASE_NAME);
+      /* We don't need a pic base, we're not producing pic.  */
+      gcc_unreachable ();
 #endif
 
       targetm.asm_out.internal_label (asm_out_file, "L",
@@ -8845,12 +8843,18 @@  output_set_got (rtx dest, rtx label ATTRIBUTE_UNUSED)
       xops[2] = gen_rtx_SYMBOL_REF (Pmode, ggc_strdup (name));
       xops[2] = gen_rtx_MEM (QImode, xops[2]);
       output_asm_insn ("call\t%X2", xops);
-      /* Output the Mach-O "canonical" label name ("Lxx$pb") here too.  This
-         is what will be referenced by the Mach-O PIC subsystem.  */
+
 #if TARGET_MACHO
-      if (!label)
+      /* Output the Mach-O "canonical" pic base label name ("Lxx$pb") here.
+         This is what will be referenced by the Mach-O PIC subsystem.  */
+      if (machopic_should_output_picbase_label () || !label)
 	ASM_OUTPUT_LABEL (asm_out_file, MACHOPIC_FUNCTION_BASE_NAME);
-      else
+
+      /* When we are restoring the pic base at the site of a nonlocal label,
+         and we decided to emit the pic base above, we will still output a
+         local label used for calculating the correction offset (even though
+         the offset will be 0 in that case).  */
+      if (label)
         targetm.asm_out.internal_label (asm_out_file, "L",
 					   CODE_LABEL_NUMBER (label));
 #endif
@@ -8932,7 +8936,8 @@  ix86_save_reg (unsigned int regno, bool maybe_eh_return)
       && (df_regs_ever_live_p (REAL_PIC_OFFSET_TABLE_REGNUM)
 	  || crtl->profile
 	  || crtl->calls_eh_return
-	  || crtl->uses_const_pool))
+	  || crtl->uses_const_pool
+	  || cfun->has_nonlocal_label))
     return ix86_select_alt_pic_regnum () == INVALID_REGNUM;
 
   if (crtl->calls_eh_return && maybe_eh_return)
diff --git a/gcc/config/i386/i386.md b/gcc/config/i386/i386.md
index 2777e9c..62e4a05 100644
--- a/gcc/config/i386/i386.md
+++ b/gcc/config/i386/i386.md
@@ -222,6 +222,8 @@ 
   UNSPECV_XEND
   UNSPECV_XABORT
   UNSPECV_XTEST
+
+  UNSPECV_NLGR
 ])
 
 ;; Constants to represent rounding modes in the ROUND instruction
@@ -16227,7 +16229,37 @@ 
     emit_insn (gen_set_got (pic_offset_table_rtx));
   DONE;
 })
-
+
+(define_insn_and_split "nonlocal_goto_receiver"
+  [(unspec_volatile [(const_int 0)] UNSPECV_NLGR)]
+  "TARGET_MACHO && !TARGET_64BIT && flag_pic"
+{
+  if (crtl->uses_pic_offset_table)
+    return "#";
+  else
+    return ""; /* No pic reg restore needed.  */
+}
+  "&& reload_completed"
+  [(const_int 0)]
+{
+  if (crtl->uses_pic_offset_table)
+    {
+      rtx xops[3];
+      rtx label_rtx = gen_label_rtx ();
+      rtx tmp;
+
+      /* Get a new pic base.  */
+      emit_insn (gen_set_got_labelled (pic_offset_table_rtx, label_rtx));
+      /* Correct this with the offset from the new to the old.  */
+      xops[0] = xops[1] = pic_offset_table_rtx;
+      label_rtx = gen_rtx_LABEL_REF (SImode, label_rtx);
+      tmp = gen_rtx_UNSPEC (Pmode, gen_rtvec (1, label_rtx), UNSPEC_MACHOPIC_OFFSET);
+      xops[2] = gen_rtx_CONST (Pmode, tmp);
+      ix86_expand_binary_operator (MINUS, SImode, xops);
+    }
+  DONE;
+})
+
 ;; Avoid redundant prefixes by splitting HImode arithmetic to SImode.
 
 (define_split