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 [170.10.133.124]) by sourceware.org (Postfix) with ESMTP id 305753857C56 for ; Fri, 21 May 2021 19:13:22 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 305753857C56 Received: from mail-qv1-f71.google.com (mail-qv1-f71.google.com [209.85.219.71]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-292-FOZJ8LfnN22pOlKwgwXA7Q-1; Fri, 21 May 2021 15:13:20 -0400 X-MC-Unique: FOZJ8LfnN22pOlKwgwXA7Q-1 Received: by mail-qv1-f71.google.com with SMTP id w14-20020a056214012eb02901f3a4388530so9664814qvs.17 for ; Fri, 21 May 2021 12:13:20 -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:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=2H5TQAPbqZ2MkULbaqULFJ8SEvvnWaXX7DcngxI2lrE=; b=oasQI3t3b5gu/FBpj3OuJC2Y2gd1tDlzl2qBOEEeIzw8nMFJRXz+LW9wAhg5ry8hrL 2e5jGodPlXUbpubFNoJXL6x6c/9XPPWo6Jw1vKH5SNkE85l9aXIl3N+HjJJRDGJ0yCgo f7wLeP32Hpq/Q8XRkzaZZbdN2tt2rAKfAZjx4PGCAL+M/JzbXRg/lRaCFUgRHO77iGaL 7ytK/tQ9J9TLlgy+G1+PI/A9Xzh1VkyeZ1C4BZdJyKxOz0yf+W502kC7qXzcnNtboBpQ NtaGycyYdpBEuXJBGuUZiQUBOWcUO87bdL+J8VCX/AzclqmkJJCJZD12PKk2CHxISGKO BJ8A== X-Gm-Message-State: AOAM531/4nHl5YjpN40TSflKunFqa4VwyOkaRg8zNSIj44mJEWKzIXlW A+2XSku38fg13kzx7LVyrQ36mY4Cu73uU9b6lcr4aPuIYIAzBnjAry8GVUkFCPhoY/9uV1tpxv8 r5jDFCfTgiFnduWrGVqjtD9SZ48R/EaTL7GqMoMMLYVu9fbmpGRQou76loxzZAUbtfQ== X-Received: by 2002:a0c:c709:: with SMTP id w9mr14841079qvi.37.1621624399673; Fri, 21 May 2021 12:13:19 -0700 (PDT) X-Google-Smtp-Source: ABdhPJy+oSI4/zGK7KrVrQP5OMondfms2R8exiJIREOG5BRpIb1m1OfZOe3npPBcUVwJBCq3h0P4og== X-Received: by 2002:a0c:c709:: with SMTP id w9mr14841035qvi.37.1621624399274; Fri, 21 May 2021 12:13:19 -0700 (PDT) Received: from [192.168.1.148] ([130.44.159.43]) by smtp.gmail.com with ESMTPSA id f21sm4750705qtq.1.2021.05.21.12.13.18 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 21 May 2021 12:13:18 -0700 (PDT) Subject: Re: [PATCH 1/2] c-family: Copy DECL_USER_ALIGN even if DECL_ALIGN is similar. To: Martin Sebor , Robin Dapp , GCC Patches References: From: Jason Merrill Message-ID: Date: Fri, 21 May 2021 15:13:18 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.9.0 MIME-Version: 1.0 In-Reply-To: X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-12.0 required=5.0 tests=BAYES_00, BODY_8BITS, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, 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: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 21 May 2021 19:13:23 -0000 On 5/19/21 6:03 PM, Martin Sebor via Gcc-patches wrote: > On 5/3/21 8:53 AM, Robin Dapp via Gcc-patches wrote: >> Hi, >> >> on s390 a warning test fails: >> >> inline int ATTR ((cold, aligned (8))) >> finline_hot_noret_align (int); >> >> inline int ATTR ((warn_unused_result)) >> finline_hot_noret_align (int); >> >> inline int ATTR ((aligned (4))) >>    finline_hot_noret_align (int);  /* { dg-warning "ignoring attribute >> .aligned \\(4\\). because it conflicts with attribute .aligned \\(8\\)." >> >> This test actually uncovered two problems.  First, on s390 the default >> function alignment is 8 bytes.  When the second decl above is merged >> with the first one, DECL_USER_ALIGN is only copied if DECL_ALIGN (old) >> > DECL_ALIGN (new).  Subsequently, when merging the third decl, no >> warning is emitted since DECL_USER_ALIGN is unset. >> >> This patch also copies DECL_USER_ALIGN if DECL_ALIGN (old) == >> DECL_ALIGN (new) && DECL_USER_ALIGN (olddecl) != DECL_USER_ALIGN >> (newdecl)). >> >> >> Then, while going through the related files I also noticed that we >> emit a wrong warning for: >> >> inline int ATTR ((aligned (32))) >> finline_align (int); >> >> inline int ATTR ((aligned (4))) >>    finline_align (int);  /* { dg-warning "ignoring attribute .aligned >> \\(4\\). because it conflicts with attribute .aligned \\(32\\)." "" } */ >> >> What we emit is >> >> warning: ignoring attribute ‘aligned (4)’ because it conflicts with >> attribute ‘aligned (8)’ [-Wattributes]. >> >> This is due to the short circuit evaluation in c-attribs.c: >> >>             && ((curalign = DECL_ALIGN (decl)) > bitalign >>                 || ((lastalign = DECL_ALIGN (last_decl)) > bitalign))) >> >> where lastalign is only initialized when curalign > bitalign.  On s390 >> this is not the case and lastalign is used zero-initialized in the >> following code. >> >> On top, the following condition >>    else if (!warn_if_not_aligned_p >>            && TREE_CODE (decl) == FUNCTION_DECL >>            && DECL_ALIGN (decl) > bitalign) >> >> seems to be fully covered by >>      else if (TREE_CODE (decl) == FUNCTION_DECL >>         && ((curalign = DECL_ALIGN (decl)) > bitalign >>             || ((lastalign = DECL_ALIGN (last_decl)) > bitalign))) >> >> and so is essentially dead. I therefore removed it as there does not >> seem to be a test anywhere for the error message ( "alignment for %q+D >> was previously specified as %d and may not be decreased") either. > > The removal of the dead code looks good to me.  The change to > "re-init lastalign" doesn't seem right.  When it's zero it means > the conflict is between two attributes on the same declaration, > in which case the note shouldn't be printed (it would just point > to the same location as the warning). Agreed. > diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c > index c1f652d1dc9..d132b6fd3b6 100644 > --- a/gcc/c-family/c-attribs.c > +++ b/gcc/c-family/c-attribs.c > @@ -2317,6 +2317,10 @@ common_handle_aligned_attribute (tree *node, tree > name, tree args, int flags, >        && ((curalign = DECL_ALIGN (decl)) > bitalign >            || ((lastalign = DECL_ALIGN (last_decl)) > bitalign))) >     { > +      /* Re-init lastalign in case we short-circuit the condition, > +     i.e.  curalign > bitalign.  */ > +      lastalign = DECL_ALIGN (last_decl); > + >       /* Either a prior attribute on the same declaration or one >      on a prior declaration of the same function specifies >      stricter alignment than this attribute.  */ > > For the C/C++ FE changes: > > diff --git a/gcc/c/c-decl.c b/gcc/c/c-decl.c > index 3ea4708c507..2ea5051e9cd 100644 > --- a/gcc/c/c-decl.c > +++ b/gcc/c/c-decl.c > @@ -2620,6 +2620,9 @@ merge_decls (tree newdecl, tree olddecl, tree > newtype, tree oldtype) >       SET_DECL_ALIGN (newdecl, DECL_ALIGN (olddecl)); >       DECL_USER_ALIGN (newdecl) |= DECL_USER_ALIGN (olddecl); >     } > +      else if (DECL_ALIGN (olddecl) == DECL_ALIGN (newdecl) > +           && DECL_USER_ALIGN (olddecl) != DECL_USER_ALIGN (newdecl)) > +    DECL_USER_ALIGN (newdecl) |= DECL_USER_ALIGN (olddecl); > > This should be the same as > >  DECL_USER_ALIGN (newdecl) = 1; > > so I would suggest to use that for clarity. I'm not sure that's clearer, though it certainly is equivalent. I'm happy either way. The braces around this statement in the parallel C++ change are unnecessary. I'm a little nervous about the front-end changes given that we were specifically not setting *_USER_ALIGN in this case before, but I'm not aware of any rationale for that, so I'm content to make the change and see if anything breaks. Jason