From: Carlos O'Donell <carlos@redhat.com>
To: "H.J. Lu" <hjl.tools@gmail.com>, libc-alpha@sourceware.org
Subject: Re: [PATCH v2] Properly check stack alignment [BZ #27901]
Date: Mon, 24 May 2021 10:33:19 -0400 [thread overview]
Message-ID: <e5f8d0b4-cc44-7153-338a-ece5ad41e7ec@redhat.com> (raw)
In-Reply-To: <20210524130647.737409-1-hjl.tools@gmail.com>
On 5/24/21 9:06 AM, H.J. Lu via Libc-alpha wrote:
> 1. Replace
>
> if ((((uintptr_t) &_d) & (__alignof (double) - 1)) != 0)
>
> which may be optimized by compiler, with
>
> int
> __attribute__ ((weak, noclone, noinline))
> is_aligned (void *p, int align)
> {
> return (((uintptr_t) p) & (align - 1)) != 0;
> }
OK. Skips using the ipa suggestion from Florian because that would
require gcc 8 to build glibc and we only currently require gcc 6.2
in general.
>
> 2. Add TEST_STACK_ALIGN_INIT to TEST_STACK_ALIGN.
> 3. Add a common TEST_STACK_ALIGN_INIT to check 16-byte stack alignment
> for both i386 and x86-64.
> 4. Update powerpc to use TEST_STACK_ALIGN_INIT.
LGTM.
Reviewed-by: Carlos O'Donell <carlos@redhat.com>
> ---
> sysdeps/generic/tst-stack-align.h | 40 ++++++++++++++++---------
> sysdeps/i386/i686/tst-stack-align.h | 44 ---------------------------
> sysdeps/i386/tst-stack-align.h | 41 -------------------------
> sysdeps/powerpc/tst-stack-align.h | 27 +++++------------
> sysdeps/x86/tst-stack-align.h | 28 ++++++++++++++++++
> sysdeps/x86_64/tst-stack-align.h | 46 -----------------------------
> 6 files changed, 61 insertions(+), 165 deletions(-)
> delete mode 100644 sysdeps/i386/i686/tst-stack-align.h
> delete mode 100644 sysdeps/i386/tst-stack-align.h
> create mode 100644 sysdeps/x86/tst-stack-align.h
> delete mode 100644 sysdeps/x86_64/tst-stack-align.h
>
> diff --git a/sysdeps/generic/tst-stack-align.h b/sysdeps/generic/tst-stack-align.h
> index b41f9d3c12..6f7c4260b5 100644
> --- a/sysdeps/generic/tst-stack-align.h
> +++ b/sysdeps/generic/tst-stack-align.h
> @@ -1,4 +1,5 @@
> -/* Copyright (C) 2003-2021 Free Software Foundation, Inc.
> +/* Check stack alignment. Generic version.
> + Copyright (C) 2003-2021 Free Software Foundation, Inc.
OK.
> This file is part of the GNU C Library.
>
> The GNU C Library is free software; you can redistribute it and/or
> @@ -18,17 +19,28 @@
> #include <stdio.h>
> #include <stdint.h>
>
> +int
> +__attribute__ ((weak, noclone, noinline))
> +is_aligned (void *p, int align)
> +{
> + return (((uintptr_t) p) & (align - 1)) != 0;
> +}
OK. Uses weak as suggested by Florian.
> +
> +#ifndef TEST_STACK_ALIGN_INIT
> +# define TEST_STACK_ALIGN_INIT() 0
> +#endif
> +
> #define TEST_STACK_ALIGN() \
> - ({ \
> - double _d = 12.0; \
> - long double _ld = 15.0; \
> - int _ret = 0; \
> - printf ("double: %g %p %zu\n", _d, &_d, __alignof (double)); \
> - if ((((uintptr_t) &_d) & (__alignof (double) - 1)) != 0) \
> - _ret = 1; \
> - \
> - printf ("ldouble: %Lg %p %zu\n", _ld, &_ld, __alignof (long double)); \
> - if ((((uintptr_t) &_ld) & (__alignof (long double) - 1)) != 0) \
> - _ret = 1; \
> - _ret; \
> - })
> + ({ \
> + double _d = 12.0; \
> + long double _ld = 15.0; \
> + int _ret = TEST_STACK_ALIGN_INIT (); \
> + \
> + printf ("double: %g %p %zu\n", _d, &_d, __alignof (double)); \
> + _ret += is_aligned (&_d, __alignof (double)); \
> + \
> + printf ("ldouble: %Lg %p %zu\n", _ld, &_ld, \
> + __alignof (long double)); \
> + _ret += is_aligned (&_ld, __alignof (long double)); \
> + _ret; \
> + })
OK.
> diff --git a/sysdeps/i386/i686/tst-stack-align.h b/sysdeps/i386/i686/tst-stack-align.h
> deleted file mode 100644
> index fc3852ea5d..0000000000
> --- a/sysdeps/i386/i686/tst-stack-align.h
> +++ /dev/null
> @@ -1,44 +0,0 @@
> -/* Copyright (C) 2003-2021 Free Software Foundation, Inc.
> - This file is part of the GNU C Library.
> -
> - The GNU C Library is free software; you can redistribute it and/or
> - modify it under the terms of the GNU Lesser General Public
> - License as published by the Free Software Foundation; either
> - version 2.1 of the License, or (at your option) any later version.
> -
> - The GNU C Library is distributed in the hope that it will be useful,
> - but WITHOUT ANY WARRANTY; without even the implied warranty of
> - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> - Lesser General Public License for more details.
> -
> - You should have received a copy of the GNU Lesser General Public
> - License along with the GNU C Library; if not, see
> - <https://www.gnu.org/licenses/>. */
> -
> -#include <stdio.h>
> -#include <stdint.h>
> -#ifndef __SSE__
> -#include_next <tst-stack-align.h>
> -#else
> -#include <xmmintrin.h>
> -
> -#define TEST_STACK_ALIGN() \
> - ({ \
> - __m128 _m; \
> - double _d = 12.0; \
> - long double _ld = 15.0; \
> - int _ret = 0; \
> - printf ("__m128: %p %zu\n", &_m, __alignof (__m128)); \
> - if ((((uintptr_t) &_m) & (__alignof (__m128) - 1)) != 0) \
> - _ret = 1; \
> - \
> - printf ("double: %g %p %zu\n", _d, &_d, __alignof (double)); \
> - if ((((uintptr_t) &_d) & (__alignof (double) - 1)) != 0) \
> - _ret = 1; \
> - \
> - printf ("ldouble: %Lg %p %zu\n", _ld, &_ld, __alignof (long double)); \
> - if ((((uintptr_t) &_ld) & (__alignof (long double) - 1)) != 0) \
> - _ret = 1; \
> - _ret; \
> - })
> -#endif
OK. Remove.
> diff --git a/sysdeps/i386/tst-stack-align.h b/sysdeps/i386/tst-stack-align.h
> deleted file mode 100644
> index 7891e9a03e..0000000000
> --- a/sysdeps/i386/tst-stack-align.h
> +++ /dev/null
> @@ -1,41 +0,0 @@
> -/* Copyright (C) 2004-2021 Free Software Foundation, Inc.
> - This file is part of the GNU C Library.
> -
> - The GNU C Library is free software; you can redistribute it and/or
> - modify it under the terms of the GNU Lesser General Public
> - License as published by the Free Software Foundation; either
> - version 2.1 of the License, or (at your option) any later version.
> -
> - The GNU C Library is distributed in the hope that it will be useful,
> - but WITHOUT ANY WARRANTY; without even the implied warranty of
> - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> - Lesser General Public License for more details.
> -
> - You should have received a copy of the GNU Lesser General Public
> - License along with the GNU C Library; if not, see
> - <https://www.gnu.org/licenses/>. */
> -
> -#include <stdio.h>
> -#include <stdint.h>
> -
> -typedef struct { int i[4]; } int_al16 __attribute__((aligned (16)));
> -
> -#define TEST_STACK_ALIGN() \
> - ({ \
> - int_al16 _m; \
> - double _d = 12.0; \
> - long double _ld = 15.0; \
> - int _ret = 0; \
> - printf ("int_al16: %p %zu\n", &_m, __alignof (int_al16)); \
> - if ((((uintptr_t) &_m) & (__alignof (int_al16) - 1)) != 0) \
> - _ret = 1; \
> - \
> - printf ("double: %g %p %zu\n", _d, &_d, __alignof (double)); \
> - if ((((uintptr_t) &_d) & (__alignof (double) - 1)) != 0) \
> - _ret = 1; \
> - \
> - printf ("ldouble: %Lg %p %zu\n", _ld, &_ld, __alignof (long double)); \
> - if ((((uintptr_t) &_ld) & (__alignof (long double) - 1)) != 0) \
> - _ret = 1; \
> - _ret; \
> - })
OK. Remove.
> diff --git a/sysdeps/powerpc/tst-stack-align.h b/sysdeps/powerpc/tst-stack-align.h
> index 2b4a3671b4..be055b0385 100644
> --- a/sysdeps/powerpc/tst-stack-align.h
> +++ b/sysdeps/powerpc/tst-stack-align.h
> @@ -1,4 +1,5 @@
> -/* Copyright (C) 2005-2021 Free Software Foundation, Inc.
> +/* Check stack alignment. PowerPC version.
> + Copyright (C) 2005-2021 Free Software Foundation, Inc.
OK.
> This file is part of the GNU C Library.
>
> The GNU C Library is free software; you can redistribute it and/or
> @@ -15,10 +16,7 @@
> License along with the GNU C Library; if not, see
> <https://www.gnu.org/licenses/>. */
>
> -#include <stdio.h>
> -#include <stdint.h>
> -
> -#define TEST_STACK_ALIGN() \
> +#define TEST_STACK_ALIGN_INIT() \
> ({ \
> /* Altivec __vector int etc. needs 16byte aligned stack. \
> Instead of using altivec.h here, use aligned attribute instead. */ \
> @@ -27,20 +25,9 @@
> int _i __attribute__((aligned (16))); \
> int _j[3]; \
> } _s = { ._i = 18, ._j[0] = 19, ._j[1] = 20, ._j[2] = 21 }; \
> - double _d = 12.0; \
> - long double _ld = 15.0; \
> - int _ret = 0; \
> printf ("__vector int: { %d, %d, %d, %d } %p %zu\n", _s._i, _s._j[0], \
> _s._j[1], _s._j[2], &_s, __alignof (_s)); \
> - if ((((uintptr_t) &_s) & (__alignof (_s) - 1)) != 0) \
> - _ret = 1; \
> - \
> - printf ("double: %g %p %zu\n", _d, &_d, __alignof (double)); \
> - if ((((uintptr_t) &_d) & (__alignof (double) - 1)) != 0) \
> - _ret = 1; \
> - \
> - printf ("ldouble: %Lg %p %zu\n", _ld, &_ld, __alignof (long double)); \
> - if ((((uintptr_t) &_ld) & (__alignof (long double) - 1)) != 0) \
> - _ret = 1; \
> - _ret; \
> - })
> + is_aligned (&_s, __alignof (_s)); \
OK. Remove generic tests and leave only the vector aligned test.
> + })
> +
> +#include_next <tst-stack-align.h>
> diff --git a/sysdeps/x86/tst-stack-align.h b/sysdeps/x86/tst-stack-align.h
> new file mode 100644
> index 0000000000..578d5c4a80
> --- /dev/null
> +++ b/sysdeps/x86/tst-stack-align.h
> @@ -0,0 +1,28 @@
> +/* Check stack alignment. X86 version.
> + Copyright (C) 2021 Free Software Foundation, Inc.
OK.
> + This file is part of the GNU C Library.
> +
> + The GNU C Library is free software; you can redistribute it and/or
> + modify it under the terms of the GNU Lesser General Public
> + License as published by the Free Software Foundation; either
> + version 2.1 of the License, or (at your option) any later version.
> +
> + The GNU C Library is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + Lesser General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public
> + License along with the GNU C Library; if not, see
> + <https://www.gnu.org/licenses/>. */
> +
> +typedef struct { int i[16]; } int_al16 __attribute__((aligned (16)));
> +
> +#define TEST_STACK_ALIGN_INIT() \
> + ({ \
> + int_al16 _m; \
> + printf ("int_al16: %p %zu\n", &_m, __alignof (int_al16)); \
> + is_aligned (&_m, __alignof (int_al16)); \
> + })
OK. Check only the specific type for x86.
> +
> +#include_next <tst-stack-align.h>
> diff --git a/sysdeps/x86_64/tst-stack-align.h b/sysdeps/x86_64/tst-stack-align.h
> deleted file mode 100644
> index b3f93f7cab..0000000000
> --- a/sysdeps/x86_64/tst-stack-align.h
> +++ /dev/null
> @@ -1,46 +0,0 @@
> -/* Copyright (C) 2003-2021 Free Software Foundation, Inc.
> - This file is part of the GNU C Library.
> -
> - The GNU C Library is free software; you can redistribute it and/or
> - modify it under the terms of the GNU Lesser General Public
> - License as published by the Free Software Foundation; either
> - version 2.1 of the License, or (at your option) any later version.
> -
> - The GNU C Library is distributed in the hope that it will be useful,
> - but WITHOUT ANY WARRANTY; without even the implied warranty of
> - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> - Lesser General Public License for more details.
> -
> - You should have received a copy of the GNU Lesser General Public
> - License along with the GNU C Library; if not, see
> - <https://www.gnu.org/licenses/>. */
> -
> -#include <stdio.h>
> -#include <stdint.h>
> -
> -#define TEST_STACK_ALIGN() \
> - ({ \
> - /* AMD64 ABI mandates 16byte aligned stack. \
> - Unfortunately, current GCC doesn't support __int128 or __float128 \
> - types, so use aligned attribute instead. */ \
> - struct _S \
> - { \
> - int _i __attribute__((aligned (16))); \
> - int _pad[3]; \
> - } _s = { ._i = 18 }; \
> - double _d = 12.0; \
> - long double _ld = 15.0; \
> - int _ret = 0; \
> - printf ("__int128: %d %p %zu\n", _s._i, &_s, __alignof (_s)); \
> - if ((((uintptr_t) &_s) & (__alignof (_s) - 1)) != 0) \
> - _ret = 1; \
> - \
> - printf ("double: %g %p %zu\n", _d, &_d, __alignof (double)); \
> - if ((((uintptr_t) &_d) & (__alignof (double) - 1)) != 0) \
> - _ret = 1; \
> - \
> - printf ("ldouble: %Lg %p %zu\n", _ld, &_ld, __alignof (long double)); \
> - if ((((uintptr_t) &_ld) & (__alignof (long double) - 1)) != 0) \
> - _ret = 1; \
> - _ret; \
> - })
>
OK. Remove covered by generic test.
--
Cheers,
Carlos.
prev parent reply other threads:[~2021-05-24 14:33 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-05-24 13:06 H.J. Lu
2021-05-24 14:33 ` Carlos O'Donell [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=e5f8d0b4-cc44-7153-338a-ece5ad41e7ec@redhat.com \
--to=carlos@redhat.com \
--cc=hjl.tools@gmail.com \
--cc=libc-alpha@sourceware.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).