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
