* [PATCH] libgccjit: Add new gcc_jit_context_new_blob entry point @ 2020-06-03 10:11 Andrea Corallo 2020-06-03 15:03 ` David Malcolm 0 siblings, 1 reply; 14+ messages in thread From: Andrea Corallo @ 2020-06-03 10:11 UTC (permalink / raw) To: gcc-patches, jit; +Cc: nd, David Malcolm [-- Attachment #1: Type: text/plain, Size: 2474 bytes --] Hi all, I'd like to submit this patch to extend libgccjit to allow for storing binary blobs (equivalent to initialized char arrays). The use-case is when libgccjit is used as a back-end for dynamic programming languages. In this case is often necessary to store serialized objects into the compilation unit as support to the generated code. The proposed entry point is the following: gcc_jit_lvalue * gcc_jit_context_new_blob (gcc_jit_context *ctxt, gcc_jit_location *loc, enum gcc_jit_global_kind kind, const void *ptr, size_t size, const char *name); This create a special kind of libgccjit global that will be initialized with the memory content pointed by 'ptr'. I've added a testcase and the regression is clean. I've also tested it with the libgccjit based Emacs disabling the current workaround we have for this. Feedback is very welcome. Andrea gcc/jit/ChangeLog 2020-06-02 Andrea Corallo <andrea.corallo@arm.com> * docs/topics/compatibility.rst (LIBGCCJIT_ABI_14): New ABI tag. * docs/topics/expressions.rst (Binary Blobs): New section documenting gcc_jit_context_new_blob. * jit-common.h: Document class blob. * jit-playback.c (playback::context::global_new_decl) (playback::context::global_finalize_lvalue): New methods. (playback::context::new_global): Make use of global_new_decl, global_finalize_lvalue. (playback::context::new_blob): New method. * jit-playback.h (new_blob, global_new_decl): New method declarations. * jit-recording.c (recording::context::new_blob) (recording::blob::replay_into) (recording::global::write_qualifier_to_dump): New methods. (recording::global::write_to_dump): Use write_qualifier_to_dump. (recording::blob::write_to_dump): New method. * jit-recording.h (class blob): New class. (class global): Have m_kind and m_name as protected. (class global): Remove FINAL qualifier to replay_into and write_to_dump. (class global): New method write_qualifier_to_dump decl. (class context): New method new_blob decl. * libgccjit++.h (context::new_blob): New method. * libgccjit.c (gcc_jit_lvalue_as_rvalue): New function. * libgccjit.h (gcc_jit_context_new_blob): New function declaration. (LIBGCCJIT_HAVE_gcc_jit_context_new_blob): New macro. * libgccjit.map (LIBGCCJIT_ABI_14): New ABI tag. gcc/testsuite/ChangeLog 2020-06-02 Andrea Corallo <andrea.corallo@arm.com> * jit.dg/all-non-failing-tests.h: Add test-blob.c. * jit.dg/test-blob.c: New test. [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: 0001-Add-new-gcc_jit_context_new_blob-entry-point.patch --] [-- Type: text/x-diff, Size: 20317 bytes --] From b861fbbfbcbf18cc1b69dc4b7f91c596af40c8e4 Mon Sep 17 00:00:00 2001 From: Andrea Corallo <andrea.corallo@arm.com> Date: Sat, 30 May 2020 10:33:08 +0100 Subject: [PATCH] Add new gcc_jit_context_new_blob entry point gcc/jit/ChangeLog 2020-06-02 Andrea Corallo <andrea.corallo@arm.com> * docs/topics/compatibility.rst (LIBGCCJIT_ABI_14): New ABI tag. * docs/topics/expressions.rst (Binary Blobs): New section documenting gcc_jit_context_new_blob. * jit-common.h: Document class blob. * jit-playback.c (playback::context::global_new_decl) (playback::context::global_finalize_lvalue): New methods. (playback::context::new_global): Make use of global_new_decl, global_finalize_lvalue. (playback::context::new_blob): New method. * jit-playback.h (new_blob, global_new_decl): New method declarations. * jit-recording.c (recording::context::new_blob) (recording::blob::replay_into) (recording::global::write_qualifier_to_dump): New methods. (recording::global::write_to_dump): Use write_qualifier_to_dump. (recording::blob::write_to_dump): New method. * jit-recording.h (class blob): New class. (class global): Have m_kind and m_name as protected. (class global): Remove FINAL qualifier to replay_into and write_to_dump. (class global): New method write_qualifier_to_dump decl. (class context): New method new_blob decl. * libgccjit++.h (context::new_blob): New method. * libgccjit.c (gcc_jit_lvalue_as_rvalue): New function. * libgccjit.h (gcc_jit_context_new_blob): New function declaration. (LIBGCCJIT_HAVE_gcc_jit_context_new_blob): New macro. * libgccjit.map (LIBGCCJIT_ABI_14): New ABI tag. gcc/testsuite/ChangeLog 2020-06-02 Andrea Corallo <andrea.corallo@arm.com> * jit.dg/all-non-failing-tests.h: Add test-blob.c. * jit.dg/test-blob.c: New test. --- gcc/jit/docs/topics/compatibility.rst | 5 ++ gcc/jit/docs/topics/expressions.rst | 49 ++++++++++ gcc/jit/jit-common.h | 1 + gcc/jit/jit-playback.c | 67 ++++++++++++-- gcc/jit/jit-playback.h | 16 ++++ gcc/jit/jit-recording.c | 95 +++++++++++++++++--- gcc/jit/jit-recording.h | 48 +++++++++- gcc/jit/libgccjit++.h | 21 +++++ gcc/jit/libgccjit.c | 30 +++++++ gcc/jit/libgccjit.h | 17 ++++ gcc/jit/libgccjit.map | 7 +- gcc/testsuite/jit.dg/all-non-failing-tests.h | 7 ++ gcc/testsuite/jit.dg/test-blob.c | 54 +++++++++++ 13 files changed, 393 insertions(+), 24 deletions(-) create mode 100644 gcc/testsuite/jit.dg/test-blob.c diff --git a/gcc/jit/docs/topics/compatibility.rst b/gcc/jit/docs/topics/compatibility.rst index 0c0ce070d722..3d9684c73b4b 100644 --- a/gcc/jit/docs/topics/compatibility.rst +++ b/gcc/jit/docs/topics/compatibility.rst @@ -215,3 +215,8 @@ entrypoints: * :func:`gcc_jit_version_minor` * :func:`gcc_jit_version_patchlevel` + +``LIBGCCJIT_ABI_14`` +-------------------- +``LIBGCCJIT_ABI_14`` covers the addition of +:func:`gcc_jit_context_new_blob` diff --git a/gcc/jit/docs/topics/expressions.rst b/gcc/jit/docs/topics/expressions.rst index db2f2ca2e9c6..deb46594d80a 100644 --- a/gcc/jit/docs/topics/expressions.rst +++ b/gcc/jit/docs/topics/expressions.rst @@ -576,6 +576,55 @@ Global variables referring to it. Analogous to using an "extern" global from a header file. +Binary Blobs +**************** + +.. function:: gcc_jit_context_new_blob (gcc_jit_context *ctxt,\ + gcc_jit_location *loc,\ + enum gcc_jit_global_kind kind,\ + const void *ptr,\ + size_t size,\ + const char *name); + + Add a new global variable equivalent to an initialized array of + char to the context. + + The parameter ``name`` must be non-NULL. The call takes a copy of the + underlying string, so it is valid to pass in a pointer to an on-stack + buffer. + + Similarly the parameter ``ptr`` must be non-NULL. The call copies + the memory content pointed by ``ptr`` for ``size`` bytes. This + content will be stores in the compilation unit and used as + initialization value of the array. + + .. c:macro:: GCC_JIT_GLOBAL_EXPORTED + + Global is defined by the client code and is visible + by name outside of this JIT context via + :c:func:`gcc_jit_result_get_global` (and this value is required for + the global to be accessible via that entrypoint). + + .. c:macro:: GCC_JIT_GLOBAL_INTERNAL + + Global is defined by the client code, but is invisible + outside of it. Analogous to a "static" global within a .c file. + Specifically, the variable will only be visible within this + context and within child contexts. + + .. c:macro:: GCC_JIT_GLOBAL_IMPORTED + + Global is not defined by the client code; we're merely + referring to it. Analogous to using an "extern" global from a + header file. + + This entrypoint was added in :ref:`LIBGCCJIT_ABI_14`; you can test for + its presence using + + .. code-block:: c + + #ifdef LIBGCCJIT_HAVE_gcc_jit_context_new_blob + Working with pointers, structs and unions ----------------------------------------- diff --git a/gcc/jit/jit-common.h b/gcc/jit/jit-common.h index 4570bd2d717f..7a1578d10e89 100644 --- a/gcc/jit/jit-common.h +++ b/gcc/jit/jit-common.h @@ -127,6 +127,7 @@ namespace recording { class lvalue; class local; class global; + class blob; class param; class base_call; class function_pointer; diff --git a/gcc/jit/jit-playback.h b/gcc/jit/jit-playback.h index 074434a9f6b2..1cc674a8ddd9 100644 --- a/gcc/jit/jit-playback.h +++ b/gcc/jit/jit-playback.h @@ -111,6 +111,14 @@ public: type *type, const char *name); + lvalue * + new_blob (location *loc, + enum gcc_jit_global_kind kind, + type *type, + const void *ptr, + size_t size, + const char *name); + template <typename HOST_TYPE> rvalue * new_rvalue_from_const (type *type, @@ -266,6 +274,14 @@ private: const char * get_path_s_file () const; const char * get_path_so_file () const; + tree + global_new_decl (location *loc, + enum gcc_jit_global_kind kind, + type *type, + const char *name); + lvalue * + global_finalize_lvalue (tree inner); + private: /* Functions for implementing "compile". */ diff --git a/gcc/jit/jit-playback.c b/gcc/jit/jit-playback.c index d2c8bb4c154d..8cd986942bc7 100644 --- a/gcc/jit/jit-playback.c +++ b/gcc/jit/jit-playback.c @@ -506,14 +506,14 @@ new_function (location *loc, return func; } -/* Construct a playback::lvalue instance (wrapping a tree). */ +/* In use by new_global and new_blob. */ -playback::lvalue * +tree playback::context:: -new_global (location *loc, - enum gcc_jit_global_kind kind, - type *type, - const char *name) +global_new_decl (location *loc, + enum gcc_jit_global_kind kind, + type *type, + const char *name) { gcc_assert (type); gcc_assert (name); @@ -543,6 +543,15 @@ new_global (location *loc, if (loc) set_tree_location (inner, loc); + return inner; +} + +/* In use by new_global and new_blob. */ + +playback::lvalue * +playback::context:: +global_finalize_lvalue (tree inner) +{ varpool_node::get_create (inner); varpool_node::finalize_decl (inner); @@ -552,6 +561,52 @@ new_global (location *loc, return new lvalue (this, inner); } +/* Construct a playback::lvalue instance (wrapping a tree). */ + +playback::lvalue * +playback::context:: +new_global (location *loc, + enum gcc_jit_global_kind kind, + type *type, + const char *name) +{ + tree inner = global_new_decl (loc, kind, type, name); + + return global_finalize_lvalue (inner); +} + +playback::lvalue * +playback::context:: +new_blob (location *loc, + enum gcc_jit_global_kind kind, + type *type, + const void *ptr, + size_t size, + const char *name) +{ + tree inner = global_new_decl (loc, kind, type, name); + + static vec<constructor_elt, va_gc> *constructor_elements; + const char *p = (const char *)ptr; + for (size_t i = 0; i < size; i++) + { + /* Compare with 'output_init_element' c-typeck.c:9691. */ + constructor_elt celt = + { build_int_cst (long_unsigned_type_node, i), + build_int_cst (char_type_node, p[i]) }; + vec_safe_push (constructor_elements, celt); + } + /* Compare with 'pop_init_level' c-typeck.c:8780. */ + tree ctor = build_constructor (type->as_tree (), constructor_elements); + constructor_elements = NULL; + + /* Compare with 'store_init_value' c-typeck.c:7555. */ + DECL_INITIAL (inner) = ctor; + + return global_finalize_lvalue (inner); +} + + /* Implementation of the various gcc::jit::playback::context::new_rvalue_from_const <HOST_TYPE> methods. diff --git a/gcc/jit/jit-recording.h b/gcc/jit/jit-recording.h index 726b9c4b8371..5e2762ff8785 100644 --- a/gcc/jit/jit-recording.h +++ b/gcc/jit/jit-recording.h @@ -204,6 +204,13 @@ public: rvalue *max_value, block *block); + lvalue * + new_blob (location *loc, + enum gcc_jit_global_kind kind, + const void *ptr, + size_t size, + const char *name); + void set_str_option (enum gcc_jit_str_option opt, const char *value); @@ -1329,11 +1336,14 @@ public: m_name (name) {} - void replay_into (replayer *) FINAL OVERRIDE; + void replay_into (replayer *) OVERRIDE; void visit_children (rvalue_visitor *) FINAL OVERRIDE {} - void write_to_dump (dump &d) FINAL OVERRIDE; + void write_to_dump (dump &d) OVERRIDE; + +protected: + void write_qualifier_to_dump (dump &d); private: string * make_debug_string () FINAL OVERRIDE { return m_name; } @@ -1343,11 +1353,43 @@ private: return PRECEDENCE_PRIMARY; } -private: +protected: enum gcc_jit_global_kind m_kind; string *m_name; }; +class blob : public global +{ +public: + blob (context *ctxt, + location *loc, + enum gcc_jit_global_kind kind, + void *ptr, + size_t size, + string *name) + : global (ctxt, loc, kind, + ctxt->new_array_type (loc, + ctxt->get_type (GCC_JIT_TYPE_CHAR), + size), + name), + m_ptr (ptr), + m_size (size) + {} + + ~blob () + { + free (m_ptr); + } + + void replay_into (replayer *) FINAL OVERRIDE; + + void write_to_dump (dump &d) FINAL OVERRIDE; + +private: + void *m_ptr; + size_t m_size; +}; + template <typename HOST_TYPE> class memento_of_new_rvalue_from_const : public rvalue { diff --git a/gcc/jit/jit-recording.c b/gcc/jit/jit-recording.c index b73cd76a0a02..b23ebdc57d2c 100644 --- a/gcc/jit/jit-recording.c +++ b/gcc/jit/jit-recording.c @@ -1229,6 +1229,29 @@ recording::context::new_case (recording::rvalue *min_value, return result; } +/* Create a recording::blob instance and add it to this context's list + of mementos. + + Implements the post-error-checking part of + gcc_jit_context_new_blob. */ + +recording::lvalue * +recording::context::new_blob (recording::location *loc, + enum gcc_jit_global_kind kind, + const void *ptr, + size_t size, + const char *name) +{ + void *data = xmalloc (size); + recording::blob *result = + new recording::blob (this, loc, kind, memcpy (data, ptr, size), size, + new_string (name)); + record (result); + m_globals.safe_push (result); + + return result; +} + /* Set the given string option for this context, or add an error if it's not recognized. @@ -4399,6 +4422,38 @@ recording::global::replay_into (replayer *r) playback_string (m_name))); } +void +recording::blob::replay_into (replayer *r) +{ + set_playback_obj (r->new_blob (playback_location (r, m_loc), + m_kind, + m_type->playback_type (), + m_ptr, + m_size, + playback_string (m_name))); +} + +void +recording::global::write_qualifier_to_dump (dump &d) +{ + switch (m_kind) + { + default: + gcc_unreachable (); + + case GCC_JIT_GLOBAL_EXPORTED: + break; + + case GCC_JIT_GLOBAL_INTERNAL: + d.write ("static "); + break; + + case GCC_JIT_GLOBAL_IMPORTED: + d.write ("extern "); + break; + } +} + /* Override the default implementation of recording::memento::write_to_dump for globals. This will be of the form: @@ -4424,25 +4479,37 @@ recording::global::write_to_dump (dump &d) if (d.update_locations ()) m_loc = d.make_location (); - switch (m_kind) - { - default: - gcc_unreachable (); + write_qualifier_to_dump (d); - case GCC_JIT_GLOBAL_EXPORTED: - break; + d.write ("%s %s;\n", + m_type->get_debug_string (), + get_debug_string ()); +} - case GCC_JIT_GLOBAL_INTERNAL: - d.write ("static "); - break; - case GCC_JIT_GLOBAL_IMPORTED: - d.write ("extern "); - break; - } - d.write ("%s %s;\n", +/* Override the default implementation of + recording::memento::write_to_dump for blobs. */ + +void +recording::blob::write_to_dump (dump &d) +{ + if (d.update_locations ()) + m_loc = d.make_location (); + + write_qualifier_to_dump (d); + + d.write ("%s %s =\n { ", m_type->get_debug_string (), get_debug_string ()); + + const char *p = (const char *)m_ptr; + for (size_t i = 0; i < m_size; i++) + { + d.write ("0x%x, ", p[i]); + if (i && !(i % 64)) + d.write ("\n "); + } + d.write ("};\n"); } /* A table of enum gcc_jit_global_kind values expressed in string diff --git a/gcc/jit/libgccjit++.h b/gcc/jit/libgccjit++.h index 69e67766640c..bb663e9aca1f 100644 --- a/gcc/jit/libgccjit++.h +++ b/gcc/jit/libgccjit++.h @@ -182,6 +182,12 @@ namespace gccjit const std::string &name, location loc = location ()); + lvalue new_blob (enum gcc_jit_global_kind kind, + const void *ptr, + size_t size, + const std::string &name, + location loc = location ()); + rvalue new_rvalue (type numeric_type, int value) const; rvalue new_rvalue (type numeric_type, @@ -860,6 +866,21 @@ context::new_global (enum gcc_jit_global_kind kind, name.c_str ())); } +inline lvalue +context::new_blob (enum gcc_jit_global_kind kind, + const void *ptr, + size_t size, + const std::string &name, + location loc) +{ + return lvalue (gcc_jit_context_new_blob (m_inner_ctxt, + loc.get_inner_location (), + kind, + ptr, + size, + name.c_str ())); +} + inline rvalue context::new_rvalue (type numeric_type, int value) const diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h index 1c5a12e9c015..a66be3568fa0 100644 --- a/gcc/jit/libgccjit.h +++ b/gcc/jit/libgccjit.h @@ -788,6 +788,23 @@ gcc_jit_context_new_global (gcc_jit_context *ctxt, gcc_jit_type *type, const char *name); +#define LIBGCCJIT_HAVE_gcc_jit_context_new_blob + +/* Create a binary blob. + + This API entrypoint was added in LIBGCCJIT_ABI_14; you can test for its + presence using + #ifdef LIBGCCJIT_HAVE_gcc_jit_context_new_blob +*/ + +extern gcc_jit_lvalue * +gcc_jit_context_new_blob (gcc_jit_context *ctxt, + gcc_jit_location *loc, + enum gcc_jit_global_kind kind, + const void *ptr, + size_t size, + const char *name); + /* Upcasting. */ extern gcc_jit_object * gcc_jit_lvalue_as_object (gcc_jit_lvalue *lvalue); diff --git a/gcc/jit/libgccjit.c b/gcc/jit/libgccjit.c index a29e9885e59b..a504d568b388 100644 --- a/gcc/jit/libgccjit.c +++ b/gcc/jit/libgccjit.c @@ -1097,6 +1097,36 @@ gcc_jit_context_new_global (gcc_jit_context *ctxt, return (gcc_jit_lvalue *)ctxt->new_global (loc, kind, type, name); } +/* Public entrypoint. See description in libgccjit.h. + + After error-checking, the real work is done by the + gcc::jit::recording::context::new_blob method, in + jit-recording.c. */ + +gcc_jit_lvalue * +gcc_jit_context_new_blob (gcc_jit_context *ctxt, + gcc_jit_location *loc, + enum gcc_jit_global_kind kind, + const void *ptr, + size_t size, + const char *name) +{ + RETURN_NULL_IF_FAIL (ctxt, NULL, loc, "NULL context"); + JIT_LOG_FUNC (ctxt->get_logger ()); + /* LOC can be NULL. */ + RETURN_NULL_IF_FAIL_PRINTF1 ( + ((kind >= GCC_JIT_GLOBAL_EXPORTED) + && (kind <= GCC_JIT_GLOBAL_IMPORTED)), + ctxt, loc, + "unrecognized value for enum gcc_jit_global_kind: %i", + kind); + RETURN_NULL_IF_FAIL (name, ctxt, loc, "NULL ptr"); + /* SIZE can be zero. */ + RETURN_NULL_IF_FAIL (name, ctxt, loc, "NULL name"); + + return (gcc_jit_lvalue *)ctxt->new_blob (loc, kind, ptr, size, name); +} + /* Public entrypoint. See description in libgccjit.h. After error-checking, this calls the trivial diff --git a/gcc/jit/libgccjit.map b/gcc/jit/libgccjit.map index 6137dd4b4b03..40c3605310d8 100644 --- a/gcc/jit/libgccjit.map +++ b/gcc/jit/libgccjit.map @@ -186,4 +186,9 @@ LIBGCCJIT_ABI_13 { gcc_jit_version_major; gcc_jit_version_minor; gcc_jit_version_patchlevel; -} LIBGCCJIT_ABI_12; \ No newline at end of file +} LIBGCCJIT_ABI_12; + +LIBGCCJIT_ABI_14 { + global: + gcc_jit_context_new_blob; +} LIBGCCJIT_ABI_13; diff --git a/gcc/testsuite/jit.dg/all-non-failing-tests.h b/gcc/testsuite/jit.dg/all-non-failing-tests.h index ad469dad6994..7eaf373e6ed8 100644 --- a/gcc/testsuite/jit.dg/all-non-failing-tests.h +++ b/gcc/testsuite/jit.dg/all-non-failing-tests.h @@ -67,6 +67,13 @@ #undef create_code #undef verify_code +/* test-blob.c */ +#define create_code create_code_blob +#define verify_code verify_code_blob +#include "test-blob.c" +#undef create_code +#undef verify_code + /* test-calling-external-function.c */ #define create_code create_code_calling_external_function #define verify_code verify_code_calling_external_function diff --git a/gcc/testsuite/jit.dg/test-blob.c b/gcc/testsuite/jit.dg/test-blob.c new file mode 100644 index 000000000000..d344975dd5a7 --- /dev/null +++ b/gcc/testsuite/jit.dg/test-blob.c @@ -0,0 +1,54 @@ +#include <stdlib.h> +#include <stdio.h> +#include <string.h> +#include <time.h> + +#include "libgccjit.h" + +#include "harness.h" + +#define BIG_BLOB_SIZE (1 << 12) /* 4KB. */ + +static char test_blob1[] = { 0xc, 0xa, 0xf, 0xf, 0xe }; +static char test_blob2[] = { 0x3, 0x2, 0x1, 0x0, 0x1, 0x2, 0x3 }; +static char test_blob3[BIG_BLOB_SIZE]; + +void +create_code (gcc_jit_context *ctxt, void *user_data) +{ + /* Let's try to inject the equivalent of: + + char bin_blob1[] = { 0xc, 0xa, 0xf, 0xf, 0xe }; + char bin_blob2[] = { 0x3, 0x2, 0x1, 0x0, 0x1, 0x2, 0x3 }; + char bin_blob3[4096]... + */ + + gcc_jit_context_new_blob (ctxt, NULL, GCC_JIT_GLOBAL_EXPORTED, test_blob1, + sizeof (test_blob1), "bin_blob1"); + + gcc_jit_context_new_blob (ctxt, NULL, GCC_JIT_GLOBAL_EXPORTED, test_blob2, + sizeof (test_blob2), "bin_blob2"); + + for (size_t i = 0; i < BIG_BLOB_SIZE; i++) + test_blob3[i] = i * i + i; + gcc_jit_context_new_blob (ctxt, NULL, GCC_JIT_GLOBAL_EXPORTED, test_blob3, + sizeof (test_blob3), "bin_blob3"); +} + +void +verify_code (gcc_jit_context *ctxt, gcc_jit_result *result) +{ + CHECK_NON_NULL (result); + void *blob = gcc_jit_result_get_global (result, "bin_blob1"); + CHECK_NON_NULL (blob); + CHECK_VALUE (memcmp (test_blob1, blob, sizeof (test_blob1)), 0); + + blob = gcc_jit_result_get_global (result, "bin_blob2"); + CHECK_NON_NULL (blob); + CHECK_VALUE (memcmp (test_blob2, blob, sizeof (test_blob2)), 0); + + blob = gcc_jit_result_get_global (result, "bin_blob3"); + CHECK_NON_NULL (blob); + CHECK_VALUE (memcmp (test_blob3, blob, sizeof (test_blob3)), 0); + +} -- 2.17.1 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] libgccjit: Add new gcc_jit_context_new_blob entry point 2020-06-03 10:11 [PATCH] libgccjit: Add new gcc_jit_context_new_blob entry point Andrea Corallo @ 2020-06-03 15:03 ` David Malcolm 2020-06-03 16:36 ` Andrea Corallo 0 siblings, 1 reply; 14+ messages in thread From: David Malcolm @ 2020-06-03 15:03 UTC (permalink / raw) To: Andrea Corallo, gcc-patches, jit; +Cc: nd On Wed, 2020-06-03 at 12:11 +0200, Andrea Corallo wrote: > Hi all, > > I'd like to submit this patch to extend libgccjit to allow for > storing > binary blobs (equivalent to initialized char arrays). > > The use-case is when libgccjit is used as a back-end for dynamic > programming languages. In this case is often necessary to store > serialized objects into the compilation unit as support to the > generated code. > > The proposed entry point is the following: > > gcc_jit_lvalue * > gcc_jit_context_new_blob (gcc_jit_context *ctxt, > gcc_jit_location *loc, > enum gcc_jit_global_kind kind, > const void *ptr, > size_t size, > const char *name); > > This create a special kind of libgccjit global that will be > initialized > with the memory content pointed by 'ptr'. > > I've added a testcase and the regression is clean. I've also tested > it with the libgccjit based Emacs disabling the current workaround we > have > for this. > > Feedback is very welcome. Thanks for the patch. Is this entrypoint only needed for the ahead-of-time use case? If the client code is instead going to do an in-memory compilation, then I believe it can simply build the buffer of data in memory and expose it to the jitted code via gcc_jit_context_new_rvalue_from_ptr. It occurred to me that the entrypoint is combining two things: - creating a global char[] - creating an initializer for that global which got me wondering if we should instead have a way to add initializers for globals. My first thought was something like: gcc_jit_context_new_global_with_initializer which would be like gcc_jit_context_new_global but would have an additional gcc_jit_rvalue *init_value param? The global would have to be of kind GCC_JIT_GLOBAL_EXPORTED or GCC_JIT_GLOBAL_INTERNAL, not GCC_JIT_GLOBAL_IMPORTED. Alternatively, maybe it would be better to have gcc_jit_global_set_initializer (gcc_jit_lvalue *global, gcc_jit_rvalue *init_val); to make the API more flexible. But even if we had this, we'd still need some way to create the rvalue for that initial value. Also, maybe there ought to be a distinction between rvalues that can vary at runtime vs those that can be computed at compile-time (and are thus suitable for use in static initialization). I suspect you may have gone through the same thought process and come up with a simpler approach. (I'm mostly just "thinking out loud" here, sorry if it isn't very coherent). Should it be a "const char[]" rather than just a "char[]"? One specific nit about the patch: in compatibility.rst, there should be a: .. _LIBGCCJIT_ABI_14: label before the heading. Hope this is constructive Dave > Andrea > > gcc/jit/ChangeLog > > 2020-06-02 Andrea Corallo <andrea.corallo@arm.com> > > * docs/topics/compatibility.rst (LIBGCCJIT_ABI_14): New ABI > tag. > > * docs/topics/expressions.rst (Binary Blobs): New section > documenting gcc_jit_context_new_blob. > > * jit-common.h: Document class blob. > > * jit-playback.c (playback::context::global_new_decl) > (playback::context::global_finalize_lvalue): New methods. > (playback::context::new_global): Make use of global_new_decl, > global_finalize_lvalue. > (playback::context::new_blob): New method. > > * jit-playback.h (new_blob, global_new_decl): New method > declarations. > > * jit-recording.c (recording::context::new_blob) > (recording::blob::replay_into) > (recording::global::write_qualifier_to_dump): New methods. > (recording::global::write_to_dump): Use > write_qualifier_to_dump. > (recording::blob::write_to_dump): New method. > > * jit-recording.h (class blob): New class. > (class global): Have m_kind and m_name as protected. > (class global): Remove FINAL qualifier to replay_into and > write_to_dump. > (class global): New method write_qualifier_to_dump decl. > (class context): New method new_blob decl. > > * libgccjit++.h (context::new_blob): New method. > > * libgccjit.c (gcc_jit_lvalue_as_rvalue): New function. > > * libgccjit.h (gcc_jit_context_new_blob): New function > declaration. > (LIBGCCJIT_HAVE_gcc_jit_context_new_blob): New macro. > > * libgccjit.map (LIBGCCJIT_ABI_14): New ABI tag. > > gcc/testsuite/ChangeLog > > 2020-06-02 Andrea Corallo <andrea.corallo@arm.com> > > * jit.dg/all-non-failing-tests.h: Add test-blob.c. > > * jit.dg/test-blob.c: New test. > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] libgccjit: Add new gcc_jit_context_new_blob entry point 2020-06-03 15:03 ` David Malcolm @ 2020-06-03 16:36 ` Andrea Corallo 2020-07-01 16:14 ` Andrea Corallo 0 siblings, 1 reply; 14+ messages in thread From: Andrea Corallo @ 2020-06-03 16:36 UTC (permalink / raw) To: David Malcolm; +Cc: gcc-patches, jit, nd David Malcolm <dmalcolm@redhat.com> writes: > Thanks for the patch. > > Is this entrypoint only needed for the ahead-of-time use case? If the > client code is instead going to do an in-memory compilation, then I > believe it can simply build the buffer of data in memory and expose it > to the jitted code via gcc_jit_context_new_rvalue_from_ptr. Hi Dave, correct, if you can leak pointers into the generated code this entry point is likely to be unnecessary. > It occurred to me that the entrypoint is combining two things: > - creating a global char[] > - creating an initializer for that global > > which got me wondering if we should instead have a way to add > initializers for globals. > > My first thought was something like: > > gcc_jit_context_new_global_with_initializer > > which would be like gcc_jit_context_new_global but would have an > additional gcc_jit_rvalue *init_value param? > The global would have to be of kind GCC_JIT_GLOBAL_EXPORTED or > GCC_JIT_GLOBAL_INTERNAL, not GCC_JIT_GLOBAL_IMPORTED. > > Alternatively, maybe it would be better to have > > gcc_jit_global_set_initializer (gcc_jit_lvalue *global, > gcc_jit_rvalue *init_val); > > to make the API more flexible. > > But even if we had this, we'd still need some way to create the rvalue > for that initial value. Also, maybe there ought to be a distinction > between rvalues that can vary at runtime vs those that can be computed > at compile-time (and are thus suitable for use in static > initialization). > > I suspect you may have gone through the same thought process and come > up with a simpler approach. (I'm mostly just "thinking out loud" > here, sorry if it isn't very coherent). Yes I had kind of similar thoughs. Ideally would be good to have a generic solution, the complication is that as you mentioned not every rvalue is suitable for initializing every global, but rather the opposite. My fear was that the space to be covered would be non trivial for a robust solution in this case. Also I believe we currently have no way to express in libgccjit rvalues an array with some content, so how to use this as initializer? Perhaps also we should have a new type gcc_jit_initializer? On the other hand I thought that for simple things like integers the problem is tipically already solved with an assignment in some init code (infact I think so far nobody complained) while the real standing limitation is for blobs (perhaps I'm wrong). And in the end if you can stuff some data in, you can use it later for any scope. Another "hybrid" solution would be to have specific entry point for each type of the subset we want to allow for static initialization. This way we still control the creation of the initializer internally so it's safe. In this view this blob entry point would be just one of these (probably with a different name like 'gcc_jit_context_new_glob_init_char_array'). > Should it be a "const char[]" rather than just a "char[]"? Good question, I believe depends on the usecase so I left out the const. Perhaps should be a parameter of the entry point. > One specific nit about the patch: in compatibility.rst, there should be > a: > > .. _LIBGCCJIT_ABI_14: > > label before the heading. Ops > Hope this is constructive > Dave Sure it is. Thanks Andrea ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] libgccjit: Add new gcc_jit_context_new_blob entry point 2020-06-03 16:36 ` Andrea Corallo @ 2020-07-01 16:14 ` Andrea Corallo 2020-07-14 10:00 ` Andrea Corallo 2020-07-24 22:05 ` David Malcolm 0 siblings, 2 replies; 14+ messages in thread From: Andrea Corallo @ 2020-07-01 16:14 UTC (permalink / raw) To: David Malcolm; +Cc: jit, nd, gcc-patches Andrea Corallo <andrea.corallo@arm.com> writes: >> It occurred to me that the entrypoint is combining two things: >> - creating a global char[] >> - creating an initializer for that global >> >> which got me wondering if we should instead have a way to add >> initializers for globals. >> >> My first thought was something like: >> >> gcc_jit_context_new_global_with_initializer >> >> which would be like gcc_jit_context_new_global but would have an >> additional gcc_jit_rvalue *init_value param? >> The global would have to be of kind GCC_JIT_GLOBAL_EXPORTED or >> GCC_JIT_GLOBAL_INTERNAL, not GCC_JIT_GLOBAL_IMPORTED. >> >> Alternatively, maybe it would be better to have >> >> gcc_jit_global_set_initializer (gcc_jit_lvalue *global, >> gcc_jit_rvalue *init_val); >> >> to make the API more flexible. >> >> But even if we had this, we'd still need some way to create the rvalue >> for that initial value. Also, maybe there ought to be a distinction >> between rvalues that can vary at runtime vs those that can be computed >> at compile-time (and are thus suitable for use in static >> initialization). >> >> I suspect you may have gone through the same thought process and come >> up with a simpler approach. (I'm mostly just "thinking out loud" >> here, sorry if it isn't very coherent). > > Yes I had kind of similar thoughs. > > Ideally would be good to have a generic solution, the complication is > that as you mentioned not every rvalue is suitable for initializing > every global, but rather the opposite. My fear was that the space to be > covered would be non trivial for a robust solution in this case. > > Also I believe we currently have no way to express in libgccjit rvalues > an array with some content, so how to use this as initializer? Perhaps > also we should have a new type gcc_jit_initializer? > > On the other hand I thought that for simple things like integers the > problem is tipically already solved with an assignment in some init code > (infact I think so far nobody complained) while the real standing > limitation is for blobs (perhaps I'm wrong). And in the end if you can > stuff some data in, you can use it later for any scope. > > Another "hybrid" solution would be to have specific entry point for each > type of the subset we want to allow for static initialization. This way > we still control the creation of the initializer internally so it's > safe. In this view this blob entry point would be just one of these > (probably with a different name like > 'gcc_jit_context_new_glob_init_char_array'). > Hi Dave, wanted to ask if you formed an opinion about the patch and/or more in general the problem of static initialize data. Thanks Andrea ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] libgccjit: Add new gcc_jit_context_new_blob entry point 2020-07-01 16:14 ` Andrea Corallo @ 2020-07-14 10:00 ` Andrea Corallo 2020-07-24 22:05 ` David Malcolm 1 sibling, 0 replies; 14+ messages in thread From: Andrea Corallo @ 2020-07-14 10:00 UTC (permalink / raw) To: David Malcolm; +Cc: jit, nd, gcc-patches Andrea Corallo <andrea.corallo@arm.com> writes: > Andrea Corallo <andrea.corallo@arm.com> writes: > >>> It occurred to me that the entrypoint is combining two things: >>> - creating a global char[] >>> - creating an initializer for that global >>> >>> which got me wondering if we should instead have a way to add >>> initializers for globals. >>> >>> My first thought was something like: >>> >>> gcc_jit_context_new_global_with_initializer >>> >>> which would be like gcc_jit_context_new_global but would have an >>> additional gcc_jit_rvalue *init_value param? >>> The global would have to be of kind GCC_JIT_GLOBAL_EXPORTED or >>> GCC_JIT_GLOBAL_INTERNAL, not GCC_JIT_GLOBAL_IMPORTED. >>> >>> Alternatively, maybe it would be better to have >>> >>> gcc_jit_global_set_initializer (gcc_jit_lvalue *global, >>> gcc_jit_rvalue *init_val); >>> >>> to make the API more flexible. >>> >>> But even if we had this, we'd still need some way to create the rvalue >>> for that initial value. Also, maybe there ought to be a distinction >>> between rvalues that can vary at runtime vs those that can be computed >>> at compile-time (and are thus suitable for use in static >>> initialization). >>> >>> I suspect you may have gone through the same thought process and come >>> up with a simpler approach. (I'm mostly just "thinking out loud" >>> here, sorry if it isn't very coherent). >> >> Yes I had kind of similar thoughs. >> >> Ideally would be good to have a generic solution, the complication is >> that as you mentioned not every rvalue is suitable for initializing >> every global, but rather the opposite. My fear was that the space to be >> covered would be non trivial for a robust solution in this case. >> >> Also I believe we currently have no way to express in libgccjit rvalues >> an array with some content, so how to use this as initializer? Perhaps >> also we should have a new type gcc_jit_initializer? >> >> On the other hand I thought that for simple things like integers the >> problem is tipically already solved with an assignment in some init code >> (infact I think so far nobody complained) while the real standing >> limitation is for blobs (perhaps I'm wrong). And in the end if you can >> stuff some data in, you can use it later for any scope. >> >> Another "hybrid" solution would be to have specific entry point for each >> type of the subset we want to allow for static initialization. This way >> we still control the creation of the initializer internally so it's >> safe. In this view this blob entry point would be just one of these >> (probably with a different name like >> 'gcc_jit_context_new_glob_init_char_array'). >> > > Hi Dave, > > wanted to ask if you formed an opinion about the patch and/or more in > general the problem of static initialize data. > > Thanks > > Andrea Ping ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] libgccjit: Add new gcc_jit_context_new_blob entry point 2020-07-01 16:14 ` Andrea Corallo 2020-07-14 10:00 ` Andrea Corallo @ 2020-07-24 22:05 ` David Malcolm 2020-07-24 22:12 ` David Malcolm 1 sibling, 1 reply; 14+ messages in thread From: David Malcolm @ 2020-07-24 22:05 UTC (permalink / raw) To: Andrea Corallo; +Cc: jit, nd, gcc-patches On Wed, 2020-07-01 at 18:14 +0200, Andrea Corallo wrote: > Andrea Corallo <andrea.corallo@arm.com> writes: > > > > It occurred to me that the entrypoint is combining two things: > > > - creating a global char[] > > > - creating an initializer for that global > > > > > > which got me wondering if we should instead have a way to add > > > initializers for globals. > > > > > > My first thought was something like: > > > > > > gcc_jit_context_new_global_with_initializer > > > > > > which would be like gcc_jit_context_new_global but would have an > > > additional gcc_jit_rvalue *init_value param? > > > The global would have to be of kind GCC_JIT_GLOBAL_EXPORTED or > > > GCC_JIT_GLOBAL_INTERNAL, not GCC_JIT_GLOBAL_IMPORTED. > > > > > > Alternatively, maybe it would be better to have > > > > > > gcc_jit_global_set_initializer (gcc_jit_lvalue *global, > > > gcc_jit_rvalue *init_val); > > > > > > to make the API more flexible. > > > > > > But even if we had this, we'd still need some way to create the > > > rvalue > > > for that initial value. Also, maybe there ought to be a > > > distinction > > > between rvalues that can vary at runtime vs those that can be > > > computed > > > at compile-time (and are thus suitable for use in static > > > initialization). > > > > > > I suspect you may have gone through the same thought process and > > > come > > > up with a simpler approach. (I'm mostly just "thinking out > > > loud" > > > here, sorry if it isn't very coherent). > > > > Yes I had kind of similar thoughs. > > > > Ideally would be good to have a generic solution, the complication > > is > > that as you mentioned not every rvalue is suitable for initializing > > every global, but rather the opposite. My fear was that the space > > to be > > covered would be non trivial for a robust solution in this case. > > > > Also I believe we currently have no way to express in libgccjit > > rvalues > > an array with some content, so how to use this as > > initializer? Perhaps > > also we should have a new type gcc_jit_initializer? > > > > On the other hand I thought that for simple things like integers > > the > > problem is tipically already solved with an assignment in some init > > code > > (infact I think so far nobody complained) while the real standing > > limitation is for blobs (perhaps I'm wrong). And in the end if you > > can > > stuff some data in, you can use it later for any scope. > > > > Another "hybrid" solution would be to have specific entry point for > > each > > type of the subset we want to allow for static > > initialization. This way > > we still control the creation of the initializer internally so it's > > safe. In this view this blob entry point would be just one of > > these > > (probably with a different name like > > 'gcc_jit_context_new_glob_init_char_array'). > > > > Hi Dave, > > wanted to ask if you formed an opinion about the patch and/or more in > general the problem of static initialize data. Sorry for the delay in responding. Another idea that occurred to me is, as before, to generalize the entrypoint to cover creating globals with arbitrary types and to support initializers (either with a new gcc_jit_context_new_global_with_initializer or a new: gcc_jit_global_set_initializer as above, but instead of passing in a gcc_jit_rvalue *init_val, to pass in a const void *initializer size_t num_bytes pair pointing to the initializer buffer, with the behavior that these are the bytes that will be used for the initial value of the global, whatever that means for the given type. Something like either: extern gcc_jit_lvalue * gcc_jit_context_new_global_with_initializer (gcc_jit_context *ctxt, gcc_jit_location *loc, enum gcc_jit_global_kind kind, gcc_jit_type *type, const char *name, const void *initializer, size_t num_bytes); or: extern void gcc_jit_global_set_initializer (gcc_jit_lvalue *global, const void *initializer, size_t num_bytes); or somesuch. The former is similar to the proposed new entrypoint from your patch, but gains a type argument (and is modeled more closely on the existing entrypoint for creating globals). I haven't thought this through in detail, and I'm not sure exactly how it would work for arbitrary types, but I thought it worth sharing. (For example I can think of nasty issues if we ever want to support cross-compilation, e.g. where sizeof types or endianness differs between host and target). Maybe we can make it work initially for char[] types, rejecting others, assuming that that's the most pressing need in your work, and we can add support for other types internally as followups without needing to add new entrypoints? Thoughts? Dave ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] libgccjit: Add new gcc_jit_context_new_blob entry point 2020-07-24 22:05 ` David Malcolm @ 2020-07-24 22:12 ` David Malcolm 2020-08-03 8:07 ` Andrea Corallo 0 siblings, 1 reply; 14+ messages in thread From: David Malcolm @ 2020-07-24 22:12 UTC (permalink / raw) To: Andrea Corallo; +Cc: jit, nd, gcc-patches On Fri, 2020-07-24 at 18:05 -0400, David Malcolm via Gcc-patches wrote: [...] > I haven't thought this through in detail, and I'm not sure exactly > how > it would work for arbitrary types, but I thought it worth sharing. > (For example I can think of nasty issues if we ever want to support > cross-compilation, e.g. where sizeof types or endianness differs > between host and target). ...which is an argument in favor of retaining the name "blob", perhaps as the name of the argument in the header file e.g.: extern void gcc_jit_global_set_initializer (gcc_jit_lvalue *global, const void *blob, size_t num_bytes); as a subtle hint to the user that they need to be wary about binary layouts ("here be dragons"). [...] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] libgccjit: Add new gcc_jit_context_new_blob entry point 2020-07-24 22:12 ` David Malcolm @ 2020-08-03 8:07 ` Andrea Corallo 2020-08-06 19:53 ` David Malcolm 0 siblings, 1 reply; 14+ messages in thread From: Andrea Corallo @ 2020-08-03 8:07 UTC (permalink / raw) To: David Malcolm; +Cc: jit, nd, gcc-patches [-- Attachment #1: Type: text/plain, Size: 3245 bytes --] David Malcolm <dmalcolm@redhat.com> writes: > On Fri, 2020-07-24 at 18:05 -0400, David Malcolm via Gcc-patches wrote: > > [...] > >> I haven't thought this through in detail, and I'm not sure exactly >> how >> it would work for arbitrary types, but I thought it worth sharing. >> (For example I can think of nasty issues if we ever want to support >> cross-compilation, e.g. where sizeof types or endianness differs >> between host and target). > > ...which is an argument in favor of retaining the name "blob", perhaps > as the name of the argument in the header file e.g.: > > extern void > gcc_jit_global_set_initializer (gcc_jit_lvalue *global, > const void *blob, > size_t num_bytes); > > > as a subtle hint to the user that they need to be wary about binary > layouts ("here be dragons"). > > [...] Hi Dave & all, following up this is my take on the implementation of: gcc_jit_global_set_initializer (gcc_jit_lvalue *global, const void *blob, size_t num_bytes); 'global' must be an array but in the seek of generality it now supports all the various integral types and is not limited to char[]. As you anticipated the implementation I came up is currently not safe for cross-compilation, not sure is requirement tho. make check-jit is clean Feedback very welcome Thanks! Andrea gcc/jit/ChangeLog 2020-08-01 Andrea Corallo <andrea.corallo@arm.com> * docs/topics/compatibility.rst (LIBGCCJIT_ABI_14): New ABI tag. * docs/topics/expressions.rst (gcc_jit_global_set_initializer): Document new entry point in section 'Global variables'. * jit-playback.c (global_new_decl, global_finalize_lvalue): New method. (playback::context::new_global): Make use of global_new_decl, global_finalize_lvalue. (load_blob_in_ctor): New template function in use by the following. (playback::context::new_global_initialized): New method. * jit-playback.h (class context): Decl 'new_global_initialized', 'global_new_decl', 'global_finalize_lvalue'. (lvalue::set_initializer): Add implementation. * jit-recording.c (recording::memento_of_get_pointer::get_size) (recording::memento_of_get_type::get_size): Add implementation. (recording::global::write_initializer_reproducer): New function in use by 'recording::global::write_reproducer'. (recording::global::replay_into) (recording::global::write_to_dump) (recording::global::write_reproducer): Handle initialized case. * jit-recording.h (class type): Decl 'get_size' and 'num_elements'. * libgccjit++.h (class lvalue): Declare new 'set_initializer' method. (class lvalue): Decl 'is_global' and 'set_initializer'. (class class global) Decl 'write_initializer_reproducer'. Add 'm_initializer', 'm_initializer_num_bytes' fields. Implement 'set_initializer'. * libgccjit.c (gcc_jit_global_set_initializer): New function. * libgccjit.h (gcc_jit_global_set_initializer): New function declaration. * libgccjit.map (LIBGCCJIT_ABI_14): New ABI tag. gcc/testsuite/ChangeLog 2020-08-01 Andrea Corallo <andrea.corallo@arm.com> * jit.dg/all-non-failing-tests.h: Add test-blob.c. * jit.dg/test-global-set-initializer.c: New testcase. [-- Attachment #2: 0001-Add-new-gcc_jit_global_set_initializer-entry-point.patch --] [-- Type: text/plain, Size: 24482 bytes --] From 74bd96fde6d4baed978555279d17898b989c00ad Mon Sep 17 00:00:00 2001 From: Andrea Corallo <andrea.corallo@arm.com> Date: Sat, 30 May 2020 10:33:08 +0100 Subject: [PATCH] Add new gcc_jit_global_set_initializer entry point gcc/jit/ChangeLog 2020-08-01 Andrea Corallo <andrea.corallo@arm.com> * docs/topics/compatibility.rst (LIBGCCJIT_ABI_14): New ABI tag. * docs/topics/expressions.rst (gcc_jit_global_set_initializer): Document new entry point in section 'Global variables'. * jit-playback.c (global_new_decl, global_finalize_lvalue): New method. (playback::context::new_global): Make use of global_new_decl, global_finalize_lvalue. (load_blob_in_ctor): New template function in use by the following. (playback::context::new_global_initialized): New method. * jit-playback.h (class context): Decl 'new_global_initialized', 'global_new_decl', 'global_finalize_lvalue'. (lvalue::set_initializer): Add implementation. * jit-recording.c (recording::memento_of_get_pointer::get_size) (recording::memento_of_get_type::get_size): Add implementation. (recording::global::write_initializer_reproducer): New function in use by 'recording::global::write_reproducer'. (recording::global::replay_into) (recording::global::write_to_dump) (recording::global::write_reproducer): Handle initialized case. * jit-recording.h (class type): Decl 'get_size' and 'num_elements'. * libgccjit++.h (class lvalue): Declare new 'set_initializer' method. (class lvalue): Decl 'is_global' and 'set_initializer'. (class class global) Decl 'write_initializer_reproducer'. Add 'm_initializer', 'm_initializer_num_bytes' fields. Implement 'set_initializer'. * libgccjit.c (gcc_jit_global_set_initializer): New function. * libgccjit.h (gcc_jit_global_set_initializer): New function declaration. * libgccjit.map (LIBGCCJIT_ABI_14): New ABI tag. gcc/testsuite/ChangeLog 2020-08-01 Andrea Corallo <andrea.corallo@arm.com> * jit.dg/all-non-failing-tests.h: Add test-blob.c. * jit.dg/test-global-set-initializer.c: New testcase. --- gcc/jit/docs/topics/compatibility.rst | 7 + gcc/jit/docs/topics/expressions.rst | 21 +++ gcc/jit/jit-playback.c | 105 +++++++++++++- gcc/jit/jit-playback.h | 17 +++ gcc/jit/jit-recording.c | 137 +++++++++++++++++- gcc/jit/jit-recording.h | 26 ++++ gcc/jit/libgccjit++.h | 10 ++ gcc/jit/libgccjit.c | 36 +++++ gcc/jit/libgccjit.h | 14 ++ gcc/jit/libgccjit.map | 7 +- gcc/testsuite/jit.dg/all-non-failing-tests.h | 7 + .../jit.dg/test-global-set-initializer.c | 78 ++++++++++ 12 files changed, 453 insertions(+), 12 deletions(-) create mode 100644 gcc/testsuite/jit.dg/test-global-set-initializer.c diff --git a/gcc/jit/docs/topics/compatibility.rst b/gcc/jit/docs/topics/compatibility.rst index bb3387fa583b..6bfa101ed718 100644 --- a/gcc/jit/docs/topics/compatibility.rst +++ b/gcc/jit/docs/topics/compatibility.rst @@ -219,3 +219,10 @@ entrypoints: * :func:`gcc_jit_version_minor` * :func:`gcc_jit_version_patchlevel` + +.. _LIBGCCJIT_ABI_14: + +``LIBGCCJIT_ABI_14`` +-------------------- +``LIBGCCJIT_ABI_14`` covers the addition of +:func:`gcc_jit_global_set_initializer` diff --git a/gcc/jit/docs/topics/expressions.rst b/gcc/jit/docs/topics/expressions.rst index d783ceea51a8..7699dcfd27be 100644 --- a/gcc/jit/docs/topics/expressions.rst +++ b/gcc/jit/docs/topics/expressions.rst @@ -582,6 +582,27 @@ Global variables referring to it. Analogous to using an "extern" global from a header file. +.. function:: gcc_jit_lvalue *\ + gcc_jit_global_set_initializer (gcc_jit_lvalue *global,\ + const void *blob,\ + size_t num_bytes) + + Set an initializer for an object using the memory content pointed + by ``blob`` for ``num_bytes``. ``global`` must be an arrays of an + integral type. + + The parameter ``blob`` must be non-NULL. The call copies the memory + pointed by ``blob`` for ``num_bytes`` bytes, so it is valid to pass + in a pointer to an on-stack buffer. The content will be stored in + the compilation unit and used as initialization value of the array. + + This entrypoint was added in :ref:`LIBGCCJIT_ABI_14`; you can test for + its presence using + + .. code-block:: c + + #ifdef LIBGCCJIT_HAVE_global_set_initializer + Working with pointers, structs and unions ----------------------------------------- diff --git a/gcc/jit/jit-playback.h b/gcc/jit/jit-playback.h index f9b3e675368c..50b69753bb42 100644 --- a/gcc/jit/jit-playback.h +++ b/gcc/jit/jit-playback.h @@ -111,6 +111,15 @@ public: type *type, const char *name); + lvalue * + new_global_initialized (location *loc, + enum gcc_jit_global_kind kind, + type *type, + size_t element_size, + size_t initializer_num_elem, + const void *initializer, + const char *name); + template <typename HOST_TYPE> rvalue * new_rvalue_from_const (type *type, @@ -266,6 +275,14 @@ private: const char * get_path_s_file () const; const char * get_path_so_file () const; + tree + global_new_decl (location *loc, + enum gcc_jit_global_kind kind, + type *type, + const char *name); + lvalue * + global_finalize_lvalue (tree inner); + private: /* Functions for implementing "compile". */ diff --git a/gcc/jit/jit-playback.c b/gcc/jit/jit-playback.c index 0fddf04da873..52fc92f5928c 100644 --- a/gcc/jit/jit-playback.c +++ b/gcc/jit/jit-playback.c @@ -510,14 +510,14 @@ new_function (location *loc, return func; } -/* Construct a playback::lvalue instance (wrapping a tree). */ +/* In use by new_global and new_global_initialized. */ -playback::lvalue * +tree playback::context:: -new_global (location *loc, - enum gcc_jit_global_kind kind, - type *type, - const char *name) +global_new_decl (location *loc, + enum gcc_jit_global_kind kind, + type *type, + const char *name) { gcc_assert (type); gcc_assert (name); @@ -547,6 +547,15 @@ new_global (location *loc, if (loc) set_tree_location (inner, loc); + return inner; +} + +/* In use by new_global and new_global_initialized. */ + +playback::lvalue * +playback::context:: +global_finalize_lvalue (tree inner) +{ varpool_node::get_create (inner); varpool_node::finalize_decl (inner); @@ -556,6 +565,90 @@ new_global (location *loc, return new lvalue (this, inner); } +/* Construct a playback::lvalue instance (wrapping a tree). */ + +playback::lvalue * +playback::context:: +new_global (location *loc, + enum gcc_jit_global_kind kind, + type *type, + const char *name) +{ + tree inner = global_new_decl (loc, kind, type, name); + + return global_finalize_lvalue (inner); +} + +/* Fill 'constructor_elements' with the memory content of + 'initializer'. Each element of the initializer is of the size of + type T. In use by new_global_initialized.*/ + +template<typename T> +static void +load_blob_in_ctor (vec<constructor_elt, va_gc> *&constructor_elements, + size_t num_elem, + const void *initializer) +{ + /* Loosely based on 'output_init_element' c-typeck.c:9691. */ + const T *p = (const T *)initializer; + tree node = make_unsigned_type (BITS_PER_UNIT * sizeof (T)); + for (size_t i = 0; i < num_elem; i++) + { + constructor_elt celt = + { build_int_cst (long_unsigned_type_node, i), + build_int_cst (node, p[i]) }; + vec_safe_push (constructor_elements, celt); + } +} + +/* Construct an initialized playback::lvalue instance (wrapping a + tree). */ + +playback::lvalue * +playback::context:: +new_global_initialized (location *loc, + enum gcc_jit_global_kind kind, + type *type, + size_t element_size, + size_t initializer_num_elem, + const void *initializer, + const char *name) +{ + tree inner = global_new_decl (loc, kind, type, name); + + static vec<constructor_elt, va_gc> *constructor_elements; + + switch (element_size) + { + case 1: + load_blob_in_ctor<uint8_t> (constructor_elements, initializer_num_elem, + initializer); + break; + case 2: + load_blob_in_ctor<uint16_t> (constructor_elements, initializer_num_elem, + initializer); + break; + case 4: + load_blob_in_ctor<uint32_t> (constructor_elements, initializer_num_elem, + initializer); + break; + case 8: + load_blob_in_ctor<uint64_t> (constructor_elements, initializer_num_elem, + initializer); + break; + default: + gcc_unreachable (); + } + /* Compare with 'pop_init_level' c-typeck.c:8780. */ + tree ctor = build_constructor (type->as_tree (), constructor_elements); + constructor_elements = NULL; + + /* Compare with 'store_init_value' c-typeck.c:7555. */ + DECL_INITIAL (inner) = ctor; + + return global_finalize_lvalue (inner); +} + /* Implementation of the various gcc::jit::playback::context::new_rvalue_from_const <HOST_TYPE> methods. diff --git a/gcc/jit/jit-recording.h b/gcc/jit/jit-recording.h index 726b9c4b8371..7a7a61a0e126 100644 --- a/gcc/jit/jit-recording.h +++ b/gcc/jit/jit-recording.h @@ -502,6 +502,7 @@ public: This will return NULL if it's not valid to dereference this type. The caller is responsible for setting an error. */ virtual type *dereference () = 0; + virtual size_t get_size () { gcc_unreachable (); } /* Dynamic casts. */ virtual function_type *dyn_cast_function_type () { return NULL; } @@ -534,6 +535,7 @@ public: virtual type *is_array () = 0; virtual bool is_void () const { return false; } virtual bool has_known_size () const { return true; } + virtual int num_elements () { gcc_unreachable (); } bool is_numeric () const { @@ -569,6 +571,8 @@ public: type *dereference () FINAL OVERRIDE; + size_t get_size () FINAL OVERRIDE; + bool accepts_writes_from (type *rtype) FINAL OVERRIDE { if (m_kind == GCC_JIT_TYPE_VOID_PTR) @@ -610,6 +614,8 @@ public: type *dereference () FINAL OVERRIDE { return m_other_type; } + size_t get_size () FINAL OVERRIDE; + bool accepts_writes_from (type *rtype) FINAL OVERRIDE; void replay_into (replayer *r) FINAL OVERRIDE; @@ -755,6 +761,7 @@ class array_type : public type bool is_bool () const FINAL OVERRIDE { return false; } type *is_pointer () FINAL OVERRIDE { return NULL; } type *is_array () FINAL OVERRIDE { return m_element_type; } + int num_elements () FINAL OVERRIDE { return m_num_elements; } void replay_into (replayer *) FINAL OVERRIDE; @@ -1107,6 +1114,8 @@ public: const char *access_as_rvalue (reproducer &r) OVERRIDE; virtual const char *access_as_lvalue (reproducer &r); + virtual bool is_global () const { return false; } + virtual void set_initializer (const void *, size_t) { gcc_unreachable (); } }; class param : public lvalue @@ -1335,8 +1344,23 @@ public: void write_to_dump (dump &d) FINAL OVERRIDE; + virtual bool is_global () const FINAL OVERRIDE { return true; } + + virtual void + set_initializer (const void *initializer, + size_t num_bytes) FINAL OVERRIDE + { + if (m_initializer) + free (m_initializer); + m_initializer = xmalloc (num_bytes); + memcpy (m_initializer, initializer, num_bytes); + m_initializer_num_bytes = num_bytes; + } + private: string * make_debug_string () FINAL OVERRIDE { return m_name; } + template <typename T> + void write_initializer_reproducer (const char *id, reproducer &r); void write_reproducer (reproducer &r) FINAL OVERRIDE; enum precedence get_precedence () const FINAL OVERRIDE { @@ -1346,6 +1370,8 @@ private: private: enum gcc_jit_global_kind m_kind; string *m_name; + void *m_initializer = NULL; + size_t m_initializer_num_bytes = 0; }; template <typename HOST_TYPE> diff --git a/gcc/jit/jit-recording.c b/gcc/jit/jit-recording.c index b73cd76a0a02..f97de00f63c3 100644 --- a/gcc/jit/jit-recording.c +++ b/gcc/jit/jit-recording.c @@ -2175,6 +2175,57 @@ recording::type::access_as_type (reproducer &r) return r.get_identifier (this); } +/* Override of default implementation of + recording::type::get_size. + + Return the size in bytes. This is in use for global + initialization. */ + +size_t +recording::memento_of_get_type::get_size () +{ + int size; + switch (m_kind) + { + case GCC_JIT_TYPE_VOID: + return 0; + case GCC_JIT_TYPE_BOOL: + case GCC_JIT_TYPE_CHAR: + case GCC_JIT_TYPE_SIGNED_CHAR: + case GCC_JIT_TYPE_UNSIGNED_CHAR: + return 1; + case GCC_JIT_TYPE_SHORT: + case GCC_JIT_TYPE_UNSIGNED_SHORT: + size = SHORT_TYPE_SIZE; + break; + case GCC_JIT_TYPE_INT: + case GCC_JIT_TYPE_UNSIGNED_INT: + size = INT_TYPE_SIZE; + break; + case GCC_JIT_TYPE_LONG: + case GCC_JIT_TYPE_UNSIGNED_LONG: + size = LONG_TYPE_SIZE; + break; + case GCC_JIT_TYPE_LONG_LONG: + case GCC_JIT_TYPE_UNSIGNED_LONG_LONG: + size = LONG_LONG_TYPE_SIZE; + break; + case GCC_JIT_TYPE_FLOAT: + size = FLOAT_TYPE_SIZE; + break; + case GCC_JIT_TYPE_DOUBLE: + size = DOUBLE_TYPE_SIZE; + break; + case GCC_JIT_TYPE_LONG_DOUBLE: + size = LONG_DOUBLE_TYPE_SIZE; + break; + default: + gcc_unreachable (); + } + + return size / BITS_PER_UNIT; +} + /* Implementation of pure virtual hook recording::type::dereference for recording::memento_of_get_type. */ @@ -2482,6 +2533,15 @@ recording::memento_of_get_type::write_reproducer (reproducer &r) /* The implementation of class gcc::jit::recording::memento_of_get_pointer. */ +/* Override of default implementation of + recording::type::get_size for get_pointer. */ + +size_t +recording::memento_of_get_pointer::get_size () +{ + return POINTER_SIZE / BITS_PER_UNIT; +} + /* Override of default implementation of recording::type::accepts_writes_from for get_pointer. @@ -4393,10 +4453,20 @@ recording::block::dump_edges_to_dot (pretty_printer *pp) void recording::global::replay_into (replayer *r) { - set_playback_obj (r->new_global (playback_location (r, m_loc), - m_kind, - m_type->playback_type (), - playback_string (m_name))); + set_playback_obj ( + m_initializer + ? 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), + m_kind, + m_type->playback_type (), + playback_string (m_name))); } /* Override the default implementation of @@ -4440,9 +4510,26 @@ recording::global::write_to_dump (dump &d) d.write ("extern "); break; } - d.write ("%s %s;\n", + + d.write ("%s %s", m_type->get_debug_string (), get_debug_string ()); + + if (!m_initializer) + { + d.write (";\n"); + return; + } + + d.write ("=\n { "); + const char *p = (const char *)m_initializer; + for (size_t i = 0; i < m_initializer_num_bytes; i++) + { + d.write ("0x%x, ", p[i]); + if (i && !(i % 64)) + d.write ("\n "); + } + d.write ("};\n"); } /* A table of enum gcc_jit_global_kind values expressed in string @@ -4454,6 +4541,27 @@ static const char * const global_kind_reproducer_strings[] = { "GCC_JIT_GLOBAL_IMPORTED" }; +template <typename T> +void +recording::global::write_initializer_reproducer (const char *id, reproducer &r) +{ + const char *init_id = r.make_tmp_identifier ("init_for", this); + r.write (" %s %s[] =\n {", + m_type->dereference ()->get_debug_string (), + init_id); + + const T *p = (const T *)m_initializer; + for (size_t i = 0; i < m_initializer_num_bytes / sizeof (T); i++) + { + r.write ("%lu, ", (uint64_t)p[i]); + if (i && !(i % 64)) + r.write ("\n "); + } + r.write ("};\n"); + r.write (" gcc_jit_global_set_initializer (%s, %s, sizeof (%s));\n", + id, init_id, init_id); +} + /* Implementation of recording::memento::write_reproducer for globals. */ void @@ -4472,6 +4580,25 @@ recording::global::write_reproducer (reproducer &r) global_kind_reproducer_strings[m_kind], r.get_identifier_as_type (get_type ()), m_name->get_debug_string ()); + + if (m_initializer) + switch (m_type->dereference ()->get_size ()) + { + case 1: + write_initializer_reproducer<uint8_t> (id, r); + break; + case 2: + write_initializer_reproducer<uint16_t> (id, r); + break; + case 4: + write_initializer_reproducer<uint32_t> (id, r); + break; + case 8: + write_initializer_reproducer<uint64_t> (id, r); + break; + default: + gcc_unreachable (); + } } /* The implementation of the various const-handling classes: diff --git a/gcc/jit/libgccjit++.h b/gcc/jit/libgccjit++.h index 69e67766640c..1b9ef1a5db98 100644 --- a/gcc/jit/libgccjit++.h +++ b/gcc/jit/libgccjit++.h @@ -488,6 +488,7 @@ namespace gccjit location loc = location ()); rvalue get_address (location loc = location ()); + lvalue set_initializer (const void *blob, size_t num_bytes); }; class param : public lvalue @@ -1737,6 +1738,15 @@ lvalue::get_address (location loc) loc.get_inner_location ())); } +inline lvalue +lvalue::set_initializer (const void *blob, size_t num_bytes) +{ + gcc_jit_global_set_initializer (get_inner_lvalue (), + blob, + num_bytes); + return *this; +} + // class param : public lvalue inline param::param () : lvalue () {} inline param::param (gcc_jit_param *inner) diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h index 1c5a12e9c015..08b855230f6a 100644 --- a/gcc/jit/libgccjit.h +++ b/gcc/jit/libgccjit.h @@ -788,6 +788,20 @@ gcc_jit_context_new_global (gcc_jit_context *ctxt, gcc_jit_type *type, const char *name); +#define LIBGCCJIT_HAVE_global_set_initializer + +/* Set a static initializer for a global return the global itself. + + This API entrypoint was added in LIBGCCJIT_ABI_14; you can test for its + presence using + #ifdef LIBGCCJIT_HAVE_gcc_jit_global_set_initializer +*/ + +extern gcc_jit_lvalue * +gcc_jit_global_set_initializer (gcc_jit_lvalue *global, + const void *blob, + size_t num_bytes); + /* Upcasting. */ extern gcc_jit_object * gcc_jit_lvalue_as_object (gcc_jit_lvalue *lvalue); diff --git a/gcc/jit/libgccjit.c b/gcc/jit/libgccjit.c index 3d04f6db3aff..80c5aa6ac115 100644 --- a/gcc/jit/libgccjit.c +++ b/gcc/jit/libgccjit.c @@ -1117,6 +1117,42 @@ gcc_jit_context_new_global (gcc_jit_context *ctxt, return (gcc_jit_lvalue *)ctxt->new_global (loc, kind, type, name); } +/* Public entrypoint. See description in libgccjit.h. + + After error-checking, the real work is done by the + gcc::jit::recording::global::set_initializer method, in + jit-recording.c. */ + +extern gcc_jit_lvalue * +gcc_jit_global_set_initializer (gcc_jit_lvalue *global, + const void *blob, + size_t num_bytes) +{ + RETURN_NULL_IF_FAIL (global, NULL, NULL, "NULL global"); + RETURN_NULL_IF_FAIL (blob, NULL, NULL, "NULL blob"); + RETURN_NULL_IF_FAIL_PRINTF1 (global->is_global (), NULL, NULL, + "global \"%s\" not a global", + global->get_debug_string ()); + + gcc::jit::recording::type *lval_type = global->get_type (); + RETURN_NULL_IF_FAIL_PRINTF1 (lval_type->is_array (), NULL, NULL, + "global \"%s\" not an array", + global->get_debug_string ()); + RETURN_NULL_IF_FAIL_PRINTF1 (lval_type->dereference ()->is_int (), NULL, NULL, + "global \"%s\" not an array of integral type", + global->get_debug_string ()); + RETURN_NULL_IF_FAIL_PRINTF1 ( + lval_type->dereference ()->get_size() * lval_type->num_elements () + == num_bytes, + NULL, NULL, + "global \"%s\" and initializer don't match size", + global->get_debug_string ()); + + global->set_initializer (blob, num_bytes); + + return global; +} + /* Public entrypoint. See description in libgccjit.h. After error-checking, this calls the trivial diff --git a/gcc/jit/libgccjit.map b/gcc/jit/libgccjit.map index 6137dd4b4b03..a6e67e781a4b 100644 --- a/gcc/jit/libgccjit.map +++ b/gcc/jit/libgccjit.map @@ -186,4 +186,9 @@ LIBGCCJIT_ABI_13 { gcc_jit_version_major; gcc_jit_version_minor; gcc_jit_version_patchlevel; -} LIBGCCJIT_ABI_12; \ No newline at end of file +} LIBGCCJIT_ABI_12; + +LIBGCCJIT_ABI_14 { + global: + gcc_jit_global_set_initializer; +} LIBGCCJIT_ABI_13; diff --git a/gcc/testsuite/jit.dg/all-non-failing-tests.h b/gcc/testsuite/jit.dg/all-non-failing-tests.h index 632ab8cfb2e4..4202eb7798b7 100644 --- a/gcc/testsuite/jit.dg/all-non-failing-tests.h +++ b/gcc/testsuite/jit.dg/all-non-failing-tests.h @@ -174,6 +174,13 @@ #undef create_code #undef verify_code +/* test-global-set-initializer.c */ +#define create_code create_code_global_set_initializer +#define verify_code verify_code_global_set_initializer +#include "test-global-set-initializer.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-global-set-initializer.c b/gcc/testsuite/jit.dg/test-global-set-initializer.c new file mode 100644 index 000000000000..d38aba7d73f5 --- /dev/null +++ b/gcc/testsuite/jit.dg/test-global-set-initializer.c @@ -0,0 +1,78 @@ +#include <stdlib.h> +#include <stdio.h> +#include <string.h> +#include <time.h> + +#include "libgccjit.h" + +#include "harness.h" + +#define BIG_BLOB_SIZE (1 << 12) /* 4KB. */ + +static signed char test_blob1[] = { 0xc, 0xa, 0xf, 0xf, 0xe }; +static unsigned test_blob2[] = { 0x3, 0x2, 0x1, 0x0, 0x1, 0x2, 0x3 }; +static unsigned char test_blob3[BIG_BLOB_SIZE]; + +void +create_code (gcc_jit_context *ctxt, void *user_data) +{ + /* Let's try to inject the equivalent of: + + signed char bin_blob1[] = { 0xc, 0xa, 0xf, 0xf, 0xe }; + unsigned bin_blob2[] = { 0x3, 0x2, 0x1, 0x0, 0x1, 0x2, 0x3 }; + unsigned char bin_blob3[4096]... + */ + gcc_jit_type *unsigned_char_type = + gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_UNSIGNED_CHAR); + gcc_jit_type *signed_char_type = + gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_SIGNED_CHAR); + gcc_jit_type *unsigned_type = + gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_UNSIGNED_INT); + + gcc_jit_lvalue *glob = + gcc_jit_context_new_global ( + ctxt, NULL, GCC_JIT_GLOBAL_EXPORTED, + gcc_jit_context_new_array_type (ctxt, NULL, signed_char_type, + sizeof (test_blob1)), + "bin_blob1"); + gcc_jit_global_set_initializer (glob, test_blob1, sizeof (test_blob1)); + + glob = + gcc_jit_context_new_global ( + ctxt, NULL, GCC_JIT_GLOBAL_EXPORTED, + gcc_jit_context_new_array_type ( + ctxt, NULL, unsigned_type, + sizeof (test_blob2) / sizeof (*test_blob2)), + "bin_blob2"); + gcc_jit_global_set_initializer (glob, test_blob2, + sizeof (test_blob2)); + + for (size_t i = 0; i < BIG_BLOB_SIZE; i++) + test_blob3[i] = i * i + i; + glob = + gcc_jit_context_new_global ( + ctxt, NULL, GCC_JIT_GLOBAL_EXPORTED, + gcc_jit_context_new_array_type (ctxt, NULL, unsigned_char_type, + sizeof (test_blob3)), + "bin_blob3"); + gcc_jit_global_set_initializer (glob, test_blob3, sizeof (test_blob3)); +} + +void +verify_code (gcc_jit_context *ctxt, gcc_jit_result *result) +{ + CHECK_NON_NULL (result); + void *glob = gcc_jit_result_get_global (result, "bin_blob1"); + CHECK_NON_NULL (glob); + CHECK_VALUE (memcmp (test_blob1, glob, sizeof (test_blob1)), 0); + + glob = gcc_jit_result_get_global (result, "bin_blob2"); + CHECK_NON_NULL (glob); + CHECK_VALUE (memcmp (test_blob2, glob, + sizeof (test_blob2)), 0); + + glob = gcc_jit_result_get_global (result, "bin_blob3"); + CHECK_NON_NULL (glob); + CHECK_VALUE (memcmp (test_blob3, glob, sizeof (test_blob3)), 0); + +} -- 2.17.1 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] libgccjit: Add new gcc_jit_context_new_blob entry point 2020-08-03 8:07 ` Andrea Corallo @ 2020-08-06 19:53 ` David Malcolm 2020-08-19 7:17 ` [PATCH V2] " Andrea Corallo 0 siblings, 1 reply; 14+ messages in thread From: David Malcolm @ 2020-08-06 19:53 UTC (permalink / raw) To: Andrea Corallo; +Cc: jit, nd, gcc-patches On Mon, 2020-08-03 at 10:07 +0200, Andrea Corallo wrote: > David Malcolm <dmalcolm@redhat.com> writes: > > > On Fri, 2020-07-24 at 18:05 -0400, David Malcolm via Gcc-patches wrote: > > > > [...] > > > >> I haven't thought this through in detail, and I'm not sure exactly > >> how > >> it would work for arbitrary types, but I thought it worth sharing. > >> (For example I can think of nasty issues if we ever want to support > >> cross-compilation, e.g. where sizeof types or endianness differs > >> between host and target). > > > > ...which is an argument in favor of retaining the name "blob", perhaps > > as the name of the argument in the header file e.g.: > > > > extern void > > gcc_jit_global_set_initializer (gcc_jit_lvalue *global, > > const void *blob, > > size_t num_bytes); > > > > > > as a subtle hint to the user that they need to be wary about binary > > layouts ("here be dragons"). > > > > [...] > > Hi Dave & all, > > following up this is my take on the implementation of: > > gcc_jit_global_set_initializer (gcc_jit_lvalue *global, > const void *blob, > size_t num_bytes); > > 'global' must be an array but in the seek of generality it now supports > all the various integral types and is not limited to char[]. > > As you anticipated the implementation I came up is currently not safe > for cross-compilation, not sure is requirement tho. > > make check-jit is clean > > Feedback very welcome > > Thanks! Thanks for the updated patch. Comments inline below. > Andrea > > gcc/jit/ChangeLog > > 2020-08-01 Andrea Corallo <andrea.corallo@arm.com> > > * docs/topics/compatibility.rst (LIBGCCJIT_ABI_14): New ABI tag. > * docs/topics/expressions.rst (gcc_jit_global_set_initializer): > Document new entry point in section 'Global variables'. > * jit-playback.c (global_new_decl, global_finalize_lvalue): New > method. > (playback::context::new_global): Make use of global_new_decl, > global_finalize_lvalue. > (load_blob_in_ctor): New template function in use by the > following. > (playback::context::new_global_initialized): New method. > * jit-playback.h (class context): Decl 'new_global_initialized', > 'global_new_decl', 'global_finalize_lvalue'. > (lvalue::set_initializer): Add implementation. > * jit-recording.c (recording::memento_of_get_pointer::get_size) > (recording::memento_of_get_type::get_size): Add implementation. > (recording::global::write_initializer_reproducer): New function in > use by 'recording::global::write_reproducer'. > (recording::global::replay_into) > (recording::global::write_to_dump) > (recording::global::write_reproducer): Handle > initialized case. > * jit-recording.h (class type): Decl 'get_size' and > 'num_elements'. > * libgccjit++.h (class lvalue): Declare new 'set_initializer' > method. > (class lvalue): Decl 'is_global' and 'set_initializer'. > (class class global) Decl 'write_initializer_reproducer'. Add > 'm_initializer', 'm_initializer_num_bytes' fields. Implement > 'set_initializer'. > * libgccjit.c (gcc_jit_global_set_initializer): New function. > * libgccjit.h (gcc_jit_global_set_initializer): New function > declaration. > * libgccjit.map (LIBGCCJIT_ABI_14): New ABI tag. > > gcc/testsuite/ChangeLog > > 2020-08-01 Andrea Corallo <andrea.corallo@arm.com> > > * jit.dg/all-non-failing-tests.h: Add test-blob.c. > * jit.dg/test-global-set-initializer.c: New testcase. [...] > diff --git a/gcc/jit/docs/topics/expressions.rst b/gcc/jit/docs/topics/expressions.rst > index d783ceea51a8..7699dcfd27be 100644 > --- a/gcc/jit/docs/topics/expressions.rst > +++ b/gcc/jit/docs/topics/expressions.rst > @@ -582,6 +582,27 @@ Global variables > referring to it. Analogous to using an "extern" global from a > header file. > > +.. function:: gcc_jit_lvalue *\ > + gcc_jit_global_set_initializer (gcc_jit_lvalue *global,\ > + const void *blob,\ > + size_t num_bytes) > + > + Set an initializer for an object using the memory content pointed > + by ``blob`` for ``num_bytes``. ``global`` must be an arrays of an Typo: "arrays" -> "array" > + integral type. Why the non-void return type? Looking at libgccjit.c I see it returns "global" if it succeeds, or NULL if it fails. Wouldn't it be better to simply have void return type, and rely on the normaly error-handling mechanisms? Or is this inspired by the inline asm patch? (for PR 87291) [...] > --- a/gcc/jit/jit-playback.h > +++ b/gcc/jit/jit-playback.h > @@ -111,6 +111,15 @@ public: > type *type, > const char *name); > > + lvalue * > + new_global_initialized (location *loc, > + enum gcc_jit_global_kind kind, > + type *type, > + size_t element_size, > + size_t initializer_num_elem, > + const void *initializer, > + const char *name); > + > template <typename HOST_TYPE> > rvalue * > new_rvalue_from_const (type *type, > @@ -266,6 +275,14 @@ private: > const char * get_path_s_file () const; > const char * get_path_so_file () const; > > + tree > + global_new_decl (location *loc, > + enum gcc_jit_global_kind kind, > + type *type, > + const char *name); > + lvalue * > + global_finalize_lvalue (tree inner); > + > private: > > /* Functions for implementing "compile". */ > diff --git a/gcc/jit/jit-playback.c b/gcc/jit/jit-playback.c > index 0fddf04da873..52fc92f5928c 100644 > --- a/gcc/jit/jit-playback.c > +++ b/gcc/jit/jit-playback.c > @@ -510,14 +510,14 @@ new_function (location *loc, > return func; > } > > -/* Construct a playback::lvalue instance (wrapping a tree). */ > +/* In use by new_global and new_global_initialized. */ > > -playback::lvalue * > +tree > playback::context:: > -new_global (location *loc, > - enum gcc_jit_global_kind kind, > - type *type, > - const char *name) > +global_new_decl (location *loc, > + enum gcc_jit_global_kind kind, > + type *type, > + const char *name) > { > gcc_assert (type); > gcc_assert (name); > @@ -547,6 +547,15 @@ new_global (location *loc, > if (loc) > set_tree_location (inner, loc); > > + return inner; > +} > + > +/* In use by new_global and new_global_initialized. */ > + > +playback::lvalue * > +playback::context:: > +global_finalize_lvalue (tree inner) > +{ > varpool_node::get_create (inner); > > varpool_node::finalize_decl (inner); > @@ -556,6 +565,90 @@ new_global (location *loc, > return new lvalue (this, inner); > } > > +/* Construct a playback::lvalue instance (wrapping a tree). */ > + > +playback::lvalue * > +playback::context:: > +new_global (location *loc, > + enum gcc_jit_global_kind kind, > + type *type, > + const char *name) > +{ > + tree inner = global_new_decl (loc, kind, type, name); > + > + return global_finalize_lvalue (inner); > +} > + > +/* Fill 'constructor_elements' with the memory content of > + 'initializer'. Each element of the initializer is of the size of > + type T. In use by new_global_initialized.*/ > + > +template<typename T> > +static void > +load_blob_in_ctor (vec<constructor_elt, va_gc> *&constructor_elements, > + size_t num_elem, > + const void *initializer) > +{ > + /* Loosely based on 'output_init_element' c-typeck.c:9691. */ > + const T *p = (const T *)initializer; > + tree node = make_unsigned_type (BITS_PER_UNIT * sizeof (T)); > + for (size_t i = 0; i < num_elem; i++) > + { > + constructor_elt celt = > + { build_int_cst (long_unsigned_type_node, i), > + build_int_cst (node, p[i]) }; > + vec_safe_push (constructor_elements, celt); > + } > +} > + > +/* Construct an initialized playback::lvalue instance (wrapping a > + tree). */ > + > +playback::lvalue * > +playback::context:: > +new_global_initialized (location *loc, > + enum gcc_jit_global_kind kind, > + type *type, > + size_t element_size, > + size_t initializer_num_elem, > + const void *initializer, > + const char *name) > +{ > + tree inner = global_new_decl (loc, kind, type, name); > + > + static vec<constructor_elt, va_gc> *constructor_elements; Why the use of a function-level static variable here, and why va_gc? Wouldn't an auto_vec<constructor_elt> be cleaner? Ah: is it because of the call to build_constructor? If so, can the "static" be removed and replaced with an "= NULL;" initializer? (I'm very wary of function-level static variables, as they're a place when state can "hide" between multiple in-process invocations of the compiler) > + > + switch (element_size) > + { > + case 1: > + load_blob_in_ctor<uint8_t> (constructor_elements, initializer_num_elem, > + initializer); > + break; > + case 2: > + load_blob_in_ctor<uint16_t> (constructor_elements, initializer_num_elem, > + initializer); > + break; > + case 4: > + load_blob_in_ctor<uint32_t> (constructor_elements, initializer_num_elem, > + initializer); > + break; > + case 8: > + load_blob_in_ctor<uint64_t> (constructor_elements, initializer_num_elem, > + initializer); > + break; > + default: > + gcc_unreachable (); > + } > + /* Compare with 'pop_init_level' c-typeck.c:8780. */ > + tree ctor = build_constructor (type->as_tree (), constructor_elements); > + constructor_elements = NULL; > + > + /* Compare with 'store_init_value' c-typeck.c:7555. */ > + DECL_INITIAL (inner) = ctor; > + > + return global_finalize_lvalue (inner); > +} > + > /* Implementation of the various > gcc::jit::playback::context::new_rvalue_from_const <HOST_TYPE> > methods. > diff --git a/gcc/jit/jit-recording.h b/gcc/jit/jit-recording.h > index 726b9c4b8371..7a7a61a0e126 100644 > --- a/gcc/jit/jit-recording.h > +++ b/gcc/jit/jit-recording.h > @@ -502,6 +502,7 @@ public: > This will return NULL if it's not valid to dereference this type. > The caller is responsible for setting an error. */ > virtual type *dereference () = 0; > + virtual size_t get_size () { gcc_unreachable (); } It looks like get_size is only implemented for memento_of_get_pointer and memento_of_get_type::get_size. A gcc_unreachable in the base class seems like a "code smell", but it's probably OK here: I believe the restriction here is that this should only be called on initializers for globals, and we only set these on globals of certain types, right? Please can you capture that restriction in a comment next to the gcc_unreachable. > /* Dynamic casts. */ > virtual function_type *dyn_cast_function_type () { return NULL; } > @@ -534,6 +535,7 @@ public: > virtual type *is_array () = 0; > virtual bool is_void () const { return false; } > virtual bool has_known_size () const { return true; } > + virtual int num_elements () { gcc_unreachable (); } Again, I'm wary of a virtual fn with a gcc_unreachable in the base class. Here, I believe it's only implemented for array_type. Wouldn't this be better as a non-virtual member function of array_type, and simply cast to the subclass at any caller(s)? > bool is_numeric () const > { > @@ -569,6 +571,8 @@ public: > > type *dereference () FINAL OVERRIDE; > > + size_t get_size () FINAL OVERRIDE; > + > bool accepts_writes_from (type *rtype) FINAL OVERRIDE > { > if (m_kind == GCC_JIT_TYPE_VOID_PTR) > @@ -610,6 +614,8 @@ public: > > type *dereference () FINAL OVERRIDE { return m_other_type; } > > + size_t get_size () FINAL OVERRIDE; > + > bool accepts_writes_from (type *rtype) FINAL OVERRIDE; > > void replay_into (replayer *r) FINAL OVERRIDE; > @@ -755,6 +761,7 @@ class array_type : public type > bool is_bool () const FINAL OVERRIDE { return false; } > type *is_pointer () FINAL OVERRIDE { return NULL; } > type *is_array () FINAL OVERRIDE { return m_element_type; } > + int num_elements () FINAL OVERRIDE { return m_num_elements; } > > void replay_into (replayer *) FINAL OVERRIDE; > > @@ -1107,6 +1114,8 @@ public: > > const char *access_as_rvalue (reproducer &r) OVERRIDE; > virtual const char *access_as_lvalue (reproducer &r); > + virtual bool is_global () const { return false; } > + virtual void set_initializer (const void *, size_t) { gcc_unreachable (); } Similar comments here. > class param : public lvalue > @@ -1335,8 +1344,23 @@ public: > > void write_to_dump (dump &d) FINAL OVERRIDE; > > + virtual bool is_global () const FINAL OVERRIDE { return true; } No need to add "virtual" on a function that's labelled FINAL and/or OVERRIDE. > + virtual void > + set_initializer (const void *initializer, > + size_t num_bytes) FINAL OVERRIDE > + { > + if (m_initializer) > + free (m_initializer); > + m_initializer = xmalloc (num_bytes); > + memcpy (m_initializer, initializer, num_bytes); > + m_initializer_num_bytes = num_bytes; > + } As noted above, could this be non-virtual, and use casting instead? I can't remember if this class has a dtor, but presumably we should also be freeing m_initializer there. > private: > string * make_debug_string () FINAL OVERRIDE { return m_name; } > + template <typename T> > + void write_initializer_reproducer (const char *id, reproducer &r); > void write_reproducer (reproducer &r) FINAL OVERRIDE; > enum precedence get_precedence () const FINAL OVERRIDE > { > @@ -1346,6 +1370,8 @@ private: > private: > enum gcc_jit_global_kind m_kind; > string *m_name; > + void *m_initializer = NULL; > + size_t m_initializer_num_bytes = 0; > }; Are we still targetting C++98, and are these initializers compatible with it? > template <typename HOST_TYPE> > diff --git a/gcc/jit/jit-recording.c b/gcc/jit/jit-recording.c > index b73cd76a0a02..f97de00f63c3 100644 > --- a/gcc/jit/jit-recording.c > +++ b/gcc/jit/jit-recording.c [...] > @@ -4440,9 +4510,26 @@ recording::global::write_to_dump (dump &d) > d.write ("extern "); > break; > } > - d.write ("%s %s;\n", > + > + d.write ("%s %s", > m_type->get_debug_string (), > get_debug_string ()); > + > + if (!m_initializer) > + { > + d.write (";\n"); > + return; > + } > + > + d.write ("=\n { "); > + const char *p = (const char *)m_initializer; Should be "const unsigned char"? > + for (size_t i = 0; i < m_initializer_num_bytes; i++) > + { > + d.write ("0x%x, ", p[i]); > + if (i && !(i % 64)) > + d.write ("\n "); > + } > + d.write ("};\n"); > } > > /* A table of enum gcc_jit_global_kind values expressed in string > @@ -4454,6 +4541,27 @@ static const char * const global_kind_reproducer_strings[] = { > "GCC_JIT_GLOBAL_IMPORTED" > }; > > +template <typename T> > +void > +recording::global::write_initializer_reproducer (const char *id, reproducer &r) > +{ > + const char *init_id = r.make_tmp_identifier ("init_for", this); > + r.write (" %s %s[] =\n {", > + m_type->dereference ()->get_debug_string (), > + init_id); > + > + const T *p = (const T *)m_initializer; > + for (size_t i = 0; i < m_initializer_num_bytes / sizeof (T); i++) > + { > + r.write ("%lu, ", (uint64_t)p[i]); Does this assume that "%lu" matches uint64_t? > + if (i && !(i % 64)) > + r.write ("\n "); > + } > + r.write ("};\n"); > + r.write (" gcc_jit_global_set_initializer (%s, %s, sizeof (%s));\n", > + id, init_id, init_id); > +} > + > /* Implementation of recording::memento::write_reproducer for globals. */ > > void > @@ -4472,6 +4580,25 @@ recording::global::write_reproducer (reproducer &r) > global_kind_reproducer_strings[m_kind], > r.get_identifier_as_type (get_type ()), > m_name->get_debug_string ()); > + > + if (m_initializer) > + switch (m_type->dereference ()->get_size ()) > + { > + case 1: > + write_initializer_reproducer<uint8_t> (id, r); > + break; > + case 2: > + write_initializer_reproducer<uint16_t> (id, r); > + break; > + case 4: > + write_initializer_reproducer<uint32_t> (id, r); > + break; > + case 8: > + write_initializer_reproducer<uint64_t> (id, r); > + break; > + default: > + gcc_unreachable (); > + } > } > > /* The implementation of the various const-handling classes: > diff --git a/gcc/jit/libgccjit++.h b/gcc/jit/libgccjit++.h > index 69e67766640c..1b9ef1a5db98 100644 > --- a/gcc/jit/libgccjit++.h > +++ b/gcc/jit/libgccjit++.h > @@ -488,6 +488,7 @@ namespace gccjit > location loc = location ()); > > rvalue get_address (location loc = location ()); > + lvalue set_initializer (const void *blob, size_t num_bytes); > }; > > class param : public lvalue > @@ -1737,6 +1738,15 @@ lvalue::get_address (location loc) > loc.get_inner_location ())); > } > > +inline lvalue > +lvalue::set_initializer (const void *blob, size_t num_bytes) > +{ > + gcc_jit_global_set_initializer (get_inner_lvalue (), > + blob, > + num_bytes); > + return *this; > +} (Again the return type) > // class param : public lvalue > inline param::param () : lvalue () {} > inline param::param (gcc_jit_param *inner) > diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h > index 1c5a12e9c015..08b855230f6a 100644 > --- a/gcc/jit/libgccjit.h > +++ b/gcc/jit/libgccjit.h > @@ -788,6 +788,20 @@ gcc_jit_context_new_global (gcc_jit_context *ctxt, > gcc_jit_type *type, > const char *name); > > +#define LIBGCCJIT_HAVE_global_set_initializer > + > +/* Set a static initializer for a global return the global itself. > + > + This API entrypoint was added in LIBGCCJIT_ABI_14; you can test for its > + presence using > + #ifdef LIBGCCJIT_HAVE_gcc_jit_global_set_initializer > +*/ > + > +extern gcc_jit_lvalue * > +gcc_jit_global_set_initializer (gcc_jit_lvalue *global, > + const void *blob, > + size_t num_bytes); > + > /* Upcasting. */ > extern gcc_jit_object * > gcc_jit_lvalue_as_object (gcc_jit_lvalue *lvalue); > diff --git a/gcc/jit/libgccjit.c b/gcc/jit/libgccjit.c > index 3d04f6db3aff..80c5aa6ac115 100644 > --- a/gcc/jit/libgccjit.c > +++ b/gcc/jit/libgccjit.c > @@ -1117,6 +1117,42 @@ gcc_jit_context_new_global (gcc_jit_context *ctxt, > return (gcc_jit_lvalue *)ctxt->new_global (loc, kind, type, name); > } > > +/* Public entrypoint. See description in libgccjit.h. > + > + After error-checking, the real work is done by the > + gcc::jit::recording::global::set_initializer method, in > + jit-recording.c. */ > + > +extern gcc_jit_lvalue * > +gcc_jit_global_set_initializer (gcc_jit_lvalue *global, > + const void *blob, > + size_t num_bytes) > +{ > + RETURN_NULL_IF_FAIL (global, NULL, NULL, "NULL global"); > + RETURN_NULL_IF_FAIL (blob, NULL, NULL, "NULL blob"); > + RETURN_NULL_IF_FAIL_PRINTF1 (global->is_global (), NULL, NULL, > + "global \"%s\" not a global", ^^^^^^ "lvalue \"%s\" is not a global", surely, as it's not a global. > + global->get_debug_string ()); > + gcc::jit::recording::type *lval_type = global->get_type (); > + RETURN_NULL_IF_FAIL_PRINTF1 (lval_type->is_array (), NULL, NULL, > + "global \"%s\" not an array", "not" -> "is not". > + global->get_debug_string ()); > + RETURN_NULL_IF_FAIL_PRINTF1 (lval_type->dereference ()->is_int (), NULL, NULL, > + "global \"%s\" not an array of integral type", "not" -> "is not". > + global->get_debug_string ()); > + RETURN_NULL_IF_FAIL_PRINTF1 ( > + lval_type->dereference ()->get_size() * lval_type->num_elements () > + == num_bytes, > + NULL, NULL, > + "global \"%s\" and initializer don't match size", Would be more user-friendly to specify the two values in the error message, though off the top of my head I don't remember the format code for size_t. > + global->get_debug_string ()); > + > + global->set_initializer (blob, num_bytes); > + > + return global; > +} > + [...] Thanks again for the patch; hope this is constructive Dave ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH V2] libgccjit: Add new gcc_jit_context_new_blob entry point 2020-08-06 19:53 ` David Malcolm @ 2020-08-19 7:17 ` Andrea Corallo 2020-09-09 7:51 ` Andrea Corallo 2020-09-10 22:22 ` David Malcolm 0 siblings, 2 replies; 14+ messages in thread From: Andrea Corallo @ 2020-08-19 7:17 UTC (permalink / raw) To: David Malcolm; +Cc: jit, nd, gcc-patches [-- Attachment #1: Type: text/plain, Size: 8075 bytes --] David Malcolm <dmalcolm@redhat.com> writes: > Thanks for the updated patch. Comments inline below. Hi Dave, sorry for the late reply. > [...] > >> diff --git a/gcc/jit/docs/topics/expressions.rst b/gcc/jit/docs/topics/expressions.rst >> index d783ceea51a8..7699dcfd27be 100644 >> --- a/gcc/jit/docs/topics/expressions.rst >> +++ b/gcc/jit/docs/topics/expressions.rst >> @@ -582,6 +582,27 @@ Global variables >> referring to it. Analogous to using an "extern" global from a >> header file. >> >> +.. function:: gcc_jit_lvalue *\ >> + gcc_jit_global_set_initializer (gcc_jit_lvalue *global,\ >> + const void *blob,\ >> + size_t num_bytes) >> + >> + Set an initializer for an object using the memory content pointed >> + by ``blob`` for ``num_bytes``. ``global`` must be an arrays of an > > Typo: "arrays" -> "array" Fixed >> + integral type. > > Why the non-void return type? Looking at libgccjit.c I see it returns > "global" if it succeeds, or NULL if it fails. Wouldn't it be better to > simply have void return type, and rely on the normaly error-handling > mechanisms? > Or is this inspired by the inline asm patch? (for PR 87291) The idea is that this way the user could also create the global, initialize it and directly use it like: foo (gcc_jit_global_set_initializer (gcc_jit_context_new_global (...), ...)) I left it that way, let me know if you prefer otherwise. [...] > >> --- a/gcc/jit/jit-playback.h >> +++ b/gcc/jit/jit-playback.h >> @@ -111,6 +111,15 @@ public: >> type *type, >> const char *name); >> >> + lvalue * >> + new_global_initialized (location *loc, >> + enum gcc_jit_global_kind kind, >> + type *type, >> + size_t element_size, >> + size_t initializer_num_elem, >> + const void *initializer, >> + const char *name); >> + >> template <typename HOST_TYPE> >> rvalue * >> new_rvalue_from_const (type *type, >> @@ -266,6 +275,14 @@ private: >> const char * get_path_s_file () const; >> const char * get_path_so_file () const; >> >> + tree >> + global_new_decl (location *loc, >> + enum gcc_jit_global_kind kind, >> + type *type, >> + const char *name); >> + lvalue * >> + global_finalize_lvalue (tree inner); >> + >> private: >> >> /* Functions for implementing "compile". */ >> diff --git a/gcc/jit/jit-playback.c b/gcc/jit/jit-playback.c >> index 0fddf04da873..52fc92f5928c 100644 >> --- a/gcc/jit/jit-playback.c >> +++ b/gcc/jit/jit-playback.c >> @@ -510,14 +510,14 @@ new_function (location *loc, >> return func; >> } >> >> -/* Construct a playback::lvalue instance (wrapping a tree). */ >> +/* In use by new_global and new_global_initialized. */ >> >> -playback::lvalue * >> +tree >> playback::context:: >> -new_global (location *loc, >> - enum gcc_jit_global_kind kind, >> - type *type, >> - const char *name) >> +global_new_decl (location *loc, >> + enum gcc_jit_global_kind kind, >> + type *type, >> + const char *name) >> { >> gcc_assert (type); >> gcc_assert (name); >> @@ -547,6 +547,15 @@ new_global (location *loc, >> if (loc) >> set_tree_location (inner, loc); >> >> + return inner; >> +} >> + >> +/* In use by new_global and new_global_initialized. */ >> + >> +playback::lvalue * >> +playback::context:: >> +global_finalize_lvalue (tree inner) >> +{ >> varpool_node::get_create (inner); >> >> varpool_node::finalize_decl (inner); >> @@ -556,6 +565,90 @@ new_global (location *loc, >> return new lvalue (this, inner); >> } >> >> +/* Construct a playback::lvalue instance (wrapping a tree). */ >> + >> +playback::lvalue * >> +playback::context:: >> +new_global (location *loc, >> + enum gcc_jit_global_kind kind, >> + type *type, >> + const char *name) >> +{ >> + tree inner = global_new_decl (loc, kind, type, name); >> + >> + return global_finalize_lvalue (inner); >> +} >> + >> +/* Fill 'constructor_elements' with the memory content of >> + 'initializer'. Each element of the initializer is of the size of >> + type T. In use by new_global_initialized.*/ >> + >> +template<typename T> >> +static void >> +load_blob_in_ctor (vec<constructor_elt, va_gc> *&constructor_elements, >> + size_t num_elem, >> + const void *initializer) >> +{ >> + /* Loosely based on 'output_init_element' c-typeck.c:9691. */ >> + const T *p = (const T *)initializer; >> + tree node = make_unsigned_type (BITS_PER_UNIT * sizeof (T)); >> + for (size_t i = 0; i < num_elem; i++) >> + { >> + constructor_elt celt = >> + { build_int_cst (long_unsigned_type_node, i), >> + build_int_cst (node, p[i]) }; >> + vec_safe_push (constructor_elements, celt); >> + } >> +} >> + >> +/* Construct an initialized playback::lvalue instance (wrapping a >> + tree). */ >> + >> +playback::lvalue * >> +playback::context:: >> +new_global_initialized (location *loc, >> + enum gcc_jit_global_kind kind, >> + type *type, >> + size_t element_size, >> + size_t initializer_num_elem, >> + const void *initializer, >> + const char *name) >> +{ >> + tree inner = global_new_decl (loc, kind, type, name); >> + >> + static vec<constructor_elt, va_gc> *constructor_elements; > > Why the use of a function-level static variable here, and why va_gc? > Wouldn't an auto_vec<constructor_elt> be cleaner? > Ah: is it because of the call to build_constructor? > If so, can the "static" be removed and replaced with an "= NULL;" > initializer? > > (I'm very wary of function-level static variables, as they're a place > when state can "hide" between multiple in-process invocations of the > compiler) Sorry that's due to my ignorance on how GCC GC works, I thought GC roots had to be static, fixed. [...] >> private: >> string * make_debug_string () FINAL OVERRIDE { return m_name; } >> + template <typename T> >> + void write_initializer_reproducer (const char *id, reproducer &r); >> void write_reproducer (reproducer &r) FINAL OVERRIDE; >> enum precedence get_precedence () const FINAL OVERRIDE >> { >> @@ -1346,6 +1370,8 @@ private: >> private: >> enum gcc_jit_global_kind m_kind; >> string *m_name; >> + void *m_initializer = NULL; >> + size_t m_initializer_num_bytes = 0; >> }; > > Are we still targetting C++98, and are these initializers compatible > with it? Apparently C++03 but AFAIU this should be compatible only with C++11 on, moved the zeroing into the ctor. >> template <typename HOST_TYPE> >> diff --git a/gcc/jit/jit-recording.c b/gcc/jit/jit-recording.c >> index b73cd76a0a02..f97de00f63c3 100644 >> --- a/gcc/jit/jit-recording.c >> +++ b/gcc/jit/jit-recording.c > > [...] > >> @@ -4440,9 +4510,26 @@ recording::global::write_to_dump (dump &d) >> d.write ("extern "); >> break; >> } >> - d.write ("%s %s;\n", >> + >> + d.write ("%s %s", >> m_type->get_debug_string (), >> get_debug_string ()); >> + >> + if (!m_initializer) >> + { >> + d.write (";\n"); >> + return; >> + } >> + >> + d.write ("=\n { "); >> + const char *p = (const char *)m_initializer; > > Should be "const unsigned char"? Probably better, fixed. >> + for (size_t i = 0; i < m_initializer_num_bytes; i++) >> + { >> + d.write ("0x%x, ", p[i]); >> + if (i && !(i % 64)) >> + d.write ("\n "); >> + } >> + d.write ("};\n"); >> } >> >> /* A table of enum gcc_jit_global_kind values expressed in string >> @@ -4454,6 +4541,27 @@ static const char * const global_kind_reproducer_strings[] = { >> "GCC_JIT_GLOBAL_IMPORTED" >> }; [...] > > Thanks again for the patch; hope this is constructive > Dave Sure it is, thanks for reviewing. Attached the updated version of the patch. make check-jit is clean plus I tested the new entry point with the modified Emacs. Thanks Andrea [-- Attachment #2: 0001-Add-new-gcc_jit_global_set_initializer-entry-point.patch --] [-- Type: text/plain, Size: 24798 bytes --] From 74970a5ac988e8a641967dee39ec8fa711228a91 Mon Sep 17 00:00:00 2001 From: Andrea Corallo <andrea.corallo@arm.com> Date: Sat, 30 May 2020 10:33:08 +0100 Subject: [PATCH] Add new gcc_jit_global_set_initializer entry point gcc/jit/ChangeLog 2020-08-01 Andrea Corallo <andrea.corallo@arm.com> * docs/topics/compatibility.rst (LIBGCCJIT_ABI_14): New ABI tag. * docs/topics/expressions.rst (gcc_jit_global_set_initializer): Document new entry point in section 'Global variables'. * jit-playback.c (global_new_decl, global_finalize_lvalue): New method. (playback::context::new_global): Make use of global_new_decl, global_finalize_lvalue. (load_blob_in_ctor): New template function in use by the following. (playback::context::new_global_initialized): New method. * jit-playback.h (class context): Decl 'new_global_initialized', 'global_new_decl', 'global_finalize_lvalue'. (lvalue::set_initializer): Add implementation. * jit-recording.c (recording::memento_of_get_pointer::get_size) (recording::memento_of_get_type::get_size): Add implementation. (recording::global::write_initializer_reproducer): New function in use by 'recording::global::write_reproducer'. (recording::global::replay_into) (recording::global::write_to_dump) (recording::global::write_reproducer): Handle initialized case. * jit-recording.h (class type): Decl 'get_size' and 'num_elements'. * libgccjit++.h (class lvalue): Declare new 'set_initializer' method. (class lvalue): Decl 'is_global' and 'set_initializer'. (class global) Decl 'write_initializer_reproducer'. Add 'm_initializer', 'm_initializer_num_bytes' fields. Implement 'set_initializer'. Add a destructor to free 'm_initializer'. * libgccjit.c (gcc_jit_global_set_initializer): New function. * libgccjit.h (gcc_jit_global_set_initializer): New function declaration. * libgccjit.map (LIBGCCJIT_ABI_14): New ABI tag. gcc/testsuite/ChangeLog 2020-08-01 Andrea Corallo <andrea.corallo@arm.com> * jit.dg/all-non-failing-tests.h: Add test-blob.c. * jit.dg/test-global-set-initializer.c: New testcase. --- gcc/jit/docs/topics/compatibility.rst | 7 + gcc/jit/docs/topics/expressions.rst | 21 +++ gcc/jit/jit-playback.c | 105 +++++++++++++- gcc/jit/jit-playback.h | 17 +++ gcc/jit/jit-recording.c | 137 +++++++++++++++++- gcc/jit/jit-recording.h | 38 ++++- gcc/jit/libgccjit++.h | 10 ++ gcc/jit/libgccjit.c | 38 +++++ gcc/jit/libgccjit.h | 14 ++ gcc/jit/libgccjit.map | 7 +- gcc/testsuite/jit.dg/all-non-failing-tests.h | 7 + .../jit.dg/test-global-set-initializer.c | 78 ++++++++++ 12 files changed, 466 insertions(+), 13 deletions(-) create mode 100644 gcc/testsuite/jit.dg/test-global-set-initializer.c diff --git a/gcc/jit/docs/topics/compatibility.rst b/gcc/jit/docs/topics/compatibility.rst index bb3387fa583..6bfa101ed71 100644 --- a/gcc/jit/docs/topics/compatibility.rst +++ b/gcc/jit/docs/topics/compatibility.rst @@ -219,3 +219,10 @@ entrypoints: * :func:`gcc_jit_version_minor` * :func:`gcc_jit_version_patchlevel` + +.. _LIBGCCJIT_ABI_14: + +``LIBGCCJIT_ABI_14`` +-------------------- +``LIBGCCJIT_ABI_14`` covers the addition of +:func:`gcc_jit_global_set_initializer` diff --git a/gcc/jit/docs/topics/expressions.rst b/gcc/jit/docs/topics/expressions.rst index d783ceea51a..28f81be0060 100644 --- a/gcc/jit/docs/topics/expressions.rst +++ b/gcc/jit/docs/topics/expressions.rst @@ -582,6 +582,27 @@ Global variables referring to it. Analogous to using an "extern" global from a header file. +.. function:: gcc_jit_lvalue *\ + gcc_jit_global_set_initializer (gcc_jit_lvalue *global,\ + const void *blob,\ + size_t num_bytes) + + Set an initializer for an object using the memory content pointed + by ``blob`` for ``num_bytes``. ``global`` must be an array of an + integral type. + + The parameter ``blob`` must be non-NULL. The call copies the memory + pointed by ``blob`` for ``num_bytes`` bytes, so it is valid to pass + in a pointer to an on-stack buffer. The content will be stored in + the compilation unit and used as initialization value of the array. + + This entrypoint was added in :ref:`LIBGCCJIT_ABI_14`; you can test for + its presence using + + .. code-block:: c + + #ifdef LIBGCCJIT_HAVE_gcc_jit_global_set_initializer + Working with pointers, structs and unions ----------------------------------------- diff --git a/gcc/jit/jit-playback.c b/gcc/jit/jit-playback.c index 0fddf04da87..b68e18278e3 100644 --- a/gcc/jit/jit-playback.c +++ b/gcc/jit/jit-playback.c @@ -510,14 +510,14 @@ new_function (location *loc, return func; } -/* Construct a playback::lvalue instance (wrapping a tree). */ +/* In use by new_global and new_global_initialized. */ -playback::lvalue * +tree playback::context:: -new_global (location *loc, - enum gcc_jit_global_kind kind, - type *type, - const char *name) +global_new_decl (location *loc, + enum gcc_jit_global_kind kind, + type *type, + const char *name) { gcc_assert (type); gcc_assert (name); @@ -547,6 +547,15 @@ new_global (location *loc, if (loc) set_tree_location (inner, loc); + return inner; +} + +/* In use by new_global and new_global_initialized. */ + +playback::lvalue * +playback::context:: +global_finalize_lvalue (tree inner) +{ varpool_node::get_create (inner); varpool_node::finalize_decl (inner); @@ -556,6 +565,90 @@ new_global (location *loc, return new lvalue (this, inner); } +/* Construct a playback::lvalue instance (wrapping a tree). */ + +playback::lvalue * +playback::context:: +new_global (location *loc, + enum gcc_jit_global_kind kind, + type *type, + const char *name) +{ + tree inner = global_new_decl (loc, kind, type, name); + + return global_finalize_lvalue (inner); +} + +/* Fill 'constructor_elements' with the memory content of + 'initializer'. Each element of the initializer is of the size of + type T. In use by new_global_initialized.*/ + +template<typename T> +static void +load_blob_in_ctor (vec<constructor_elt, va_gc> *&constructor_elements, + size_t num_elem, + const void *initializer) +{ + /* Loosely based on 'output_init_element' c-typeck.c:9691. */ + const T *p = (const T *)initializer; + tree node = make_unsigned_type (BITS_PER_UNIT * sizeof (T)); + for (size_t i = 0; i < num_elem; i++) + { + constructor_elt celt = + { build_int_cst (long_unsigned_type_node, i), + build_int_cst (node, p[i]) }; + vec_safe_push (constructor_elements, celt); + } +} + +/* Construct an initialized playback::lvalue instance (wrapping a + tree). */ + +playback::lvalue * +playback::context:: +new_global_initialized (location *loc, + enum gcc_jit_global_kind kind, + type *type, + size_t element_size, + size_t initializer_num_elem, + const void *initializer, + const char *name) +{ + tree inner = global_new_decl (loc, kind, type, name); + + vec<constructor_elt, va_gc> *constructor_elements = NULL; + + switch (element_size) + { + case 1: + load_blob_in_ctor<uint8_t> (constructor_elements, initializer_num_elem, + initializer); + break; + case 2: + load_blob_in_ctor<uint16_t> (constructor_elements, initializer_num_elem, + initializer); + break; + case 4: + load_blob_in_ctor<uint32_t> (constructor_elements, initializer_num_elem, + initializer); + break; + case 8: + load_blob_in_ctor<uint64_t> (constructor_elements, initializer_num_elem, + initializer); + break; + default: + gcc_unreachable (); + } + /* Compare with 'pop_init_level' c-typeck.c:8780. */ + tree ctor = build_constructor (type->as_tree (), constructor_elements); + constructor_elements = NULL; + + /* Compare with 'store_init_value' c-typeck.c:7555. */ + DECL_INITIAL (inner) = ctor; + + return global_finalize_lvalue (inner); +} + /* Implementation of the various gcc::jit::playback::context::new_rvalue_from_const <HOST_TYPE> methods. diff --git a/gcc/jit/jit-playback.h b/gcc/jit/jit-playback.h index f9b3e675368..50b69753bb4 100644 --- a/gcc/jit/jit-playback.h +++ b/gcc/jit/jit-playback.h @@ -111,6 +111,15 @@ public: type *type, const char *name); + lvalue * + new_global_initialized (location *loc, + enum gcc_jit_global_kind kind, + type *type, + size_t element_size, + size_t initializer_num_elem, + const void *initializer, + const char *name); + template <typename HOST_TYPE> rvalue * new_rvalue_from_const (type *type, @@ -266,6 +275,14 @@ private: const char * get_path_s_file () const; const char * get_path_so_file () const; + tree + global_new_decl (location *loc, + enum gcc_jit_global_kind kind, + type *type, + const char *name); + lvalue * + global_finalize_lvalue (tree inner); + private: /* Functions for implementing "compile". */ diff --git a/gcc/jit/jit-recording.c b/gcc/jit/jit-recording.c index b73cd76a0a0..39510097c6f 100644 --- a/gcc/jit/jit-recording.c +++ b/gcc/jit/jit-recording.c @@ -2175,6 +2175,57 @@ recording::type::access_as_type (reproducer &r) return r.get_identifier (this); } +/* Override of default implementation of + recording::type::get_size. + + Return the size in bytes. This is in use for global + initialization. */ + +size_t +recording::memento_of_get_type::get_size () +{ + int size; + switch (m_kind) + { + case GCC_JIT_TYPE_VOID: + return 0; + case GCC_JIT_TYPE_BOOL: + case GCC_JIT_TYPE_CHAR: + case GCC_JIT_TYPE_SIGNED_CHAR: + case GCC_JIT_TYPE_UNSIGNED_CHAR: + return 1; + case GCC_JIT_TYPE_SHORT: + case GCC_JIT_TYPE_UNSIGNED_SHORT: + size = SHORT_TYPE_SIZE; + break; + case GCC_JIT_TYPE_INT: + case GCC_JIT_TYPE_UNSIGNED_INT: + size = INT_TYPE_SIZE; + break; + case GCC_JIT_TYPE_LONG: + case GCC_JIT_TYPE_UNSIGNED_LONG: + size = LONG_TYPE_SIZE; + break; + case GCC_JIT_TYPE_LONG_LONG: + case GCC_JIT_TYPE_UNSIGNED_LONG_LONG: + size = LONG_LONG_TYPE_SIZE; + break; + case GCC_JIT_TYPE_FLOAT: + size = FLOAT_TYPE_SIZE; + break; + case GCC_JIT_TYPE_DOUBLE: + size = DOUBLE_TYPE_SIZE; + break; + case GCC_JIT_TYPE_LONG_DOUBLE: + size = LONG_DOUBLE_TYPE_SIZE; + break; + default: + gcc_unreachable (); + } + + return size / BITS_PER_UNIT; +} + /* Implementation of pure virtual hook recording::type::dereference for recording::memento_of_get_type. */ @@ -2482,6 +2533,15 @@ recording::memento_of_get_type::write_reproducer (reproducer &r) /* The implementation of class gcc::jit::recording::memento_of_get_pointer. */ +/* Override of default implementation of + recording::type::get_size for get_pointer. */ + +size_t +recording::memento_of_get_pointer::get_size () +{ + return POINTER_SIZE / BITS_PER_UNIT; +} + /* Override of default implementation of recording::type::accepts_writes_from for get_pointer. @@ -4393,10 +4453,20 @@ recording::block::dump_edges_to_dot (pretty_printer *pp) void recording::global::replay_into (replayer *r) { - set_playback_obj (r->new_global (playback_location (r, m_loc), - m_kind, - m_type->playback_type (), - playback_string (m_name))); + set_playback_obj ( + m_initializer + ? 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), + m_kind, + m_type->playback_type (), + playback_string (m_name))); } /* Override the default implementation of @@ -4440,9 +4510,26 @@ recording::global::write_to_dump (dump &d) d.write ("extern "); break; } - d.write ("%s %s;\n", + + d.write ("%s %s", m_type->get_debug_string (), get_debug_string ()); + + if (!m_initializer) + { + d.write (";\n"); + return; + } + + d.write ("=\n { "); + const unsigned char *p = (const unsigned char *)m_initializer; + for (size_t i = 0; i < m_initializer_num_bytes; i++) + { + d.write ("0x%x, ", p[i]); + if (i && !(i % 64)) + d.write ("\n "); + } + d.write ("};\n"); } /* A table of enum gcc_jit_global_kind values expressed in string @@ -4454,6 +4541,27 @@ static const char * const global_kind_reproducer_strings[] = { "GCC_JIT_GLOBAL_IMPORTED" }; +template <typename T> +void +recording::global::write_initializer_reproducer (const char *id, reproducer &r) +{ + const char *init_id = r.make_tmp_identifier ("init_for", this); + r.write (" %s %s[] =\n {", + m_type->dereference ()->get_debug_string (), + init_id); + + const T *p = (const T *)m_initializer; + for (size_t i = 0; i < m_initializer_num_bytes / sizeof (T); i++) + { + r.write ("%" PRIu64 ", ", (uint64_t)p[i]); + if (i && !(i % 64)) + r.write ("\n "); + } + r.write ("};\n"); + r.write (" gcc_jit_global_set_initializer (%s, %s, sizeof (%s));\n", + id, init_id, init_id); +} + /* Implementation of recording::memento::write_reproducer for globals. */ void @@ -4472,6 +4580,25 @@ recording::global::write_reproducer (reproducer &r) global_kind_reproducer_strings[m_kind], r.get_identifier_as_type (get_type ()), m_name->get_debug_string ()); + + if (m_initializer) + switch (m_type->dereference ()->get_size ()) + { + case 1: + write_initializer_reproducer<uint8_t> (id, r); + break; + case 2: + write_initializer_reproducer<uint16_t> (id, r); + break; + case 4: + write_initializer_reproducer<uint32_t> (id, r); + break; + case 8: + write_initializer_reproducer<uint64_t> (id, r); + break; + default: + gcc_unreachable (); + } } /* The implementation of the various const-handling classes: diff --git a/gcc/jit/jit-recording.h b/gcc/jit/jit-recording.h index 726b9c4b837..aa38e606a8d 100644 --- a/gcc/jit/jit-recording.h +++ b/gcc/jit/jit-recording.h @@ -502,6 +502,12 @@ public: This will return NULL if it's not valid to dereference this type. The caller is responsible for setting an error. */ virtual type *dereference () = 0; + /* Get the type size in bytes. + + This is implemented only for memento_of_get_type and + memento_of_get_pointer as is use for initializing globals of + these types. */ + virtual size_t get_size () { gcc_unreachable (); } /* Dynamic casts. */ virtual function_type *dyn_cast_function_type () { return NULL; } @@ -569,6 +575,8 @@ public: type *dereference () FINAL OVERRIDE; + size_t get_size () FINAL OVERRIDE; + bool accepts_writes_from (type *rtype) FINAL OVERRIDE { if (m_kind == GCC_JIT_TYPE_VOID_PTR) @@ -610,6 +618,8 @@ public: type *dereference () FINAL OVERRIDE { return m_other_type; } + size_t get_size () FINAL OVERRIDE; + bool accepts_writes_from (type *rtype) FINAL OVERRIDE; void replay_into (replayer *r) FINAL OVERRIDE; @@ -755,6 +765,7 @@ class array_type : public type bool is_bool () const FINAL OVERRIDE { return false; } type *is_pointer () FINAL OVERRIDE { return NULL; } type *is_array () FINAL OVERRIDE { return m_element_type; } + int num_elements () { return m_num_elements; } void replay_into (replayer *) FINAL OVERRIDE; @@ -1107,6 +1118,7 @@ public: const char *access_as_rvalue (reproducer &r) OVERRIDE; virtual const char *access_as_lvalue (reproducer &r); + virtual bool is_global () const { return false; } }; class param : public lvalue @@ -1327,7 +1339,14 @@ public: : lvalue (ctxt, loc, type), m_kind (kind), m_name (name) - {} + { + m_initializer = NULL; + m_initializer_num_bytes = 0; + } + ~global () + { + free (m_initializer); + } void replay_into (replayer *) FINAL OVERRIDE; @@ -1335,8 +1354,23 @@ public: void write_to_dump (dump &d) FINAL OVERRIDE; + bool is_global () const FINAL OVERRIDE { return true; } + + void + set_initializer (const void *initializer, + size_t num_bytes) + { + if (m_initializer) + free (m_initializer); + m_initializer = xmalloc (num_bytes); + memcpy (m_initializer, initializer, num_bytes); + m_initializer_num_bytes = num_bytes; + } + private: string * make_debug_string () FINAL OVERRIDE { return m_name; } + template <typename T> + void write_initializer_reproducer (const char *id, reproducer &r); void write_reproducer (reproducer &r) FINAL OVERRIDE; enum precedence get_precedence () const FINAL OVERRIDE { @@ -1346,6 +1380,8 @@ private: private: enum gcc_jit_global_kind m_kind; string *m_name; + void *m_initializer; + size_t m_initializer_num_bytes; }; template <typename HOST_TYPE> diff --git a/gcc/jit/libgccjit++.h b/gcc/jit/libgccjit++.h index 69e67766640..1b9ef1a5db9 100644 --- a/gcc/jit/libgccjit++.h +++ b/gcc/jit/libgccjit++.h @@ -488,6 +488,7 @@ namespace gccjit location loc = location ()); rvalue get_address (location loc = location ()); + lvalue set_initializer (const void *blob, size_t num_bytes); }; class param : public lvalue @@ -1737,6 +1738,15 @@ lvalue::get_address (location loc) loc.get_inner_location ())); } +inline lvalue +lvalue::set_initializer (const void *blob, size_t num_bytes) +{ + gcc_jit_global_set_initializer (get_inner_lvalue (), + blob, + num_bytes); + return *this; +} + // class param : public lvalue inline param::param () : lvalue () {} inline param::param (gcc_jit_param *inner) diff --git a/gcc/jit/libgccjit.c b/gcc/jit/libgccjit.c index 3d04f6db3af..bc45bd80b2c 100644 --- a/gcc/jit/libgccjit.c +++ b/gcc/jit/libgccjit.c @@ -1117,6 +1117,44 @@ gcc_jit_context_new_global (gcc_jit_context *ctxt, return (gcc_jit_lvalue *)ctxt->new_global (loc, kind, type, name); } +/* Public entrypoint. See description in libgccjit.h. + + After error-checking, the real work is done by the + gcc::jit::recording::global::set_initializer method, in + jit-recording.c. */ + +extern gcc_jit_lvalue * +gcc_jit_global_set_initializer (gcc_jit_lvalue *global, + const void *blob, + size_t num_bytes) +{ + RETURN_NULL_IF_FAIL (global, NULL, NULL, "NULL global"); + RETURN_NULL_IF_FAIL (blob, NULL, NULL, "NULL blob"); + RETURN_NULL_IF_FAIL_PRINTF1 (global->is_global (), NULL, NULL, + "lvalue \"%s\" not a global", + global->get_debug_string ()); + + gcc::jit::recording::type *lval_type = global->get_type (); + RETURN_NULL_IF_FAIL_PRINTF1 (lval_type->is_array (), NULL, NULL, + "global \"%s\" is not an array", + global->get_debug_string ()); + RETURN_NULL_IF_FAIL_PRINTF1 (lval_type->dereference ()->is_int (), NULL, NULL, + "global \"%s\" is not an array of integral type", + global->get_debug_string ()); + size_t lvalue_size = + lval_type->dereference ()->get_size () + * static_cast <gcc::jit::recording::array_type *> (lval_type)->num_elements (); + RETURN_NULL_IF_FAIL_PRINTF3 ( + lvalue_size == num_bytes, NULL, NULL, + "global \"%s\" of size %zu don't match initializer of size %zu", + global->get_debug_string (), lvalue_size, num_bytes); + + reinterpret_cast <gcc::jit::recording::global *> (global) + ->set_initializer (blob, num_bytes); + + return global; +} + /* Public entrypoint. See description in libgccjit.h. After error-checking, this calls the trivial diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h index 1c5a12e9c01..3cbcbef2693 100644 --- a/gcc/jit/libgccjit.h +++ b/gcc/jit/libgccjit.h @@ -788,6 +788,20 @@ gcc_jit_context_new_global (gcc_jit_context *ctxt, gcc_jit_type *type, const char *name); +#define LIBGCCJIT_HAVE_gcc_jit_global_set_initializer + +/* Set a static initializer for a global return the global itself. + + This API entrypoint was added in LIBGCCJIT_ABI_14; you can test for its + presence using + #ifdef LIBGCCJIT_HAVE_gcc_jit_global_set_initializer +*/ + +extern gcc_jit_lvalue * +gcc_jit_global_set_initializer (gcc_jit_lvalue *global, + const void *blob, + size_t num_bytes); + /* Upcasting. */ extern gcc_jit_object * gcc_jit_lvalue_as_object (gcc_jit_lvalue *lvalue); diff --git a/gcc/jit/libgccjit.map b/gcc/jit/libgccjit.map index 6137dd4b4b0..a6e67e781a4 100644 --- a/gcc/jit/libgccjit.map +++ b/gcc/jit/libgccjit.map @@ -186,4 +186,9 @@ LIBGCCJIT_ABI_13 { gcc_jit_version_major; gcc_jit_version_minor; gcc_jit_version_patchlevel; -} LIBGCCJIT_ABI_12; \ No newline at end of file +} LIBGCCJIT_ABI_12; + +LIBGCCJIT_ABI_14 { + global: + gcc_jit_global_set_initializer; +} LIBGCCJIT_ABI_13; diff --git a/gcc/testsuite/jit.dg/all-non-failing-tests.h b/gcc/testsuite/jit.dg/all-non-failing-tests.h index 632ab8cfb2e..4202eb7798b 100644 --- a/gcc/testsuite/jit.dg/all-non-failing-tests.h +++ b/gcc/testsuite/jit.dg/all-non-failing-tests.h @@ -174,6 +174,13 @@ #undef create_code #undef verify_code +/* test-global-set-initializer.c */ +#define create_code create_code_global_set_initializer +#define verify_code verify_code_global_set_initializer +#include "test-global-set-initializer.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-global-set-initializer.c b/gcc/testsuite/jit.dg/test-global-set-initializer.c new file mode 100644 index 00000000000..d38aba7d73f --- /dev/null +++ b/gcc/testsuite/jit.dg/test-global-set-initializer.c @@ -0,0 +1,78 @@ +#include <stdlib.h> +#include <stdio.h> +#include <string.h> +#include <time.h> + +#include "libgccjit.h" + +#include "harness.h" + +#define BIG_BLOB_SIZE (1 << 12) /* 4KB. */ + +static signed char test_blob1[] = { 0xc, 0xa, 0xf, 0xf, 0xe }; +static unsigned test_blob2[] = { 0x3, 0x2, 0x1, 0x0, 0x1, 0x2, 0x3 }; +static unsigned char test_blob3[BIG_BLOB_SIZE]; + +void +create_code (gcc_jit_context *ctxt, void *user_data) +{ + /* Let's try to inject the equivalent of: + + signed char bin_blob1[] = { 0xc, 0xa, 0xf, 0xf, 0xe }; + unsigned bin_blob2[] = { 0x3, 0x2, 0x1, 0x0, 0x1, 0x2, 0x3 }; + unsigned char bin_blob3[4096]... + */ + gcc_jit_type *unsigned_char_type = + gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_UNSIGNED_CHAR); + gcc_jit_type *signed_char_type = + gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_SIGNED_CHAR); + gcc_jit_type *unsigned_type = + gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_UNSIGNED_INT); + + gcc_jit_lvalue *glob = + gcc_jit_context_new_global ( + ctxt, NULL, GCC_JIT_GLOBAL_EXPORTED, + gcc_jit_context_new_array_type (ctxt, NULL, signed_char_type, + sizeof (test_blob1)), + "bin_blob1"); + gcc_jit_global_set_initializer (glob, test_blob1, sizeof (test_blob1)); + + glob = + gcc_jit_context_new_global ( + ctxt, NULL, GCC_JIT_GLOBAL_EXPORTED, + gcc_jit_context_new_array_type ( + ctxt, NULL, unsigned_type, + sizeof (test_blob2) / sizeof (*test_blob2)), + "bin_blob2"); + gcc_jit_global_set_initializer (glob, test_blob2, + sizeof (test_blob2)); + + for (size_t i = 0; i < BIG_BLOB_SIZE; i++) + test_blob3[i] = i * i + i; + glob = + gcc_jit_context_new_global ( + ctxt, NULL, GCC_JIT_GLOBAL_EXPORTED, + gcc_jit_context_new_array_type (ctxt, NULL, unsigned_char_type, + sizeof (test_blob3)), + "bin_blob3"); + gcc_jit_global_set_initializer (glob, test_blob3, sizeof (test_blob3)); +} + +void +verify_code (gcc_jit_context *ctxt, gcc_jit_result *result) +{ + CHECK_NON_NULL (result); + void *glob = gcc_jit_result_get_global (result, "bin_blob1"); + CHECK_NON_NULL (glob); + CHECK_VALUE (memcmp (test_blob1, glob, sizeof (test_blob1)), 0); + + glob = gcc_jit_result_get_global (result, "bin_blob2"); + CHECK_NON_NULL (glob); + CHECK_VALUE (memcmp (test_blob2, glob, + sizeof (test_blob2)), 0); + + glob = gcc_jit_result_get_global (result, "bin_blob3"); + CHECK_NON_NULL (glob); + CHECK_VALUE (memcmp (test_blob3, glob, sizeof (test_blob3)), 0); + +} -- 2.20.1 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH V2] libgccjit: Add new gcc_jit_context_new_blob entry point 2020-08-19 7:17 ` [PATCH V2] " Andrea Corallo @ 2020-09-09 7:51 ` Andrea Corallo 2020-09-10 22:22 ` David Malcolm 1 sibling, 0 replies; 14+ messages in thread From: Andrea Corallo @ 2020-09-09 7:51 UTC (permalink / raw) To: David Malcolm; +Cc: jit, nd, gcc-patches Andrea Corallo <andrea.corallo@arm.com> writes: [...] > Sure it is, thanks for reviewing. > > Attached the updated version of the patch. > > make check-jit is clean plus I tested the new entry point with the > modified Emacs. > > Thanks > > Andrea Ping ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH V2] libgccjit: Add new gcc_jit_context_new_blob entry point 2020-08-19 7:17 ` [PATCH V2] " Andrea Corallo 2020-09-09 7:51 ` Andrea Corallo @ 2020-09-10 22:22 ` David Malcolm 2020-09-11 10:31 ` Andrea Corallo 1 sibling, 1 reply; 14+ messages in thread From: David Malcolm @ 2020-09-10 22:22 UTC (permalink / raw) To: Andrea Corallo; +Cc: jit, nd, gcc-patches On Wed, 2020-08-19 at 09:17 +0200, Andrea Corallo wrote: > David Malcolm <dmalcolm@redhat.com> writes: > > > Thanks for the updated patch. Comments inline below. > > Hi Dave, > > sorry for the late reply. Likewise, sorry. [...] > > Why the non-void return type? Looking at libgccjit.c I see it returns > > "global" if it succeeds, or NULL if it fails. Wouldn't it be better to > > simply have void return type, and rely on the normaly error-handling > > mechanisms? > > Or is this inspired by the inline asm patch? (for PR 87291) > > The idea is that this way the user could also create the global, > initialize it and directly use it like: > > foo (gcc_jit_global_set_initializer (gcc_jit_context_new_global (...), ...)) > > I left it that way, let me know if you prefer otherwise. I don't have a strong preference here, so let's go with what you've written. [...] > >> +/* Construct an initialized playback::lvalue instance (wrapping a > >> + tree). */ > >> + > >> +playback::lvalue * > >> +playback::context:: > >> +new_global_initialized (location *loc, > >> + enum gcc_jit_global_kind kind, > >> + type *type, > >> + size_t element_size, > >> + size_t initializer_num_elem, > >> + const void *initializer, > >> + const char *name) > >> +{ > >> + tree inner = global_new_decl (loc, kind, type, name); > >> + > >> + static vec<constructor_elt, va_gc> *constructor_elements; > > > > Why the use of a function-level static variable here, and why va_gc? > > Wouldn't an auto_vec<constructor_elt> be cleaner? > > Ah: is it because of the call to build_constructor? > > If so, can the "static" be removed and replaced with an "= NULL;" > > initializer? > > > > (I'm very wary of function-level static variables, as they're a place > > when state can "hide" between multiple in-process invocations of the > > compiler) > > Sorry that's due to my ignorance on how GCC GC works, I thought GC roots > had to be static, fixed. FWIW I don't think this is a GC root; the only GTY-marked roots in gcc/jit are those in dummy-frontend.c Looking at gcc/jit/notes.txt, this playback code is called in the "No GC in here" region within jit_langhook_parse_file. [...] > diff --git a/gcc/jit/docs/topics/expressions.rst b/gcc/jit/docs/topics/expressions.rst > index d783ceea51a..28f81be0060 100644 > --- a/gcc/jit/docs/topics/expressions.rst > +++ b/gcc/jit/docs/topics/expressions.rst > @@ -582,6 +582,27 @@ Global variables > referring to it. Analogous to using an "extern" global from a > header file. > > +.. function:: gcc_jit_lvalue *\ > + gcc_jit_global_set_initializer (gcc_jit_lvalue *global,\ > + const void *blob,\ > + size_t num_bytes) > + > + Set an initializer for an object using the memory content pointed > + by ``blob`` for ``num_bytes``. ``global`` must be an array of an > + integral type. > + > + The parameter ``blob`` must be non-NULL. The call copies the memory > + pointed by ``blob`` for ``num_bytes`` bytes, so it is valid to pass > + in a pointer to an on-stack buffer. The content will be stored in > + the compilation unit and used as initialization value of the array. Please document here that the return value is "global". [...] > +template<typename T> > +static void > +load_blob_in_ctor (vec<constructor_elt, va_gc> *&constructor_elements, > + size_t num_elem, > + const void *initializer) > +{ > + /* Loosely based on 'output_init_element' c-typeck.c:9691. */ > + const T *p = (const T *)initializer; > + tree node = make_unsigned_type (BITS_PER_UNIT * sizeof (T)); > + for (size_t i = 0; i < num_elem; i++) > + { > + constructor_elt celt = > + { build_int_cst (long_unsigned_type_node, i), > + build_int_cst (node, p[i]) }; > + vec_safe_push (constructor_elements, celt); In theory it would be slightly quicker to grow the vector once, and use vec_quick_push inside the loop, but given how much allocation we're doing it may be lost in the noise. > + } > +} > + > +/* Construct an initialized playback::lvalue instance (wrapping a > + tree). */ > + > +playback::lvalue * > +playback::context:: > +new_global_initialized (location *loc, > + enum gcc_jit_global_kind kind, > + type *type, > + size_t element_size, > + size_t initializer_num_elem, > + const void *initializer, > + const char *name) > +{ > + tree inner = global_new_decl (loc, kind, type, name); > + > + vec<constructor_elt, va_gc> *constructor_elements = NULL; > + > + switch (element_size) > + { > + case 1: > + load_blob_in_ctor<uint8_t> (constructor_elements, initializer_num_elem, > + initializer); > + break; > + case 2: > + load_blob_in_ctor<uint16_t> (constructor_elements, initializer_num_elem, > + initializer); > + break; > + case 4: > + load_blob_in_ctor<uint32_t> (constructor_elements, initializer_num_elem, > + initializer); > + break; > + case 8: > + load_blob_in_ctor<uint64_t> (constructor_elements, initializer_num_elem, > + initializer); > + break; > + default: > + gcc_unreachable (); Is there a way to hit this gcc_unreachable? If I'm reading it right, presumably this is only going to be called on the result of get_size, and this can only return the sizes covered by the cases, right? If so, please add a comment to this effect. [...] > diff --git a/gcc/jit/jit-recording.c b/gcc/jit/jit-recording.c > index b73cd76a0a0..39510097c6f 100644 > --- a/gcc/jit/jit-recording.c > +++ b/gcc/jit/jit-recording.c > @@ -2175,6 +2175,57 @@ recording::type::access_as_type (reproducer &r) > return r.get_identifier (this); > } > > +/* Override of default implementation of > + recording::type::get_size. > + > + Return the size in bytes. This is in use for global > + initialization. */ > + > +size_t > +recording::memento_of_get_type::get_size () > +{ > + int size; > + switch (m_kind) > + { > + case GCC_JIT_TYPE_VOID: > + return 0; > + case GCC_JIT_TYPE_BOOL: > + case GCC_JIT_TYPE_CHAR: > + case GCC_JIT_TYPE_SIGNED_CHAR: > + case GCC_JIT_TYPE_UNSIGNED_CHAR: > + return 1; > + case GCC_JIT_TYPE_SHORT: > + case GCC_JIT_TYPE_UNSIGNED_SHORT: > + size = SHORT_TYPE_SIZE; > + break; > + case GCC_JIT_TYPE_INT: > + case GCC_JIT_TYPE_UNSIGNED_INT: > + size = INT_TYPE_SIZE; > + break; > + case GCC_JIT_TYPE_LONG: > + case GCC_JIT_TYPE_UNSIGNED_LONG: > + size = LONG_TYPE_SIZE; > + break; > + case GCC_JIT_TYPE_LONG_LONG: > + case GCC_JIT_TYPE_UNSIGNED_LONG_LONG: > + size = LONG_LONG_TYPE_SIZE; > + break; > + case GCC_JIT_TYPE_FLOAT: > + size = FLOAT_TYPE_SIZE; > + break; > + case GCC_JIT_TYPE_DOUBLE: > + size = DOUBLE_TYPE_SIZE; > + break; > + case GCC_JIT_TYPE_LONG_DOUBLE: > + size = LONG_DOUBLE_TYPE_SIZE; > + break; > + default: > + gcc_unreachable (); > + } > + > + return size / BITS_PER_UNIT; > +} > + > /* Implementation of pure virtual hook recording::type::dereference for > recording::memento_of_get_type. */ > [...] > @@ -4472,6 +4580,25 @@ recording::global::write_reproducer (reproducer &r) > global_kind_reproducer_strings[m_kind], > r.get_identifier_as_type (get_type ()), > m_name->get_debug_string ()); > + > + if (m_initializer) > + switch (m_type->dereference ()->get_size ()) > + { > + case 1: > + write_initializer_reproducer<uint8_t> (id, r); > + break; > + case 2: > + write_initializer_reproducer<uint16_t> (id, r); > + break; > + case 4: > + write_initializer_reproducer<uint32_t> (id, r); > + break; > + case 8: > + write_initializer_reproducer<uint64_t> (id, r); > + break; > + default: > + gcc_unreachable (); Similar comments as above. > diff --git a/gcc/jit/jit-recording.h b/gcc/jit/jit-recording.h > index 726b9c4b837..aa38e606a8d 100644 > --- a/gcc/jit/jit-recording.h > +++ b/gcc/jit/jit-recording.h > @@ -502,6 +502,12 @@ public: > This will return NULL if it's not valid to dereference this type. > The caller is responsible for setting an error. */ > virtual type *dereference () = 0; > + /* Get the type size in bytes. > + > + This is implemented only for memento_of_get_type and > + memento_of_get_pointer as is use for initializing globals of "as is use" -> "as it is used" [...] > diff --git a/gcc/jit/libgccjit.c b/gcc/jit/libgccjit.c > index 3d04f6db3af..bc45bd80b2c 100644 > --- a/gcc/jit/libgccjit.c > +++ b/gcc/jit/libgccjit.c [...] > +extern gcc_jit_lvalue * > +gcc_jit_global_set_initializer (gcc_jit_lvalue *global, > + const void *blob, > + size_t num_bytes) > +{ [...] > + RETURN_NULL_IF_FAIL_PRINTF3 ( > + lvalue_size == num_bytes, NULL, NULL, > + "global \"%s\" of size %zu don't match initializer of size %zu", > + global->get_debug_string (), lvalue_size, num_bytes); Please reword (following the pattern of the other "mismatching" errors in libgccjit.c) to: "mismatching sizes:" " global \"%s\" has size %zu whereas initializer has size %zu", global->get_debug_string (), lvalue_size, num_bytes); [...] > + reinterpret_cast <gcc::jit::recording::global *> (global) > + ->set_initializer (blob, num_bytes); Was there a reason for using reinterpret_cast here, rather than static_cast? The file uses static_cast for this kind of thing throughout, so I'd prefer that here. [...] > diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h > index 1c5a12e9c01..3cbcbef2693 100644 > --- a/gcc/jit/libgccjit.h > +++ b/gcc/jit/libgccjit.h > @@ -788,6 +788,20 @@ gcc_jit_context_new_global (gcc_jit_context *ctxt, > gcc_jit_type *type, > const char *name); > > +#define LIBGCCJIT_HAVE_gcc_jit_global_set_initializer > + > +/* Set a static initializer for a global return the global itself. Please reword to: /* Set an initial value for a global, which must be an array of integral type. Return the global itself. > + > + This API entrypoint was added in LIBGCCJIT_ABI_14; you can test for its > + presence using > + #ifdef LIBGCCJIT_HAVE_gcc_jit_global_set_initializer > +*/ > + > +extern gcc_jit_lvalue * > +gcc_jit_global_set_initializer (gcc_jit_lvalue *global, > + const void *blob, > + size_t num_bytes); > + [...] OK for master with these nits fixed, assuming your usual testing. Thanks! Dave ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH V2] libgccjit: Add new gcc_jit_context_new_blob entry point 2020-09-10 22:22 ` David Malcolm @ 2020-09-11 10:31 ` Andrea Corallo 2020-09-11 11:27 ` David Malcolm 0 siblings, 1 reply; 14+ messages in thread From: Andrea Corallo @ 2020-09-11 10:31 UTC (permalink / raw) To: David Malcolm; +Cc: jit, nd, gcc-patches Hi Dave, thanks for the review! David Malcolm <dmalcolm@redhat.com> writes: [...] > Was there a reason for using reinterpret_cast here, rather than > static_cast? Yes the reason is that apparently we can't use static_cast for that: "error: invalid ‘static_cast’ from type ‘gcc_jit_lvalue*’ to type ‘gcc::jit::recording::global*’" Why is that is another question I fear I'm not very qualified to answer. If you feel is necessary and want to suggest some rework to improve this I'll be happy to put some effort in if you like. >> diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h >> index 1c5a12e9c01..3cbcbef2693 100644 >> --- a/gcc/jit/libgccjit.h >> +++ b/gcc/jit/libgccjit.h >> @@ -788,6 +788,20 @@ gcc_jit_context_new_global (gcc_jit_context *ctxt, >> gcc_jit_type *type, >> const char *name); >> >> +#define LIBGCCJIT_HAVE_gcc_jit_global_set_initializer >> + >> +/* Set a static initializer for a global return the global itself. [...] > > OK for master with these nits fixed, assuming your usual testing. > > Thanks! > Dave After applying the suggestions and having tested it I've installed the patch as 4ecc0061c40. Thanks! Andrea ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH V2] libgccjit: Add new gcc_jit_context_new_blob entry point 2020-09-11 10:31 ` Andrea Corallo @ 2020-09-11 11:27 ` David Malcolm 0 siblings, 0 replies; 14+ messages in thread From: David Malcolm @ 2020-09-11 11:27 UTC (permalink / raw) To: Andrea Corallo; +Cc: jit, nd, gcc-patches On Fri, 2020-09-11 at 12:31 +0200, Andrea Corallo wrote: > Hi Dave, > > thanks for the review! > > David Malcolm <dmalcolm@redhat.com> writes: > > [...] > > > Was there a reason for using reinterpret_cast here, rather than > > static_cast? > > Yes the reason is that apparently we can't use static_cast for that: > > "error: invalid ‘static_cast’ from type ‘gcc_jit_lvalue*’ to type > ‘gcc::jit::recording::global*’" > > Why is that is another question I fear I'm not very qualified to > answer. > If you feel is necessary and want to suggest some rework to improve > this > I'll be happy to put some effort in if you like. It's because we're effectively downcasting from lvalue to global. This is safe because of the check for global->is_global (). A slightly cleaner way to do it would be to have a dyn_cast_global vfunc here rather than is_global, and to use the result - but I don't think it matters. [...] > > > After applying the suggestions and having tested it I've > > > installed > the > patch as 4ecc0061c40. > > Thanks! Thanks Dave ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2020-09-11 11:27 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-06-03 10:11 [PATCH] libgccjit: Add new gcc_jit_context_new_blob entry point Andrea Corallo 2020-06-03 15:03 ` David Malcolm 2020-06-03 16:36 ` Andrea Corallo 2020-07-01 16:14 ` Andrea Corallo 2020-07-14 10:00 ` Andrea Corallo 2020-07-24 22:05 ` David Malcolm 2020-07-24 22:12 ` David Malcolm 2020-08-03 8:07 ` Andrea Corallo 2020-08-06 19:53 ` David Malcolm 2020-08-19 7:17 ` [PATCH V2] " Andrea Corallo 2020-09-09 7:51 ` Andrea Corallo 2020-09-10 22:22 ` David Malcolm 2020-09-11 10:31 ` Andrea Corallo 2020-09-11 11:27 ` David Malcolm
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).