From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by sourceware.org (Postfix) with ESMTP id D3FE13857C7F for ; Mon, 24 May 2021 14:33:23 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org D3FE13857C7F Received: from mail-qv1-f72.google.com (mail-qv1-f72.google.com [209.85.219.72]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-565-CcCeTrmTNp6t4Xb0SxAqYw-1; Mon, 24 May 2021 10:33:22 -0400 X-MC-Unique: CcCeTrmTNp6t4Xb0SxAqYw-1 Received: by mail-qv1-f72.google.com with SMTP id e2-20020ad442a20000b02901f3586a14easo20252339qvr.12 for ; Mon, 24 May 2021 07:33:22 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:organization :message-id:date:user-agent:mime-version:in-reply-to :content-language:content-transfer-encoding; bh=Tbx/bMHrkyerBUiKhjhnSAFkQB6SwuTtHlj4rtspdDk=; b=dRr8J5jcIqmbXdUc6mAC7Z/sVJIxE2NbDV1UAMbWDCcg9sjeCb6gaCD7IXTYu0zMxj UKQcXHPGkB9xj0GB2tce5zrCB1QdnFL5L0tH5DIF10FOrpghjZpl6vooEsSSgu9kQe7N e8m5CXEgenX7yd5OOmeYqW5WWfoGsyrevL8O0CKeCXuEutdg1vFgNQ07bIjnDgnzYdBO AArpZlYLZBnyatcJS/Udl1yTlslFbZbvLHEY4uC50JKhQ9JlHKrSaUu2Mji9JkcCgsP8 2fs5euxrLJ0eNISKTTYVU8DBUjFyIF5a+nvqRtCLgkBRC33/9z9S5Zc5O4Wg1A17mNEP F9Cg== X-Gm-Message-State: AOAM530EpI2wKvdSDmyvA6T6593fNGJabFbNXZWEWRAteJ2/1GafHZAm 8GNihMp/RrPmczZusyDnaOvAb4XzPu7KEeIBMj07YogouA0pI+CKr+Rk/E2RwqucUhEJumDY27y TPmNFWf9WM79oxAY/kL6w8zvLWEsznic1Cf3biF5mtdw/7pHFj9g5AFHdVqO+cLae3QUKMA== X-Received: by 2002:a37:7605:: with SMTP id r5mr7849033qkc.321.1621866801434; Mon, 24 May 2021 07:33:21 -0700 (PDT) X-Google-Smtp-Source: ABdhPJzxA5+bI30m05k2tWn74Qa5ut4xyaKMgTLGyfRwOs8vYGtiHv+4ZdO79Gc4x243XL6SEKjziw== X-Received: by 2002:a37:7605:: with SMTP id r5mr7849003qkc.321.1621866801048; Mon, 24 May 2021 07:33:21 -0700 (PDT) Received: from [192.168.1.16] (198-84-214-74.cpe.teksavvy.com. [198.84.214.74]) by smtp.gmail.com with ESMTPSA id t11sm11300995qkm.123.2021.05.24.07.33.20 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 24 May 2021 07:33:20 -0700 (PDT) Subject: Re: [PATCH v2] Properly check stack alignment [BZ #27901] To: "H.J. Lu" , libc-alpha@sourceware.org References: <20210524130647.737409-1-hjl.tools@gmail.com> From: Carlos O'Donell Organization: Red Hat Message-ID: Date: Mon, 24 May 2021 10:33:19 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.10.1 MIME-Version: 1.0 In-Reply-To: <20210524130647.737409-1-hjl.tools@gmail.com> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-11.8 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, NICE_REPLY_A, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 24 May 2021 14:33:26 -0000 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 > --- > 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 > #include > > +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 > - . */ > - > -#include > -#include > -#ifndef __SSE__ > -#include_next > -#else > -#include > - > -#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 > - . */ > - > -#include > -#include > - > -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 > . */ > > -#include > -#include > - > -#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 > 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 > + . */ > + > +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 > 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 > - . */ > - > -#include > -#include > - > -#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.