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

[David Malcolm <dmalcolm@redhat.com>]

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