[PATCH v2 0/1] libgccjit: Add the function attributes for setting the ABI

[Andrew Pinski via Sourceware Forge]

Andrew Pinski (pinskia) <pinskia@gcc.gnu.org>) commented on the code:


> +++ gcc/jit/jit-playback.cc
> @@ -547,6 +547,38 @@ const char* fn_attribute_to_string (gcc_jit_fn_attribute attr)
>        return "weak";
>      case GCC_JIT_FN_ATTRIBUTE_NONNULL:
>        return "nonnull";
> +    case GCC_JIT_FN_ATTRIBUTE_ARM_CMSE_NONSECURE_CALL:
Does it make sense to do this way or should it be a target specific hook to add the ability to have new fn attributes here?
>From a GCC point of view I rather have hooks rather than target specific items in generic code.
I know rustc and clang handle the target specific code in general code all the time but I think that is bad design.

> +++ gcc/jit/jit-playback.cc
> @@ -547,6 +547,38 @@ const char* fn_attribute_to_string (gcc_jit_fn_attribute attr)
>        return "weak";
>      case GCC_JIT_FN_ATTRIBUTE_NONNULL:
>        return "nonnull";
> +    case GCC_JIT_FN_ATTRIBUTE_ARM_CMSE_NONSECURE_CALL:
Could you please explain how that API would look like?

> +++ gcc/jit/jit-playback.cc
> @@ -547,6 +547,38 @@ const char* fn_attribute_to_string (gcc_jit_fn_attribute attr)
>        return "weak";
>      case GCC_JIT_FN_ATTRIBUTE_NONNULL:
>        return "nonnull";
> +    case GCC_JIT_FN_ATTRIBUTE_ARM_CMSE_NONSECURE_CALL:
There will be a range defined for in gcc_jit_fn_attribute for target specific enums.
And then you have headers for the target you are compiling which defines attributes in that range. And you dispatch to the hook for that for the target.

> +++ gcc/jit/jit-playback.cc
> @@ -547,6 +547,38 @@ const char* fn_attribute_to_string (gcc_jit_fn_attribute attr)
>        return "weak";
>      case GCC_JIT_FN_ATTRIBUTE_NONNULL:
>        return "nonnull";
> +    case GCC_JIT_FN_ATTRIBUTE_ARM_CMSE_NONSECURE_CALL:
You mean something like:
```
enum gcc_jit_fn_attribute
{
  GCC_JIT_FN_ATTRIBUTE_ALIAS,
  GCC_JIT_FN_ATTRIBUTE_ALWAYS_INLINE,
  GCC_JIT_FN_ATTRIBUTE_INLINE,
  GCC_JIT_FN_ATTRIBUTE_NOINLINE,
  GCC_JIT_FN_ATTRIBUTE_TARGET,
  GCC_JIT_FN_ATTRIBUTE_USED,
  GCC_JIT_FN_ATTRIBUTE_VISIBILITY,
  GCC_JIT_FN_ATTRIBUTE_COLD,
  GCC_JIT_FN_ATTRIBUTE_RETURNS_TWICE,
  GCC_JIT_FN_ATTRIBUTE_PURE,
  GCC_JIT_FN_ATTRIBUTE_CONST,
  GCC_JIT_FN_ATTRIBUTE_WEAK,
  GCC_JIT_FN_ATTRIBUTE_NONNULL,

#include "target-fn-attributes.def"
}
```
?

> +++ gcc/jit/jit-playback.cc
> @@ -547,6 +547,38 @@ const char* fn_attribute_to_string (gcc_jit_fn_attribute attr)
>        return "weak";
>      case GCC_JIT_FN_ATTRIBUTE_NONNULL:
>        return "nonnull";
> +    case GCC_JIT_FN_ATTRIBUTE_ARM_CMSE_NONSECURE_CALL:
More like:
```
enum gcc_jit_fn_attribute
{
...
  GCC_JIT_FN_ATTRIBUTE_TARGET_START,
};

#include "target-libgccjit.h"
```

And then inside target-libgccjit.h for the specific target, define a new enum that starts at GCC_JIT_FN_ATTRIBUTE_TARGET_START.

> +++ gcc/jit/jit-playback.cc
> @@ -547,6 +547,38 @@ const char* fn_attribute_to_string (gcc_jit_fn_attribute attr)
>        return "weak";
>      case GCC_JIT_FN_ATTRIBUTE_NONNULL:
>        return "nonnull";
> +    case GCC_JIT_FN_ATTRIBUTE_ARM_CMSE_NONSECURE_CALL:
`libgccjit` aims to provide a [strong ABI guarantee](https://gcc.gnu.org/onlinedocs/jit/topics/compatibility.html). How would you keep that guarantee with this approach?

> +++ gcc/jit/jit-playback.cc
> @@ -547,6 +547,38 @@ const char* fn_attribute_to_string (gcc_jit_fn_attribute attr)
>        return "weak";
>      case GCC_JIT_FN_ATTRIBUTE_NONNULL:
>        return "nonnull";
> +    case GCC_JIT_FN_ATTRIBUTE_ARM_CMSE_NONSECURE_CALL:
@antoyo wrote in https://forge.sourceware.org/gcc/gcc-TEST/pulls/109#issuecomment-3110:

> `libgccjit` aims to provide a [strong ABI guarantee](https://gcc.gnu.org/onlinedocs/jit/topics/compatibility.html). How would you keep that guarantee with this approach?

I don't see how that is NOT backwards compatible. Put GCC_JIT_FN_ATTRIBUTE_TARGET_START higher up if needed so you can add more inbetween. Like say `0x10_00_00`.
The ABI/API guarantee between targets is NOT talked about.

> +++ gcc/jit/jit-playback.cc
> @@ -547,6 +547,38 @@ const char* fn_attribute_to_string (gcc_jit_fn_attribute attr)
>        return "weak";
>      case GCC_JIT_FN_ATTRIBUTE_NONNULL:
>        return "nonnull";
> +    case GCC_JIT_FN_ATTRIBUTE_ARM_CMSE_NONSECURE_CALL:
How would you handle the bindings from other programming languages to the C API of libgccjit if we follow this approach?

> +++ gcc/jit/jit-playback.cc
> @@ -547,6 +547,38 @@ const char* fn_attribute_to_string (gcc_jit_fn_attribute attr)
>        return "weak";
>      case GCC_JIT_FN_ATTRIBUTE_NONNULL:
>        return "nonnull";
> +    case GCC_JIT_FN_ATTRIBUTE_ARM_CMSE_NONSECURE_CALL:
How does they work right now? Is there some processing of the header file to generate it?
If so that is how. Instead of processing the header file you process the preprocessed source.

> +++ gcc/jit/jit-playback.cc
> @@ -547,6 +547,38 @@ const char* fn_attribute_to_string (gcc_jit_fn_attribute attr)
>        return "weak";
>      case GCC_JIT_FN_ATTRIBUTE_NONNULL:
>        return "nonnull";
> +    case GCC_JIT_FN_ATTRIBUTE_ARM_CMSE_NONSECURE_CALL:
If @dmalcolm confirms that he's OK with having the libgccjit API different depending on the target, I'll implement these changes.
I wonder if this could leads to problems or ABI breakages if at some point we have a multi-arch GCC.

> +++ gcc/jit/jit-playback.cc
> @@ -547,6 +547,38 @@ const char* fn_attribute_to_string (gcc_jit_fn_attribute attr)
>        return "weak";
>      case GCC_JIT_FN_ATTRIBUTE_NONNULL:
>        return "nonnull";
> +    case GCC_JIT_FN_ATTRIBUTE_ARM_CMSE_NONSECURE_CALL:
I think I prefer @antoyo's approach here.  I'm not sure why we went with an enum for identifying attributes in gcc_jit_function_add_attribute and gcc_jit_function_add_string_attribute; perhaps using const char * would have been better.

But given that we did, I prefer having a unified enum giving all possible attributes - this might be better if we need to support cross-compilation in the future.

> +++ gcc/jit/jit-playback.cc
> @@ -547,6 +547,38 @@ const char* fn_attribute_to_string (gcc_jit_fn_attribute attr)
>        return "weak";
>      case GCC_JIT_FN_ATTRIBUTE_NONNULL:
>        return "nonnull";
> +    case GCC_JIT_FN_ATTRIBUTE_ARM_CMSE_NONSECURE_CALL:
Patch looks good to me; thanks.


--
https://forge.sourceware.org/gcc/gcc-TEST/pulls/109#issuecomment-3100