From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 88454 invoked by alias); 5 Dec 2018 16:05:59 -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 88437 invoked by uid 89); 5 Dec 2018 16:05:58 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy=H*p:D*gmail.com, hardcoding X-HELO: mail-qt1-f195.google.com Received: from mail-qt1-f195.google.com (HELO mail-qt1-f195.google.com) (209.85.160.195) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 05 Dec 2018 16:05:57 +0000 Received: by mail-qt1-f195.google.com with SMTP id v11so22829438qtc.2 for ; Wed, 05 Dec 2018 08:05:56 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=uBxnVJ8Vu2G0GZaoFrIPGQ4UK1IGgE6thYvkjZQnlCo=; b=oaaPyOfXalT+hNoTXRtG8nkj7XLY7/bEQZnpStHClcqpdaZp6gU+jPwVT37lPT5cHz dlMC19ZOyXPnWJejEmSEvBQSzSTUBSk3f5TYWuUxXVxUEMsRcQFYBTBvpigsg5IZRvkF QKjzlkjmOSr5pgNt/K8NX3B6+FUOa72MXAPNovykbv5y9sGEjvm3j5JhrARZhW2Tc9Tg r/fDqa+V0YZGVU9OazohOyaR8x4I9Las3M8vGXS+q0sz6t2OvT1E7vwwCUH4i9utAdpY u2D+qOqhPG9am/baO7cGmM8PJwZ46v9KwIVNNrKXfcV7ujq1x6pXwvxIiV3x8P3FHY9S Ys9g== Return-Path: Received: from localhost.localdomain (97-118-99-160.hlrn.qwest.net. [97.118.99.160]) by smtp.gmail.com with ESMTPSA id p3sm13861609qkp.48.2018.12.05.08.05.54 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 05 Dec 2018 08:05:54 -0800 (PST) Subject: Re: [PATCH] be more permissive about function alignments (PR 88208) To: Rainer Orth Cc: Gcc Patch List , Eric Botcazou References: <19112bc8-83a2-2c35-2841-d95087cd1178@gmail.com> From: Martin Sebor Message-ID: <142d4627-9422-2c4c-1443-5db2fbc389bc@gmail.com> Date: Wed, 05 Dec 2018 16:05:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.3.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2018-12/txt/msg00290.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? I have done that in the past(*) but hardcoding target-specific assumptions into a general test didn't feel right either given the design of the test suite (separate target tests). [*] see the tangle of #ifdefs in gcc/testsuite/gcc.dg/attr-aligned.c > > 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? Thanks! Martin