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

[Guillaume Gomez <guillaume1.gomez@gmail.com>]

> It sounds like the patch you have locally is ready, but it has some
> nontrivial changes compared to the last version you posted to the list.
> Please post your latest version to the list.

Sure!

This patch adds the support for attributes on functions and variables. It does
so by adding the following functions:

* gcc_jit_function_add_attribute
* gcc_jit_function_add_string_attribute
* gcc_jit_function_add_integer_array_attribute
* gcc_jit_lvalue_add_string_attribute

It adds the following types:

* gcc_jit_fn_attribute
* gcc_jit_variable_attribute

It adds tests to ensure that the attributes are correctly applied.

> Do you have push rights, or do you need me to push it for you?

I have push rights so I'll merge the patch myself. But thanks for offering to
do it.

Le jeu. 11 janv. 2024 à 23:38, David Malcolm <dmalcolm@redhat.com> a écrit :
>
> On Thu, 2024-01-11 at 22:40 +0100, Guillaume Gomez wrote:
> > Hi David,
> >
> > > 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.
> >
> > Ah indeed. Mix-up on my end. Fixed it.
> >
> > > test-restrict.c is a pre-existing testcase, so please don't delete
> > > its
> > > entry.
> >
> > Ah indeed, I went too quickly and thought it was a test I renamed...
> >
> > > 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 ?
> >
> > I messed up a bit, fixed it thanks to you. I didn't run the script in
> > my last
> > update but just did:
> >
> > ```
> > $ contrib/gcc-changelog/git_check_commit.py $(git log -1 --format=%h)
> > Checking 3849ee2eadf0eeec2b0080a5142ced00be96a60d: OK
> > ```
> >
> > > Otherwise, looks good, assuming that the patch has been tested with
> > > the
> > > full jit testsuite.
> >
> > When rebasing on upstream yesterday I discovered that two tests
> > were not working anymore. For the first one, it was simply because of
> > the changes in `dummy-frontend.cc`. For the second one
> > (test-noinline-attribute.c), it was because the rules for inlining
> > changed
> > since we wrote this patch apparently (our fork is very late). Antoni
> > discovered
> > that we could just add a call to `asm` to prevent this from happening
> > so I
> > added it.
> >
> > So yes, all jit tests are passing as expected. :)
>
> Good.
>
> It sounds like the patch you have locally is ready, but it has some
> nontrivial changes compared to the last version you posted to the list.
> Please post your latest version to the list.
>
> Do you have push rights, or do you need me to push it for you?
>
> Thanks
> Dave
>
> >
> > Le jeu. 11 janv. 2024 à 19:46, David Malcolm <dmalcolm@redhat.com> a
> > écrit :
> > >
> > > 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
> > >
> >
>