From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 48349 invoked by alias); 6 Mar 2020 14:41:20 -0000 Mailing-List: contact jit-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Post: List-Help: List-Subscribe: Sender: jit-owner@gcc.gnu.org Received: (qmail 48337 invoked by uid 89); 6 Mar 2020 14:41:20 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Checked: by ClamAV 0.100.3 on sourceware.org X-Virus-Found: No X-Spam-SWARE-Status: No, score=-10.8 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.1 spammy= X-Spam-Status: No, score=-10.8 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on sourceware.org X-Spam-Level: X-HELO: us-smtp-delivery-1.mimecast.com Received: from us-smtp-1.mimecast.com (HELO us-smtp-delivery-1.mimecast.com) (205.139.110.61) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 06 Mar 2020 14:41:18 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1583505677; 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=+GGS3uiSs7v3NgklVBe9Z4u9zz+Ewc2+JKuFzG56hRk=; b=UsLCqPLBB+mnFZohH3TB0frs+CoVK0VsGRR20to7y66HpFx3kBCZ2/EOxvZmCfDcf86ojA +XNjBGd4yfV5G3xbBEIrSylCBaFISEBirLA5wSnbPh5IPGrAPtFLfbL9e3dJOzoJHvNTkf ySJLCEkW+/YgrYhVzN2KNEi9865vo0A= 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-290-PP33RfJPNDWS7yBhpw83Ng-1; Fri, 06 Mar 2020 09:41:13 -0500 X-MC-Unique: PP33RfJPNDWS7yBhpw83Ng-1 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 37AA0107ACCA; Fri, 6 Mar 2020 14:41:12 +0000 (UTC) Received: from ovpn-116-56.phx2.redhat.com (ovpn-116-56.phx2.redhat.com [10.3.116.56]) by smtp.corp.redhat.com (Postfix) with ESMTP id 8C35F10016EB; Fri, 6 Mar 2020 14:41:11 +0000 (UTC) Message-ID: <9b9497617cfc4c30068f7078414a6a8df5db9d88.camel@redhat.com> Subject: Re: [PATCH][gcc] libgccjit: introduce version entry points From: David Malcolm To: Andrea Corallo , "gcc-patches@gcc.gnu.org" , "jit@gcc.gnu.org" Cc: nd Date: Wed, 01 Jan 2020 00:00:00 -0000 In-Reply-To: References: User-Agent: Evolution 3.32.5 (3.32.5-1.fc30) MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2020-q1/txt/msg00011.txt On Thu, 2020-03-05 at 21:34 -0500, David Malcolm wrote: > On Thu, 2020-01-16 at 11:11 +0000, Andrea Corallo wrote: Responding to my own ideas about thread-safety. [...] > My first thought here was that we should have a way to get all three > at > once, but it turns out that parse_basever does its own caching > internally. > > I don't think the current implementation is thread-safe; > parse_basever > has: > > static int s_major = -1, s_minor, s_patchlevel; > > if (s_major == -1) > if (sscanf (BASEVER, "%d.%d.%d", &s_major, &s_minor, > &s_patchlevel) != 3) > { > sscanf (BASEVER, "%d.%d", &s_major, &s_minor); > s_patchlevel = 0; > } > > I think there's a race here: if two threads call parse_basever at the > same time, it looks like: > (1) thread A could set s_major > (2) thread B could read s_major, find it's set > (3) thread B could read the uninitialized s_minor > (4) thread A sets s_minor > and various similar issues. > > One fix might be to add a version mutex to libgccjit.c; maybe > something > like the following (caveat: I haven't tried compiling this): > > /* A mutex around the cached state in parse_basever. > Ideally this would be within parse_basever, but the mutex is only > needed > by libgccjit. */ > > static pthread_mutex_t version_mutex = PTHREAD_MUTEX_INITIALIZER; > > struct version_info > { > /* Default constructor. Populate via parse_basever, > guarded by version_mutex. */ > version_info () > { > pthread_mutex_lock (&version_mutex); > parse_basever (&major, &minor, &patchlevel); > pthread_mutex_unlock (&version_mutex); > } > > int major; > int minor; > int patchlevel; > }; > > int > gcc_jit_version_major (void) > { > version_info vi; > return vi.major; > } > > int > gcc_jit_version_minor (void) > { > version_info vi; > return vi.minor; > } > > int > gcc_jit_version_patchlevel (void) > { > version_info vi; > return vi.patchlevel; > } > > Is adding a mutex a performance issue? How frequently are these > going > to be called? > > Alternatively, maybe make these functions take a gcc_jit_context and > cache the version information within the context? (since the API > requires multithreaded programs to use their own locking if threads > share a context) In retrospect, I don't think this other approach would work: the state is within parse_basever, so if two threads both determine they need to access it at about the same time, then they will race. > Or some kind of caching in libgccjit.c? (perhaps simply by making > the > version_info instances above static? my memory of C++ function- > static > init rules and what we can rely on on our minimal compiler is a > little > hazy) I'd hoped that we could rely on static init being thread-safe, but in general it isn't, according to: https://eli.thegreenplace.net/2011/08/30/construction-of-function-static-variables-in-c-is-not-thread-safe (apparently GCC 4 onwards makes it so, but other compilers don't) >From what I can tell parse_basever is only called once for the regular compiler use-case. So maybe it makes sense to remove the caching from it, and move the caching to libgccjit.c where we can have a mutex (AFAIK none of the rest of the host code uses mutexes). Or split out the actual parsing logic into a parse_basever_uncached that libgccjit.c can use, and manage its own caching with a pthread mutex like in my suggested version_info code above. Thoughts? Dave