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!


Le mar. 9 janv. 2024 à 20:59, David Malcolm <dmalcolm@redhat.com> a écrit :
>
> On Wed, 2023-11-15 at 17:53 +0100, Guillaume Gomez wrote:
> > Hi,
> >
> > This patch adds the (incomplete) support for function and variable
> > attributes. The added attributes are the ones we're using in
> > rustc_codegen_gcc but all the groundwork is done to add more (and we
> > will very likely add more as we didn't add all the ones we use in
> > rustc_codegen_gcc yet).
> >
> > The only big question with this patch is about `inline`. We currently
> > handle it as an attribute because it is more convenient for us but is
> > it ok or should we create a separate function to mark a function as
> > inlined?
> >
> > Thanks in advance for the review.
>
> Thanks for the patch; sorry for the delay in reviewing.
>
> At a high-level I think the API is OK as-is, but I have some nitpicks
> with the implementation:
>
> [...snip...]
>
> > diff --git a/gcc/jit/docs/topics/types.rst b/gcc/jit/docs/topics/types.rst
> > index d8c1d15d69d..6c72c99cbd9 100644
> > --- a/gcc/jit/docs/topics/types.rst
> > +++ b/gcc/jit/docs/topics/types.rst
>
> [...snip...]
>
> > +.. 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
>
> [...snip...]
>
> > diff --git a/gcc/jit/dummy-frontend.cc b/gcc/jit/dummy-frontend.cc
> > index a729086bafb..898b4d6e7f8 100644
> > --- a/gcc/jit/dummy-frontend.cc
> > +++ b/gcc/jit/dummy-frontend.cc
>
> It's unfortunate that jit/dummy-frontend.cc has its own copy of the
> material in c-common/c-attribs.cc.  I glanced through this code, and it
> seems that there are already various differences between the two copies
> in the existing code, and the patch adds more such differences.
>
> Bother - but I think this part of the patch is inevitable (and OK)
> given the existing state of attribute handling here.
>
> [...snip...]
>
> 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 rustcc,
> right?
>
> [..snip...]
>
> > diff --git a/gcc/jit/libgccjit.cc b/gcc/jit/libgccjit.cc
> > index 0451b4df7f9..337d4ea3b95 100644
> > --- a/gcc/jit/libgccjit.cc
> > +++ b/gcc/jit/libgccjit.cc
> > @@ -3965,6 +3965,51 @@ gcc_jit_type_get_aligned (gcc_jit_type *type,
> >    return (gcc_jit_type *)type->get_aligned (alignment_in_bytes);
> >  }
> >
> > +void
> > +gcc_jit_function_add_attribute (gcc_jit_function *func,
> > +                             gcc_jit_fn_attribute attribute)
> > +{
> > +  RETURN_IF_FAIL (func, NULL, NULL, "NULL func");
> > +
> > +  func->add_attribute (attribute);
>
> Ideally should validate parameter "attribute" here with a
> RETURN_IF_FAIL.
>
> > +}
> > +
> > +void
> > +gcc_jit_function_add_string_attribute (gcc_jit_function *func,
> > +                                    gcc_jit_fn_attribute attribute,
> > +                                    const char* value)
> > +{
> > +  RETURN_IF_FAIL (func, NULL, NULL, "NULL func");
>
> Likewise, ideally should validate parameter "attribute" here with a
> RETURN_IF_FAIL.
>
> Can "value" be NULL?  If not, then we should add a RETURN_IF_FAIL for
> it here at the API boundary.
>
> > +
> > +  func->add_string_attribute (attribute, value);
> > +}
> > +
> > +/* This function adds an attribute with multiple integer values.  For example
> > +   `nonnull(1, 2)`.  The numbers in `values` are supposed to map how they
> > +   should be written in C code.  So for `nonnull(1, 2)`, you should pass `1`
> > +   and `2` in `values` (and set `length` to `2`). */
> > +void
> > +gcc_jit_function_add_integer_array_attribute (gcc_jit_function *func,
> > +                                           gcc_jit_fn_attribute attribute,
> > +                                           const int* values,
> > +                                           size_t length)
> > +{
> > +  RETURN_IF_FAIL (func, NULL, NULL, "NULL func");
>
> As before, ideally should validate parameter "attribute" here with a
> RETURN_IF_FAIL.
>
> > +  RETURN_IF_FAIL (values, NULL, NULL, "NULL values");
> > +
> > +  func->add_integer_array_attribute (attribute, values, length);
> > +}
> > +
> > +void
> > +gcc_jit_lvalue_add_string_attribute (gcc_jit_lvalue *variable,
> > +                                  gcc_jit_variable_attribute attribute,
> > +                                  const char* value)
> > +{
> > +  RETURN_IF_FAIL (variable, NULL, NULL, "NULL variable");
>
> As before, we should validate parameters "attribute" and "value" here
> with RETURN_IF_FAILs.
>
> We should also validate here that "variable" is indeed a variable, not
> some arbitrary lvalue e.g. the address of the element of an array (or
> whatever).
>
>
> > +
> > +  variable->add_string_attribute (attribute, value);
> > +}
> > +
>
> [...snip...]
>
> > diff --git a/gcc/testsuite/jit.dg/jit.exp b/gcc/testsuite/jit.dg/jit.exp
> > index 8bf7e51c24f..657b454a003 100644
> > --- a/gcc/testsuite/jit.dg/jit.exp
> > +++ b/gcc/testsuite/jit.dg/jit.exp
> > @@ -899,8 +899,41 @@ proc jit-verify-assembler-output { args } {
> >       pass "${asm_filename} output pattern test, ${dg-output-text}"
> >       verbose "Passed test for output pattern ${dg-output-text}" 3
> >      }
> > +}
> > +
> > +# Assuming that a .s file has been written out named
> > +# OUTPUT_FILENAME, check that the argument doesn't match
> > +# the output file.
> > +proc jit-verify-assembler-output-not { args } {
> > +    verbose "jit-verify-assembler: $args"
> > +
> > +    set dg-output-text [lindex $args 0]
> > +    verbose "dg-output-text: ${dg-output-text}"
> > +
> > +    upvar 2 name name
> > +    verbose "name: $name"
> > +
> > +    upvar 2 prog prog
> > +    verbose "prog: $prog"
> > +    set asm_filename [jit-get-output-filename $prog]
> > +    verbose "  asm_filename: ${asm_filename}"
> >
> > +    # Read the assembly file.
> > +    set f [open $asm_filename r]
> > +    set content [read $f]
> > +    close $f
> > +
> > +    # Verify that the assembly matches the regex.
> > +    if { [regexp ${dg-output-text} $content] } {
> > +     fail "${asm_filename} output pattern test, is ${content}, should match ${dg-output-text}"
>
> The wording of the "fail" message seems wrong; presumably this should
> read "should not match", rather than "should match".
>
> > +     verbose "Failed test for output pattern ${dg-output-text}" 3
> > +    } else {
> > +     pass "${asm_filename} output pattern test, ${dg-output-text}"
> > +     verbose "Passed test for output pattern ${dg-output-text}" 3
> > +    }
> >  }
> > +
> > +
> >  # Assuming that a .o file has been written out named
> >  # OUTPUT_FILENAME, invoke the driver to try to turn it into
> >  # an executable, and try to run the result.
>
> [...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. */
>
> The above comment doesn't make sense to me; harness.h effectively sets
> -O3; and -O2 is wanted by this test, right?
>
> I think the comment can be omitted given that the intent below is
> clear.
>
> > +#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. */
>
> Again, this comment doesn't make sense to me, but I think it can be
> removed.
>
> > +#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...]
>
> > +
> > +  /* if (x >>= 1) */
> > +  /* Since gccjit doesn't (yet?) have support for `>>=` operator, we will decompose it into:
> > +     `if (x = x >> 1)` */
>
> I think it does (in theory, at least), via:
>
>   gcc_jit_block_add_assignment_op
>
> with
>
>   GCC_JIT_BINARY_OP_RSHIFT
>
> But I haven't tried it, and there's no need to update the test to make
> use of it.
>
> [...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..84933e60010
> > --- /dev/null
> > +++ b/gcc/testsuite/jit.dg/test-noinline-attribute.c
> > @@ -0,0 +1,114 @@
> > +/* { 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. */
>
> Again, please lose this 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. */
>
> Likewise.
>
> > +#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. */
>
> Likewise.
>
> > +#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...]
>
> 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.
>
> 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.
>
> Thanks again for the patch.
> Dave
>