Re: [PATCH][gcc] libgccjit: introduce version entry points

[David Malcolm <dmalcolm@redhat.com>]

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