public inbox for jit@gcc.gnu.org
 help / color / mirror / Atom feed
From: Antoni Boucher <bouanto@zoho.com>
To: David Malcolm <dmalcolm@redhat.com>,
	jit@gcc.gnu.org,  gcc-patches@gcc.gnu.org
Subject: Re: [PATCH] libgccjit: Add support for types used by atomic builtins [PR96066] [PR96067]
Date: Fri, 03 Dec 2021 23:37:30 -0500	[thread overview]
Message-ID: <f30e077e29f9f2f1085816f3b9c7e38263d422e7.camel@zoho.com> (raw)
In-Reply-To: <8e5c4b562f6c232e592c0a585777cf325321bf54.camel@zoho.com>

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


  reply	other threads:[~2021-12-04  4:37 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-18  1:02 Antoni Boucher
2021-05-20 20:24 ` David Malcolm
2021-05-26 21:58   ` Antoni Boucher
2021-11-20 16:27   ` Antoni Boucher
2021-11-20 18:50     ` David Malcolm
2021-11-21 21:44       ` Antoni Boucher
2021-12-04  4:37         ` Antoni Boucher [this message]
2021-12-09 20:25         ` David Malcolm

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=f30e077e29f9f2f1085816f3b9c7e38263d422e7.camel@zoho.com \
    --to=bouanto@zoho.com \
    --cc=dmalcolm@redhat.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=jit@gcc.gnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).