diff mbox series

[v2] x86: Fix -fsplit-stack feature detection via TARGET_CAN_SPLIT_STACK

Message ID 20220221220449.20130-1-soeren@soeren-tempel.net
State New
Headers
Series [v2] x86: Fix -fsplit-stack feature detection via TARGET_CAN_SPLIT_STACK |

Commit Message

Li, Pan2 via Gcc-patches Feb. 21, 2022, 10:04 p.m. UTC 
  From: Sören Tempel <soeren@soeren-tempel.net>

Since commit c163647ffbc9a20c8feb6e079dbecccfe016c82e -fsplit-stack
is only supported on glibc targets. However, this original commit
required some fixups. As part of the fixup, the changes to the
gnu-user-common.h and gnu.h were partially reverted in commit
60953a23d57b13a672f751bec0c6eefc059eb1ab thus causing TARGET_CAN_SPLIT_STACK
to be defined for non-glibc targets even though -fsplit-stack is
actually not supported and attempting to use it causes a runtime error.

This causes gcc internal code, such as ./gcc/go/gospec.c to not
correctly detect that -fsplit-stack is not supported and thus causes
gccgo to fail compilation on non-glibc targets.

This commit ensures that TARGET_CAN_SPLIT_STACK is only set if the
default libc is glibc. It is presently unclear to me if there is a
better way to detect glibc at pre-processor time.

The proposed changes have been tested on x86 and x86_64 Alpine Linux
(which uses musl libc) and fix compilation of gccgo for this target.

Signed-off-by: Sören Tempel <soeren@soeren-tempel.net>

gcc/ChangeLog:

	* config/i386/gnu-user-common.h (defined): Only define
        TARGET_CAN_SPLIT_STACK for glibc targets.
	* config/i386/gnu.h (defined): Ditto.
---
Changes since v1: Use (DEFAULT_LIBC == LIBC_GLIBC) instead of
OPTION_GLIBC_P to detect use of glibc in a pre-processor context.

Is there a better way to detect use of glibc in the config header?

 gcc/config/i386/gnu-user-common.h | 5 +++--
 gcc/config/i386/gnu.h             | 5 +++--
 2 files changed, 6 insertions(+), 4 deletions(-)

Comments

Sören Tempel March 14, 2022, 7:07 p.m. UTC | #1 
Ping.

Summary: Currently, the macro guards for TARGET_CAN_SPLIT_STACK are not
aligned with the implementation of ix86_supports_split_stack on x86.
That is, on musl systems TARGET_CAN_SPLIT_STACK is defined even though
-fsplit-stack is not supported (via ix86_supports_split_stack). This
prevents gccgo (which uses TARGET_CAN_SPLIT_STACK) from compiling on
musl systems. The proposed patch attempts to align the behavior of
ix86_supports_split_stack and TARGET_CAN_SPLIT_STACK.

See: https://gcc.gnu.org/pipermail/gcc-patches/2022-February/590688.html

Greetings,
Sören

Sören Tempel <soeren@soeren-tempel.net> wrote:
> Since commit c163647ffbc9a20c8feb6e079dbecccfe016c82e -fsplit-stack
> is only supported on glibc targets. However, this original commit
> required some fixups. As part of the fixup, the changes to the
> gnu-user-common.h and gnu.h were partially reverted in commit
> 60953a23d57b13a672f751bec0c6eefc059eb1ab thus causing TARGET_CAN_SPLIT_STACK
> to be defined for non-glibc targets even though -fsplit-stack is
> actually not supported and attempting to use it causes a runtime error.
> 
> This causes gcc internal code, such as ./gcc/go/gospec.c to not
> correctly detect that -fsplit-stack is not supported and thus causes
> gccgo to fail compilation on non-glibc targets.
> 
> This commit ensures that TARGET_CAN_SPLIT_STACK is only set if the
> default libc is glibc. It is presently unclear to me if there is a
> better way to detect glibc at pre-processor time.
> 
> The proposed changes have been tested on x86 and x86_64 Alpine Linux
> (which uses musl libc) and fix compilation of gccgo for this target.
> 
> Signed-off-by: Sören Tempel <soeren@soeren-tempel.net>
> 
> gcc/ChangeLog:
> 
> 	* config/i386/gnu-user-common.h (defined): Only define
>         TARGET_CAN_SPLIT_STACK for glibc targets.
> 	* config/i386/gnu.h (defined): Ditto.
> ---
> Changes since v1: Use (DEFAULT_LIBC == LIBC_GLIBC) instead of
> OPTION_GLIBC_P to detect use of glibc in a pre-processor context.
> 
> Is there a better way to detect use of glibc in the config header?
> 
>  gcc/config/i386/gnu-user-common.h | 5 +++--
>  gcc/config/i386/gnu.h             | 5 +++--
>  2 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/gcc/config/i386/gnu-user-common.h b/gcc/config/i386/gnu-user-common.h
> index 00226f5a455..e126c3fa9fa 100644
> --- a/gcc/config/i386/gnu-user-common.h
> +++ b/gcc/config/i386/gnu-user-common.h
> @@ -66,7 +66,8 @@ along with GCC; see the file COPYING3.  If not see
>  #define STACK_CHECK_STATIC_BUILTIN 1
>  
>  /* We only build the -fsplit-stack support in libgcc if the
> -   assembler has full support for the CFI directives.  */
> -#if HAVE_GAS_CFI_PERSONALITY_DIRECTIVE
> +   assembler has full support for the CFI directives.  Also
> +   we only support -fsplit-stack on glibc targets.  */
> +#if HAVE_GAS_CFI_PERSONALITY_DIRECTIVE && (DEFAULT_LIBC == LIBC_GLIBC)
>  #define TARGET_CAN_SPLIT_STACK
>  #endif
> diff --git a/gcc/config/i386/gnu.h b/gcc/config/i386/gnu.h
> index 25fbc07f58c..17494333bb9 100644
> --- a/gcc/config/i386/gnu.h
> +++ b/gcc/config/i386/gnu.h
> @@ -41,8 +41,9 @@ along with GCC.  If not, see <http://www.gnu.org/licenses/>.
>  #define TARGET_THREAD_SSP_OFFSET        0x14
>  
>  /* We only build the -fsplit-stack support in libgcc if the
> -   assembler has full support for the CFI directives.  */
> -#if HAVE_GAS_CFI_PERSONALITY_DIRECTIVE
> +   assembler has full support for the CFI directives.  Also
> +   we only support -fsplit-stack on glibc targets.  */
> +#if HAVE_GAS_CFI_PERSONALITY_DIRECTIVE && (DEFAULT_LIBC == LIBC_GLIBC)
>  #define TARGET_CAN_SPLIT_STACK
>  #endif
>  /* We steal the last transactional memory word.  */
Sören Tempel May 1, 2022, 2:20 p.m. UTC | #2 
Pinging this again. This time with i386 port maintainers in CC.

Summary: The macro guards for TARGET_CAN_SPLIT_STACK on i386 are not
aligned with the implementation of ix86_supports_split_stack. That is,
on systems using musl libc ix86_supports_split_stack errors-out (since
-fsplit-stack is not supported in conjunction with musl) but the feature
test macro TARGET_CAN_SPLIT_STACK is still defined on these systems.
This prevents gccgo from compiling on musl-based systems. On other
architectures which support -fsplit-stack TARGET_CAN_SPLIT_STACK
seems to be defined correctly.

See: https://gcc.gnu.org/pipermail/gcc-patches/2022-February/590688.html

Greetings,
Sören Tempel

Sören Tempel <soeren@soeren-tempel.net> wrote:
> Ping.
> 
> Summary: Currently, the macro guards for TARGET_CAN_SPLIT_STACK are not
> aligned with the implementation of ix86_supports_split_stack on x86.
> That is, on musl systems TARGET_CAN_SPLIT_STACK is defined even though
> -fsplit-stack is not supported (via ix86_supports_split_stack). This
> prevents gccgo (which uses TARGET_CAN_SPLIT_STACK) from compiling on
> musl systems. The proposed patch attempts to align the behavior of
> ix86_supports_split_stack and TARGET_CAN_SPLIT_STACK.
> 
> See: https://gcc.gnu.org/pipermail/gcc-patches/2022-February/590688.html
> 
> Greetings,
> Sören
> 
> Sören Tempel <soeren@soeren-tempel.net> wrote:
> > Since commit c163647ffbc9a20c8feb6e079dbecccfe016c82e -fsplit-stack
> > is only supported on glibc targets. However, this original commit
> > required some fixups. As part of the fixup, the changes to the
> > gnu-user-common.h and gnu.h were partially reverted in commit
> > 60953a23d57b13a672f751bec0c6eefc059eb1ab thus causing TARGET_CAN_SPLIT_STACK
> > to be defined for non-glibc targets even though -fsplit-stack is
> > actually not supported and attempting to use it causes a runtime error.
> > 
> > This causes gcc internal code, such as ./gcc/go/gospec.c to not
> > correctly detect that -fsplit-stack is not supported and thus causes
> > gccgo to fail compilation on non-glibc targets.
> > 
> > This commit ensures that TARGET_CAN_SPLIT_STACK is only set if the
> > default libc is glibc. It is presently unclear to me if there is a
> > better way to detect glibc at pre-processor time.
> > 
> > The proposed changes have been tested on x86 and x86_64 Alpine Linux
> > (which uses musl libc) and fix compilation of gccgo for this target.
> > 
> > Signed-off-by: Sören Tempel <soeren@soeren-tempel.net>
> > 
> > gcc/ChangeLog:
> > 
> > 	* config/i386/gnu-user-common.h (defined): Only define
> >         TARGET_CAN_SPLIT_STACK for glibc targets.
> > 	* config/i386/gnu.h (defined): Ditto.
> > ---
> > Changes since v1: Use (DEFAULT_LIBC == LIBC_GLIBC) instead of
> > OPTION_GLIBC_P to detect use of glibc in a pre-processor context.
> > 
> > Is there a better way to detect use of glibc in the config header?
> > 
> >  gcc/config/i386/gnu-user-common.h | 5 +++--
> >  gcc/config/i386/gnu.h             | 5 +++--
> >  2 files changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/gcc/config/i386/gnu-user-common.h b/gcc/config/i386/gnu-user-common.h
> > index 00226f5a455..e126c3fa9fa 100644
> > --- a/gcc/config/i386/gnu-user-common.h
> > +++ b/gcc/config/i386/gnu-user-common.h
> > @@ -66,7 +66,8 @@ along with GCC; see the file COPYING3.  If not see
> >  #define STACK_CHECK_STATIC_BUILTIN 1
> >  
> >  /* We only build the -fsplit-stack support in libgcc if the
> > -   assembler has full support for the CFI directives.  */
> > -#if HAVE_GAS_CFI_PERSONALITY_DIRECTIVE
> > +   assembler has full support for the CFI directives.  Also
> > +   we only support -fsplit-stack on glibc targets.  */
> > +#if HAVE_GAS_CFI_PERSONALITY_DIRECTIVE && (DEFAULT_LIBC == LIBC_GLIBC)
> >  #define TARGET_CAN_SPLIT_STACK
> >  #endif
> > diff --git a/gcc/config/i386/gnu.h b/gcc/config/i386/gnu.h
> > index 25fbc07f58c..17494333bb9 100644
> > --- a/gcc/config/i386/gnu.h
> > +++ b/gcc/config/i386/gnu.h
> > @@ -41,8 +41,9 @@ along with GCC.  If not, see <http://www.gnu.org/licenses/>.
> >  #define TARGET_THREAD_SSP_OFFSET        0x14
> >  
> >  /* We only build the -fsplit-stack support in libgcc if the
> > -   assembler has full support for the CFI directives.  */
> > -#if HAVE_GAS_CFI_PERSONALITY_DIRECTIVE
> > +   assembler has full support for the CFI directives.  Also
> > +   we only support -fsplit-stack on glibc targets.  */
> > +#if HAVE_GAS_CFI_PERSONALITY_DIRECTIVE && (DEFAULT_LIBC == LIBC_GLIBC)
> >  #define TARGET_CAN_SPLIT_STACK
> >  #endif
> >  /* We steal the last transactional memory word.  */
Uros Bizjak May 3, 2022, 7:28 a.m. UTC | #3 
On Sun, May 1, 2022 at 4:20 PM Sören Tempel <soeren@soeren-tempel.net> wrote:
>
> Pinging this again. This time with i386 port maintainers in CC.
>
> Summary: The macro guards for TARGET_CAN_SPLIT_STACK on i386 are not
> aligned with the implementation of ix86_supports_split_stack. That is,
> on systems using musl libc ix86_supports_split_stack errors-out (since
> -fsplit-stack is not supported in conjunction with musl) but the feature
> test macro TARGET_CAN_SPLIT_STACK is still defined on these systems.
> This prevents gccgo from compiling on musl-based systems. On other
> architectures which support -fsplit-stack TARGET_CAN_SPLIT_STACK
> seems to be defined correctly.
>
> See: https://gcc.gnu.org/pipermail/gcc-patches/2022-February/590688.html
>
> Greetings,
> Sören Tempel
>
> Sören Tempel <soeren@soeren-tempel.net> wrote:
> > Ping.
> >
> > Summary: Currently, the macro guards for TARGET_CAN_SPLIT_STACK are not
> > aligned with the implementation of ix86_supports_split_stack on x86.
> > That is, on musl systems TARGET_CAN_SPLIT_STACK is defined even though
> > -fsplit-stack is not supported (via ix86_supports_split_stack). This
> > prevents gccgo (which uses TARGET_CAN_SPLIT_STACK) from compiling on
> > musl systems. The proposed patch attempts to align the behavior of
> > ix86_supports_split_stack and TARGET_CAN_SPLIT_STACK.
> >
> > See: https://gcc.gnu.org/pipermail/gcc-patches/2022-February/590688.html
> >
> > Greetings,
> > Sören
> >
> > Sören Tempel <soeren@soeren-tempel.net> wrote:
> > > Since commit c163647ffbc9a20c8feb6e079dbecccfe016c82e -fsplit-stack
> > > is only supported on glibc targets. However, this original commit
> > > required some fixups. As part of the fixup, the changes to the
> > > gnu-user-common.h and gnu.h were partially reverted in commit
> > > 60953a23d57b13a672f751bec0c6eefc059eb1ab thus causing TARGET_CAN_SPLIT_STACK
> > > to be defined for non-glibc targets even though -fsplit-stack is
> > > actually not supported and attempting to use it causes a runtime error.
> > >
> > > This causes gcc internal code, such as ./gcc/go/gospec.c to not
> > > correctly detect that -fsplit-stack is not supported and thus causes
> > > gccgo to fail compilation on non-glibc targets.
> > >
> > > This commit ensures that TARGET_CAN_SPLIT_STACK is only set if the
> > > default libc is glibc. It is presently unclear to me if there is a
> > > better way to detect glibc at pre-processor time.
> > >
> > > The proposed changes have been tested on x86 and x86_64 Alpine Linux
> > > (which uses musl libc) and fix compilation of gccgo for this target.
> > >
> > > Signed-off-by: Sören Tempel <soeren@soeren-tempel.net>
> > >
> > > gcc/ChangeLog:
> > >
> > >     * config/i386/gnu-user-common.h (defined): Only define
> > >         TARGET_CAN_SPLIT_STACK for glibc targets.
> > >     * config/i386/gnu.h (defined): Ditto.

This looks good to me, so OK.

Thanks,
Uros.

> > > ---
> > > Changes since v1: Use (DEFAULT_LIBC == LIBC_GLIBC) instead of
> > > OPTION_GLIBC_P to detect use of glibc in a pre-processor context.
> > >
> > > Is there a better way to detect use of glibc in the config header?
> > >
> > >  gcc/config/i386/gnu-user-common.h | 5 +++--
> > >  gcc/config/i386/gnu.h             | 5 +++--
> > >  2 files changed, 6 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/gcc/config/i386/gnu-user-common.h b/gcc/config/i386/gnu-user-common.h
> > > index 00226f5a455..e126c3fa9fa 100644
> > > --- a/gcc/config/i386/gnu-user-common.h
> > > +++ b/gcc/config/i386/gnu-user-common.h
> > > @@ -66,7 +66,8 @@ along with GCC; see the file COPYING3.  If not see
> > >  #define STACK_CHECK_STATIC_BUILTIN 1
> > >
> > >  /* We only build the -fsplit-stack support in libgcc if the
> > > -   assembler has full support for the CFI directives.  */
> > > -#if HAVE_GAS_CFI_PERSONALITY_DIRECTIVE
> > > +   assembler has full support for the CFI directives.  Also
> > > +   we only support -fsplit-stack on glibc targets.  */
> > > +#if HAVE_GAS_CFI_PERSONALITY_DIRECTIVE && (DEFAULT_LIBC == LIBC_GLIBC)
> > >  #define TARGET_CAN_SPLIT_STACK
> > >  #endif
> > > diff --git a/gcc/config/i386/gnu.h b/gcc/config/i386/gnu.h
> > > index 25fbc07f58c..17494333bb9 100644
> > > --- a/gcc/config/i386/gnu.h
> > > +++ b/gcc/config/i386/gnu.h
> > > @@ -41,8 +41,9 @@ along with GCC.  If not, see <http://www.gnu.org/licenses/>.
> > >  #define TARGET_THREAD_SSP_OFFSET        0x14
> > >
> > >  /* We only build the -fsplit-stack support in libgcc if the
> > > -   assembler has full support for the CFI directives.  */
> > > -#if HAVE_GAS_CFI_PERSONALITY_DIRECTIVE
> > > +   assembler has full support for the CFI directives.  Also
> > > +   we only support -fsplit-stack on glibc targets.  */
> > > +#if HAVE_GAS_CFI_PERSONALITY_DIRECTIVE && (DEFAULT_LIBC == LIBC_GLIBC)
> > >  #define TARGET_CAN_SPLIT_STACK
> > >  #endif
> > >  /* We steal the last transactional memory word.  */
Sören Tempel May 18, 2022, 7:34 a.m. UTC | #4 
Hi,

Uros Bizjak <ubizjak@gmail.com> wrote:
> > > > gcc/ChangeLog:
> > > >
> > > >     * config/i386/gnu-user-common.h (defined): Only define
> > > >         TARGET_CAN_SPLIT_STACK for glibc targets.
> > > >     * config/i386/gnu.h (defined): Ditto.
> 
> This looks good to me, so OK.
> 
> Thanks,
> Uros.

I am not deeply familiar with the GCC development process but since it
seems that this hasn't been merged yet: Is there anything else that I
need to do or what is the process for getting this patch upstreamed? (:

Thank you,
Sören

> > > > ---
> > > > Changes since v1: Use (DEFAULT_LIBC == LIBC_GLIBC) instead of
> > > > OPTION_GLIBC_P to detect use of glibc in a pre-processor context.
> > > >
> > > > Is there a better way to detect use of glibc in the config header?
> > > >
> > > >  gcc/config/i386/gnu-user-common.h | 5 +++--
> > > >  gcc/config/i386/gnu.h             | 5 +++--
> > > >  2 files changed, 6 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/gcc/config/i386/gnu-user-common.h b/gcc/config/i386/gnu-user-common.h
> > > > index 00226f5a455..e126c3fa9fa 100644
> > > > --- a/gcc/config/i386/gnu-user-common.h
> > > > +++ b/gcc/config/i386/gnu-user-common.h
> > > > @@ -66,7 +66,8 @@ along with GCC; see the file COPYING3.  If not see
> > > >  #define STACK_CHECK_STATIC_BUILTIN 1
> > > >
> > > >  /* We only build the -fsplit-stack support in libgcc if the
> > > > -   assembler has full support for the CFI directives.  */
> > > > -#if HAVE_GAS_CFI_PERSONALITY_DIRECTIVE
> > > > +   assembler has full support for the CFI directives.  Also
> > > > +   we only support -fsplit-stack on glibc targets.  */
> > > > +#if HAVE_GAS_CFI_PERSONALITY_DIRECTIVE && (DEFAULT_LIBC == LIBC_GLIBC)
> > > >  #define TARGET_CAN_SPLIT_STACK
> > > >  #endif
> > > > diff --git a/gcc/config/i386/gnu.h b/gcc/config/i386/gnu.h
> > > > index 25fbc07f58c..17494333bb9 100644
> > > > --- a/gcc/config/i386/gnu.h
> > > > +++ b/gcc/config/i386/gnu.h
> > > > @@ -41,8 +41,9 @@ along with GCC.  If not, see <http://www.gnu.org/licenses/>.
> > > >  #define TARGET_THREAD_SSP_OFFSET        0x14
> > > >
> > > >  /* We only build the -fsplit-stack support in libgcc if the
> > > > -   assembler has full support for the CFI directives.  */
> > > > -#if HAVE_GAS_CFI_PERSONALITY_DIRECTIVE
> > > > +   assembler has full support for the CFI directives.  Also
> > > > +   we only support -fsplit-stack on glibc targets.  */
> > > > +#if HAVE_GAS_CFI_PERSONALITY_DIRECTIVE && (DEFAULT_LIBC == LIBC_GLIBC)
> > > >  #define TARGET_CAN_SPLIT_STACK
> > > >  #endif
> > > >  /* We steal the last transactional memory word.  */
Uros Bizjak May 18, 2022, 9:19 a.m. UTC | #5 
On Wed, May 18, 2022 at 9:34 AM Sören Tempel <soeren@soeren-tempel.net> wrote:
>
> Hi,
>
> Uros Bizjak <ubizjak@gmail.com> wrote:
> > > > > gcc/ChangeLog:
> > > > >
> > > > >     * config/i386/gnu-user-common.h (defined): Only define
> > > > >         TARGET_CAN_SPLIT_STACK for glibc targets.
> > > > >     * config/i386/gnu.h (defined): Ditto.
> >
> > This looks good to me, so OK.
> >
> > Thanks,
> > Uros.
>
> I am not deeply familiar with the GCC development process but since it
> seems that this hasn't been merged yet: Is there anything else that I
> need to do or what is the process for getting this patch upstreamed? (:

You should state in your patch submission that you don't have commit rights.

Anyway, please send me (privately) the updated patch, I'll commit it
in your name later today.

Uros.
diff mbox series

Patch

diff --git a/gcc/config/i386/gnu-user-common.h b/gcc/config/i386/gnu-user-common.h
index 00226f5a455..e126c3fa9fa 100644
--- a/gcc/config/i386/gnu-user-common.h
+++ b/gcc/config/i386/gnu-user-common.h
@@ -66,7 +66,8 @@  along with GCC; see the file COPYING3.  If not see
 #define STACK_CHECK_STATIC_BUILTIN 1
 
 /* We only build the -fsplit-stack support in libgcc if the
-   assembler has full support for the CFI directives.  */
-#if HAVE_GAS_CFI_PERSONALITY_DIRECTIVE
+   assembler has full support for the CFI directives.  Also
+   we only support -fsplit-stack on glibc targets.  */
+#if HAVE_GAS_CFI_PERSONALITY_DIRECTIVE && (DEFAULT_LIBC == LIBC_GLIBC)
 #define TARGET_CAN_SPLIT_STACK
 #endif
diff --git a/gcc/config/i386/gnu.h b/gcc/config/i386/gnu.h
index 25fbc07f58c..17494333bb9 100644
--- a/gcc/config/i386/gnu.h
+++ b/gcc/config/i386/gnu.h
@@ -41,8 +41,9 @@  along with GCC.  If not, see <http://www.gnu.org/licenses/>.
 #define TARGET_THREAD_SSP_OFFSET        0x14
 
 /* We only build the -fsplit-stack support in libgcc if the
-   assembler has full support for the CFI directives.  */
-#if HAVE_GAS_CFI_PERSONALITY_DIRECTIVE
+   assembler has full support for the CFI directives.  Also
+   we only support -fsplit-stack on glibc targets.  */
+#if HAVE_GAS_CFI_PERSONALITY_DIRECTIVE && (DEFAULT_LIBC == LIBC_GLIBC)
 #define TARGET_CAN_SPLIT_STACK
 #endif
 /* We steal the last transactional memory word.  */