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

[David Malcolm <dmalcolm@redhat.com>]

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
> > 
>