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 ESMTPS id CF1473858008 for ; Fri, 16 Feb 2024 12:38:58 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org CF1473858008 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 CF1473858008 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.133.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1708087141; cv=none; b=t/OnKXsn3d6hTANzChTD+FVUCeuh5LohFf96WM9hI0pSl64dZuf4oldOuc9NkpGnzWvZGRZOtjcHsnwxXBHGqPzD1z0vtmuqOfyJPtBI9yuJUWUYV/hrDpf+Lq50ZKDM+tAcugWavJPhnxvE5L2K2lqcLJxp6k7y1PhRowb3aMs= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1708087141; c=relaxed/simple; bh=L7Kp/QrEMeSmG80A4513H5XKAZ+8YOHycM6acwLvjLk=; h=DKIM-Signature:MIME-Version:From:Date:Message-ID:Subject:To; b=apiITOcBfhn8I4DnMLfNyaMkjTh1dbK6YW2F7Be8pyr4kWaV7oQG31OFnHy0Q1he7As6490HcTl/g3CiEVTmGBDIp9YmSs0eQx+Wd87ZY/k/n68dRGyOH3riy7x12xWCmrYlUm5egENWwX2OKzM/uyRPol5iz1PG61PtFEWVz+0= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1708087138; 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: in-reply-to:in-reply-to:references:references; bh=r5he+xbKpoUtHWk7MawqpN2mdnUfiMKxmgx2+q/G97U=; b=Iee86gziV/GJzmcKplYBljd270ZctZAKUVS7rirhU1Q8w3hxbntS2qsm1OW06TyAAWVO7R x54jyJpfMSN6hs8BrIkpoF1VW0XngrCfZF5OUHp2YHdBtDXbq5cGOuXK7bQ+lkrUaIA2pO sC+j/U2Ur0LSqgF/LeX1bTibetVuoKY= Received: from mail-yb1-f197.google.com (mail-yb1-f197.google.com [209.85.219.197]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-182-SyiNQkheOgWBs6g4XvnStg-1; Fri, 16 Feb 2024 07:38:55 -0500 X-MC-Unique: SyiNQkheOgWBs6g4XvnStg-1 Received: by mail-yb1-f197.google.com with SMTP id 3f1490d57ef6-dc6b5d1899eso3952384276.0 for ; Fri, 16 Feb 2024 04:38:55 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1708087134; x=1708691934; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=r5he+xbKpoUtHWk7MawqpN2mdnUfiMKxmgx2+q/G97U=; b=nfNt00C07blQ0ihJg1dZJCpC/WBo3xd+0bD0X6BO2ZzRW/CETmqTIAVFSfF5e8h+nu 2X4m+ixMQSx4dJEgKHlOEpmBS1p+VQnx/HjIWIvTXmHDA4ze5NOda2R5Sy1MWRZFgRzE ldsatYnI5fLG61BPmchCs1slw4BxoGahymHGxDXjHBNlu7CMhWJNIt/ZxTyqUFqcbdYA xXN2hvCzqct2+nEjKciK/3j1m7GoqEXOAMBAPK7Gu9+ZEoWFGMaB/CtmLE4ee4Txix+U CVv2yF8xUVgyPncNslHiPh/8waN7ZljvR335l/UJlYITzz96ORA0fcOO/3q6lcIBZxMI r+XA== X-Forwarded-Encrypted: i=1; AJvYcCUbEx80zEoGFB3Js0aXbfsoMp0Shgw2EBrnhzYboqrXfejSDa96fjB3vo2B42iIyTzKAr6zTVuJfrxwVu9Y4PL1Hsmx/0GVgg== X-Gm-Message-State: AOJu0Yy47ngSoEQHCGr/Oq2MvMDKshtR6dZHSMwTddbh4ZxgsZeike41 1kmfVU22i8feWuNEf7FDEyAvBtwZfE0UV3SlSrpuqUhyTLX8ZubvEtUk1lvGSJNElitLhe8J9SR TYgMh2liVXPn4MZKNexeVtnGu5CIV3Ddd8mermGq+eg4hejuBr0Y/HKHS1BjCyXTO/LXlX8xMtI rIuC5ZakcYaZb0Ko/J6yodjUuWU7hkbQ== X-Received: by 2002:a25:be06:0:b0:dc2:2ae4:d639 with SMTP id h6-20020a25be06000000b00dc22ae4d639mr6207879ybk.20.1708087134559; Fri, 16 Feb 2024 04:38:54 -0800 (PST) X-Google-Smtp-Source: AGHT+IHeKQwGcvwa+yoS87Qm1RH1RkbwWXhohWee6tXQ0ZIa8ZD2yaZFgbe2orr4Q/YI7qDMwIzw81ilz9Hnsv9NN5Y= X-Received: by 2002:a25:be06:0:b0:dc2:2ae4:d639 with SMTP id h6-20020a25be06000000b00dc22ae4d639mr6207867ybk.20.1708087134214; Fri, 16 Feb 2024 04:38:54 -0800 (PST) MIME-Version: 1.0 References: <9f2bdd3a973d98199bb5b322baa575ab2fba8a58.camel@xry111.site> In-Reply-To: From: Jonathan Wakely Date: Fri, 16 Feb 2024 12:38:38 +0000 Message-ID: Subject: Re: [PATCH] libstdc++: atomic: Add missing clear_padding in __atomic_float constructor To: xndcn Cc: Jakub Jelinek , Xi Ruoyao , "H.J. Lu" , GCC Patches , libstdc++@gcc.gnu.org X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-6.5 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_H4,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=unavailable 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 Fri, 2 Feb 2024 at 16:52, xndcn wrote: > > Thank you for your careful review! > > > But we don't need a new one if it's going to be used in exactly one test and if the new option does the same thing for all targets that run the test. > Got it, thanks. Now add option "-latomic" directly, but it still rely > on the trick "[atomic_link_flags [get_multilibs]]" > > > No, because the patch is supposed to prevent the infinite loop, and so there's no need to stop it looping after 10s. It won't loop at all. > Thanks, deleted. > > > We only need to clear padding for long double, not float and double, right? > Yes, actually there is a check "if constexpr > (__atomic_impl::__maybe_has_padding<_Fp>())". > But "__atomic_impl::__clear_padding(_M_fp); " is indeed simply, so fixed here. > > > Why can't we run this on all targets? > Got it, now target option deleted. > > > There's no reason to use __builtin_memset here, just include and use std::memcpy. > Thanks, fixed. > > > It definitely does have padding, just say "long double has padding bits on x86" > Thanks, fixed. > > So here comes the latest patch: Thanks. I've applied the patch to my tree, but the new test fails pretty reliably. The infinite loop in std::atomic::fetch_add is fixed by clearing padding in the constructor, but the test fails on the compare_exchange_weak or compare_exchange_strong lines here: > + as.fetch_add(t); > + long double s = f + t; > + t = as.load(); > + VERIFY(s == t); // padding ignored on float comparing > + fill_padding(s); > + VERIFY(as.compare_exchange_weak(s, f)); // padding cleared on cmpexchg > + fill_padding(f); > + VERIFY(as.compare_exchange_strong(f, t)); // padding cleared on cmpexchg > I think the problem is here in __atomic_impl::__compare_exchange: if (__atomic_compare_exchange(__pval, __pexp, __pi, __is_weak, int(__s), int(__f))) return true; Even though padding in *__pexp and *__pi has been cleared, the value of *__pval after a successful __atomic_compare_exchange has non-zero padding. That means that the next compare_exchange will fail, because we assume that the stored value always has zeroed padding bits. Here's a gdb session showing that __atomic_compare_exchange stores a value with non-zero padding: Breakpoint 2, test01 () at compare_exchange_padding.cc:43 43 long double s2 = s; (gdb) n 44 fill_padding(s2); (gdb) 45 while (!as.compare_exchange_weak(s2, f)) // padding cleared on compexchg (gdb) p/x as._M_fp $11 = 0x40008000000000000000 (gdb) step std::__atomic_float::compare_exchange_weak (this=0x7fffffffd8c0, __expected=@0x7fffffffd8a0: 2, __desired=0.5, __order=std::memory_order::seq_cst) at /home/jwakely/gcc/14/include/c++/14.0.1/bits/atomic_base.h:1387 1387 return compare_exchange_weak(__expected, __desired, __order, (gdb) step std::__atomic_float::compare_exchange_weak (this=0x7fffffffd8c0, __expected=@0x7fffffffd8a0: 2, __desired=0.5, __success=std::memory_order::seq_cst, __failure=std::memory_order::seq_cst) at /home/jwakely/gcc/14/include/c++/14.0.1/bits/atomic_base.h:1347 1347 return __atomic_impl::compare_exchange_weak(&_M_fp, (gdb) step std::__atomic_impl::compare_exchange_weak (__check_padding=false, __failure=std::memory_order::seq_cst, __success=std::memory_order::seq_cst, __desired=0.5, __expected=@0x7fffffffd8a0: 2, __ptr=0x7fffffffd8c0) at /home/jwakely/gcc/14/include/c++/14.0.1/bits/atomic_base.h:1123 1123 return __atomic_impl::__compare_exchange<_AtomicRef>( (gdb) std::__atomic_impl::__compare_exchange (__f=std::memory_order::seq_cst, __s=std::memory_order::seq_cst, __is_weak=true, __i=, __e=@0x7fffffffd8a0: 2, __val=@0x7fffffffd8c0: 2) at /home/jwakely/gcc/14/include/c++/14.0.1/bits/atomic_base.h:994 994 __glibcxx_assert(__is_valid_cmpexch_failure_order(__f)); (gdb) n 997 _Tp* const __pval = std::__addressof(__val); (gdb) 1008 _Vp* const __pi = __atomic_impl::__clear_padding(__i); (gdb) 1010 _Vp __exp = __e; (gdb) 1012 _Vp* const __pexp = __atomic_impl::__clear_padding(__exp); (gdb) 1016 if (__atomic_compare_exchange(__pval, __pexp, __pi, (gdb) p/x *__pval $12 = 0x40008000000000000000 (gdb) p/x *__pexp $13 = 0x40008000000000000000 (gdb) p/x *__pi $14 = 0x3ffe8000000000000000 (gdb) n 1018 return true; (gdb) p/x *__pval $15 = 0x7ffff7bf3ffe8000000000000000 (gdb) We stored *__pi which has zero padding, but the result in *__pval has non-zero padding. This doesn't seem to be gdb being misleading by loading *__pval into a FP register which doesn't preserve the zero padding, because if I do this then it fails: as.fetch_add(t); VERIFY(as.load() == s); __builtin_clear_padding(&s); VERIFY( std::memcmp(&s, &as, sizeof(s)) == 0 ); So the value stored by fetch_add (which uses compare_exchange_weak in a loop) really doesn't have zero padding bits. I'm not sure what's going on here yet, maybe __atomic_compare_exchange recognizes that the pointers are long double* and so only copies the non-padding bits into the value? I think I'll push the fix to the __atomic_float constructor, but with a simplified test case that doesn't include the compare_exchange_{weak,strong} calls, until I figure out how to fix it.