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.129.124]) by sourceware.org (Postfix) with ESMTPS id 679653858D20 for ; Tue, 20 Feb 2024 00:12:17 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 679653858D20 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 679653858D20 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.129.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1708387940; cv=none; b=HqAXF8BGfhNb8LphmnzZEnthMrstIQ1nA5+ombIXVXSDm3WflHiiWGxkgn+LCPTEcTajUv2go2wSeRRNgQJYh5kjOtaUA6HZaSvnJE46YNQauEjpYtEclBO83tW/MoNNlEOdokloC3vJoyOoBy960+Hp08io0wWCY1rDA+Qapn4= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1708387940; c=relaxed/simple; bh=lv7bmpJZJaeLfGnOcvrmAlZ9AgEKKrFhJYNBvaE3dIg=; h=DKIM-Signature:Message-ID:Date:MIME-Version:Subject:To:From; b=v3woTs7kB7Ef2+z93k1ddkxTARyfAsZ8fedsyNvJ/uLPF5ANEum9o8QiRLdiDkt3XJ6Nc+UuiRjRdl6FyBKRlA8JfSEFFIUOzg7a2Dj+sAMxIXj5r7IniUArTca9VRMjyQfp43IM9bBZTQwU6Fdxodj5ZsPucUWSXOm3hG2ayCo= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1708387937; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=vV8etGmvnNxGcxMUyH2Z2nZ/g1c7gmFPtEXH3myijYc=; b=CLSGnnq4dHrLmWPbAD1njTfq7uxqml8ROE+pWU/SFeR7zgbV1ideZgN4kxeSM8V6MVU3Tn S/2f5OvdXe4fMJGoMn6O5N+yBgxa6hcTqy5J4OFrNDEgiodYaXnso05UZ5uNVpQ6pP2wR+ vhaekQTgsFYGx9Y6spV8D98oZOC07qQ= Received: from mail-wm1-f71.google.com (mail-wm1-f71.google.com [209.85.128.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-269-cRE2VaEJPTSfPku5g9ez3Q-1; Mon, 19 Feb 2024 19:12:15 -0500 X-MC-Unique: cRE2VaEJPTSfPku5g9ez3Q-1 Received: by mail-wm1-f71.google.com with SMTP id 5b1f17b1804b1-412557adc00so15200215e9.2 for ; Mon, 19 Feb 2024 16:12:15 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1708387934; x=1708992734; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=vV8etGmvnNxGcxMUyH2Z2nZ/g1c7gmFPtEXH3myijYc=; b=vxb2IfW1zdVhgM+eAdcfCXBhKxz4ZtNFuna1tco5RZpLw2wqSIgpedFNY26PH5IMq5 8A9+ssdMAsYv3gK39AJ/eD3wEBu8UqsEtUDfUMbvnD5j+uRLsRGDVdrTERJ4sw4KNhms f+kFm+LCbzQp7P6At4z1ASPFDCxWK2ECDnnIujfLBYI77obW1Oq7sJsPMnJt7gbBmBXI AyL9vGeA/YltIlblm+YZ83Qy0Rs4qga/k3Hohm/MocQPc0MRG05dMjhSDpUbKdwmfYUQ rzhobiMY76FxwP6Mk+mky72VPcPMp/Fq9cuBGegGPJ5kcxMxpGv+ancSKf0BIgbRi37v XqFw== X-Gm-Message-State: AOJu0YwRgsAWyDzUwl+KNu9FUeNS7vRLGesMf598gmZHaa9RrEyRqbwE 2ujS/QrQlfAl8OQzI+jZ/XZZVLPh/GotOL2V2xgHlbFDYTvEFK8nrjou3mY+mpy/7hgjSmqRS5E 4l9w0R9OrilM0guUy51Y6vO0CGUfu7x+ecnJmpSl41sCekSD5O0rVA6U= X-Received: by 2002:a05:600c:190c:b0:411:cb30:8e00 with SMTP id j12-20020a05600c190c00b00411cb308e00mr10232397wmq.3.1708387934008; Mon, 19 Feb 2024 16:12:14 -0800 (PST) X-Google-Smtp-Source: AGHT+IGbLBmEjBJaH4V0fqfQkc5OZ+BSE8TO3zRwnLwvO5Q31rtpJXtUE5RibRlzR/5f0WZ7kwmvhA== X-Received: by 2002:a05:600c:190c:b0:411:cb30:8e00 with SMTP id j12-20020a05600c190c00b00411cb308e00mr10232383wmq.3.1708387933554; Mon, 19 Feb 2024 16:12:13 -0800 (PST) Received: from ?IPV6:2a02:6b63:24c5:0:c6f7:d0f4:2019:ee02? ([2a02:6b63:24c5:0:c6f7:d0f4:2019:ee02]) by smtp.gmail.com with ESMTPSA id 6-20020a05600c22c600b004120537210esm12697380wmg.46.2024.02.19.16.12.12 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 19 Feb 2024 16:12:12 -0800 (PST) Message-ID: <5988812a-3f21-4ef4-9205-fe267c356d58@redhat.com> Date: Tue, 20 Feb 2024 00:12:11 +0000 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] c-family, c++: Fix up handling of types which may have padding in __atomic_{compare_}exchange To: Jakub Jelinek Cc: gcc-patches@gcc.gnu.org, Jonathan Wakely , Richard Biener References: <9f2bdd3a973d98199bb5b322baa575ab2fba8a58.camel@xry111.site> From: Jason Merrill In-Reply-To: X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-6.2 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 2/19/24 02:55, Jakub Jelinek wrote: > On Fri, Feb 16, 2024 at 01:51:54PM +0000, Jonathan Wakely wrote: >> Ah, although __atomic_compare_exchange only takes pointers, the >> compiler replaces that with a call to __atomic_compare_exchange_n >> which takes the newval by value, which presumably uses an 80-bit FP >> register and so the padding bits become indeterminate again. > > The problem is that __atomic_{,compare_}exchange lowering if it has > a supported atomic 1/2/4/8/16 size emits code like: > _3 = *p2; > _4 = VIEW_CONVERT_EXPR (_3); Hmm, I find that surprising; I thought of VIEW_CONVERT_EXPR as working on lvalues, not rvalues, based on the documentation describing it as roughly *(type2 *)&X. Now I see that gimplify_expr does what you describe, and I wonder what the advantage of that is. That seems to go back to richi's r206420 for PR59471. r270579 for PR limited it to cases where the caller wants an rvalue; perhaps it should also be avoided when the operand is an INDIRECT_REF? > so if long double or some small struct etc. has some carefully filled > padding bits, those bits can be lost on the assignment. The library call > for __atomic_{,compare_}exchange would actually work because it woiuld > load the value from memory using integral type or memcpy. > E.g. on > void > foo (long double *a, long double *b, long double *c) > { > __atomic_compare_exchange (a, b, c, false, __ATOMIC_RELAXED, __ATOMIC_RELAXED); > } > we end up with -O0 with: > fldt (%rax) > fstpt -48(%rbp) > movq -48(%rbp), %rax > movq -40(%rbp), %rdx > i.e. load *c from memory into 387 register, store it back to uninitialized > stack slot (the padding bits are now random in there) and then load a > __uint128_t (pair of GPR regs). The problem is that we first load it using > whatever type the pointer points to and then VIEW_CONVERT_EXPR that value: > p2 = build_indirect_ref (loc, p2, RO_UNARY_STAR); > p2 = build1 (VIEW_CONVERT_EXPR, I_type, p2); > The following patch fixes that by creating a MEM_REF instead, with the > I_type type, but with the original pointer type on the second argument for > aliasing purposes, so we actually preserve the padding bits that way. > I've done that for types which may have padding and also for > non-integral/pointer types, because I fear even on floating point types > like double or float which don't have padding bits the copying through > floating point could misbehave in presence of sNaNs or unsupported bit > combinations. > With this patch instead of the above assembly we emit > movq 8(%rax), %rdx > movq (%rax), %rax > I had to add support for MEM_REF in pt.cc, though with the assumption > that it has been already originally created with non-dependent > types/operands (which is the case here for the __atomic*exchange lowering). > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? > > 2024-02-19 Jakub Jelinek > > gcc/c-family/ > * c-common.cc (resolve_overloaded_atomic_exchange): For > non-integral/pointer types or types which may need padding > instead of setting p1 to VIEW_CONVERT_EXPR (*p1), set it to > MEM_REF with p1 and (typeof (p1)) 0 operands and I_type type. > (resolve_overloaded_atomic_compare_exchange): Similarly for p2. > gcc/cp/ > * pt.cc (tsubst_expr): Handle MEM_REF. > gcc/testsuite/ > * g++.dg/ext/atomic-5.C: New test. > > --- gcc/c-family/c-common.cc.jj 2024-02-16 17:33:43.995739790 +0100 > +++ gcc/c-family/c-common.cc 2024-02-17 11:11:34.029474214 +0100 > @@ -7794,8 +7794,23 @@ resolve_overloaded_atomic_exchange (loca > p0 = build1 (VIEW_CONVERT_EXPR, I_type_ptr, p0); > (*params)[0] = p0; > /* Convert new value to required type, and dereference it. */ > - p1 = build_indirect_ref (loc, p1, RO_UNARY_STAR); > - p1 = build1 (VIEW_CONVERT_EXPR, I_type, p1); > + if ((!INTEGRAL_TYPE_P (TREE_TYPE (TREE_TYPE (p1))) > + && !POINTER_TYPE_P (TREE_TYPE (TREE_TYPE (p1)))) > + || clear_padding_type_may_have_padding_p (TREE_TYPE (TREE_TYPE (p1)))) > + { > + /* If *p1 type can have padding or may involve floating point which > + could e.g. be promoted to wider precision and demoted afterwards, > + state of padding bits might not be preserved. */ > + build_indirect_ref (loc, p1, RO_UNARY_STAR); > + p1 = build2_loc (loc, MEM_REF, I_type, > + build1 (VIEW_CONVERT_EXPR, I_type_ptr, p1), > + build_zero_cst (TREE_TYPE (p1))); > + } > + else > + { > + p1 = build_indirect_ref (loc, p1, RO_UNARY_STAR); > + p1 = build1 (VIEW_CONVERT_EXPR, I_type, p1); > + } > (*params)[1] = p1; > > /* Move memory model to the 3rd position, and end param list. */ > @@ -7874,8 +7889,23 @@ resolve_overloaded_atomic_compare_exchan > (*params)[1] = p1; > > /* Convert desired value to required type, and dereference it. */ > - p2 = build_indirect_ref (loc, p2, RO_UNARY_STAR); > - p2 = build1 (VIEW_CONVERT_EXPR, I_type, p2); > + if ((!INTEGRAL_TYPE_P (TREE_TYPE (TREE_TYPE (p2))) > + && !POINTER_TYPE_P (TREE_TYPE (TREE_TYPE (p2)))) > + || clear_padding_type_may_have_padding_p (TREE_TYPE (TREE_TYPE (p2)))) > + { > + /* If *p2 type can have padding or may involve floating point which > + could e.g. be promoted to wider precision and demoted afterwards, > + state of padding bits might not be preserved. */ > + build_indirect_ref (loc, p2, RO_UNARY_STAR); > + p2 = build2_loc (loc, MEM_REF, I_type, > + build1 (VIEW_CONVERT_EXPR, I_type_ptr, p2), > + build_zero_cst (TREE_TYPE (p2))); > + } > + else > + { > + p2 = build_indirect_ref (loc, p2, RO_UNARY_STAR); > + p2 = build1 (VIEW_CONVERT_EXPR, I_type, p2); > + } > (*params)[2] = p2; > > /* The rest of the parameters are fine. NULL means no special return value > --- gcc/cp/pt.cc.jj 2024-02-14 14:26:19.695811596 +0100 > +++ gcc/cp/pt.cc 2024-02-17 11:05:47.988237152 +0100 > @@ -20088,6 +20088,14 @@ tsubst_expr (tree t, tree args, tsubst_f > RETURN (r); > } > > + case MEM_REF: > + { > + tree op0 = RECUR (TREE_OPERAND (t, 0)); > + tree op1 = RECUR (TREE_OPERAND (t, 0)); > + tree new_type = tsubst (TREE_TYPE (t), args, complain, in_decl); > + RETURN (build2_loc (EXPR_LOCATION (t), MEM_REF, new_type, op0, op1)); > + } > + > case NOP_EXPR: > { > tree type = tsubst (TREE_TYPE (t), args, complain, in_decl); > --- gcc/testsuite/g++.dg/ext/atomic-5.C.jj 2024-02-17 11:13:37.824770288 +0100 > +++ gcc/testsuite/g++.dg/ext/atomic-5.C 2024-02-17 11:14:54.077720732 +0100 > @@ -0,0 +1,42 @@ > +// { dg-do compile { target c++14 } } > + > +template > +void > +foo (long double *ptr, long double *val, long double *ret) > +{ > + __atomic_exchange (ptr, val, ret, __ATOMIC_RELAXED); > +} > + > +template > +bool > +bar (long double *ptr, long double *exp, long double *des) > +{ > + return __atomic_compare_exchange (ptr, exp, des, false, > + __ATOMIC_RELAXED, __ATOMIC_RELAXED); > +} > + > +bool > +baz (long double *p, long double *q, long double *r) > +{ > + foo<0> (p, q, r); > + foo<1> (p + 1, q + 1, r + 1); > + return bar<0> (p + 2, q + 2, r + 2) || bar<1> (p + 3, q + 3, r + 3); > +} > + > +constexpr int > +qux (long double *ptr, long double *val, long double *ret) > +{ > + __atomic_exchange (ptr, val, ret, __ATOMIC_RELAXED); > + return 0; > +} > + > +constexpr bool > +corge (long double *ptr, long double *exp, long double *des) > +{ > + return __atomic_compare_exchange (ptr, exp, des, false, > + __ATOMIC_RELAXED, __ATOMIC_RELAXED); > +} > + > +long double a[6]; > +const int b = qux (a, a + 1, a + 2); > +const bool c = corge (a + 3, a + 4, a + 5); > > Jakub >