On Mon, 2016-05-23 at 14:26 +0200, Basile Starynkevitch wrote:
> Hello All,
>
> As I explained in https://gcc.gnu.org/ml/jit/2016-q2/msg00042.html it
> is
> difficult (or tricky without using dirty tricks involving the GCC
> plugin
> headers) to use GCCJIT to emit code equivalent to the following C
> file:
>
> extern int a;
> int get_atomic_a (void) {
> return __atomic_load_n (&a, __ATOMIC_SEQ_CST);
> }
>
> The issue is that __ATOMIC_SEQ_CST is a magic preprocessor (but non
> -standard!) symbol which might not be available
> (or might have a different value) in the C code for GCCJIT building
> such an AST.
>
> So we need a function to retrieve some magic integral value from the
> GCCJIT compiler.
>
> The attached patch (relative to trunk svn 236583) is a first attempt
> to solve that issue
> (and also give ability to query some other magic numbers).
>
> Proposed ChangeLog entry (in gcc/jit/)
>
> 2016-05-23 Basile Starynkevitch <bas...@starynkevitch.net>
> * libgccjit.h (LIBGCCJIT_HAVE_gcc_jit_magic_int): New macro.
> (gcc_jit_magic_int): New public function declaration.
>
> * libgccjit.c: Include "cppbuiltin.h", "options.h", "flag
> -types.h"
> (gcc_jit_magic_int): New function.
>
> * libgccjit.map: Add gcc_jit_magic_int to LIBGCCJIT_ABI_6.
>
> Comments (or an ok to commit) are welcome. (I am not sure that
> __SANITIZE_ADDRESS__ is correctly handled,
> because I would believe that optimization flags are not globals in
> GCCJIT)
Sorry about the delay in responding to this.
> Index: gcc/jit/libgccjit.h
> ===================================================================
> --- gcc/jit/libgccjit.h (revision 236583)
> +++ gcc/jit/libgccjit.h (working copy)
> @@ -1387,6 +1387,27 @@
> gcc_jit_rvalue_set_bool_require_tail_call (gcc_jit_rvalue *call,
> int require_tail_call);
>
> +
> + /* Magical integer values useful in the compiler; similar to
> + predefined C macros like __GNUC__, __GNUC_MINOR__,
> + __GNUC_PATCHLEVEL__, __ATOMIC_RELAXED, __ATOMIC_SEQ_CST,
> + __ATOMIC_ACQUIRE, __ATOMIC_RELEASE, __ATOMIC_ACQ_REL,
> + __ATOMIC_CONSUME, __PIC__, __PIE__, etc. Typical usage would
be:
> +
> + bool err=false;
> + int mypic = gcc_jit_magic_int("__PIC__", &err);
> + if (err) somethinggotwrong();
> +
> + This function is expected to be rarely called, typically once at
> + initialization time.
> +
> + This API entrypoint was added in LIBGCCJIT_ABI_6; you can test
for its
> + presence using
> + #ifdef LIBGCCJIT_HAVE_gcc_jit_magic_int
> + */
> +#define LIBGCCJIT_HAVE_gcc_jit_magic_int
> +extern int gcc_jit_magic_int(const char*name, bool*errp);
Does the client code need to be able to check to see if a name is
defined, or can they assume it always is?
If the latter, how about:
extern int
gcc_jit_context_get_predefined_int (gcc_jit_context *ctxt,
const char *name);
and have it set an error on the context if the name isn't
recognized?
If they need to be able to check it, I prefer:
extern int
gcc_jit_context_get_predefined_int (gcc_jit_context *ctxt,
const char *name,
int *out);
i.e. have the return value contain the "was name recognized" flag
and *out be written to (I'd use "bool", but we don't require its
availability).
I'm not fond of "magic int", but I'm not fond of my proposed name
either.
> #ifdef __cplusplus
> }
> #endif /* __cplusplus */
> Index: gcc/jit/libgccjit.c
> ===================================================================
> --- gcc/jit/libgccjit.c (revision 236583)
> +++ gcc/jit/libgccjit.c (working copy)
> @@ -23,6 +23,9 @@
> #include "coretypes.h"
> #include "timevar.h"
> #include "typed-splay-tree.h"
> +#include "cppbuiltin.h"
> +#include "options.h"
> +#include "flag-types.h"
>
> #include "libgccjit.h"
> #include "jit-recording.h"
> @@ -2970,3 +2973,44 @@
>
> call->set_require_tail_call (require_tail_call);
> }
> +
> +
> +/* Public entrypoint. See description in libgccjit.h. */
> +
> +int gcc_jit_magic_int(const char*name, bool*errp)
> +{
> + static int major, minor, patchlevel;
> + if (!major) /* call once: */
> + parse_basever (&major, &minor, &patchlevel);
These vars are global state, and they've not guarded by a mutex.
Can they be moved to the gcc_jit_context instead? (actually,
to gcc::jit::recording::context). That way they'd be covered by
the general policies on thread-safety here:
https://gcc.gnu.org/onlinedocs/jit/topics/contexts.html#thread-safety
That said, see the question below about the "__GNUC_*" vars.
> +
> + RETURN_VAL_IF_FAIL (name,
> + errp?((*errp=true),0):0,
The above line is too terse for my taste.
> + NULL, NULL,
> + "NULL name");
> + RETURN_VAL_IF_FAIL (name[0] == '_' && name[1] == '_',
> + errp?((*errp=true),0):0,
> + NULL, NULL,
> + "name should start with two underscores");
> +#define HAVE_MAGIC_INT(NamStr,Val) do { \
Please use block caps for the macro arguments.
> + if (!strcmp(name, NamStr)) { \
> + if (errp) *errp = false; \
> + return Val; }} while(0)
> + // keep these in alphabetical order...
Please use a C-style comment here.
> + HAVE_MAGIC_INT("__ATOMIC_ACQUIRE", MEMMODEL_ACQUIRE);
> + HAVE_MAGIC_INT("__ATOMIC_ACQ_REL", MEMMODEL_ACQ_REL);
> + HAVE_MAGIC_INT("__ATOMIC_CONSUME", MEMMODEL_CONSUME);
> + HAVE_MAGIC_INT("__ATOMIC_RELAXED", MEMMODEL_RELAXED);
> + HAVE_MAGIC_INT("__ATOMIC_RELEASE", MEMMODEL_RELEASE);
> + HAVE_MAGIC_INT("__ATOMIC_SEQ_CST", MEMMODEL_SEQ_CST);
It sounds like we have a concrete use-case for these.
> + HAVE_MAGIC_INT("__GNUC_MINOR__", minor);
> + HAVE_MAGIC_INT("__GNUC_PATCHLEVEL__", patchlevel);
> + HAVE_MAGIC_INT("__GNUC__", major);
What's the use-case for these? It seems a bit weird to expose
them as "GNU C" when libgccjit isn't C.
> + HAVE_MAGIC_INT("__PIC__", flag_pic);
> + HAVE_MAGIC_INT("__PIE__", flag_pie);
> + HAVE_MAGIC_INT("__SANITIZE_ADDRESS__", flag_sanitize &
SANITIZE_ADDRESS);
> + HAVE_MAGIC_INT("__SANITIZE_THREAD__", flag_sanitize &
SANITIZE_THREAD);
These variables are determined by command-line options, and hence
are going to vary based on gcc_jit_context_add_command_line_option
in a way that's difficult to implement from libgccjit.c; the command
line options effectively get values in jit-playback.c when we
construct an argv for toplev.
> +#undef HAVE_MAGIC_INT
> + RETURN_VAL_IF_FAIL_PRINTF1 (false, errp?((*errp=true),0):0,
> + NULL, NULL,
> + "unknown magic int name: %s", name);
> +}
> Index: gcc/jit/libgccjit.map
> ===================================================================
> --- gcc/jit/libgccjit.map (revision 236583)
> +++ gcc/jit/libgccjit.map (working copy)
> @@ -149,4 +149,5 @@
> LIBGCCJIT_ABI_6 {
> global:
> gcc_jit_rvalue_set_bool_require_tail_call;
> + gcc_jit_magic_int;
> } LIBGCCJIT_ABI_5;
>
Please create a fresh ABI tag for the new symbol.
The patch is missing test cases; it needs to have at least 2
(one showing success, one showing failure).
A full patch would also need documentation.
It strikes me that if we add new names to this in the future,
we'd have no way to introspect client binaries to see which
version of the entrypoint they're expecting. I don't know
if this is a problem. One very different approach that would
solve this would be to have one symbol per magic int, maybe
for them to simply be const data, rather than functions:
#define LIBGCCJIT_HAVE_gcc_jit__ATOMIC_ACQUIRE
extern const int gcc_jit__ATOMIC_ACQUIRE;
#define LIBGCCJIT_HAVE_gcc_jit__ATOMIC_ACQ_REL
extern const int gcc_jit__ATOMIC_ACQ_REL;
/* etc */
and to add these to libgccjit.map.
I'm not sure if the above is a good idea. It seems much simpler that
having an API entrypoint, but maybe there are drawbacks I've not
thought of.
Thoughts?
Dave
