From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 45019 invoked by alias); 6 Mar 2020 02:35:00 -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 45001 invoked by uid 89); 6 Mar 2020 02:35:00 -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=-23.3 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.1 spammy= X-Spam-Status: No, score=-23.3 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,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 02:34:58 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1583462097; 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=LeNzo1uyScs4NgHcJYs5XN9JekJSajyxrF8PUczeJ2s=; b=ac/WNp1y3m7V3Jf/EQKiluKTLe9lVaK0lndX9ZouX40X9q6LODen5fVx28qV+mMSwOAcxn QfqLd2sERHp8qu7uUWX6CjLianwUARUZVc61zwaCV8n661dQSWX9RA1Wd9xTNqcH27vyO2 Iro/6ln96h6A05eQO922wdslP1is7Zw= 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-128-lY6g85cxPQWDPLBctF_6RA-1; Thu, 05 Mar 2020 21:34:55 -0500 X-MC-Unique: lY6g85cxPQWDPLBctF_6RA-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 8299075FF2; Fri, 6 Mar 2020 02:34:54 +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 D087E73865; Fri, 6 Mar 2020 02:34:53 +0000 (UTC) Message-ID: 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.79 on 10.5.11.15 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/msg00007.txt On Thu, 2020-01-16 at 11:11 +0000, Andrea Corallo wrote: > Hi, second version of the patch here cleaning up an unnecessary > change. > > Does not introduce regressions with make check-jit. > > Andrea > > gcc/jit/ChangeLog > 2020-??-?? Andrea Corallo > > * docs/topics/compatibility.rst (LIBGCCJIT_ABI_13): New ABI tag > plus add version paragraph. > * libgccjit++.h (namespace gccjit::version): Add new namespace. > * libgccjit.c (gcc_jit_version_major, gcc_jit_version_minor) > (gcc_jit_version_patchlevel): New functions. > * libgccjit.h (LIBGCCJIT_HAVE_gcc_jit_version): New macro. > (gcc_jit_version_major, gcc_jit_version_minor) > (gcc_jit_version_patchlevel): New functions. > * libgccjit.map (LIBGCCJIT_ABI_13) New ABI tag. > > gcc/testsuite/ChangeLog > 2020-??-?? Andrea Corallo > > * jit.dg/test-version.c: New testcase. [...] Thanks for the patch; sorry for the delay in reviewing this. Out of interest, do you have a specific use for this, or is it more speculative? > diff --git a/gcc/jit/libgccjit.c b/gcc/jit/libgccjit.c > index 83055fc297b..572c82f053c 100644 > --- a/gcc/jit/libgccjit.c > +++ b/gcc/jit/libgccjit.c > @@ -23,6 +23,7 @@ along with GCC; see the file COPYING3. If not see > #include "coretypes.h" > #include "timevar.h" > #include "typed-splay-tree.h" > +#include "cppbuiltin.h" > > #include "libgccjit.h" > #include "jit-recording.h" > @@ -3175,3 +3176,27 @@ gcc_jit_context_new_rvalue_from_vector (gcc_jit_context *ctxt, > as_vec_type, > (gcc::jit::recording::rvalue **)elements); > } > + > +extern int > +gcc_jit_version_major (void) > +{ > + int major, minor, patchlevel; > + parse_basever (&major, &minor, &patchlevel); > + return major; > +} > + > +extern int > +gcc_jit_version_minor (void) > +{ > + int major, minor, patchlevel; > + parse_basever (&major, &minor, &patchlevel); > + return minor; > +} > + > +extern int > +gcc_jit_version_patchlevel (void) > +{ > + int major, minor, patchlevel; > + parse_basever (&major, &minor, &patchlevel); > + return patchlevel; > +} 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) 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) > diff --git a/gcc/testsuite/jit.dg/test-version.c b/gcc/testsuite/jit.dg/test-version.c > new file mode 100644 > index 00000000000..4338a00018b > --- /dev/null > +++ b/gcc/testsuite/jit.dg/test-version.c > @@ -0,0 +1,26 @@ > +#include > +#include > + > +#include "libgccjit.h" > + > +#include "harness.h" > + > +#ifndef LIBGCCJIT_HAVE_gcc_jit_version > +#error LIBGCCJIT_HAVE_gcc_jit_version was not defined > +#endif > + > +void > +create_code (gcc_jit_context *ctxt, void *user_data) > +{ > + /* Do nothing. */ > +} > + > +void > +verify_code (gcc_jit_context *ctxt, gcc_jit_result *result) > +{ > + if (!gcc_jit_version_major ()) > + fail ("Major version is zero"); > + /* Minor and patchlevel can be zero. */ > + gcc_jit_version_minor (); > + gcc_jit_version_patchlevel (); > +} Please add this testcase to the "testcases" array in gcc/testsuite/jit.dg/all-non-failing-tests.h (see the comment towards the end of jit/docs/internals/index.rst). In particular this will exercise it from multiple threads. Dave