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

[Andrea Corallo <andrea.corallo@arm.com>]

Hi,

thanks for reviewing, I've totally missed this multi-thread aspect.

David Malcolm <dmalcolm@redhat.com> writes:

> 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.

Agree

>> 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?

What would be the advantage in splitting the cached and uncached
versions at this stage?  If we accept that parse_basever is not thread
safe we can just protect it with the mutex, we'll have to use one anyway
also if we keep the cache in the jit code.

Coming back on your question I assume client code will call very rarely
this function (especially if we compare against the compile times
involved) so I don't think we can have a perf issue here.

My vote is to go for the mutex solution you've first suggested.

  Andrea