From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 101532 invoked by alias); 29 Nov 2018 03:48:43 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 101282 invoked by uid 89); 29 Nov 2018 03:48:24 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: =?ISO-8859-1?Q?No, score=-11.9 required=5.0 tests=BAYES_00,GIT_PATCH_2,GIT_PATCH_3,KAM_SHORT,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=so, of, of=c2, an?= X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 29 Nov 2018 03:48:20 +0000 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 708883001A72; Thu, 29 Nov 2018 03:48:07 +0000 (UTC) Received: from localhost.localdomain (ovpn-112-30.rdu2.redhat.com [10.10.112.30]) by smtp.corp.redhat.com (Postfix) with ESMTP id 71C4A26192; Thu, 29 Nov 2018 03:48:05 +0000 (UTC) Subject: Re: [PATCH] be more permissive about function alignments (PR 88208) To: Martin Sebor , Gcc Patch List References: <19112bc8-83a2-2c35-2841-d95087cd1178@gmail.com> From: Jeff Law Openpgp: preference=signencrypt Message-ID: Date: Thu, 29 Nov 2018 03:48:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.3.0 MIME-Version: 1.0 In-Reply-To: <19112bc8-83a2-2c35-2841-d95087cd1178@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2018-11/txt/msg02407.txt.bz2 On 11/27/18 9:32 PM, Martin Sebor wrote: > The tests for the new __builtin_has_attribute function have been > failing on a number of targets because of a couple of assumptions > that only hold on some. > > First, they expect that it's safe to apply attribute aligned with > a smaller alignment than the target provides when GCC rejects such > arguments.  The tests pass on i86 and elsewhere but fail on > strictly aligned targets like aarch64 or sparc.  After some testing > and thinking I don't think this is helpful -- I believe it's better > to instead silently accept attributes that ask for a less restrictive > alignment than the function ultimately ends up with (see * below). > This is what testing shows Clang does on those targets.  The attached > patch implements this change. > > Second, the tests assume that the priority forms of the constructor > and destructor attributes are universally supported.  That's also > not the case, even though the manual doesn't mention that.  To > avoid these failures the attached patch moves the priority forms > of the attribute constructor and destructor tests into its own > file that's compiled only for init_priority targets. > > Finally, I noticed that attribute aligned accepts zero as > an argument even though it's not a power of two as the manual > documents as a precondition (zero is treated the same as > the attribute without an argument).  A zero argument is likely > to be a mistake, especially when the zero comes from macro > expansion, that users might want to know about.  Clang rejects > a zero with an error but I think a warning is more in line with > established GCC practice, so the patch also implements that. > > Besides x86_64-linux, I tested this change with cross-compilers > for aarch64-linux-elf, powerpc64le-linux, and sparc-solaris2.11. > I added tests for the changed aligned attribute for those targets > To make the gcc.dg/builtin-has-attribute.c test pass with > the cross-compilers I changed from a runtime test into a compile > only one. > > Martin > > PS I'm not happy about duplicating the same test across all those > targets.  It would be much nicer to have a single test somewhere > in dg.exp #include a target-specific header with macros describing > the target-specific parameters. > > [*] See the following discussion for some background: >   https://gcc.gnu.org/ml/gcc/2018-11/msg00127.html > > gcc-88208.diff > > PR testsuite/88208 - new test case c-c++-common/builtin-has-attribute-3.c in r266335 has multiple excess errors > > gcc/ChangeLog: > PR testsuite/88208 > * doc/extend.texi (attribute constructor): Clarify. > > gcc/c/ChangeLog: > > PR testsuite/88208 > * c-decl.c (declspec_add_alignas): Adjust call to check_user_alignment. > > gcc/c-family/ChangeLog: > > PR testsuite/88208 > * c-attribs.c (common_handle_aligned_attribute): Silently avoid setting > alignments to values less than the target requires. > (has_attribute): For attribute aligned consider both the attribute > and the alignment bits. > * c-common.c (c_init_attributes): Optionally issue a warning for > zero alignment. > > gcc/testsuite/ChangeLog: > > PR testsuite/88208 > * gcc.dg/attr-aligned-2.c: New test. > * gcc.dg/builtin-has-attribute.c: Adjust. > * c-c++-common/builtin-has-attribute-2.c: Same. > * c-c++-common/builtin-has-attribute-3.c: Same. > * c-c++-common/builtin-has-attribute-4.c: Same. > * c-c++-common/builtin-has-attribute-5.c: New test. > * gcc.target/aarch64/attr-aligned.c: Same. > * gcc.target/i386/attr-aligned.c: Same. > * gcc.target/powerpc/attr-aligned.c: Same. > * gcc.target/sparc/attr-aligned.c: Same. > > Index: gcc/c/c-decl.c > =================================================================== > --- gcc/c/c-decl.c (revision 266521) > +++ gcc/c/c-decl.c (working copy) > @@ -11061,12 +11061,15 @@ struct c_declspecs * > declspecs_add_alignas (location_t loc, > struct c_declspecs *specs, tree align) > { > - int align_log; > specs->alignas_p = true; > specs->locations[cdw_alignas] = loc; > if (align == error_mark_node) > return specs; > - align_log = check_user_alignment (align, false, true); > + > + /* Only accept the alignment if it's valid and greater than > + the current one. Zere is invalid but by C11 required to be > + silently ignored. */ s/Zere/Zero/ OK with the nit fixed. jeff