Thanks for your reviews! Here's the updated patch, ready for another review. See comments/questions below. 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: > > > diff --git a/gcc/jit/docs/topics/expressions.rst > b/gcc/jit/docs/topics/expressions.rst > > index 396259ef07e..b39f6c02527 100644 > > --- a/gcc/jit/docs/topics/expressions.rst > > +++ b/gcc/jit/docs/topics/expressions.rst > > @@ -539,6 +539,18 @@ where the rvalue is computed by reading from > > the > storage area. > >   > >     in C. > >   > > +.. function:: void > > +              gcc_jit_lvalue_set_link_section (gcc_jit_lvalue > *lvalue, > > +                                               const char *name) > > + > > +   Set the link section of a variable; analogous to: > > + > > +   .. code-block:: c > > + > > +     int variable __attribute__((section(".section"))); > > + > > +   in C. > > Please rename param "name" to "section_name".  Your implementation > requires that it be non-NULL (rather than having NULL unset the > section), so please specify that it must be non-NULL in the docs. > > Please add the usual "This entrypoint was added in" text to state > which > API version it was added in. > > > + > >  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. > > > diff --git a/gcc/jit/jit-recording.c b/gcc/jit/jit-recording.c > > index 117ff70114c..d54f878cc6b 100644 > > --- a/gcc/jit/jit-recording.c > > +++ b/gcc/jit/jit-recording.c > > @@ -3713,6 +3713,11 @@ recording::lvalue::get_address > (recording::location *loc) > >    return result; > >  } > >   > > +void recording::lvalue::set_link_section (const char *name) > > +{ > > +  m_link_section = new_string (name); > > +} > > + > >  /* The implementation of class gcc::jit::recording::param.  */ > >   > >  /* Implementation of pure virtual hook > recording::memento::replay_into > > @@ -4547,8 +4552,7 @@ recording::block::dump_edges_to_dot > (pretty_printer *pp) > >  void > >  recording::global::replay_into (replayer *r) > >  { > > -  set_playback_obj ( > > -    m_initializer > > +  playback::lvalue *global = m_initializer > >      ? r->new_global_initialized (playback_location (r, m_loc), > >                                  m_kind, > >                                  m_type->playback_type (), > > @@ -4560,7 +4564,12 @@ recording::global::replay_into (replayer *r) > >      : 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()); > > +  } > > Coding convention nits: don't use {} when it's just one statement > (which I think is a bad convention, but it is the project's > convention). > Missing spaces between function name and open-paren in both calls > here. > > > > +  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. > > > > 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? > > > +void > > +gcc_jit_lvalue_set_link_section (gcc_jit_lvalue *lvalue, > > +                           const char *name) > > +{ > > +  RETURN_IF_FAIL (name, NULL, NULL, "NULL name"); > > +  lvalue->set_link_section(name); > > Missing a space between function name and open-paren. > > > > diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h > > index 5c722c2c57f..21553ede3de 100644 > > --- a/gcc/jit/libgccjit.h > > +++ b/gcc/jit/libgccjit.h > > @@ -1072,6 +1072,19 @@ extern gcc_jit_rvalue * > >  gcc_jit_lvalue_get_address (gcc_jit_lvalue *lvalue, > >                             gcc_jit_location *loc); > >   > > +#define LIBGCCJIT_HAVE_gcc_jit_lvalue_set_link_section > > + > > +/* Set the link section of a global variable; analogous to: > > +     __attribute__((section("section_name"))) > > +   in C. > > + > > +   This API entrypoint was added in LIBGCCJIT_ABI_18; you can test > for its > > +   presence using > > +     #ifdef LIBGCCJIT_HAVE_gcc_jit_lvalue_set_tls_model  */ > > Wrong #ifdef in the comment. > > > +extern void > > +gcc_jit_lvalue_set_link_section (gcc_jit_lvalue *lvalue, > > +                           const char *name); > > Rename param "name" to "section_name" to match the comment. > > > diff --git a/gcc/jit/libgccjit.map b/gcc/jit/libgccjit.map > > index 337ea6c7fe4..9e722c2bde1 100644 > > --- a/gcc/jit/libgccjit.map > > +++ b/gcc/jit/libgccjit.map > > @@ -205,3 +205,14 @@ LIBGCCJIT_ABI_15 { > >      gcc_jit_extended_asm_add_clobber; > >      gcc_jit_context_add_top_level_asm; > >  } LIBGCCJIT_ABI_14; > > + > > +LIBGCCJIT_ABI_16 { > > +} LIBGCCJIT_ABI_15; > > + > > +LIBGCCJIT_ABI_17 { > > +} LIBGCCJIT_ABI_16; > > + > > +LIBGCCJIT_ABI_18 { > > +  global: > > +    gcc_jit_lvalue_set_link_section; > > +} LIBGCCJIT_ABI_17; > > I have some other patches of yours to review (presumably where the > other ABI things are); sorry about that.  I'll try to get to them > today. > > > > 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 > > + > >  /* test-hello-world.c */ > >  #define create_code create_code_hello_world > >  #define verify_code verify_code_hello_world > > diff --git a/gcc/testsuite/jit.dg/test-link-section.c > b/gcc/testsuite/jit.dg/test-link-section.c > > new file mode 100644 > > index 00000000000..546c1e95b92 > > --- /dev/null > > +++ b/gcc/testsuite/jit.dg/test-link-section.c > > @@ -0,0 +1,25 @@ > > +#include > > +#include > > + > > +#include "libgccjit.h" > > + > > +#include "harness.h" > > + > > +void > > +create_code (gcc_jit_context *ctxt, void *user_data) > > +{ > > +  /* Let's try to inject the equivalent of: > > +     int foo __attribute__((section(".section"))); > > +  */ > > +  gcc_jit_type *int_type = > > +    gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_INT); > > +  gcc_jit_lvalue *foo = > > +    gcc_jit_context_new_global ( > > +      ctxt, NULL, GCC_JIT_GLOBAL_EXPORTED, int_type, "foo"); > > +  gcc_jit_lvalue_set_link_section(foo, "section"); > > +} > > + > > +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. > > 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. > > > Hope this is constructive > Dave >