From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 68163 invoked by alias); 5 Aug 2016 12:48:46 -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 68080 invoked by uid 89); 5 Aug 2016 12:48:33 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,SPF_PASS autolearn=ham version=3.3.2 spammy=Moreover, among X-HELO: mx2.suse.de Received: from mx2.suse.de (HELO mx2.suse.de) (195.135.220.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (CAMELLIA256-SHA encrypted) ESMTPS; Fri, 05 Aug 2016 12:48:32 +0000 Received: from relay1.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id A722FAB1E; Fri, 5 Aug 2016 12:48:28 +0000 (UTC) Subject: Re: [PATCH 1/4] Cherry-pick fprofile-generate-atomic from google/gcc-4_9 branch To: Nathan Sidwell , gcc-patches@gcc.gnu.org References: <53c4f874443d63de99393a9816041ba75f1d605e.1470041316.git.mliska@suse.cz> <95628fa2-8719-bb82-d1ab-ca8d46355020@acm.org> <0ec1ad3f-0fc7-507e-878c-907adae1c011@suse.cz> <0e248978-3717-1d78-b3b1-9fc60ded5c8e@suse.cz> <882f0f98-9b33-a764-833b-ca61796f3143@acm.org> <87f2bc4f-c4df-eadd-aec6-a937ed0ccaba@acm.org> <1253ac69-3301-f185-e43a-a34cadf8f51e@suse.cz> <67fda6d2-9b3e-a0d1-effc-34e1115030b2@acm.org> Cc: jh@suse.cz From: =?UTF-8?Q?Martin_Li=c5=a1ka?= Message-ID: Date: Fri, 05 Aug 2016 12:48:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2 MIME-Version: 1.0 In-Reply-To: <67fda6d2-9b3e-a0d1-effc-34e1115030b2@acm.org> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2016-08/txt/msg00450.txt.bz2 On 08/05/2016 02:38 PM, Nathan Sidwell wrote: > On 08/05/16 04:55, Martin Liška wrote: > >> Thank you for very intensive brainstorming ;) Well I still believe that following code >> is not thread safe, let's image following situation: >> > > yeah, you're right. > >>> we could do better by using compare_exchange storing value, and detect the race I mentioned: >>> >>> gcov_t expected, val; >>> atomic_load (&counter[0], &val, ...); >> >> [thread 1]: value == 1, read val == 1 // scheduled here >> >>> gcov_t delta = val == value ? 1 : -1; >>> atomic_add (&counter[1], delta); >>> if (delta < 0) { >>> retry: >>> /* can we set counter[0]? */ >>> atomic_load (&counter[1], &expected, ...); >>> if (expected < 0) { >>> bool stored = atomic_compare_exchange (&counter[0], &val, &value, ...); >>> if (!stored && val != value) >>> goto retry; >> [thread 2]: value == 2, just updated counter[0] to 2 >> // after that [thread 1] continue, but wrongly does counter[1]++, but value != counter[0] >>> atomic_add (&counter[1], 2, ...); > > Bah. but (a) does it matter enough? and (b) if so does changing the delta<0 handling to store a count of 1 solve it?: (answer: no) > > gcov_t expected, val; > A:atomic_load (&counter[0], &val, ...); > gcov_t delta = val == value ? 1 : -1; > B:atomic_add (&counter[1], delta); > > if (delta < 0) { > /* can we set counter[0]? */ > C:atomic_load (&counter[1], &expected, ...); > if (expected < 0) { > D:atomic_store (&counter[0], &value); > E: atomic_store (&counter[1], 1); > } > atomic_add (&counter[1], 2, ...); > > > thread-1: value = 1, reads '1' at A > thread-2: value = 2, reads '1' at A > thread-2: decrements count @ B > thread-2: reads -1 at C > thread-2: write 2 at D > thread-2: stores 1 at E > thread-1: increments count @ B (finally) > > So we still can go awry. But the code's simpler. Like you said, I don't think it's possible to solve without an atomic update to both counter[0] & counter[1]. > > >> Well, I wrote attached test-case which should trigger a data-race, but TSAN is silent: > > I'm not too surprised. The race window is tiny and you put a printf in the middle of one path. I suspect if you put a sleep/printf on the counter[1] increment path you'll see it more often. > > nathan Ok, after all the experimenting and inventing "almost" thread-safe code, I incline to not to include __gcov_one_value_profiler_body_atomic counter. The final solution is cumbersome and probably does not worth doing. Moreover, even having a thread-safe implementation, result of an indirect call counter is not going to be stable among different runs (due to a single value storage capability). If you agree, I'll prepare a final version of patch? Thanks, Martin