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 [63.128.21.124]) by sourceware.org (Postfix) with ESMTP id D8217393C86C for ; Fri, 13 Nov 2020 21:16:54 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org D8217393C86C Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-328-9gMuDOKOPYanGGynLuTvkg-1; Fri, 13 Nov 2020 16:16:52 -0500 X-MC-Unique: 9gMuDOKOPYanGGynLuTvkg-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 37EAD6415F; Fri, 13 Nov 2020 21:16:51 +0000 (UTC) Received: from localhost (unknown [10.33.36.92]) by smtp.corp.redhat.com (Postfix) with ESMTP id D2BBD27BDC; Fri, 13 Nov 2020 21:16:50 +0000 (UTC) Date: Fri, 13 Nov 2020 21:16:50 +0000 From: Jonathan Wakely To: Mike Crowe Cc: libstdc++@gcc.gnu.org, gcc-patches@gcc.gnu.org Subject: Re: [committed] libstdc++: Optimise std::future::wait_for and fix futex polling Message-ID: <20201113211650.GK503596@redhat.com> References: <20201112234940.GA475138@redhat.com> <20201113110218.GG503596@redhat.com> <20201113172522.GI503596@redhat.com> <20201113202934.GA23583@mcrowe.com> <20201113211254.GJ503596@redhat.com> MIME-Version: 1.0 In-Reply-To: <20201113211254.GJ503596@redhat.com> X-Clacks-Overhead: GNU Terry Pratchett X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline X-Spam-Status: No, score=-8.1 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_H5, 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: libstdc++@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libstdc++ mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 13 Nov 2020 21:16:56 -0000 On 13/11/20 21:12 +0000, Jonathan Wakely wrote: >On 13/11/20 20:29 +0000, Mike Crowe via Libstdc++ wrote: >>On Friday 13 November 2020 at 17:25:22 +0000, Jonathan Wakely wrote: >>>+ // Return the relative duration from (now_s + now_ns) to (abs_s + abs_ns) >>>+ // as a timespec. >>>+ struct timespec >>>+ relative_timespec(chrono::seconds abs_s, chrono::nanoseconds abs_ns, >>>+ time_t now_s, long now_ns) >>>+ { >>>+ struct timespec rt; >>>+ >>>+ // Did we already time out? >>>+ if (now_s > abs_s.count()) >>>+ { >>>+ rt.tv_sec = -1; >>>+ return rt; >>>+ } >>>+ >>>+ auto rel_s = abs_s.count() - now_s; >>>+ >>>+ // Avoid overflows >>>+ if (rel_s > __gnu_cxx::__int_traits::__max) >>>+ rel_s = __gnu_cxx::__int_traits::__max; >>>+ else if (rel_s < __gnu_cxx::__int_traits::__min) >>>+ rel_s = __gnu_cxx::__int_traits::__min; >> >>I may be missing something, but if the line above executes... >> >>>+ >>>+ // Convert the absolute timeout value to a relative timeout >>>+ rt.tv_sec = rel_s; >>>+ rt.tv_nsec = abs_ns.count() - now_ns; >>>+ if (rt.tv_nsec < 0) >>>+ { >>>+ rt.tv_nsec += 1000000000; >>>+ --rt.tv_sec; >> >>...and so does this line above, then I think that we'll end up >>underflowing. (Presumably rt.tv_sec will wrap round to being some time in >>2038 on most 32-bit targets.) > >Ugh. > >>I'm currently trying to persuade myself that this can actually happen and >>if so work out how to come up with a test case for it. > >Maybe something like: > >auto d = chrono::floor(system_clock::now().time_since_epoch() - seconds(INT_MAX + 2LL)); >fut.wait_until(system_clock::time_point(d)); > >This will create a sys_time with a value that is slightly more than >INT_MAX seconds before the current time, with a zero nanoseconds >component. The difference between the gettimeofday result and this >time will be slightly more negative than INT_MIN and so will overflow >a 32-bit time_t, causing the code to use __int_traits::__min. >As long as the gettimeofday call doesn't happen to also have a zero >nanoseconds component, the difference of the nanoseconds values will >be negative, and we will decrement the time_t value. > >>I don't believe that this part changed in your later patch. > >Right. Thanks for catching this. > >The attached patch should fix it. There's no point clamping very >negative values to the minimum time_t value, since any relative >timeout less than zero has already passed. So we can just use -1 >there (and not bother with the tv_nsec field at all): > > else if (rel_s <= 0) [[unlikely]] > { > rt.tv_sec = -1; > } Gah, no, this has to be < 0 not <= 0 otherwise we treat 0.1s as -1.1 And we should probably lose the [[unlikely]] there, since it's quite feasible that an absolute time in the recent past gets used.