On Mon, 2019-06-24 at 15:26 +0000, Andrea Corallo wrote:
> Hi all,
> second version here of the gcc_jit_context_new_bitfield patch
> addressing
> review comments.
>
> Checked with make check-jit runs clean.
>
> Bests
>
> Andrea
>
> 2019-06-20 Andrea Corallo andrea.cora...@arm.com
>
> * docs/topics/compatibility.rst (LIBGCCJIT_ABI_12): New ABI tag.
> * docs/topics/types.rst: Add gcc_jit_context_new_bitfield.
> * jit-common.h (namespace recording): Add class bitfield.
> * jit-playback.c:
> (DECL_C_BIT_FIELD, SET_DECL_C_BIT_FIELD): Add macros.
> (playback::context::new_bitfield): New method.
> (playback::compound_type::set_fields): Add bitfield support.
> (playback::lvalue::mark_addressable): Was jit_mark_addressable make
> this
> a method of lvalue plus return a bool to communicate success.
> (playback::lvalue::get_address): Check for jit_mark_addressable
> return
> value.
> * jit-playback.h (new_bitfield): New method.
> (class bitfield): New class.
> (class lvalue): Add jit_mark_addressable method.
> * jit-recording.c (recording::context::new_bitfield): New method.
> (recording::bitfield::replay_into): New method.
> (recording::bitfield::write_to_dump): Likewise.
> (recording::bitfield::make_debug_string): Likewise.
> (recording::bitfield::write_reproducer): Likewise.
> * jit-recording.h (class context): Add new_bitfield method.
> (class field): Make it derivable by class bitfield.
> (class bitfield): Add new class.
> * libgccjit++.h (class context): Add new_bitfield method.
> * libgccjit.c (struct gcc_jit_bitfield): New structure.
> (gcc_jit_context_new_bitfield): New function.
> * libgccjit.h
> (LIBGCCJIT_HAVE_gcc_jit_context_new_bitfield) New macro.
> (gcc_jit_context_new_bitfield): New function.
> * libgccjit.map (LIBGCCJIT_ABI_12) New ABI tag.
>
>
> 2019-06-20 Andrea Corallo andrea.cora...@arm.com
>
> * jit.dg/all-non-failing-tests.h: Add test-accessing-bitfield.c.
> * jit.dg/test-accessing-bitfield.c: New testcase.
> * jit.dg/test-error-gcc_jit_context_new_bitfield-invalid-type.c:
> Likewise.
> * jit.dg/test-error-gcc_jit_context_new_bitfield-invalid-width.c:
> Likewise.
> * jit.dg/test-error-gcc_jit_lvalue_get_address-bitfield.c:
> Likewise.
Thanks for the updated patch.
[...]
> diff --git a/gcc/jit/jit-playback.c b/gcc/jit/jit-playback.c
> index b74495c..a6a9e2d 100644
> --- a/gcc/jit/jit-playback.c
> +++ b/gcc/jit/jit-playback.c
> @@ -47,6 +47,12 @@ along with GCC; see the file COPYING3. If not see
> #include "jit-builtins.h"
> #include "jit-tempdir.h"
>
> +/* Compare with gcc/c-family/c-common.h
> + This is redifined here to avoid depending from the C frontend. */
> +#define DECL_C_BIT_FIELD(NODE) \
> + (DECL_LANG_FLAG_4 (FIELD_DECL_CHECK (NODE)) == 1)
> +#define SET_DECL_C_BIT_FIELD(NODE) \
> + (DECL_LANG_FLAG_4 (FIELD_DECL_CHECK (NODE)) = 1)
Can you rename these (and their users) from
*_C_BIT_FIELD
to
*_JIT_BIT_FIELD
(and update the comment please).
Nit: "redifined" -> "redefined".
[...]
> diff --git a/gcc/testsuite/jit.dg/all-non-failing-tests.h
> b/gcc/testsuite/jit.dg/all-non-failing-tests.h
> index 9a10418..f7af8e9 100644
> --- a/gcc/testsuite/jit.dg/all-non-failing-tests.h
> +++ b/gcc/testsuite/jit.dg/all-non-failing-tests.h
> @@ -8,6 +8,13 @@
> hooks provided by each test case. */
> #define COMBINED_TEST
>
> +/* test-accessing-bitfield.c */
> +#define create_code create_code_accessing_bitfield
> +#define verify_code verify_code_accessing_bitfield
> +#include "test-accessing-bitfield.c"
> +#undef create_code
> +#undef verify_code
> +
> /* test-accessing-struct.c */
> #define create_code create_code_accessing_struct
> #define verify_code verify_code_accessing_struct
You should also add an entry containing
create_code_accessing_bitfield
verify_code_accessing_bitfield
to the "testcases" array at the bottom of the file, so that the new
tests are also run as part of test-combination.c and test-threads.c;
hopefully there are no conflicts with existing tests in the suite (e.g.
names of generated functions within the gcc_jit_context).
[...]
> diff --git
> a/gcc/testsuite/jit.dg/test-error-gcc_jit_context_new_bitfield-invalid-width.c
>
> b/gcc/testsuite/jit.dg/test-error-gcc_jit_context_new_bitfield-invalid-width.c
> new file mode 100644
> index 0000000..6cb151b
> --- /dev/null
> +++
> b/gcc/testsuite/jit.dg/test-error-gcc_jit_context_new_bitfield-invalid-width.c
> @@ -0,0 +1,40 @@
> +#include <stdlib.h>
> +#include <stdio.h>
> +
> +#include "libgccjit.h"
> +
> +#include "harness.h"
> +
> +/* Try to declare a bit-field with invalid width. */
> +
> +void
> +create_code (gcc_jit_context *ctxt, void *user_data)
> +{
> + gcc_jit_type *short_type =
> + gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_SHORT);
> + gcc_jit_field *i =
> + gcc_jit_context_new_bitfield (ctxt,
> + NULL,
> + short_type,
> + 3,
> + "i");
> + gcc_jit_field *j =
> + gcc_jit_context_new_bitfield (ctxt,
> + NULL,
> + short_type,
> + 57,
> + "j");
> + gcc_jit_field *fields[] = {i, j};
> + gcc_jit_context_new_struct_type (ctxt, NULL, "bit_foo", 2, fields);
> +}
> +
> +void
> +verify_code (gcc_jit_context *ctxt, gcc_jit_result *result)
> +{
> + CHECK_VALUE (result, NULL);
> +
> + /* Verify that the correct error message was emitted. */
> + CHECK_STRING_VALUE (gcc_jit_context_get_first_error (ctxt),
> + "width of bit-field j (width: 57) is wider than its type "
> + "(width: 16)");
I'm slightly nervous here that the test is hardcoding that a short is
16 bits
(AFAIK, we guarantee that a short >= 16 bits; I'm not sure if we
guarantee that it's equal to 16 bits).
> diff --git
> a/gcc/testsuite/jit.dg/test-error-gcc_jit_lvalue_get_address-bitfield.c
> b/gcc/testsuite/jit.dg/test-error-gcc_jit_lvalue_get_address-bitfield.c
> new file mode 100644
> index 0000000..35a7c8b
> --- /dev/null
> +++ b/gcc/testsuite/jit.dg/test-error-gcc_jit_lvalue_get_address-bitfield.c
> @@ -0,0 +1,77 @@
> +#include <stdlib.h>
> +#include <stdio.h>
> +
> +#include "libgccjit.h"
> +
> +#include "harness.h"
> +
> +/* Try to dereference a bit-field. */
> +
> +void
> +create_code (gcc_jit_context *ctxt, void *user_data)
> +{
> + /* Let's try to inject the equivalent of:
> +
> + struct bit_foo
> + {
> + int i:3;
> + int j:3;
> + };
> +
> + int *
> + test_access (struct bit_foo *f)
> + {
> + return &(f->j);
> + }
> + */
> + gcc_jit_type *int_type =
> + gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_INT);
> + gcc_jit_field *i =
> + gcc_jit_context_new_bitfield (ctxt,
> + NULL,
> + int_type,
> + 3,
> + "i");
> + gcc_jit_field *j =
> + gcc_jit_context_new_bitfield (ctxt,
> + NULL,
> + int_type,
> + 3,
> + "j");
> + gcc_jit_field *fields[] = {i, j};
> + gcc_jit_struct *struct_type =
> + gcc_jit_context_new_struct_type (ctxt, NULL, "bit_foo", 2, fields);
> + gcc_jit_type *ptr_type =
> + gcc_jit_type_get_pointer (gcc_jit_struct_as_type (struct_type));
> +
> + /* Build the test function. */
> + gcc_jit_param *param_f =
> + gcc_jit_context_new_param (ctxt, NULL, ptr_type, "f");
> + gcc_jit_function *test_fn =
> + gcc_jit_context_new_function (ctxt, NULL,
> + GCC_JIT_FUNCTION_EXPORTED,
> + gcc_jit_type_get_pointer (int_type),
> + "test_access",
> + 1, ¶m_f,
> + 0);
> + gcc_jit_block *block = gcc_jit_function_new_block (test_fn, NULL);
> + gcc_jit_block_end_with_return (
> + block,
> + NULL,
> + gcc_jit_lvalue_get_address (
> + gcc_jit_rvalue_dereference_field (
> + gcc_jit_param_as_rvalue (param_f),
> + NULL,
> + j),
> + NULL));
> +}
> +
> +void
> +verify_code (gcc_jit_context *ctxt, gcc_jit_result *result)
> +{
> + CHECK_VALUE (result, NULL);
> +
> + /* Verify that the correct error message was emitted. */
> + CHECK_STRING_VALUE (gcc_jit_context_get_first_error (ctxt),
> + "cannot take address of bit-field");
> +}
As noted in the other review, this testcase could be simplified by just
calling gcc_jit_lvalue_get_address directly, without building a
function.
Dave
