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

[David Malcolm <dmalcolm@redhat.com>]

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?

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