On Fri, 2023-11-17 at 17:48 -0500, Antoni Boucher wrote:
> Hi.
> This adds functions to set the personality function (bug 112603).
>
> I'm not sure I can make a test for this: it seems the personality
> function will not be set if there are no try/catch inside the
> functions.
> Do you know a way to keep the personality function that is set in
> this
> case?
>
> Or should we wait until I send the patch for try/catch?
Thanks for the patch
> diff --git a/gcc/expr.cc b/gcc/expr.cc
> index 556bcf7ef59..25d50289b24 100644
> --- a/gcc/expr.cc
> +++ b/gcc/expr.cc
> @@ -13559,6 +13559,14 @@ build_personality_function (const char *lang)
>
> name = ACONCAT (("__", lang, "_personality", unwind_and_version, NULL));
>
> + return build_personality_function_with_name (name);
> +}
> +
> +tree
> +build_personality_function_with_name (const char *name)
> +{
> + tree decl, type;
> +
> type = build_function_type_list (unsigned_type_node,
> integer_type_node, integer_type_node,
> long_long_unsigned_type_node,
I confess I'm not very familiar with personalities, sorry; hopefully a
reviewer who is can take a look at the non-jit parts of the patch,
though FWIW they look fairly trivial.
> diff --git a/gcc/jit/docs/topics/compatibility.rst
b/gcc/jit/docs/topics/compatibility.rst
> index ebede440ee4..31c3ef6401a 100644
> --- a/gcc/jit/docs/topics/compatibility.rst
> +++ b/gcc/jit/docs/topics/compatibility.rst
> @@ -378,3 +378,13 @@ alignment of a variable:
> --------------------
> ``LIBGCCJIT_ABI_25`` covers the addition of
> :func:`gcc_jit_type_get_restrict`
> +
> +.. _LIBGCCJIT_ABI_26:
> +
> +``LIBGCCJIT_ABI_26``
> +--------------------
> +``LIBGCCJIT_ABI_26`` covers the addition of functions to set the personality
> +function:
> +
> + * :func:`gcc_jit_function_set_personality_function`
> + * :func:`gcc_jit_set_global_personality_function_name`
Obviously you're going to need to update the number if the other patch
goes in first.
> diff --git a/gcc/jit/docs/topics/functions.rst
> b/gcc/jit/docs/topics/functions.rst
> index cf5cb716daf..e59885c3549 100644
> --- a/gcc/jit/docs/topics/functions.rst
> +++ b/gcc/jit/docs/topics/functions.rst
> @@ -197,6 +197,34 @@ Functions
>
> .. type:: gcc_jit_case
>
> +.. function:: void
> + gcc_jit_function_set_personality_function (gcc_jit_function
> *fn,
> + gcc_jit_function
> *personality_func)
> +
> + Set the personality function of ``fn`` to ``personality_func``.
> +
> + were added in :ref:`LIBGCCJIT_ABI_26`; you can test for their presence
> + using
Likewise here.
Is there a URL in the main GCC docs we can link to that describes what
this is for?
Are there restrictions on what a personality_func is? Sorry for my
ignorance here.
> +
> + .. code-block:: c
> +
> + #ifdef LIBGCCJIT_HAVE_PERSONALITY_FUNCTION
> +
> +.. function:: void
> + gcc_jit_set_global_personality_function_name (char* name)
> +
> + Set the global personality function.
> +
> + This is mainly useful to prevent the optimizer from unsetting the
> personality
> + function set on other functions.
> +
> + were added in :ref:`LIBGCCJIT_ABI_26`; you can test for their presence
> + using
Likewise here: ABI numbering, and a URL for more info on what this is.
> +
> + .. code-block:: c
> +
> + #ifdef LIBGCCJIT_HAVE_PERSONALITY_FUNCTION
> +
> Blocks
> ------
> .. type:: gcc_jit_block
> diff --git a/gcc/jit/dummy-frontend.cc b/gcc/jit/dummy-frontend.cc
> index a729086bafb..c9dedd59b24 100644
> --- a/gcc/jit/dummy-frontend.cc
> +++ b/gcc/jit/dummy-frontend.cc
> @@ -146,6 +146,20 @@ const struct attribute_spec jit_format_attribute_table[]
> =
> { NULL, 0, 0, false, false, false, false, NULL, NULL }
> };
>
> +char* jit_personality_func_name = NULL;
> +static tree personality_decl;
> +
> +/* FIXME: This is a hack to preserve trees that we create from the
> + garbage collector. */
> +
> +static GTY (()) tree jit_gc_root;
> +
> +void
> +jit_preserve_from_gc (tree t)
> +{
> + jit_gc_root = tree_cons (NULL_TREE, t, jit_gc_root);
> +}
If I'm reading the patch correctly, this is only used by
jit_langhook_eh_personality, to preserve personality_decl.
Can't you just add a GTY (()) marker to personality_decl's decl
instead, as:
static GTY (()) tree personality_decl;
[...]
> diff --git a/gcc/jit/libgccjit.cc b/gcc/jit/libgccjit.cc
> index 0451b4df7f9..67d9036249e 100644
> --- a/gcc/jit/libgccjit.cc
> +++ b/gcc/jit/libgccjit.cc
> @@ -3590,6 +3590,28 @@ gcc_jit_context_add_command_line_option
> (gcc_jit_context *ctxt,
> ctxt->add_command_line_option (optname);
> }
>
> +/* Public entrypoint. See description in libgccjit.h.
> + After error-checking, the real work is done by the
> + gcc::jit::recording::function::set_personality_function method, in
> + jit-recording.c. */
> +
> +void
> +gcc_jit_function_set_personality_function (gcc_jit_function *fn,
> + gcc_jit_function *personality_func)
> +{
> + RETURN_IF_FAIL (fn, NULL, NULL, "NULL function");
I see various unconditional dereferences of m_personality_function, so
am I right in assuming that personality_function must be non-NULL? If
so we should document that, and reject NULL here.
> +
> + fn->set_personality_function (personality_func);
> +}
> +
> +extern char* jit_personality_func_name;
> +
> +void
> +gcc_jit_set_global_personality_function_name (char* name)
> +{
> + jit_personality_func_name = name;
This probably should be per-context state, rather than a global
variable. So I think this needs a gcc_jit_context * param, which must
be non-null, and we'd have a new field m_personality_func_name of the
recording::context, rather than the global.
Also everywhere else in the API we take a copy of the string, rather
than via a pointer. Is there a reason for doing this here? Otherwise,
this param should be a const char *, and we should xstrdup it (and free
any existing value).
[...]
> diff --git a/gcc/jit/libgccjit.map b/gcc/jit/libgccjit.map
> index 8b90a0e2ff3..2a0eb819a52 100644
> --- a/gcc/jit/libgccjit.map
> +++ b/gcc/jit/libgccjit.map
> @@ -276,3 +276,9 @@ LIBGCCJIT_ABI_25 {
> global:
> gcc_jit_type_get_restrict;
> } LIBGCCJIT_ABI_24;
> +
> +LIBGCCJIT_ABI_26 {
> + global:
> + gcc_jit_set_global_personality_function_name;
> + gcc_jit_function_set_personality_function;
> +} LIBGCCJIT_ABI_25;
Same note as above about ABI numbering.
> diff --git a/gcc/testsuite/jit.dg/all-non-failing-tests.h
> b/gcc/testsuite/jit.dg/all-non-failing-tests.h
> index e762563f9bd..3785a788ffa 100644
> --- a/gcc/testsuite/jit.dg/all-non-failing-tests.h
> +++ b/gcc/testsuite/jit.dg/all-non-failing-tests.h
> @@ -322,6 +322,9 @@
> /* test-setting-alignment.c: This can't be in the testcases array as it
> is target-specific. */
>
> +/* test-personality-function.c: This can't be in the testcases array as it
> + is target-specific. */
...and because it modifies per-context state.
[...]
Hope the above makes sense; sorry again about my ignorance of the
underlying personality stuff.
Dave
