[X86] Fix PR82920 (code part).
diff mbox series

Message ID 10A4B8FF-4F9C-488F-9011-E1B0EBCA02BC@googlemail.com
State New
Headers show
Series
  • [X86] Fix PR82920 (code part).
Related show

Commit Message

Iain Sandoe May 10, 2019, 9:03 p.m. UTC
Hi!

PR82920 is about CET fails on Darwin.

Initially, this was expected to be just a testsuite issue, however it turns out that the indirection thunks code has inconsistent handling of the output of labels.  Thus some of the output is missing the leading “_” on Darwin, which breaks ABI and won’t link.

Since most of the tests are scan-asms that check for what’s expected, they currently pass on Darwin but will begin failing when the codegen is fixed.  Thus there is  larger, but mechanical, testsuite change needed to deal with this.   I will post that if anyone’s interested, but otherwise will just apply it once the codgen fix is agreed.

The patch factors out some common code that writes out the jumps and uses the regular output scheme that accounts for __USER_LABEL_PREFIX__.

I will note in passing that there’s very little PIC test coverage for the indirection thunks code, although Darwin is PIC-only for m64 - Linux has only a few tests.

OK for trunk?
Backports?
Iain

gcc/

	* config/i386/i386.c (ix86_output_jmp_thunk_or_indirect): New.
	(ix86_output_indirect_branch_via_reg): Use output mechanism accounting for
	__USR_LABEL_PREFIX.
	(ix86_output_indirect_branch_via_push): Likewise.
	(ix86_output_function_return): Likewise.
	(ix86_output_indirect_function_return): Likewise.

From 4da5837cd7bbe61b6d2687e552e3afb5bfdb2765 Mon Sep 17 00:00:00 2001
From: Iain Sandoe <iain@sandoe.co.uk>
Date: Tue, 7 May 2019 07:27:19 -0400
Subject: [PATCH] [Darwin] Fix PR82920 - code changes.

Emit labels using machinery that includes the __USER_LABEL_PREFIX__
---
 gcc/config/i386/i386.c | 48 ++++++++++++++++++++++++------------------
 1 file changed, 27 insertions(+), 21 deletions(-)

Comments

Uros Bizjak May 10, 2019, 9:22 p.m. UTC | #1
On Fri, May 10, 2019 at 11:03 PM Iain Sandoe <idsandoe@googlemail.com> wrote:
>
> Hi!
>
> PR82920 is about CET fails on Darwin.
>
> Initially, this was expected to be just a testsuite issue, however it turns out that the indirection thunks code has inconsistent handling of the output of labels.  Thus some of the output is missing the leading “_” on Darwin, which breaks ABI and won’t link.
>
> Since most of the tests are scan-asms that check for what’s expected, they currently pass on Darwin but will begin failing when the codegen is fixed.  Thus there is  larger, but mechanical, testsuite change needed to deal with this.   I will post that if anyone’s interested, but otherwise will just apply it once the codgen fix is agreed.
>
> The patch factors out some common code that writes out the jumps and uses the regular output scheme that accounts for __USER_LABEL_PREFIX__.
>
> I will note in passing that there’s very little PIC test coverage for the indirection thunks code, although Darwin is PIC-only for m64 - Linux has only a few tests.
>
> OK for trunk?

OK for trunk if the patch doesn't regress x86_64-linux.

> Backports?

Also OK for backports after a couple of days of soaking in the trunk
without problems (so autotesters will test the patched compiler from
the trunk).

Thanks,
Uros.

> Iain
>
> gcc/
>
>         * config/i386/i386.c (ix86_output_jmp_thunk_or_indirect): New.
>         (ix86_output_indirect_branch_via_reg): Use output mechanism accounting for
>         __USR_LABEL_PREFIX.
>         (ix86_output_indirect_branch_via_push): Likewise.
>         (ix86_output_function_return): Likewise.
>         (ix86_output_indirect_function_return): Likewise.
>
> From 4da5837cd7bbe61b6d2687e552e3afb5bfdb2765 Mon Sep 17 00:00:00 2001
> From: Iain Sandoe <iain@sandoe.co.uk>
> Date: Tue, 7 May 2019 07:27:19 -0400
> Subject: [PATCH] [Darwin] Fix PR82920 - code changes.
>
> Emit labels using machinery that includes the __USER_LABEL_PREFIX__
> ---
>  gcc/config/i386/i386.c | 48 ++++++++++++++++++++++++------------------
>  1 file changed, 27 insertions(+), 21 deletions(-)
>
> diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
> index c51d775b89..08aa9d9475 100644
> --- a/gcc/config/i386/i386.c
> +++ b/gcc/config/i386/i386.c
> @@ -15130,6 +15130,20 @@ ix86_nopic_noplt_attribute_p (rtx call_op)
>    return false;
>  }
>
> +/* Helper to output the jmp/call.  */
> +static void
> +ix86_output_jmp_thunk_or_indirect (const char *thunk_name, const int regno)
> +{
> +  if (thunk_name != NULL)
> +    {
> +      fprintf (asm_out_file, "\tjmp\t");
> +      assemble_name (asm_out_file, thunk_name);
> +      putc ('\n', asm_out_file);
> +    }
> +  else
> +    output_indirect_thunk (regno);
> +}
> +
>  /* Output indirect branch via a call and return thunk.  CALL_OP is a
>     register which contains the branch target.  XASM is the assembly
>     template for CALL_OP.  Branch is a tail call if SIBCALL_P is true.
> @@ -15168,17 +15182,14 @@ ix86_output_indirect_branch_via_reg (rtx call_op, bool sibcall_p)
>      thunk_name = NULL;
>
>    if (sibcall_p)
> -    {
> -      if (thunk_name != NULL)
> -       fprintf (asm_out_file, "\tjmp\t%s\n", thunk_name);
> -      else
> -       output_indirect_thunk (regno);
> -    }
> +     ix86_output_jmp_thunk_or_indirect (thunk_name, regno);
>    else
>      {
>        if (thunk_name != NULL)
>         {
> -         fprintf (asm_out_file, "\tcall\t%s\n", thunk_name);
> +         fprintf (asm_out_file, "\tcall\t");
> +         assemble_name (asm_out_file, thunk_name);
> +         putc ('\n', asm_out_file);
>           return;
>         }
>
> @@ -15199,10 +15210,7 @@ ix86_output_indirect_branch_via_reg (rtx call_op, bool sibcall_p)
>
>        ASM_OUTPUT_INTERNAL_LABEL (asm_out_file, indirectlabel1);
>
> -      if (thunk_name != NULL)
> -       fprintf (asm_out_file, "\tjmp\t%s\n", thunk_name);
> -      else
> -       output_indirect_thunk (regno);
> +     ix86_output_jmp_thunk_or_indirect (thunk_name, regno);
>
>        ASM_OUTPUT_INTERNAL_LABEL (asm_out_file, indirectlabel2);
>
> @@ -15259,10 +15267,7 @@ ix86_output_indirect_branch_via_push (rtx call_op, const char *xasm,
>    if (sibcall_p)
>      {
>        output_asm_insn (push_buf, &call_op);
> -      if (thunk_name != NULL)
> -       fprintf (asm_out_file, "\tjmp\t%s\n", thunk_name);
> -      else
> -       output_indirect_thunk (regno);
> +      ix86_output_jmp_thunk_or_indirect (thunk_name, regno);
>      }
>    else
>      {
> @@ -15318,10 +15323,7 @@ ix86_output_indirect_branch_via_push (rtx call_op, const char *xasm,
>
>        output_asm_insn (push_buf, &call_op);
>
> -      if (thunk_name != NULL)
> -       fprintf (asm_out_file, "\tjmp\t%s\n", thunk_name);
> -      else
> -       output_indirect_thunk (regno);
> +      ix86_output_jmp_thunk_or_indirect (thunk_name, regno);
>
>        ASM_OUTPUT_INTERNAL_LABEL (asm_out_file, indirectlabel2);
>
> @@ -15420,7 +15422,9 @@ ix86_output_function_return (bool long_p)
>           indirect_thunk_name (thunk_name, INVALID_REGNUM, need_prefix,
>                                true);
>           indirect_return_needed |= need_thunk;
> -         fprintf (asm_out_file, "\tjmp\t%s\n", thunk_name);
> +         fprintf (asm_out_file, "\tjmp\t");
> +         assemble_name (asm_out_file, thunk_name);
> +         putc ('\n', asm_out_file);
>         }
>        else
>         output_indirect_thunk (INVALID_REGNUM);
> @@ -15460,7 +15464,9 @@ ix86_output_indirect_function_return (rtx ret_op)
>               indirect_return_via_cx = true;
>               indirect_thunks_used |= 1 << CX_REG;
>             }
> -         fprintf (asm_out_file, "\tjmp\t%s\n", thunk_name);
> +         fprintf (asm_out_file, "\tjmp\t");
> +         assemble_name (asm_out_file, thunk_name);
> +         putc ('\n', asm_out_file);
>         }
>        else
>         output_indirect_thunk (regno);
> --
> 2.17.1
>
>
Iain Sandoe May 12, 2019, 7:14 p.m. UTC | #2
> On 10 May 2019, at 22:22, Uros Bizjak <ubizjak@gmail.com> wrote:
> 
> On Fri, May 10, 2019 at 11:03 PM Iain Sandoe <idsandoe@googlemail.com> wrote:
>> 

>> PR82920 is about CET fails on Darwin.
>> 
>> Initially, this was expected to be just a testsuite issue, however it turns out that the indirection thunks code has inconsistent handling of the output of labels.  Thus some of the output is missing the leading “_” on Darwin, which breaks ABI and won’t link.
>> 
>> Since most of the tests are scan-asms that check for what’s expected, they currently pass on Darwin but will begin failing when the codegen is fixed.  Thus there is  larger, but mechanical, testsuite change needed to deal with this.   I will post that if anyone’s interested, but otherwise will just apply it once the codgen fix is agreed.
>> 
>> The patch factors out some common code that writes out the jumps and uses the regular output scheme that accounts for __USER_LABEL_PREFIX__.
>> 
>> I will note in passing that there’s very little PIC test coverage for the indirection thunks code, although Darwin is PIC-only for m64 - Linux has only a few tests.
>> 
>> OK for trunk?
> 
> OK for trunk if the patch doesn't regress x86_64-linux.

tested on x86_64-linux-gnu and x86_64-darwin (m32 and m64 in both cases).
Applied to trunk.

Many thanks to Dominique for working though some of the many testsuite changes needed.
There’s a second set of test changes needed to fix the PR completely, but those are disjoint
to the ones below which result from the codegen changes.

thanks
Iain

gcc/

2019-05-12  Iain Sandoe  <iain@sandoe.co.uk>

	PR target/82920
	* config/i386/i386.c (ix86_output_jmp_thunk_or_indirect): New.
	(ix86_output_indirect_branch_via_reg): Use output mechanism
	accounting for __USER_LABEL_PREFIX__.
	(ix86_output_indirect_branch_via_push): Likewise.
	(ix86_output_function_return): Likewise.
	(ix86_output_indirect_function_return): Likewise.

gcc/testsuite/

2019-05-12  Iain Sandoe  <iain@sandoe.co.uk>
	    Dominique d'Humieres  <dominiq@gcc.gnu.org>

	PR target/82920
	* gcc.target/i386/indirect-thunk-1.c: Adjust scan-asms for Darwin,
	do not use -fno-pic on Darwin.
	* gcc.target/i386/indirect-thunk-2.c: Likewise.
	* gcc.target/i386/indirect-thunk-3.c: Likewise.
	* gcc.target/i386/indirect-thunk-4.c: Likewise.
	* gcc.target/i386/indirect-thunk-7.c: Likewise.
	* gcc.target/i386/indirect-thunk-attr-1.c: Likewise.
	* gcc.target/i386/indirect-thunk-attr-2.c: Likewise.
	* gcc.target/i386/indirect-thunk-attr-3.c: Likewise.
	* gcc.target/i386/indirect-thunk-attr-4.c: Likewise.
	* gcc.target/i386/indirect-thunk-attr-5.c: Likewise.
	* gcc.target/i386/indirect-thunk-attr-6.c: Likewise.
	* gcc.target/i386/indirect-thunk-attr-7.c: Likewise.
	* gcc.target/i386/indirect-thunk-attr-8.c: Likewise.
	* gcc.target/i386/indirect-thunk-extern-1.c: Likewise.
	* gcc.target/i386/indirect-thunk-extern-2.c: Likewise.
	* gcc.target/i386/indirect-thunk-extern-3.c: Likewise.
	* gcc.target/i386/indirect-thunk-extern-4.c: Likewise.
	* gcc.target/i386/indirect-thunk-extern-7.c: Likewise.
	* gcc.target/i386/indirect-thunk-inline-1.c: Likewise.
	* gcc.target/i386/indirect-thunk-inline-2.c: Likewise.
	* gcc.target/i386/indirect-thunk-inline-3.c: Likewise.
	* gcc.target/i386/indirect-thunk-inline-4.c: Likewise.
	* gcc.target/i386/indirect-thunk-inline-7.c: Likewise.
	* gcc.target/i386/indirect-thunk-register-1.c: Likewise.
	* gcc.target/i386/indirect-thunk-register-2.c: Likewise.
	* gcc.target/i386/indirect-thunk-register-3.c: Likewise.
	* gcc.target/i386/indirect-thunk-register-4.c: Likewise.
	* gcc.target/i386/ret-thunk-1.c: Likewise.
	* gcc.target/i386/ret-thunk-10.c: Likewise.
	* gcc.target/i386/ret-thunk-11.c: Likewise.
	* gcc.target/i386/ret-thunk-12.c: Likewise.
	* gcc.target/i386/ret-thunk-13.c: Likewise.
	* gcc.target/i386/ret-thunk-14.c: Likewise.
	* gcc.target/i386/ret-thunk-15.c: Likewise.
	* gcc.target/i386/ret-thunk-16.c: Likewise.
	* gcc.target/i386/ret-thunk-2.c: Likewise.
	* gcc.target/i386/ret-thunk-22.c: Likewise.
	* gcc.target/i386/ret-thunk-23.c: Likewise.
	* gcc.target/i386/ret-thunk-24.c: Likewise.
	* gcc.target/i386/ret-thunk-3.c: Likewise.
	* gcc.target/i386/ret-thunk-4.c: Likewise.
	* gcc.target/i386/ret-thunk-5.c: Likewise.
	* gcc.target/i386/ret-thunk-6.c: Likewise.
	* gcc.target/i386/ret-thunk-7.c: Likewise.
	* gcc.target/i386/ret-thunk-8.c: Likewise.
	* gcc.target/i386/ret-thunk-9.c: Likewise.

Patch
diff mbox series

diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index c51d775b89..08aa9d9475 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -15130,6 +15130,20 @@  ix86_nopic_noplt_attribute_p (rtx call_op)
   return false;
 }
 
+/* Helper to output the jmp/call.  */
+static void
+ix86_output_jmp_thunk_or_indirect (const char *thunk_name, const int regno)
+{
+  if (thunk_name != NULL)
+    {
+      fprintf (asm_out_file, "\tjmp\t");
+      assemble_name (asm_out_file, thunk_name);
+      putc ('\n', asm_out_file);
+    }
+  else
+    output_indirect_thunk (regno);
+}
+
 /* Output indirect branch via a call and return thunk.  CALL_OP is a
    register which contains the branch target.  XASM is the assembly
    template for CALL_OP.  Branch is a tail call if SIBCALL_P is true.
@@ -15168,17 +15182,14 @@  ix86_output_indirect_branch_via_reg (rtx call_op, bool sibcall_p)
     thunk_name = NULL;
 
   if (sibcall_p)
-    {
-      if (thunk_name != NULL)
-	fprintf (asm_out_file, "\tjmp\t%s\n", thunk_name);
-      else
-	output_indirect_thunk (regno);
-    }
+     ix86_output_jmp_thunk_or_indirect (thunk_name, regno);
   else
     {
       if (thunk_name != NULL)
 	{
-	  fprintf (asm_out_file, "\tcall\t%s\n", thunk_name);
+	  fprintf (asm_out_file, "\tcall\t");
+	  assemble_name (asm_out_file, thunk_name);
+	  putc ('\n', asm_out_file);
 	  return;
 	}
 
@@ -15199,10 +15210,7 @@  ix86_output_indirect_branch_via_reg (rtx call_op, bool sibcall_p)
 
       ASM_OUTPUT_INTERNAL_LABEL (asm_out_file, indirectlabel1);
 
-      if (thunk_name != NULL)
-	fprintf (asm_out_file, "\tjmp\t%s\n", thunk_name);
-      else
-	output_indirect_thunk (regno);
+     ix86_output_jmp_thunk_or_indirect (thunk_name, regno);
 
       ASM_OUTPUT_INTERNAL_LABEL (asm_out_file, indirectlabel2);
 
@@ -15259,10 +15267,7 @@  ix86_output_indirect_branch_via_push (rtx call_op, const char *xasm,
   if (sibcall_p)
     {
       output_asm_insn (push_buf, &call_op);
-      if (thunk_name != NULL)
-	fprintf (asm_out_file, "\tjmp\t%s\n", thunk_name);
-      else
-	output_indirect_thunk (regno);
+      ix86_output_jmp_thunk_or_indirect (thunk_name, regno);
     }
   else
     {
@@ -15318,10 +15323,7 @@  ix86_output_indirect_branch_via_push (rtx call_op, const char *xasm,
 
       output_asm_insn (push_buf, &call_op);
 
-      if (thunk_name != NULL)
-	fprintf (asm_out_file, "\tjmp\t%s\n", thunk_name);
-      else
-	output_indirect_thunk (regno);
+      ix86_output_jmp_thunk_or_indirect (thunk_name, regno);
 
       ASM_OUTPUT_INTERNAL_LABEL (asm_out_file, indirectlabel2);
 
@@ -15420,7 +15422,9 @@  ix86_output_function_return (bool long_p)
 	  indirect_thunk_name (thunk_name, INVALID_REGNUM, need_prefix,
 			       true);
 	  indirect_return_needed |= need_thunk;
-	  fprintf (asm_out_file, "\tjmp\t%s\n", thunk_name);
+	  fprintf (asm_out_file, "\tjmp\t");
+	  assemble_name (asm_out_file, thunk_name);
+	  putc ('\n', asm_out_file);
 	}
       else
 	output_indirect_thunk (INVALID_REGNUM);
@@ -15460,7 +15464,9 @@  ix86_output_indirect_function_return (rtx ret_op)
 	      indirect_return_via_cx = true;
 	      indirect_thunks_used |= 1 << CX_REG;
 	    }
-	  fprintf (asm_out_file, "\tjmp\t%s\n", thunk_name);
+	  fprintf (asm_out_file, "\tjmp\t");
+	  assemble_name (asm_out_file, thunk_name);
+	  putc ('\n', asm_out_file);
 	}
       else
 	output_indirect_thunk (regno);