From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id B40E9385840D for ; Sat, 20 Nov 2021 18:50:56 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org B40E9385840D Received: from mail-qk1-f198.google.com (mail-qk1-f198.google.com [209.85.222.198]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-41-dhjxuiAEPs-wLHaBvh6p7w-1; Sat, 20 Nov 2021 13:50:55 -0500 X-MC-Unique: dhjxuiAEPs-wLHaBvh6p7w-1 Received: by mail-qk1-f198.google.com with SMTP id c1-20020a05620a0ce100b00468060d41ecso10816229qkj.19 for ; Sat, 20 Nov 2021 10:50:55 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:subject:from:to:date:in-reply-to :references:user-agent:mime-version:content-transfer-encoding; bh=6EVPe4vUT6eXZ4GuFy++T16/Pbb5/Rdq3q2HZhOxF8Y=; b=ZrnISg7JypgfmocO7mZopVEgA7X/HfLNi6VVG5n3Jncf7MoXw2MKZEgAWKE3nPTCdH DxFnQ45rRTTPtfragxCRgxXh4djG0+d9qodld2QBC55NxJCPHIstbhXhoEgmDIBGlEkO kTes7y1u0r/HaGUQKNmJFhaS4+ywNTSR9Lo/2wVxVp4SqRpAiykohjw8mHABhKgiRddP cx7an3f4Rsj69na+3B8UBS3n45lRRhk1JnRqHUyHvgYZdnmOYI/bdM1dQQc+SYok8cGp 4DxVDPhyt0+Azqr1fbzPIWdNJ2u1Y9Z8gvvOrZq/moTls6+pAIodT19WinWVweksdTJM AHkA== X-Gm-Message-State: AOAM532/6SzCSX2v2HlwHkBSgKIN1LNA7/6p7/LINPazzFS30U7ZBA5X QHNoVOWlgPJ1XnueYuw2PpfJWpd4VZWTvlyHAeIH0xqRRq7tXjRk+dQgJTktJY6wNk7F0lZElDz aIsRum5Q= X-Received: by 2002:a05:622a:554:: with SMTP id m20mr17050412qtx.382.1637434254482; Sat, 20 Nov 2021 10:50:54 -0800 (PST) X-Google-Smtp-Source: ABdhPJxtO0WMLrTUpfOItqn9RCSmMEYmxUPmBdq1fX32aDqN5eQHqg1QzmuRThOxGhyJcqlkY3AZJw== X-Received: by 2002:a05:622a:554:: with SMTP id m20mr17050378qtx.382.1637434254174; Sat, 20 Nov 2021 10:50:54 -0800 (PST) Received: from t14s.localdomain (c-73-69-212-193.hsd1.nh.comcast.net. [73.69.212.193]) by smtp.gmail.com with ESMTPSA id g12sm1776026qtk.69.2021.11.20.10.50.53 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 20 Nov 2021 10:50:53 -0800 (PST) Message-ID: <45c7ea23708a4ff029c0d8711f809230e90ea057.camel@redhat.com> Subject: Re: [PATCH] libgccjit: Add support for types used by atomic builtins [PR96066] [PR96067] From: David Malcolm To: Antoni Boucher , jit@gcc.gnu.org, gcc-patches@gcc.gnu.org Date: Sat, 20 Nov 2021 13:50:52 -0500 In-Reply-To: <3abfd5cf6546892f25a01c1772121c848487a196.camel@zoho.com> References: <212418d2a4b81cfff1e125ddb12d0b4d10d8404a.camel@zoho.com> <3abfd5cf6546892f25a01c1772121c848487a196.camel@zoho.com> User-Agent: Evolution 3.38.4 (3.38.4-1.fc33) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-12.9 required=5.0 tests=BAYES_00, BODY_8BITS, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=unavailable autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: jit@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Jit mailing list List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 20 Nov 2021 18:50:58 -0000 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