Re: [PATCH] libgccjit: Add ways to set the personality function

[David Malcolm <dmalcolm@redhat.com>]

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