From patchwork Fri Aug 5 08:55:15 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: =?utf-8?q?Martin_Li=C5=A1ka?= X-Patchwork-Id: 656092 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3s5LHY4ZnSz9svs for ; Fri, 5 Aug 2016 18:55:37 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b=rJniJkUx; dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :subject:to:references:cc:from:message-id:date:mime-version :in-reply-to:content-type; q=dns; s=default; b=dVb6fk1Z1FRr4gz1C Xfb+M1w4jwcZIdipvy08MNJSnIYze2PcmlKh3eB/ecXepkS89xfyXogc0tK2lVOY Rdq9kBQBB3iT+t+9L+yteZg4Vsh9iP79i3t1L+YM6hvgfRRjZ0aQrXHBYY9/PN/e 0JbBqDDUh6k5erKasWYOqCzo3M= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :subject:to:references:cc:from:message-id:date:mime-version :in-reply-to:content-type; s=default; bh=76mPuBBJTM60AikCPCmtMGi 2jHI=; b=rJniJkUxudm4xvlWQXxez3LFzM3t7QqwjcNnfepDzAX2wVgsMEer2Nl 07r4a0c+BgqLFMDbKcXC0mwlxcgZg9kOHKDtiSW4LuwoyUSdSiPY7fLlrl2kTkR0 83hrsrK7N9cNs/F6Bz3liegjbbidywodZVeM4eluvwOGi9XcCs3k= Received: (qmail 127798 invoked by alias); 5 Aug 2016 08:55:29 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 127789 invoked by uid 89); 5 Aug 2016 08:55:28 -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=wrongly, retval 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 08:55:18 +0000 Received: from relay1.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id ABA15AAEF; Fri, 5 Aug 2016 08:55:15 +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> Cc: jh@suse.cz From: =?UTF-8?Q?Martin_Li=c5=a1ka?= Message-ID: <1253ac69-3301-f185-e43a-a34cadf8f51e@suse.cz> Date: Fri, 5 Aug 2016 10:55:15 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2 MIME-Version: 1.0 In-Reply-To: <87f2bc4f-c4df-eadd-aec6-a937ed0ccaba@acm.org> X-IsSubscribed: yes On 08/04/2016 07:03 PM, Nathan Sidwell wrote: > On 08/04/16 12:43, Nathan Sidwell wrote: > >> How about: >> gcov_t expected; >> atomic_load (&counter[0], val, ...); >> gcov_t delta = val == value ? 1 : -1; >> atomic_add (&counter[1], delta); <-- or atomic_add_fetch >> if (delta < 0) { >> /* can we set counter[0]? */ >> atomic_load (&counter[1], &expected, ...); >> if (expected < 0) { >> atomic_store (&counter[0], value, ...); >> atomic_add (&counter[1], 2, ...); >> } >> } >> atomic_add (&counter[2], 1, ...); > Hi. Thank you for very intensive brainstorming ;) Well I still believe that following code is not thread safe, let's image following situation: > 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, ...); > } > } > atomic_add (&counter[2], 1, ...); > > This corrects the off-by one issue. > > nathan Well, I wrote attached test-case which should trigger a data-race, but TSAN is silent: $ g++ race.cc -pthread -fprofile-generate -g -fsanitize=thread -fprofile-update=atomic $ ./a.out In main: creating thread 0 In main: creating thread 1 new counter[1] value, N:0 In main: creating thread 2 new counter[1] value, N:1 new counter[1] value, N:2 new counter[1] value, N:3 new counter[1] value, N:4 new counter[1] value, N:5 new counter[1] value, N:6 new counter[1] value, N:7 new counter[1] value, N:8 new counter[1] value, N:9 new counter[1] value, N:10 new counter[1] value, N:11 new counter[1] value, N:12 new counter[1] value, N:12 new counter[1] value, N:13 new counter[1] value, N:14 new counter[1] value, N:15 new counter[1] value, N:16 In main: creating thread 3 In main: creating thread 4 In main: creating thread 5 In main: creating thread 6 In main: creating thread 7 In main: creating thread 8 In main: creating thread 9 In main: creating thread 10 In main: creating thread 11 In main: creating thread 12 In main: creating thread 13 In main: creating thread 14 In main: creating thread 15 However, not updating arc counters with atomic operations causes really many races: $ g++ race.cc -pthread -fprofile-generate -g -fsanitize=thread $ ./a.out 2>&1 | grep 'data race' | wc -l 110 Sample: WARNING: ThreadSanitizer: data race (pid=11424) Read of size 8 at 0x000000606718 by thread T4: #0 A::foo() /tmp/race.cc:10 (a.out+0x000000401e78) Previous write of size 8 at 0x000000606718 by thread T1: [failed to restore the stack] Location is global '__gcov0._ZN1A3fooEv' of size 16 at 0x000000606710 (a.out+0x000000606718) Thread T4 (tid=11429, running) created by main thread at: #0 pthread_create ../../../../libsanitizer/tsan/tsan_interceptors.cc:876 (libtsan.so.0+0x00000002ad2d) #1 main /tmp/race.cc:43 (a.out+0x000000401afb) Thread T1 (tid=11426, finished) created by main thread at: #0 pthread_create ../../../../libsanitizer/tsan/tsan_interceptors.cc:876 (libtsan.so.0+0x00000002ad2d) #1 main /tmp/race.cc:43 (a.out+0x000000401afb) Maybe I miss something and my tester sample is wrong (please apply attached patch to use original __gcov_one_value_profiler_body)? Thanks, Martin diff --git a/gcc/tree-profile.c b/gcc/tree-profile.c index 6e96fa9..42b780f 100644 --- a/gcc/tree-profile.c +++ b/gcc/tree-profile.c @@ -188,8 +188,8 @@ gimple_init_edge_profiler (void) profiler_fn_name = "__gcov_indirect_call_profiler_v2"; if (PARAM_VALUE (PARAM_INDIR_CALL_TOPN_PROFILE)) profiler_fn_name = "__gcov_indirect_call_topn_profiler"; - if (flag_profile_update == PROFILE_UPDATE_ATOMIC) - profiler_fn_name = "__gcov_indirect_call_profiler_v2_atomic"; +// if (flag_profile_update == PROFILE_UPDATE_ATOMIC) +// profiler_fn_name = "__gcov_indirect_call_profiler_v2_atomic"; tree_indirect_call_profiler_fn = build_fn_decl (profiler_fn_name, ic_profiler_fn_type); diff --git a/libgcc/libgcov-profiler.c b/libgcc/libgcov-profiler.c index c1e287d..d43932e 100644 --- a/libgcc/libgcov-profiler.c +++ b/libgcc/libgcov-profiler.c @@ -70,6 +70,8 @@ __gcov_pow2_profiler (gcov_type *counters, gcov_type value) In any case, COUNTERS[2] is incremented. */ +static int counter = 0; + static inline void __gcov_one_value_profiler_body (gcov_type *counters, gcov_type value) { @@ -77,6 +79,7 @@ __gcov_one_value_profiler_body (gcov_type *counters, gcov_type value) counters[1]++; else if (counters[1] == 0) { + fprintf (stderr, "new counter[1] value, N:%d\n", counter++); counters[1] = 1; counters[0] = value; }