David: PING
In case you missed it, that's the last patch left to review for now.
Le dimanche 21 novembre 2021 à 16:44 -0500, Antoni Boucher a écrit :
> 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
> >
>
