From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from sender4-pp-o91.zoho.com (sender4-pp-o91.zoho.com [136.143.188.91]) by sourceware.org (Postfix) with ESMTPS id 5F7DB393C851; Sat, 3 Oct 2020 18:14:20 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 5F7DB393C851 ARC-Seal: i=1; a=rsa-sha256; t=1601748854; cv=none; d=zohomail.com; s=zohoarc; b=OvuvpK4kVLgaJ0vnyLkuV1jJR6nrzlMjIO6xmyl26n1pCTvIcjesV9tpP+7Qv3V/i4oqIeJnuAfI2h6E2P8fVhJra50vyTZISTzDFfh3WiaQ9w3Ig21Rw42F40VEyX+PHBr2IKFo8+0ocGgWkj6dg0QjBUwFfoY1MbF3iod+VfE= ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=zohomail.com; s=zohoarc; t=1601748854; h=Content-Type:Cc:Date:From:In-Reply-To:MIME-Version:Message-ID:References:Subject:To; bh=XSP7LWLd1jWn5+kkOn8BVHMZC/QhjoFCSO6cYnqPiOM=; b=cENcwvLIG+5ggY+zPgItijYT9LK4uPhYBiu8BcIu83bZAqJ3Wx26S7u/+Bm3Oc3+jv8W8r9eKSODZJSaMehessDDSlJaZJDIigtRb3uP951lauhmYH+3pi6uuLyEJa1hchlZkRFrRHQs5cLaxpPLhpPE564MjYNjJrHUWvcrGEg= ARC-Authentication-Results: i=1; mx.zohomail.com; dkim=pass header.i=zoho.com; spf=pass smtp.mailfrom=bouanto@zoho.com; dmarc=pass header.from= header.from= Received: from localhost (38.87.1.99 [38.87.1.99]) by mx.zohomail.com with SMTPS id 160174885256848.8155225662515; Sat, 3 Oct 2020 11:14:12 -0700 (PDT) Date: Sat, 3 Oct 2020 14:14:10 -0400 From: Antoni Boucher To: David Malcolm Cc: gcc-patches@gcc.gnu.org, jit@gcc.gnu.org Subject: Re: [PATCH] libgccjit: add some reflection functions in the jit C api [WIP] Message-ID: <20201003181410.iggpsfrwlmu7gq5k@bouanto-desktop.localdomain> References: <20200902010120.crnx55ev635ceiey@bouanto-desktop.localdomain> <6fe2ea355403eb177c9632e076c11c792f23c791.camel@redhat.com> <9cd00fe5cb5c03184e3a5bd939447b30450a8ca7.camel@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline In-Reply-To: <9cd00fe5cb5c03184e3a5bd939447b30450a8ca7.camel@redhat.com> X-ZohoMailClient: External X-Spam-Status: No, score=-9.8 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, KAM_SHORT, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) 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, 03 Oct 2020 18:14:22 -0000 By the way, that seemed off to return NULL for the function returning a size_t to indicate an error. So I changed it to return -1 (and return type to ssize_t). Is that the proper way to indicate an error? I also wanted to mention that this patch is still a work-in-progress as I'm adding a few other functions. On Fri, Oct 02, 2020 at 04:24:26PM -0400, David Malcolm wrote: >On Fri, 2020-10-02 at 16:17 -0400, David Malcolm wrote: >> On Tue, 2020-09-01 at 21:01 -0400, Antoni Boucher via Jit wrote: >> > Hello. >> > This WIP patch implements new reflection functions in the C API as >> > mentioned in bug 96889. >> > I'm looking forward for feedbacks on this patch. >> > It's WIP because I'll probably add a few more reflection functions. >> > Thanks. >> >> Sorry about the belated review, looks like I missed this one. >> >> At a high level, it seems reasonable. >> >> Do you have a copyright assignment in place for GCC contributions? >> See https://gcc.gnu.org/contribute.html >> >> [...] > >diff --git a/gcc/jit/docs/topics/compatibility.rst >> > b/gcc/jit/docs/topics/compatibility.rst >> > index bb3387fa583..7e786194ded 100644 >> > --- a/gcc/jit/docs/topics/compatibility.rst >> > +++ b/gcc/jit/docs/topics/compatibility.rst >> > @@ -219,3 +219,14 @@ entrypoints: >> > * :func:`gcc_jit_version_minor` >> > >> > * :func:`gcc_jit_version_patchlevel` >> > + >> > +.. _LIBGCCJIT_ABI_14: >> > + >> > +``LIBGCCJIT_ABI_14`` >> > +-------------------- >> > +``LIBGCCJIT_ABI_14`` covers the addition of reflection functions >> > via API >> > +entrypoints: >> > + >> > + * :func:`gcc_jit_function_get_return_type` >> > + >> > + * :func:`gcc_jit_function_get_param_count` >> >> This will now need bumping to 15; 14 covers the addition of >> gcc_jit_global_set_initializer. >> >> [...] >> >> > +/* Public entrypoint. See description in libgccjit.h. >> > + >> > + After error-checking, the real work is done by the >> > + gcc::jit::recording::function::get_return_type method, in >> > + jit-recording.h. */ >> > + >> > +gcc_jit_type * >> > +gcc_jit_function_get_return_type (gcc_jit_function *func) >> > +{ >> >> This one is missing a: >> RETURN_NULL_IF_FAIL (func, NULL, NULL, "NULL function"); >> >> >> > + return (gcc_jit_type *)func->get_return_type (); >> > +} >> >> [...] >> >> > diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h >> > index 1c5a12e9c01..6999ce25ca2 100644 >> > --- a/gcc/jit/libgccjit.h >> > +++ b/gcc/jit/libgccjit.h >> >> [...] >> >> > @@ -1503,6 +1511,22 @@ gcc_jit_version_minor (void); >> > extern int >> > gcc_jit_version_patchlevel (void); >> > >> > +#define LIBGCCJIT_HAVE_gcc_jit_function_reflection >> > + >> > +/* Reflection functions to get the number of parameters and return >> > types of >> > + a function from the C API. >> >> "return type", is better grammar, I think, given that "function" is >> singular. >> >> > + >> > + "vec_type" should be a vector type, created using >> > gcc_jit_type_get_vector. >> >> This line about "vec_type" seems to be a leftover from a copy&paste. >> >> >> > + This API entrypoint was added in LIBGCCJIT_ABI_14; you can test >> > for its >> > + presence using >> > + #ifdef LIBGCCJIT_HAVE_gcc_jit_function_reflection >> >> Version number will need bumping, as mentioned above. >> >> [...] >> >> > diff --git a/gcc/jit/libgccjit.map b/gcc/jit/libgccjit.map >> > index 6137dd4b4b0..b28f81a7a32 100644 >> > --- a/gcc/jit/libgccjit.map >> > +++ b/gcc/jit/libgccjit.map >> > @@ -186,4 +186,10 @@ LIBGCCJIT_ABI_13 { >> > gcc_jit_version_major; >> > gcc_jit_version_minor; >> > gcc_jit_version_patchlevel; >> > -} LIBGCCJIT_ABI_12; >> > \ No newline at end of file >> > +} LIBGCCJIT_ABI_12; >> > + >> > +LIBGCCJIT_ABI_14 { >> > + global: >> > + gcc_jit_function_get_return_type; >> > + gcc_jit_function_get_param_count; >> > +} LIBGCCJIT_ABI_13; >> >> Likewise. >> >> [...] >> >> Otherwise looks good to me. >> >> Bonus points for adding C++ bindings (and docs for them), but I don't >> know of anyone using the C++ bindings. > > >Also, please add "PR jit/96889" to the ChangeLog entries, and [PR96889] >to the subject line. > >Dave >