Thanks for the review!
I updated the patch.

See notes below.

Le samedi 20 novembre 2021 à 13:50 -0500, David Malcolm a écrit :
> 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?

Done.

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

Done.

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

Fixed.

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