Re: [PATCH][gcc] libgccjit: introduce gcc_jit_context_add_driver_option

[Kyrill Tkachov <kyrylo.tkachov@foss.arm.com>]

Hi David,

On 19/01/19 01:36, David Malcolm wrote:
> On Fri, 2019-01-18 at 19:25 +0000, Andrea Corallo wrote:
> > Hi all,
> > this patch add gcc_jit_context_add_driver_option to the libgccjit ABI
> > and a testcase for it.
> >
> > Using this interface is now possible to pass options affecting
> > assembler and linker.
> >
> > Does not introduce any new regression running make check-jit.
> >
> > Bests
> >
> >   Andrea
> >
> >
> > gcc/jit/ChangeLog
> > 2019-01-16  Andrea Corallo  andrea.corallo@arm.com
> >
> > * docs/topics/compatibility.rst (LIBGCCJIT_ABI_11): New ABI tag.
> > * docs/topics/contexts.rst (Additional driver options): New
> > section.
> > * jit-playback.c (invoke_driver): Add call to append_driver_options.
> > * jit-recording.c: Within namespace gcc::jit...
> > (recording::context::~context): Free the optnames within
> > m_driver_options.
> > (recording::context::add_driver_option): New method.
> > (recording::context::append_driver_options): New method.
> > (recording::context::dump_reproducer_to_file): Add driver
> > options.
> > * jit-recording.h: Within namespace gcc::jit...
> > (recording::context::add_driver_option): New method.
> > (recording::context::append_driver_options): New method.
> > (recording::context::m_driver_options): New field.
> > * libgccjit++.h (gccjit::context::add_driver_option): New
> > method.
> > * libgccjit.c (gcc_jit_context_add_driver_option): New API
> > entrypoint.
> > * libgccjit.h (gcc_jit_context_add_driver_option): New API
> > entrypoint.
> > (LIBGCCJIT_HAVE_gcc_jit_context_add_driver_option): New
> > macro.
> > * libgccjit.map (LIBGCCJIT_ABI_11): New ABI tag.
> >
> >
> > gcc/testsuite/ChangeLog
> > 2019-01-16  Andrea Corallo  andrea.corallo@arm.com
> >
> > * jit.dg/add-driver-options-testlib.c: Add support file for
> > test-add-driver-options.c testcase.
> > * jit.dg/all-non-failing-tests.h: Add test-add-driver-options.c
> > * jit.dg/jit.exp (jit-dg-test): Update to support
> > add-driver-options-testlib.c compilation.
> > * jit.dg/test-add-driver-options.c: New testcase.
>
> Thanks for this patch.
>
> One nit:
>
> [...snip...]
>
> > diff --git a/gcc/testsuite/jit.dg/all-non-failing-tests.h b/gcc/testsuite/jit.dg/all-non-failing-tests.h
> > index bf02e12..9f816b4 100644
> > --- a/gcc/testsuite/jit.dg/all-non-failing-tests.h
> > +++ b/gcc/testsuite/jit.dg/all-non-failing-tests.h
> > @@ -251,6 +251,13 @@
> >  #undef create_code
> >  #undef verify_code
> >
> > +/* test-add-driver-options.c */
> > +#define create_code create_code_add_driver_options
> > +#define verify_code verify_code_add_driver_options
> > +#include "test-add-driver-options.c"
> > +#undef create_code
> > +#undef verify_code
> > +
> >  /* Now expose the individual testcases as instances of this struct.  */
> >
> >  struct testcase
>
> The purpose of the above file is to allow for copies of tests to be
> built into test-combination.c and test-threads.c (to shake out state-
> handling bugs).  If you're going to embed the test into those, then
> they'd also need to be added to the "testcases" array towards the end
> of that header.
>
> But given that the new test adds options that affect the whole context,
> it's probably best not to embed it into those combined tests.  Instead,
> add a comment to all-non-failing-tests.h, similar to the one that
> reads:
>
>   /* test-extra-options.c: We don't use this one, since the extra options
>      affect the whole context.  */
>
> (changing the filename, of course).
>
> Other than that the patch looks good.
>
> Do you have your copyright assignment paperwork in place?
>

I believe Andrea's copyright assignment should be covered by the copyright assignment from Arm.

Thanks,
Kyrill

> Also, we're currently in stage 4 of development for gcc 9, so adding a
> feature to libgccjit probably requires Release Manager approval.
> (Given the recent discussion on the jit mailing list, this might not be
> the only late-breaking jit patch).
>
> Dave