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.133.124]) by sourceware.org (Postfix) with ESMTPS id 31C9E3858411 for ; Sat, 20 Nov 2021 16:21:00 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 31C9E3858411 Received: from mail-qk1-f199.google.com (mail-qk1-f199.google.com [209.85.222.199]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-462-9Oxb-yR9NpGVSvCpwDKieg-1; Sat, 20 Nov 2021 11:20:57 -0500 X-MC-Unique: 9Oxb-yR9NpGVSvCpwDKieg-1 Received: by mail-qk1-f199.google.com with SMTP id de12-20020a05620a370c00b00467697ab8a7so10539264qkb.9 for ; Sat, 20 Nov 2021 08:20:57 -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=/Fi3ua+v+zpnWt62JSG1Wl0Cb1PwigqU/IeiEGX1yIk=; b=AgBS0A0ZlEIAneiCKgG3RcG6qHsVS0d3f/o/OGEeX0WpZjvUb2hxHv5Fw44LcEuK48 RrjJtAMQSJisrBel5WMqkgLA4zhjahu2H7CIdHFiCTSRenK2mK8Ejk80fBvj232FQLqZ CTTrwwYO00oFNFZguMwptfC+vOYegSMC7Fy8swjONCD9nB/yxLT5tUpxcpSBWES2G+lD NThloVhkc6sobPMDQ2LO/NPylNGBJVcZl7Xrlv9stRj8SB1j9CYQT4Y3hMyGiYcBcnYg g6AHYtrOmixluzDi2J5vVSaFTki6bM6thhypXymXwGZ5IgALovLiBKsgAS0rbc21quao aNNg== X-Gm-Message-State: AOAM532W5r6/L811O4FPb3+UgWv5RPJJ9ugCYB7LIunB3/0wDX1rlQRU 16D+1hAJkkAXoruuAVN2fwVrFCF5NjVdjZAyMeKIe5q6pl+eT9iuc6UcztYQ/3veivJotm+1tCA 9O6JKNFI= X-Received: by 2002:a05:6214:29e9:: with SMTP id jv9mr83787447qvb.67.1637425256848; Sat, 20 Nov 2021 08:20:56 -0800 (PST) X-Google-Smtp-Source: ABdhPJzW/xvmR++xxjZXNsWjG6r7YH0TFvpAbd8Sp7u6ecKlSU6CwTgQYsYRiGrtpi/KkDjvRUnTuQ== X-Received: by 2002:a05:6214:29e9:: with SMTP id jv9mr83787417qvb.67.1637425256487; Sat, 20 Nov 2021 08:20:56 -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 x7sm1717824qko.109.2021.11.20.08.20.55 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 20 Nov 2021 08:20:55 -0800 (PST) Message-ID: <7530c6bca7aab2a29303957512be97cf090e159d.camel@redhat.com> Subject: Re: [PATCH] libgccjit: Add support for setting the link section of global variables [PR100688] From: David Malcolm To: Antoni Boucher , jit@gcc.gnu.org, gcc-patches@gcc.gnu.org Date: Sat, 20 Nov 2021 11:20:54 -0500 In-Reply-To: References: <7fde146d8c3aecb47f66f34c836e2733eb4d8a6d.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=-10.6 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_H2, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham 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 16:21:02 -0000 On Sat, 2021-11-20 at 00:58 -0500, Antoni Boucher wrote: > Thanks for your reviews! > > Here's the updated patch, ready for another review. > See comments/questions below. Thanks for the updated patch... > > I'll update the other patches over the weekend. > > Le jeudi 20 mai 2021 à 15:29 -0400, David Malcolm a écrit : > > On Wed, 2021-05-19 at 20:32 -0400, Antoni Boucher via Jit wrote: > > > Hello. > > > This patch adds support to set the link section of global > > > variables. > > > I used the ABI 18 because I submitted other patches up to 17. > > > Thanks for the review. > > > > I didn't see this email until now, and put the review in bugzilla > > instead; sorry. > > > > Here's a copy-and-paste of what I put in bugzilla: > > > > > > Thanks for the patch; I like the idea; various nits below: > > [...snip...] > > > > >  Global variables > > >  **************** > > >   > > > diff --git a/gcc/jit/jit-playback.h b/gcc/jit/jit-playback.h > > > index 825a3e172e9..8b0f65e87e8 100644 > > > --- a/gcc/jit/jit-playback.h > > > +++ b/gcc/jit/jit-playback.h > > > @@ -650,6 +650,8 @@ public: > > >   > > >  private: > > >    context *m_ctxt; > > > + > > > +protected: > > >    tree m_inner; > > >  }; > > > > I think you only use this... > > > > >   > > > @@ -670,6 +672,12 @@ public: > > >    rvalue * > > >    get_address (location *loc); > > >   > > > +  void > > > +  set_link_section (const char* name) > > > +  { > > > +    set_decl_section_name (m_inner, name); > > > +  } > > > > ...here, and you can get at rvalue::m_inner using as_tree (), so I > > don't think we need to make m_inner protected. Thanks for dropping the "protected" in the updated patch; you need to update the ChangeLog entry. > > > > > > > +  set_playback_obj (global); > > >  } > > >   > > > > [...snip....] > > > > > diff --git a/gcc/jit/jit-recording.h b/gcc/jit/jit-recording.h > > > index 03fa1160cf0..0691fac579d 100644 > > > --- a/gcc/jit/jit-recording.h > > > +++ b/gcc/jit/jit-recording.h > > > @@ -1105,7 +1105,8 @@ public: > > >    lvalue (context *ctxt, > > >           location *loc, > > >           type *type_) > > > -    : rvalue (ctxt, loc, type_) > > > +    : rvalue (ctxt, loc, type_), > > > +      m_link_section(NULL) > > >      {} > > >   > > >    playback::lvalue * > > > @@ -1127,6 +1128,10 @@ public: > > >    const char *access_as_rvalue (reproducer &r) OVERRIDE; > > >    virtual const char *access_as_lvalue (reproducer &r); > > >    virtual bool is_global () const { return false; } > > > +  void set_link_section (const char *name); > > > + > > > +protected: > > > +  string *m_link_section; > > >  }; > > > > Can it be private, rather than protected? > > m_link_section can't be private because it's used in > recording::global::replay_into. Fair enough; thanks. > > > > > diff --git a/gcc/jit/libgccjit.c b/gcc/jit/libgccjit.c > > > index 7fa948007ad..8cfa48aae24 100644 > > > --- a/gcc/jit/libgccjit.c > > > +++ b/gcc/jit/libgccjit.c > > > @@ -1953,6 +1953,18 @@ gcc_jit_lvalue_get_address (gcc_jit_lvalue > > *lvalue, > > >    return (gcc_jit_rvalue *)lvalue->get_address (loc); > > >  } > > >   > > > +/* Public entrypoint.  See description in libgccjit.h. > > > + > > > +   After error-checking, the real work is done by the > > > +   gcc::jit::recording::lvalue::set_section method in jit- > > recording.c.  */ > >                                    ^^^^^^^^^^^ > >                                    set_link_section > > > > Also, a newline here please for consistency with the other > > entrypoints. > > Where should I add a newline? Between the closing of the comment and the "void" of the fndecl (all the other entrypoints have a blank line separating them). Thanks for fixing my other nitpicks. [...snip...] > > > diff --git a/gcc/testsuite/jit.dg/all-non-failing-tests.h > > b/gcc/testsuite/jit.dg/all-non-failing-tests.h > > > index 4202eb7798b..7e3b59dee0d 100644 > > > --- a/gcc/testsuite/jit.dg/all-non-failing-tests.h > > > +++ b/gcc/testsuite/jit.dg/all-non-failing-tests.h > > > @@ -181,6 +181,13 @@ > > >  #undef create_code > > >  #undef verify_code > > >   > > > +/* test-link-section.c */ > > > +#define create_code create_code_link_section > > > +#define verify_code verify_code_link_section > > > +#include "test-link-section.c" > > > +#undef create_code > > > +#undef verify_code The updated version of the testcase doesn't have a verify_code, so it can't be in the "testcases" array. Please replace this with something like: /* test-link-section.c: This can't be in the testcases array as it doesn't have a verify_code implementation. */ (I'm trying to have all-nonfailing-tests.h refer to each non-failing test, either adding it to the testcases array, or documenting why it isn't in the array; listing them in alphabetical order) [...snip...] > > > > > > + > > > +extern void > > > +verify_code (gcc_jit_context *ctxt, gcc_jit_result *result) > > > +{ > > > +} > > > > This is OK, but ideally it would test that the section name made it > > into the generated assembler.  test-compile-to-assembler.c has a > > testcase for this which does something similar, with a DejaGnu > > directive looking for a substring in the generated asm if you want to > > attempt it. Thanks for implementing the .exp directive. > > > > One other thing: the docs should make it clear about the leading ".". > > > > If I want to create the equivalent of: > > > >    __attribute__((section(".section"))) > > > > do I call it with: > > > >    gcc_jit_lvalue_set_link_section(foo, "section"); > > > > or with: > > > >    gcc_jit_lvalue_set_link_section(foo, ".section"); > > > > It's a bit unclear to me from just reading the patch.  The example > > suggests it's the former.  In either case, the documentation should > > be > > clearer about this. Thanks for clarifying this in the docs. > Notes on the updated patch follow: > From a454cb9ce14aa6903f7be9f2a56c35ab8251e678 Mon Sep 17 00:00:00 2001 > From: Antoni Boucher > Date: Wed, 12 May 2021 07:57:54 -0400 > Subject: [PATCH] libgccjit: Add support for setting the link section of global > variables [PR100688] > > 2021-11-20 Antoni Boucher > > gcc/jit/ > PR target/100688 > * docs/topics/compatibility.rst (LIBGCCJIT_ABI_18): New ABI > tag. > * docs/topics/expressions.rst: Add documentation for the > function gcc_jit_lvalue_set_link_section. > * jit-playback.h: New function (set_link_section) and > rvalue::m_inner protected. As noted above, this part of the ChangeLog at least needs updating. > * jit-recording.c: New function (set_link_section) and > support for setting the link section. > * jit-recording.h: New function (set_link_section) and new > field m_link_section. > * libgccjit.c: New function (gcc_jit_lvalue_set_link_section). > * libgccjit.h: New function (gcc_jit_lvalue_set_link_section). > * libgccjit.map (LIBGCCJIT_ABI_18): New ABI tag. > > gcc/testsuite/ > PR target/100688 > * jit.dg/all-non-failing-tests.h: Add test-link-section.c. > * jit.dg/test-link_section.c: New test. > * jit.dg/jit.exp: New helper function to test that the > assembly contains a pattern. [...snip...] > diff --git a/gcc/jit/docs/topics/compatibility.rst b/gcc/jit/docs/topics/compatibility.rst > index 52ee3f860a7..b852f205fd7 100644 > --- a/gcc/jit/docs/topics/compatibility.rst > +++ b/gcc/jit/docs/topics/compatibility.rst > @@ -284,3 +284,12 @@ entrypoints: > * :func:`gcc_jit_struct_get_field` > > * :func:`gcc_jit_struct_get_field_count` > + > +.. _LIBGCCJIT_ABI_18: > + > +``LIBGCCJIT_ABI_18`` > +----------------------- > +``LIBGCCJIT_ABI_18`` covers the addition of an API entrypoint to set the link > +section of a variable: > + > + * :func:`gcc_jit_lvalue_set_link_section` As noted in earlier review, presumably there's a different patch that adds ABI_17? I can potentially approve this link section patch now, but that other patch would need to be pushed first. [...snip...] > @@ -4547,20 +4552,30 @@ recording::block::dump_edges_to_dot (pretty_printer *pp) > void > recording::global::replay_into (replayer *r) > { > - set_playback_obj ( > - m_initializer > - ? r->new_global_initialized (playback_location (r, m_loc), > + playback::lvalue *global; > + if (m_initializer) > + { > + global = r->new_global_initialized (playback_location (r, m_loc), > m_kind, > m_type->playback_type (), > m_type->dereference ()->get_size (), > m_initializer_num_bytes > / m_type->dereference ()->get_size (), > m_initializer, > - playback_string (m_name)) > - : r->new_global (playback_location (r, m_loc), > + playback_string (m_name)); > + } > + else > + { > + global = r->new_global (playback_location (r, m_loc), > m_kind, > m_type->playback_type (), > - playback_string (m_name))); > + playback_string (m_name)); > + } > + > + if (m_link_section != NULL) > + global->set_link_section (m_link_section->c_str ()); > + > + set_playback_obj (global); > } Looks like the patch as posted has some interaction with the initializers patch, presumably that's the ABI_17? But presumably the intent here is this hunk: + if (m_link_section != NULL) + global->set_link_section (m_link_section->c_str ()); and that indeed looks good. > diff --git a/gcc/jit/libgccjit.c b/gcc/jit/libgccjit.c > index c744b634f4b..5b413d41e2c 100644 > --- a/gcc/jit/libgccjit.c > +++ b/gcc/jit/libgccjit.c > @@ -2217,6 +2217,18 @@ gcc_jit_lvalue_get_address (gcc_jit_lvalue *lvalue, > return (gcc_jit_rvalue *)lvalue->get_address (loc); > } > > +/* Public entrypoint. See description in libgccjit.h. > + > + After error-checking, the real work is done by the > + gcc::jit::recording::lvalue::set_link_section method in jit-recording.c. */ > +void > +gcc_jit_lvalue_set_link_section (gcc_jit_lvalue *lvalue, > + const char *section_name) > +{ > + RETURN_IF_FAIL (name, NULL, NULL, "NULL section_name"); > + lvalue->set_link_section (name); > +} Thanks for renaming the param from "name" to "section_name", but it looks like you didn't rename the uses of it within the function - I'm guessing this code as-is doesn't compile. [...snip...] > diff --git a/gcc/jit/libgccjit.map b/gcc/jit/libgccjit.map > index 64e790949e8..e5bdafd0156 100644 > --- a/gcc/jit/libgccjit.map > +++ b/gcc/jit/libgccjit.map > @@ -226,3 +226,11 @@ LIBGCCJIT_ABI_16 { > gcc_jit_type_is_struct; > gcc_jit_struct_get_field_count; > } LIBGCCJIT_ABI_15; > + > +LIBGCCJIT_ABI_17 { > +} LIBGCCJIT_ABI_16; > + Same notes here as above. [...snip...] Overall the patch looks good, my comments are all just nit-picks at this point, except that obviously you need to finish updating the symbol name to "section_name" in gcc_jit_lvalue_set_link_section so that it compiles. Please check it compiles and that the testsuite passes before pushing - though I think this is waiting on another patch, right? This is OK for trunk, once those fixes and prerequisites are taken care of. Thanks again Dave