From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 84505 invoked by alias); 5 Dec 2018 13:50:46 -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 84496 invoked by uid 89); 5 Dec 2018 13:50:45 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy= 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; Wed, 05 Dec 2018 13:50:43 +0000 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id B95D6307D91B; Wed, 5 Dec 2018 13:50:42 +0000 (UTC) Received: from localhost.localdomain (ovpn-112-27.rdu2.redhat.com [10.10.112.27]) by smtp.corp.redhat.com (Postfix) with ESMTP id 57283105705C; Wed, 5 Dec 2018 13:50:41 +0000 (UTC) Subject: Re: [PATCH] be more permissive about function alignments (PR 88208) To: Rainer Orth , Martin Sebor Cc: Gcc Patch List , Eric Botcazou References: <19112bc8-83a2-2c35-2841-d95087cd1178@gmail.com> From: Jeff Law Openpgp: preference=signencrypt Message-ID: <5a0c476f-490a-d917-6103-305f2f7bfc91@redhat.com> Date: Wed, 05 Dec 2018 13:50: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: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2018-12/txt/msg00277.txt.bz2 On 12/5/18 6:09 AM, Rainer Orth wrote: > Hi Martin, > >> 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. > > why so complicated? Just have a single attr-aligned.c test, restricted > to the target cpus it supports via dg directives and with > MINALIGN/MAXALIGN definitions controlled by appropriate target macros? > > Besides, the new gcc.target/sparc/attr-aligned.c test currently FAILs on > 64-bit sparc: > > FAIL: gcc.target/sparc/attr-aligned.c (test for excess errors) > > Excess errors: > /vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.target/sparc/attr-aligned.c:29:1: error: static assertion failed: "alignof (f_) == MAXALIGN" > > alignof (f_) is 16 for sparcv9. The following patch fixes this, tested > on sparc-sun-solaris2.11 with -m32 and -m64. > > Ok for mainline? OK jeff