Re: [PATCH] Add support for function attributes and variable attributes

[David Malcolm <dmalcolm@redhat.com>]

On Thu, 2024-01-11 at 01:00 +0100, Guillaume Gomez wrote:
> Hi David.
> 
> Thanks for the review!
> 
> > > +.. function::  void\
> > > +               gcc_jit_lvalue_add_string_attribute
> > > (gcc_jit_lvalue *variable,
> > > +                                                    enum
> > > gcc_jit_fn_attribute attribute,
> >                                                                   
> > ^^
> > 
> > This got out of sync with the declaration in the header file; it
> > should
> > be enum gcc_jit_variable_attribute attribute
> 
> Indeed, good catch!
> 
> > I took a brief look through the handler functions and with the
> > above
> > caveat I didn't see anything obviously wrong.  I'm going to assume
> > this
> > code is OK given that presumably you've been testing it within
> > rustc,
> > right?
> 
> Both in rustc and in the JIT tests we added.
> 
> [..snip...]
> 
> I added all the missing `RETURN_IF_FAIL` you mentioned. None of the
> arguments should be `NULL` so it was a mistake not to check it.
> 
> [..snip...]
> 
> I removed the tests comments as you mentioned.
> 
> > Please update jit.dg/all-non-failing-tests.h for the new tests;
> > it's
> > meant to list all of the (non failing) tests alphabetically.
> 
> It's not always correctly sorted. Might be worth sending a patch
> after this
> one gets merged to fix that.
> 
> > I *think* all of the new tests aren't suitable to be run as part of
> > a
> > shared context (e.g. due to touching the optimization level or
> > examining generated asm), so they should be listed in that header
> > with
> > comments explaining why.
> 
> I added them with a comment on top of each of them.
> 
> I joined the new patch version.
> 
> Thanks again for the review!

Thanks for the updated patch.  I noticed a few minor issues:

> diff --git a/gcc/jit/docs/topics/types.rst b/gcc/jit/docs/topics/types.rst
> index bb51f037b7e..b1aedc03787 100644
> --- a/gcc/jit/docs/topics/types.rst
> +++ b/gcc/jit/docs/topics/types.rst
> @@ -553,3 +553,80 @@ Reflection API
>     .. code-block:: c
>  
>        #ifdef LIBGCCJIT_HAVE_gcc_jit_type_get_restrict
> +
> +.. function::  void\
> +               gcc_jit_function_add_attribute (gcc_jit_function *func,
> +                                               enum gcc_jit_fn_attribute attribute)
> +
> +     Add an attribute ``attribute`` to a function ``func``.
> +
> +     This is equivalent to the following code:
> +
> +  .. code-block:: c
> +
> +    __attribute__((always_inline))
> +
> +   This entrypoint was added in :ref:`LIBGCCJIT_ABI_26`; you can test for
> +   its presence using
> +
> +   .. code-block:: c
> +
> +      #ifdef LIBGCCJIT_HAVE_ATTRIBUTES
> +
> +.. function::  void\
> +               gcc_jit_function_add_string_attribute (gcc_jit_function *func,
> +                                                      enum gcc_jit_fn_attribute attribute,
> +                                                      const char *value)
> +
> +     Add a string attribute ``attribute`` with value ``value`` to a function
> +     ``func``.
> +
> +     This is equivalent to the following code:
> +
> +  .. code-block:: c
> +
> +    __attribute__ ((alias ("xxx")))
> +
> +   This entrypoint was added in :ref:`LIBGCCJIT_ABI_26`; you can test for
> +   its presence using
> +
> +   .. code-block:: c
> +
> +      #ifdef LIBGCCJIT_HAVE_ATTRIBUTES
> +
> +.. function::  void\
> +               gcc_jit_function_add_integer_array_attribute (gcc_jit_function *func,
> +                                                             enum gcc_jit_fn_attribute attribute,
> +                                                             const int *value,
> +                                                             size_t length)
> +
> +     Add an attribute ``attribute`` with ``length`` integer values ``value`` to a
> +     function ``func``. The integer values must be the same as you would write
> +     them in a C code.
> +
> +     This is equivalent to the following code:
> +
> +  .. code-block:: c
> +
> +    __attribute__ ((nonnull (1, 2)))
> +
> +   This entrypoint was added in :ref:`LIBGCCJIT_ABI_26`; you can test for
> +   its presence using
> +
> +   .. code-block:: c
> +
> +      #ifdef LIBGCCJIT_HAVE_ATTRIBUTES
> +
> +.. function::  void\
> +               gcc_jit_lvalue_add_string_attribute (gcc_jit_lvalue *variable,
> +                                                    enum gcc_jit_variable_attribute attribute,
> +                                                    const char *value)
> +
> +     Add an attribute ``attribute`` with value ``value`` to a variable ``variable``.
> +
> +   This entrypoint was added in :ref:`LIBGCCJIT_ABI_26`; you can test for
> +   its presence using
> +
> +   .. code-block:: c
> +
> +      #ifdef LIBGCCJIT_HAVE_ATTRIBUTES

The above looks correct, but the patch adds the entrypoint descriptions
to topics/types.rst, which seems like the wrong place.  The function-
related ones should be in topics/functions.rst in the "Functions"
section and the lvalue/variable one in topics/expression.rst after the
"Global variables" section.

> diff --git a/gcc/testsuite/jit.dg/all-non-failing-tests.h
b/gcc/testsuite/jit.dg/all-non-failing-tests.h
> index e762563f9bd..84001203352 100644
> --- a/gcc/testsuite/jit.dg/all-non-failing-tests.h
> +++ b/gcc/testsuite/jit.dg/all-non-failing-tests.h

[...snip...]

> @@ -313,7 +334,7 @@
>  #undef create_code
>  #undef verify_code
>  
> -/* test-restrict.c: This can't be in the testcases array as it needs
> +/* test-restrict-attribute.c: This can't be in the testcases array as it needs
>     the `-O3` flag.  */

test-restrict.c is a pre-existing testcase, so please don't delete its
entry.
BTW, the ChangeLog entry mentions adding test-restrict.c, but the patch
doesn't add it, so that part of the proposed ChangeLog is wrong.

Does the patch pass ./contrib/gcc-changelog/git_check_commit.py ?

[...snip...]

> diff --git a/gcc/testsuite/jit.dg/test-cold-attribute.c b/gcc/testsuite/jit.dg/test-cold-attribute.c
> new file mode 100644
> index 00000000000..8dc7ec5a34b
> --- /dev/null
> +++ b/gcc/testsuite/jit.dg/test-cold-attribute.c
> @@ -0,0 +1,54 @@
> +/* { dg-do compile { target x86_64-*-* } } */
> +
> +#include <stdlib.h>
> +#include <stdio.h>
> +
> +#include "libgccjit.h"
> +
> +/* We don't want set_options() in harness.h to set -O2 to see that the cold
> +   attribute affects the optimizations. */

Please delete the above comment.

> +#define TEST_ESCHEWS_SET_OPTIONS
> +static void set_options (gcc_jit_context *ctxt, const char *argv0)
> +{
> +  // Set "-O2".
> +  gcc_jit_context_set_int_option(ctxt, GCC_JIT_INT_OPTION_OPTIMIZATION_LEVEL, 2);
> +}

[...snip...]

> diff --git a/gcc/testsuite/jit.dg/test-const-attribute.c b/gcc/testsuite/jit.dg/test-const-attribute.c
> new file mode 100644
> index 00000000000..c06742d163f
> --- /dev/null
> +++ b/gcc/testsuite/jit.dg/test-const-attribute.c
> @@ -0,0 +1,134 @@
> +/* { dg-do compile { target x86_64-*-* } } */
> +
> +#include <stdlib.h>
> +#include <stdio.h>
> +
> +#include "libgccjit.h"
> +
> +/* We don't want set_options() in harness.h to set -O3 to see that the const
> +   attribute affects the optimizations. */

Please delete the above comment.

> +#define TEST_ESCHEWS_SET_OPTIONS
> +static void set_options (gcc_jit_context *ctxt, const char *argv0)
> +{
> +  // Set "-O3".
> +  gcc_jit_context_set_int_option(ctxt, GCC_JIT_INT_OPTION_OPTIMIZATION_LEVEL, 3);
> +}

[...snip...]

> diff --git a/gcc/testsuite/jit.dg/test-noinline-attribute.c b/gcc/testsuite/jit.dg/test-noinline-attribute.c
> new file mode 100644
> index 00000000000..a455b4493fd
> --- /dev/null
> +++ b/gcc/testsuite/jit.dg/test-noinline-attribute.c
> @@ -0,0 +1,121 @@
> +/* { dg-do compile { target x86_64-*-* } } */
> +
> +#include <stdlib.h>
> +#include <stdio.h>
> +
> +#include "libgccjit.h"
> +
> +/* We don't want set_options() in harness.h to set -O2 to see that the `noinline`
> +   attribute affects the optimizations. */

Please delete the above comment.

> +#define TEST_ESCHEWS_SET_OPTIONS
> +static void set_options (gcc_jit_context *ctxt, const char *argv0)
> +{
> +  // Set "-O2".
> +  gcc_jit_context_set_int_option(ctxt, GCC_JIT_INT_OPTION_OPTIMIZATION_LEVEL, 2);
> +}

[...snip...]

> diff --git a/gcc/testsuite/jit.dg/test-nonnull-attribute.c b/gcc/testsuite/jit.dg/test-nonnull-attribute.c
> new file mode 100644
> index 00000000000..3306f890657
> --- /dev/null
> +++ b/gcc/testsuite/jit.dg/test-nonnull-attribute.c
> @@ -0,0 +1,94 @@
> +/* { dg-do compile { target x86_64-*-* } } */
> +
> +#include <stdlib.h>
> +#include <stdio.h>
> +
> +#include "libgccjit.h"
> +
> +/* We don't want set_options() in harness.h to set -O2 to see that the nonnull
> +   attribute affects the optimizations. */

Please delete the above comment.


> +#define TEST_ESCHEWS_SET_OPTIONS
> +static void set_options (gcc_jit_context *ctxt, const char *argv0)
> +{
> +  // Set "-O2".
> +  gcc_jit_context_set_int_option(ctxt, GCC_JIT_INT_OPTION_OPTIMIZATION_LEVEL, 2);
> +}

[...snip...]

> diff --git a/gcc/testsuite/jit.dg/test-pure-attribute.c b/gcc/testsuite/jit.dg/test-pure-attribute.c
> new file mode 100644
> index 00000000000..0c9ba1366e0
> --- /dev/null
> +++ b/gcc/testsuite/jit.dg/test-pure-attribute.c
> @@ -0,0 +1,134 @@
> +/* { dg-do compile { target x86_64-*-* } } */
> +
> +#include <stdlib.h>
> +#include <stdio.h>
> +
> +#include "libgccjit.h"
> +
> +/* We don't want set_options() in harness.h to set -O3 to see that the pure
> +   attribute affects the optimizations. */

Please delete the above comment.

> +#define TEST_ESCHEWS_SET_OPTIONS
> +static void set_options (gcc_jit_context *ctxt, const char *argv0)
> +{
> +  // Set "-O3".
> +  gcc_jit_context_set_int_option(ctxt, GCC_JIT_INT_OPTION_OPTIMIZATION_LEVEL, 3);
> +}
> +

[...snip...]

> diff --git a/gcc/testsuite/jit.dg/test-restrict-attribute.c b/gcc/testsuite/jit.dg/test-restrict-attribute.c
> new file mode 100644
> index 00000000000..7d7444b624f
> --- /dev/null
> +++ b/gcc/testsuite/jit.dg/test-restrict-attribute.c
> @@ -0,0 +1,77 @@
> +/* { dg-do compile { target x86_64-*-* } } */
> +
> +#include <stdlib.h>
> +#include <stdio.h>
> +
> +#include "libgccjit.h"
> +
> +/* We don't want set_options() in harness.h to set -O3 to see that the restrict
> +	 attribute affects the optimizations. */

Please delete this comment.

> +#define TEST_ESCHEWS_SET_OPTIONS
> +static void set_options (gcc_jit_context *ctxt, const char *argv0)
> +{
> +	// Set "-O3".
> +	gcc_jit_context_set_int_option(ctxt, GCC_JIT_INT_OPTION_OPTIMIZATION_LEVEL, 3);
> +}
> +

[...snip...]

Otherwise, looks good, assuming that the patch has been tested with the
full jit testsuite.

Thanks again
Dave