Re: [PATCH] libgccjit: Add support for types used by atomic builtins [PR96066] [PR96067]

[David Malcolm <dmalcolm@redhat.com>]

On Sat, 2021-11-20 at 11:27 -0500, Antoni Boucher wrote:
> Hi.
> Here's the updated patch.
> Thanks for the review!

Thanks for the updated patch...

> 
> Le jeudi 20 mai 2021 à 16:24 -0400, David Malcolm a écrit :
> > On Mon, 2021-05-17 at 21:02 -0400, Antoni Boucher via Jit wrote:
> > > Hello.
> > > This patch fixes the issue with using atomic builtins in libgccjit.
> > > Thanks to review it.
> > 
> > [...snip...]
> >  
> > > diff --git a/gcc/jit/jit-recording.c b/gcc/jit/jit-recording.c
> > > index 117ff70114c..de876ff9fa6 100644
> > > --- a/gcc/jit/jit-recording.c
> > > +++ b/gcc/jit/jit-recording.c
> > > @@ -2598,8 +2598,18 @@
> > > recording::memento_of_get_pointer::accepts_writes_from (type
> > > *rtype)
> > >      return false;
> > >  
> > >    /* It's OK to assign to a (const T *) from a (T *).  */
> > > -  return m_other_type->unqualified ()
> > > -    ->accepts_writes_from (rtype_points_to);
> > > +  if (m_other_type->unqualified ()
> > > +    ->accepts_writes_from (rtype_points_to)) {
> > > +      return true;
> > > +  }
> > > +
> > > +  /* It's OK to assign to a (volatile const T *) from a (volatile
> > > const T *). */
> > > +  if (m_other_type->unqualified ()->unqualified ()
> > > +    ->accepts_writes_from (rtype_points_to->unqualified ())) {
> > > +      return true;
> > > +  }
> > 
> > Presumably you need this to get the atomic builtins working?
> > 
> > If I'm reading the above correctly, the new test doesn't distinguish
> > between the 3 different kinds of qualifiers (aligned, volatile, and
> > const), it merely tries to strip some of them off.
> > 
> > It's not valid to e.g. assign to a (aligned T *) from a (const T *).
> > 
> > Maybe we need an internal enum to discriminate between different
> > subclasses of decorated_type?

I'm still concerned about this case, my reading of the updated patch is
that this case is still not quite correctly handled (see notes below).
I don't think we currently have test coverage for assignment to e.g.
(aligned T *) from a (const T*); I feel that it should be an error,
without an explicit cast.

Please can you add a testcase for this?

If you want to go the extra mile, given that this is code created
through an API, you could have a testcase that iterates through all
possible combinations of qualifiers (for both source and destination
pointer), and verifies that libgccjit at least doesn't crash on them
(and hopefully does the right thing on each one)  :/

(perhaps doing each one in a different gcc_jit_context)

Might be nice to update test-fuzzer.c for the new qualifiers; I don't
think I've touched it in a long time.

[...snip...]

> diff --git a/gcc/jit/jit-recording.h b/gcc/jit/jit-recording.h
> index 4a994fe7094..60aaba2a246 100644
> --- a/gcc/jit/jit-recording.h
> +++ b/gcc/jit/jit-recording.h
> @@ -545,6 +545,8 @@ public:
>    virtual bool is_float () const = 0;
>    virtual bool is_bool () const = 0;
>    virtual type *is_pointer () = 0;
> +  virtual type *is_volatile () { return NULL; }
> +  virtual type *is_const () { return NULL; }
>    virtual type *is_array () = 0;
>    virtual struct_ *is_struct () { return NULL; }
>    virtual bool is_void () const { return false; }
> @@ -687,6 +689,13 @@ public:
>    /* Strip off the "const", giving the underlying type.  */
>    type *unqualified () FINAL OVERRIDE { return m_other_type; }
>  
> +  virtual bool is_same_type_as (type *other)
> +  {
> +    return m_other_type->is_same_type_as (other->is_const ());
> +  }

What happens if other_is_const () returns NULL, and
  m_other_type->is_same_type_as ()
tries to call a vfunc on it...

> +
> +  virtual type *is_const () { return m_other_type; }
> +
>    void replay_into (replayer *) FINAL OVERRIDE;
>  
>  private:
> @@ -701,9 +710,16 @@ public:
>    memento_of_get_volatile (type *other_type)
>    : decorated_type (other_type) {}
>  
> +  virtual bool is_same_type_as (type *other)
> +  {
> +    return m_other_type->is_same_type_as (other->is_volatile ());
> +  }

...with similar considerations here.

i.e. is it possible for the user to create combinations of qualifiers
that lead to a vfunc call with NULL "this" (and thus a segfault?)

> +
>    /* Strip off the "volatile", giving the underlying type.  */
>    type *unqualified () FINAL OVERRIDE { return m_other_type; }
>  
> +  virtual type *is_volatile () { return m_other_type; }
> +
>    void replay_into (replayer *) FINAL OVERRIDE;
>  

Hope this is constructive
Dave