[PATCH] x86, libgcc: Implement ia32 basic heap trampoline [PR113855].

[PATCH] x86, libgcc: Implement ia32 basic heap trampoline [PR113855].

[Iain Sandoe]

I have so far tested this on i686-darwin17 and on x86_64-linux (with 32b
multilib )with the following permutations:
C (dg.exp=*nest*), Ada : \{-m64,-m32\}\{,-ftrampoline-impl=heap\}\{,-shared-libgcc\}
Fortran \{-m64,-m32\}\{,-ftrampoline-impl=heap\}

The only fails [gnat] are expected (scanning for GNU-stack when the impl. is heap).

Testing to follow on i686-darwin9 (32b kernel) and x86_64-darwin17 with the
corresponding multilibs.

I do not have ready access to an i686-linux host.

How does this look to resolve the PR?
Iain

--- 8< ---

The initial heap trampoline implementation was targeting 64b
platforms.  As the PR demonstrates this creates an issue where it
is expected that the same symbols are exported for 32 and 64b.

Rather than conditionalize the exports and code-gen on x86_64,
this patch provides a basic implementation of the IA32 trampoline.

This also avoids potential user confusion, when a 32b target has
64b multilibs, and vice versa; which is the case for Darwin.

	PR target/113855

gcc/ChangeLog:

	* config/i386/darwin.h (DARWIN_HEAP_T_LIB): Moved to be
	available to all sub-targets.
	* config/i386/darwin32-biarch.h (DARWIN_HEAP_T_LIB): Delete.
	* config/i386/darwin64-biarch.h (DARWIN_HEAP_T_LIB): Delete.

libgcc/ChangeLog:

	* config.host: Add trampoline support to x?86-linux.
	* config/i386/heap-trampoline.c (trampoline_insns): Provide
	a variant for IA32.
	(union ix86_trampoline): Likewise.
	(__gcc_nested_func_ptr_created): Implement a basic trampoline
	for IA32.
---
 gcc/config/i386/darwin.h             |  3 ++-
 gcc/config/i386/darwin32-biarch.h    |  3 ---
 gcc/config/i386/darwin64-biarch.h    |  3 ---
 libgcc/config.host                   |  1 +
 libgcc/config/i386/heap-trampoline.c | 40 +++++++++++++++++++++++++---
 5 files changed, 40 insertions(+), 10 deletions(-)

diff --git a/gcc/config/i386/darwin.h b/gcc/config/i386/darwin.h
index 8e64b4e9b5f..bf9c45d70bb 100644
--- a/gcc/config/i386/darwin.h
+++ b/gcc/config/i386/darwin.h
@@ -119,9 +119,10 @@ along with GCC; see the file COPYING3.  If not see
 /* We default to x86_64 for single-arch builds, bi-arch overrides.  */
 #define DARWIN_ARCH_SPEC "x86_64"
 #define DARWIN_SUBARCH_SPEC DARWIN_ARCH_SPEC
+#endif
+
 #undef DARWIN_HEAP_T_LIB
 #define DARWIN_HEAP_T_LIB " -lheapt_w "
-#endif
 
 #undef SUBTARGET_EXTRA_SPECS
 #define SUBTARGET_EXTRA_SPECS                                   \
diff --git a/gcc/config/i386/darwin32-biarch.h b/gcc/config/i386/darwin32-biarch.h
index 2180f5a5352..051ad12b425 100644
--- a/gcc/config/i386/darwin32-biarch.h
+++ b/gcc/config/i386/darwin32-biarch.h
@@ -27,9 +27,6 @@ along with GCC; see the file COPYING3.  If not see
 #undef  DARWIN_SUBARCH_SPEC
 #define DARWIN_SUBARCH_SPEC DARWIN_ARCH_SPEC
 
-#undef DARWIN_HEAP_T_LIB
-#define DARWIN_HEAP_T_LIB " %{m64:-lheapt_w}"
-
 #undef SUBTARGET_EXTRA_SPECS
 #define SUBTARGET_EXTRA_SPECS                                   \
   DARWIN_EXTRA_SPECS                                            \
diff --git a/gcc/config/i386/darwin64-biarch.h b/gcc/config/i386/darwin64-biarch.h
index 620800749a8..85436791a6c 100644
--- a/gcc/config/i386/darwin64-biarch.h
+++ b/gcc/config/i386/darwin64-biarch.h
@@ -28,9 +28,6 @@ along with GCC; see the file COPYING3.  If not see
 #undef  DARWIN_SUBARCH_SPEC
 #define DARWIN_SUBARCH_SPEC DARWIN_ARCH_SPEC
 
-#undef DARWIN_HEAP_T_LIB
-#define DARWIN_HEAP_T_LIB "%{!m32:-lheapt_w}"
-
 #undef SUBTARGET_EXTRA_SPECS
 #define SUBTARGET_EXTRA_SPECS                                   \
   DARWIN_EXTRA_SPECS                                            \
diff --git a/libgcc/config.host b/libgcc/config.host
index 3e7d00f67aa..59a42d3a01f 100644
--- a/libgcc/config.host
+++ b/libgcc/config.host
@@ -774,6 +774,7 @@ i[34567]86-*-linux*)
 	tmake_file="${tmake_file} i386/t-crtpc t-crtfm i386/t-crtstuff t-dfprules"
 	tm_file="${tm_file} i386/elf-lib.h"
 	md_unwind_header=i386/linux-unwind.h
+	tmake_file="${tmake_file} i386/t-heap-trampoline"
 	;;
 i[34567]86-*-kfreebsd*-gnu | i[34567]86-*-kopensolaris*-gnu)
 	extra_parts="$extra_parts crtprec32.o crtprec64.o crtprec80.o crtfastmath.o"
diff --git a/libgcc/config/i386/heap-trampoline.c b/libgcc/config/i386/heap-trampoline.c
index 657b344c10c..1df0aa06108 100644
--- a/libgcc/config/i386/heap-trampoline.c
+++ b/libgcc/config/i386/heap-trampoline.c
@@ -29,12 +29,13 @@ void *allocate_trampoline_page (void);
 void __gcc_nested_func_ptr_created (void *chain, void *func, void *dst);
 void __gcc_nested_func_ptr_deleted (void);
 
+#if __x86_64__
 static const uint8_t trampoline_insns[] = {
-  /* movabs $<chain>,%r11  */
+  /* movabs $<func>,%r11  */
   0x49, 0xbb,
   0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
 
-  /* movabs $<func>,%r10  */
+  /* movabs $<chain>,%r10  */
   0x49, 0xba,
   0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
 
@@ -54,6 +55,33 @@ union ix86_trampoline {
   } fields;
 };
 
+#elif __i386__
+
+static const uint8_t trampoline_insns[] = {
+  /* movl $<chain>,%ecx  */
+  0xb9,
+  0x00, 0x00, 0x00, 0x00,
+
+  /* jmpl <func>-. */
+  0xe9,
+  0x00, 0x00, 0x00, 0x00,
+};
+
+union ix86_trampoline {
+  uint8_t insns[sizeof(trampoline_insns)];
+
+  struct __attribute__((packed)) fields {
+    uint8_t insn_0[1];
+    void *chain_ptr;
+    uint8_t insn_1[1];
+    uintptr_t func_offset;
+  } fields;
+};
+
+#else
+#error unsupported architecture/ABI
+#endif
+
 struct tramp_ctrl_data
 {
   struct tramp_ctrl_data *prev;
@@ -145,8 +173,14 @@ __gcc_nested_func_ptr_created (void *chain, void *func, void *dst)
 
   memcpy (trampoline->insns, trampoline_insns,
 	  sizeof(trampoline_insns));
-  trampoline->fields.func_ptr = func;
   trampoline->fields.chain_ptr = chain;
+#if __x86_64__
+  trampoline->fields.func_ptr = func;
+#elif __i386__
+  uintptr_t off_add = (uintptr_t) &trampoline->fields.func_offset;
+  off_add += 4;
+  trampoline->fields.func_offset = (uintptr_t)func - off_add;
+#endif
 
 #if __APPLE__ && __ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__ >= 101400
   /* Re-enable write protection.  */
-- 
2.39.2 (Apple Git-143)

Re: [PATCH] x86, libgcc: Implement ia32 basic heap trampoline [PR113855].

[Jakub Jelinek]

On Sat, Feb 10, 2024 at 05:14:44PM +0000, Iain Sandoe wrote:
> 	PR target/113855
> 
> gcc/ChangeLog:
> 
> 	* config/i386/darwin.h (DARWIN_HEAP_T_LIB): Moved to be
> 	available to all sub-targets.
> 	* config/i386/darwin32-biarch.h (DARWIN_HEAP_T_LIB): Delete.
> 	* config/i386/darwin64-biarch.h (DARWIN_HEAP_T_LIB): Delete.
> 
> libgcc/ChangeLog:
> 
> 	* config.host: Add trampoline support to x?86-linux.
> 	* config/i386/heap-trampoline.c (trampoline_insns): Provide
> 	a variant for IA32.
> 	(union ix86_trampoline): Likewise.
> 	(__gcc_nested_func_ptr_created): Implement a basic trampoline
> 	for IA32.

LGTM.

I bet it probably doesn't work properly for -mx32 (which defines
__x86_64__), CCing H.J. on that, but that is a preexisting issue
(and I don't have any experience with it; I guess one would either
need to add 4 bytes of padding after the func_ptr so that those
bits remain zeros as sizeof (void *) is 4, but presumably it would be
better to just use movl (but into %r10) and maybe the jmpl instead
of movabsq.

	Jakub

Re: [PATCH] x86, libgcc: Implement ia32 basic heap trampoline [PR113855].

[Iain Sandoe]

> On 10 Feb 2024, at 17:46, Jakub Jelinek <jakub@redhat.com> wrote:
> 
> On Sat, Feb 10, 2024 at 05:14:44PM +0000, Iain Sandoe wrote:
>> 	PR target/113855
>> 
>> gcc/ChangeLog:
>> 
>> 	* config/i386/darwin.h (DARWIN_HEAP_T_LIB): Moved to be
>> 	available to all sub-targets.
>> 	* config/i386/darwin32-biarch.h (DARWIN_HEAP_T_LIB): Delete.
>> 	* config/i386/darwin64-biarch.h (DARWIN_HEAP_T_LIB): Delete.
>> 
>> libgcc/ChangeLog:
>> 
>> 	* config.host: Add trampoline support to x?86-linux.
>> 	* config/i386/heap-trampoline.c (trampoline_insns): Provide
>> 	a variant for IA32.
>> 	(union ix86_trampoline): Likewise.
>> 	(__gcc_nested_func_ptr_created): Implement a basic trampoline
>> 	for IA32.
> 
> LGTM.
> 
> I bet it probably doesn't work properly for -mx32 (which defines
> __x86_64__), CCing H.J. on that, but that is a preexisting issue
> (and I don't have any experience with it; I guess one would either
> need to add 4 bytes of padding after the func_ptr so that those
> bits remain zeros as sizeof (void *) is 4, but presumably it would be
> better to just use movl (but into %r10) and maybe the jmpl instead
> of movabsq.

It seems that ix86_trampoline_init flips codegen on TARGET_64BIT
so perhaps if we make it __x86_64__ && __LP64__?

I had also considered maybe forcing the alignment of the trampoline
allocations to a cache line (or maybe at least half a cache line).

These considerations are also something to add to the improvements
we can make to generalize the handling.

For now, assuming remaining testing does not throw up any new
issues, I’ll apply this to make progress on the PR.

thanks
Iain

> 
> 	Jakub
> 

Re: [PATCH] x86, libgcc: Implement ia32 basic heap trampoline [PR113855].

[H.J. Lu]

On Sat, Feb 10, 2024 at 9:46 AM Jakub Jelinek <jakub@redhat.com> wrote:
>
> On Sat, Feb 10, 2024 at 05:14:44PM +0000, Iain Sandoe wrote:
> >       PR target/113855
> >
> > gcc/ChangeLog:
> >
> >       * config/i386/darwin.h (DARWIN_HEAP_T_LIB): Moved to be
> >       available to all sub-targets.
> >       * config/i386/darwin32-biarch.h (DARWIN_HEAP_T_LIB): Delete.
> >       * config/i386/darwin64-biarch.h (DARWIN_HEAP_T_LIB): Delete.
> >
> > libgcc/ChangeLog:
> >
> >       * config.host: Add trampoline support to x?86-linux.
> >       * config/i386/heap-trampoline.c (trampoline_insns): Provide
> >       a variant for IA32.
> >       (union ix86_trampoline): Likewise.
> >       (__gcc_nested_func_ptr_created): Implement a basic trampoline
> >       for IA32.
>
> LGTM.
>
> I bet it probably doesn't work properly for -mx32 (which defines
> __x86_64__), CCing H.J. on that, but that is a preexisting issue
> (and I don't have any experience with it; I guess one would either
> need to add 4 bytes of padding after the func_ptr so that those
> bits remain zeros as sizeof (void *) is 4, but presumably it would be
> better to just use movl (but into %r10) and maybe the jmpl instead
> of movabsq.
>
>         Jakub
>

Are there any testcases to exercise this code on Linux?

-- 
H.J.

[PATCH] libgcc: Implement -mx32 [PR113855]

[Jakub Jelinek]

On Sat, Feb 10, 2024 at 10:05:34AM -0800, H.J. Lu wrote:
> > I bet it probably doesn't work properly for -mx32 (which defines
> > __x86_64__), CCing H.J. on that, but that is a preexisting issue
> > (and I don't have any experience with it; I guess one would either
> > need to add 4 bytes of padding after the func_ptr so that those
> > bits remain zeros as sizeof (void *) is 4, but presumably it would be
> > better to just use movl (but into %r10) and maybe the jmpl instead
> > of movabsq.
> 
> Are there any testcases to exercise this code on Linux?

Here is an untested attempt to implement it for -mx32 (well, I've compiled
it with -mx32 in libgcc by hand after stubbing
/usr/include/gnu/stubs-x32.h).

Testcase could be something like:

/* { dg-do run } */
/* { dg-options "-ftrampoline-impl=heap" } */

__attribute__((noipa)) int
bar (int (*fn) (int))
{
  return fn (42) + 1;
}

int
main ()
{
  int a = 0;
  int foo (int x) { if (x != 42) __builtin_abort (); return ++a; }
  if (bar (foo) != 2 || a != 1)
    __builtin_abort ();
  if (bar (foo) != 3 || a != 2)
    __builtin_abort ();
  a = 42;
  if (bar (foo) != 44 || a != 43)
    __builtin_abort ();
  return 0;
}
but I must say I'm also surprised we have no tests for this in the
testsuite.  Sure, we'd also need to add some effective target whether
-ftrampoline-impl=heap can be used for a link/runtime test or not.

2024-02-13  Jakub Jelinek  <jakub@redhat.com>

	PR target/113855
	* config/i386/heap-trampoline.c (trampoline_insns): Use movabsq
	instead of movabs in comments.  Add -mx32 variant.

--- libgcc/config/i386/heap-trampoline.c.jj	2024-02-12 18:48:08.548886036 +0100
+++ libgcc/config/i386/heap-trampoline.c	2024-02-13 10:24:06.348020670 +0100
@@ -30,18 +30,34 @@ void __gcc_nested_func_ptr_created (void
 void __gcc_nested_func_ptr_deleted (void);
 
 #if __x86_64__
+
+#ifdef __LP64__
 static const uint8_t trampoline_insns[] = {
-  /* movabs $<func>,%r11  */
+  /* movabsq $<func>,%r11  */
   0x49, 0xbb,
   0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
 
-  /* movabs $<chain>,%r10  */
+  /* movabsq $<chain>,%r10  */
   0x49, 0xba,
   0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
 
   /* rex.WB jmpq *%r11  */
   0x41, 0xff, 0xe3
 };
+#else
+static const uint8_t trampoline_insns[] = {
+  /* movl $<func>,%r11d  */
+  0x41, 0xbb,
+  0x00, 0x00, 0x00, 0x00,
+
+  /* movl $<chain>,%r10d  */
+  0x41, 0xba,
+  0x00, 0x00, 0x00, 0x00,
+
+  /* rex.WB jmpq *%r11  */
+  0x41, 0xff, 0xe3
+};
+#endif
 
 union ix86_trampoline {
   uint8_t insns[sizeof(trampoline_insns)];


	Jakub

[PATCH] x86: Support x32 and IBT in heap trampoline

[H.J. Lu]

On Tue, Feb 13, 2024 at 10:42:52AM +0100, Jakub Jelinek wrote:
> On Sat, Feb 10, 2024 at 10:05:34AM -0800, H.J. Lu wrote:
> > > I bet it probably doesn't work properly for -mx32 (which defines
> > > __x86_64__), CCing H.J. on that, but that is a preexisting issue
> > > (and I don't have any experience with it; I guess one would either
> > > need to add 4 bytes of padding after the func_ptr so that those
> > > bits remain zeros as sizeof (void *) is 4, but presumably it would be
> > > better to just use movl (but into %r10) and maybe the jmpl instead
> > > of movabsq.
> > 
> > Are there any testcases to exercise this code on Linux?
> 
> Here is an untested attempt to implement it for -mx32 (well, I've compiled
> it with -mx32 in libgcc by hand after stubbing
> /usr/include/gnu/stubs-x32.h).
> 
> Testcase could be something like:
> 
> /* { dg-do run } */
> /* { dg-options "-ftrampoline-impl=heap" } */
> 
> __attribute__((noipa)) int
> bar (int (*fn) (int))
> {
>   return fn (42) + 1;
> }
> 
> int
> main ()
> {
>   int a = 0;
>   int foo (int x) { if (x != 42) __builtin_abort (); return ++a; }
>   if (bar (foo) != 2 || a != 1)
>     __builtin_abort ();
>   if (bar (foo) != 3 || a != 2)
>     __builtin_abort ();
>   a = 42;
>   if (bar (foo) != 44 || a != 43)
>     __builtin_abort ();
>   return 0;
> }
> but I must say I'm also surprised we have no tests for this in the
> testsuite.  Sure, we'd also need to add some effective target whether
> -ftrampoline-impl=heap can be used for a link/runtime test or not.
> 
> 2024-02-13  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR target/113855
> 	* config/i386/heap-trampoline.c (trampoline_insns): Use movabsq
> 	instead of movabs in comments.  Add -mx32 variant.
> 

It works on x32.  I modified your patch to add IBT support and pad the
trampoline to the multiple of 4 bytes.

Thanks.

H.J.
---
2024-02-13  Jakub Jelinek  <jakub@redhat.com>
	    H.J. Lu  <hjl.tools@gmail.com>

	PR target/113855
	* config/i386/heap-trampoline.c (trampoline_insns): Add IBT
	support and pad to the multiple of 4 bytes.  Use movabsq
	instead of movabs in comments.  Add -mx32 variant.
---
 libgcc/config/i386/heap-trampoline.c | 42 ++++++++++++++++++++++++++--
 1 file changed, 39 insertions(+), 3 deletions(-)

diff --git a/libgcc/config/i386/heap-trampoline.c b/libgcc/config/i386/heap-trampoline.c
index 1df0aa06108..a8637dc92d3 100644
--- a/libgcc/config/i386/heap-trampoline.c
+++ b/libgcc/config/i386/heap-trampoline.c
@@ -30,28 +30,64 @@ void __gcc_nested_func_ptr_created (void *chain, void *func, void *dst);
 void __gcc_nested_func_ptr_deleted (void);
 
 #if __x86_64__
+
+#ifdef __LP64__
 static const uint8_t trampoline_insns[] = {
-  /* movabs $<func>,%r11  */
+#if defined __CET__ && (__CET__ & 1) != 0
+  /* endbr64.  */
+  0xf3, 0x0f, 0x1e, 0xfa,
+#endif
+
+  /* movabsq $<func>,%r11  */
   0x49, 0xbb,
   0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
 
-  /* movabs $<chain>,%r10  */
+  /* movabsq $<chain>,%r10  */
   0x49, 0xba,
   0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
 
   /* rex.WB jmpq *%r11  */
-  0x41, 0xff, 0xe3
+  0x41, 0xff, 0xe3,
+
+  /* Pad to the multiple of 4 bytes.  */
+  0x90
 };
+#else
+static const uint8_t trampoline_insns[] = {
+#if defined __CET__ && (__CET__ & 1) != 0
+  /* endbr64.  */
+  0xf3, 0x0f, 0x1e, 0xfa,
+#endif
+
+  /* movl $<func>,%r11d  */
+  0x41, 0xbb,
+  0x00, 0x00, 0x00, 0x00,
+
+  /* movl $<chain>,%r10d  */
+  0x41, 0xba,
+  0x00, 0x00, 0x00, 0x00,
+
+  /* rex.WB jmpq *%r11  */
+  0x41, 0xff, 0xe3,
+
+  /* Pad to the multiple of 4 bytes.  */
+  0x90
+};
+#endif
 
 union ix86_trampoline {
   uint8_t insns[sizeof(trampoline_insns)];
 
   struct __attribute__((packed)) fields {
+#if defined __CET__ && (__CET__ & 1) != 0
+    uint8_t endbr64[4];
+#endif
     uint8_t insn_0[2];
     void *func_ptr;
     uint8_t insn_1[2];
     void *chain_ptr;
     uint8_t insn_2[3];
+    uint8_t pad;
   } fields;
 };
 
-- 
2.43.0