[v2] rust: allow to use INIT_STACK_ALL_ZERO

  With CONFIG_INIT_STACK_ALL_ZERO enabled, bindgen passes
-ftrivial-auto-var-init=zero to clang, that triggers the following
error:

 error: '-ftrivial-auto-var-init=zero' hasn't been enabled; enable it at your own peril for benchmarking purpose only with '-enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang'

However, this additional option that is currently required by clang is
going to be removed in the future (as the name of the option suggests),
likely with clang-17.

So, make sure bindgen is using this extra option if the major version of
the libclang used by bindgen is < 17.

In this way we can enable CONFIG_INIT_STACK_ALL_ZERO with CONFIG_RUST
without triggering any build error.

Link: https://github.com/llvm/llvm-project/issues/44842
Signed-off-by: Andrea Righi <andrea.righi@canonical.com>
---

Changes in v2:
 - check the version of libclang used by bindgen to determine if we need
   to pass the additional clang option

 rust/Makefile | 13 +++++++++++++
 1 file changed, 13 insertions(+)
  

On February 10, 2023 1:51:41 PM PST, Andrea Righi <andrea.righi@canonical.com> wrote:
>With CONFIG_INIT_STACK_ALL_ZERO enabled, bindgen passes
>-ftrivial-auto-var-init=zero to clang, that triggers the following
>error:
>
> error: '-ftrivial-auto-var-init=zero' hasn't been enabled; enable it at your own peril for benchmarking purpose only with '-enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang'
>
>However, this additional option that is currently required by clang is
>going to be removed in the future (as the name of the option suggests),
>likely with clang-17.
>
>So, make sure bindgen is using this extra option if the major version of
>the libclang used by bindgen is < 17.
>
>In this way we can enable CONFIG_INIT_STACK_ALL_ZERO with CONFIG_RUST
>without triggering any build error.
>
>Link: https://github.com/llvm/llvm-project/issues/44842
>Signed-off-by: Andrea Righi <andrea.righi@canonical.com>
>---
>
>Changes in v2:
> - check the version of libclang used by bindgen to determine if we need
>   to pass the additional clang option
>
> rust/Makefile | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
>diff --git a/rust/Makefile b/rust/Makefile
>index ff70c4c916f8..c77d7ce96a85 100644
>--- a/rust/Makefile
>+++ b/rust/Makefile
>@@ -269,6 +269,19 @@ BINDGEN_TARGET		:= $(BINDGEN_TARGET_$(SRCARCH))
> # some configurations, with new GCC versions, etc.
> bindgen_extra_c_flags = -w --target=$(BINDGEN_TARGET)
> 
>+# Auto variable zero-initialization requires an additional special option with
>+# clang that is going to be removed sometimes in the future (likely in
>+# clang-17), so make sure to pass this option only if clang supports it
>+# (libclang major version < 17).
>+#
>+# https://github.com/llvm/llvm-project/issues/44842
>+ifdef CONFIG_INIT_STACK_ALL_ZERO
>+libclang_maj_ver=$(shell $(BINDGEN) $(srctree)/scripts/rust_is_available_bindgen_libclang.h 2>&1 | sed -ne 's/.*clang version \([0-9]*\).*/\1/p')
>+ifeq ($(shell expr $(libclang_maj_ver) \< 17), 1)
>+bindgen_extra_c_flags += -enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang
>+endif
>+endif

This logic already exists in the top-level Makefile. How does -ftrivial-auto-var-init=zero make it into bindgen_c_flags normally? (I.e. why does the legacy -enable... option not?)

>+
> bindgen_c_flags = $(filter-out $(bindgen_skip_c_flags), $(c_flags)) \
> 	$(bindgen_extra_c_flags)
> endif
  

On Fri, Feb 10, 2023 at 02:43:56PM -0800, Kees Cook wrote:
> On February 10, 2023 1:51:41 PM PST, Andrea Righi <andrea.righi@canonical.com> wrote:
> >With CONFIG_INIT_STACK_ALL_ZERO enabled, bindgen passes
> >-ftrivial-auto-var-init=zero to clang, that triggers the following
> >error:
> >
> > error: '-ftrivial-auto-var-init=zero' hasn't been enabled; enable it at your own peril for benchmarking purpose only with '-enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang'
> >
> >However, this additional option that is currently required by clang is
> >going to be removed in the future (as the name of the option suggests),
> >likely with clang-17.
> >
> >So, make sure bindgen is using this extra option if the major version of
> >the libclang used by bindgen is < 17.
> >
> >In this way we can enable CONFIG_INIT_STACK_ALL_ZERO with CONFIG_RUST
> >without triggering any build error.
> >
> >Link: https://github.com/llvm/llvm-project/issues/44842
> >Signed-off-by: Andrea Righi <andrea.righi@canonical.com>
> >---
> >
> >Changes in v2:
> > - check the version of libclang used by bindgen to determine if we need
> >   to pass the additional clang option
> >
> > rust/Makefile | 13 +++++++++++++
> > 1 file changed, 13 insertions(+)
> >
> >diff --git a/rust/Makefile b/rust/Makefile
> >index ff70c4c916f8..c77d7ce96a85 100644
> >--- a/rust/Makefile
> >+++ b/rust/Makefile
> >@@ -269,6 +269,19 @@ BINDGEN_TARGET		:= $(BINDGEN_TARGET_$(SRCARCH))
> > # some configurations, with new GCC versions, etc.
> > bindgen_extra_c_flags = -w --target=$(BINDGEN_TARGET)
> > 
> >+# Auto variable zero-initialization requires an additional special option with
> >+# clang that is going to be removed sometimes in the future (likely in
> >+# clang-17), so make sure to pass this option only if clang supports it
> >+# (libclang major version < 17).
> >+#
> >+# https://github.com/llvm/llvm-project/issues/44842
> >+ifdef CONFIG_INIT_STACK_ALL_ZERO
> >+libclang_maj_ver=$(shell $(BINDGEN) $(srctree)/scripts/rust_is_available_bindgen_libclang.h 2>&1 | sed -ne 's/.*clang version \([0-9]*\).*/\1/p')
> >+ifeq ($(shell expr $(libclang_maj_ver) \< 17), 1)
> >+bindgen_extra_c_flags += -enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang
> >+endif
> >+endif
> 
> This logic already exists in the top-level Makefile. How does -ftrivial-auto-var-init=zero make it into bindgen_c_flags normally? (I.e. why does the legacy -enable... option not?)

If we're using gcc, the top-level Makefile doesn't set the clang
options, so in this case CONFIG_CC_HAS_AUTO_VAR_INIT_ZERO_ENABLER is
undefined.  But then bindgen always relies on libclang to parse C, even
if gcc is used globally, therefore it needs the extra clang flag.

Ideally it'd be nice if bindgen would support a gcc backend, but until
then we need to do something special here, like this kind of duplicated
logic...

-Andrea

> 
> >+
> > bindgen_c_flags = $(filter-out $(bindgen_skip_c_flags), $(c_flags)) \
> > 	$(bindgen_extra_c_flags)
> > endif
> 
> 
> -- 
> Kees Cook
  

On February 11, 2023 1:04:16 AM PST, Andrea Righi <andrea.righi@canonical.com> wrote:
>On Fri, Feb 10, 2023 at 02:43:56PM -0800, Kees Cook wrote:
>> On February 10, 2023 1:51:41 PM PST, Andrea Righi <andrea.righi@canonical.com> wrote:
>> >With CONFIG_INIT_STACK_ALL_ZERO enabled, bindgen passes
>> >-ftrivial-auto-var-init=zero to clang, that triggers the following
>> >error:
>> >
>> > error: '-ftrivial-auto-var-init=zero' hasn't been enabled; enable it at your own peril for benchmarking purpose only with '-enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang'
>> >
>> >However, this additional option that is currently required by clang is
>> >going to be removed in the future (as the name of the option suggests),
>> >likely with clang-17.
>> >
>> >So, make sure bindgen is using this extra option if the major version of
>> >the libclang used by bindgen is < 17.
>> >
>> >In this way we can enable CONFIG_INIT_STACK_ALL_ZERO with CONFIG_RUST
>> >without triggering any build error.
>> >
>> >Link: https://github.com/llvm/llvm-project/issues/44842
>> >Signed-off-by: Andrea Righi <andrea.righi@canonical.com>
>> >---
>> >
>> >Changes in v2:
>> > - check the version of libclang used by bindgen to determine if we need
>> >   to pass the additional clang option
>> >
>> > rust/Makefile | 13 +++++++++++++
>> > 1 file changed, 13 insertions(+)
>> >
>> >diff --git a/rust/Makefile b/rust/Makefile
>> >index ff70c4c916f8..c77d7ce96a85 100644
>> >--- a/rust/Makefile
>> >+++ b/rust/Makefile
>> >@@ -269,6 +269,19 @@ BINDGEN_TARGET		:= $(BINDGEN_TARGET_$(SRCARCH))
>> > # some configurations, with new GCC versions, etc.
>> > bindgen_extra_c_flags = -w --target=$(BINDGEN_TARGET)
>> > 
>> >+# Auto variable zero-initialization requires an additional special option with
>> >+# clang that is going to be removed sometimes in the future (likely in
>> >+# clang-17), so make sure to pass this option only if clang supports it
>> >+# (libclang major version < 17).
>> >+#
>> >+# https://github.com/llvm/llvm-project/issues/44842
>> >+ifdef CONFIG_INIT_STACK_ALL_ZERO
>> >+libclang_maj_ver=$(shell $(BINDGEN) $(srctree)/scripts/rust_is_available_bindgen_libclang.h 2>&1 | sed -ne 's/.*clang version \([0-9]*\).*/\1/p')
>> >+ifeq ($(shell expr $(libclang_maj_ver) \< 17), 1)
>> >+bindgen_extra_c_flags += -enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang
>> >+endif
>> >+endif
>> 
>> This logic already exists in the top-level Makefile. How does -ftrivial-auto-var-init=zero make it into bindgen_c_flags normally? (I.e. why does the legacy -enable... option not?)
>
>If we're using gcc, the top-level Makefile doesn't set the clang
>options, so in this case CONFIG_CC_HAS_AUTO_VAR_INIT_ZERO_ENABLER is
>undefined.  But then bindgen always relies on libclang to parse C, even
>if gcc is used globally, therefore it needs the extra clang flag.

Ah yes! Thank you, I keep forgetting about mixed compiler builds.

>
>Ideally it'd be nice if bindgen would support a gcc backend, but until
>then we need to do something special here, like this kind of duplicated
>logic...

Right. Yeah, good fix. One nit: the -enable... flag is removed in Clang 16+:

https://github.com/llvm/llvm-project/blob/llvmorg-16.0.0-rc2/clang/docs/ReleaseNotes.rst#deprecated-compiler-flags

With that fixed:

Reviewed-by: Kees Cook <keescook@chromium.org>
  

On Sat, Feb 11, 2023 at 05:44:33AM -0800, Kees Cook wrote:
> On February 11, 2023 1:04:16 AM PST, Andrea Righi <andrea.righi@canonical.com> wrote:
> >On Fri, Feb 10, 2023 at 02:43:56PM -0800, Kees Cook wrote:
> >> On February 10, 2023 1:51:41 PM PST, Andrea Righi <andrea.righi@canonical.com> wrote:
> >> >With CONFIG_INIT_STACK_ALL_ZERO enabled, bindgen passes
> >> >-ftrivial-auto-var-init=zero to clang, that triggers the following
> >> >error:
> >> >
> >> > error: '-ftrivial-auto-var-init=zero' hasn't been enabled; enable it at your own peril for benchmarking purpose only with '-enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang'
> >> >
> >> >However, this additional option that is currently required by clang is
> >> >going to be removed in the future (as the name of the option suggests),
> >> >likely with clang-17.
> >> >
> >> >So, make sure bindgen is using this extra option if the major version of
> >> >the libclang used by bindgen is < 17.
> >> >
> >> >In this way we can enable CONFIG_INIT_STACK_ALL_ZERO with CONFIG_RUST
> >> >without triggering any build error.
> >> >
> >> >Link: https://github.com/llvm/llvm-project/issues/44842
> >> >Signed-off-by: Andrea Righi <andrea.righi@canonical.com>
> >> >---
> >> >
> >> >Changes in v2:
> >> > - check the version of libclang used by bindgen to determine if we need
> >> >   to pass the additional clang option
> >> >
> >> > rust/Makefile | 13 +++++++++++++
> >> > 1 file changed, 13 insertions(+)
> >> >
> >> >diff --git a/rust/Makefile b/rust/Makefile
> >> >index ff70c4c916f8..c77d7ce96a85 100644
> >> >--- a/rust/Makefile
> >> >+++ b/rust/Makefile
> >> >@@ -269,6 +269,19 @@ BINDGEN_TARGET		:= $(BINDGEN_TARGET_$(SRCARCH))
> >> > # some configurations, with new GCC versions, etc.
> >> > bindgen_extra_c_flags = -w --target=$(BINDGEN_TARGET)
> >> > 
> >> >+# Auto variable zero-initialization requires an additional special option with
> >> >+# clang that is going to be removed sometimes in the future (likely in
> >> >+# clang-17), so make sure to pass this option only if clang supports it
> >> >+# (libclang major version < 17).
> >> >+#
> >> >+# https://github.com/llvm/llvm-project/issues/44842
> >> >+ifdef CONFIG_INIT_STACK_ALL_ZERO
> >> >+libclang_maj_ver=$(shell $(BINDGEN) $(srctree)/scripts/rust_is_available_bindgen_libclang.h 2>&1 | sed -ne 's/.*clang version \([0-9]*\).*/\1/p')
> >> >+ifeq ($(shell expr $(libclang_maj_ver) \< 17), 1)
> >> >+bindgen_extra_c_flags += -enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang
> >> >+endif
> >> >+endif
> >> 
> >> This logic already exists in the top-level Makefile. How does -ftrivial-auto-var-init=zero make it into bindgen_c_flags normally? (I.e. why does the legacy -enable... option not?)
> >
> >If we're using gcc, the top-level Makefile doesn't set the clang
> >options, so in this case CONFIG_CC_HAS_AUTO_VAR_INIT_ZERO_ENABLER is
> >undefined.  But then bindgen always relies on libclang to parse C, even
> >if gcc is used globally, therefore it needs the extra clang flag.
> 
> Ah yes! Thank you, I keep forgetting about mixed compiler builds.
> 
> >
> >Ideally it'd be nice if bindgen would support a gcc backend, but until
> >then we need to do something special here, like this kind of duplicated
> >logic...
> 
> Right. Yeah, good fix. One nit: the -enable... flag is removed in Clang 16+:

oic, with clang-16 we get a deprecated warning (but it doesn't fail),
then starting with clang-17 the -enable.. flag is not available anymore.

Then I agree that a better check would be version < 16, instead of < 17.

Thanks,
-Andrea

> 
> https://github.com/llvm/llvm-project/blob/llvmorg-16.0.0-rc2/clang/docs/ReleaseNotes.rst#deprecated-compiler-flags
> 
> With that fixed:
> 
> Reviewed-by: Kees Cook <keescook@chromium.org>
> 
> 
> -- 
> Kees Cook
  

On Sat, Feb 11, 2023 at 3:35 PM Andrea Righi <andrea.righi@canonical.com> wrote:
>
> oic, with clang-16 we get a deprecated warning (but it doesn't fail),
> then starting with clang-17 the -enable.. flag is not available anymore.

Wait, Kees' link says it is deprecated in Clang 16, and the removal
will happen in 18.

> Then I agree that a better check would be version < 16, instead of < 17.

Applied to `rust-fixes` with that changed. Please take a look if it is
what you expected (both content and commit message):
https://github.com/Rust-for-Linux/linux/commit/1921ea54ab0d91d7a660c5bb980fce1efc9223f9

Thanks!

Cheers,
Miguel
  

On Fri, Apr 07, 2023 at 12:52:18AM +0200, Miguel Ojeda wrote:
> On Sat, Feb 11, 2023 at 3:35 PM Andrea Righi <andrea.righi@canonical.com> wrote:
> >
> > oic, with clang-16 we get a deprecated warning (but it doesn't fail),
> > then starting with clang-17 the -enable.. flag is not available anymore.
> 
> Wait, Kees' link says it is deprecated in Clang 16, and the removal
> will happen in 18.
> 
> > Then I agree that a better check would be version < 16, instead of < 17.
> 
> Applied to `rust-fixes` with that changed. Please take a look if it is
> what you expected (both content and commit message):
> https://github.com/Rust-for-Linux/linux/commit/1921ea54ab0d91d7a660c5bb980fce1efc9223f9

The check (< 16) looks correct and the comment also looks correct to me,
this option will be removed in clang 18, as mentioned here:
https://github.com/llvm/llvm-project/blob/llvmorg-16.0.0-rc2/clang/docs/ReleaseNotes.rst#deprecated-compiler-flags

(Maybe we could add also this link as a reference)

Thanks!
-Andrea
  

On Fri, Apr 7, 2023 at 8:02 AM Andrea Righi <andrea.righi@canonical.com> wrote:
>
> The check (< 16) looks correct and the comment also looks correct to me,
> this option will be removed in clang 18, as mentioned here:
> https://github.com/llvm/llvm-project/blob/llvmorg-16.0.0-rc2/clang/docs/ReleaseNotes.rst#deprecated-compiler-flags
>
> (Maybe we could add also this link as a reference)

Thanks for taking a look -- Kees' link added!

I removed "(as the name of the option suggests)" from the commit
message, by the way, since the name of the option suggests the
original `-ftrivial` option was to be removed, not the `-enable` one.
If that understanding is wrong, please let me know!

Cheers,
Miguel
  

On Sun, Apr 09, 2023 at 10:49:58PM +0200, Miguel Ojeda wrote:
> On Fri, Apr 7, 2023 at 8:02 AM Andrea Righi <andrea.righi@canonical.com> wrote:
> >
> > The check (< 16) looks correct and the comment also looks correct to me,
> > this option will be removed in clang 18, as mentioned here:
> > https://github.com/llvm/llvm-project/blob/llvmorg-16.0.0-rc2/clang/docs/ReleaseNotes.rst#deprecated-compiler-flags
> >
> > (Maybe we could add also this link as a reference)
> 
> Thanks for taking a look -- Kees' link added!
> 
> I removed "(as the name of the option suggests)" from the commit
> message, by the way, since the name of the option suggests the
> original `-ftrivial` option was to be removed, not the `-enable` one.
> If that understanding is wrong, please let me know!

Yes, that looks more clear, the name of the option is a bit misleading. :)

To be clear what is going to be removed is
-enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang,
in this way clang will be compatible with gcc and they can both use
-ftrivial-auto-var-init=zero.

Thanks,
-Andrea



> 
> Cheers,
> Miguel
  

On Tue, Apr 11, 2023 at 9:11 AM Andrea Righi <andrea.righi@canonical.com> wrote:
>
> Yes, that looks more clear, the name of the option is a bit misleading. :)
>
> To be clear what is going to be removed is
> -enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang,
> in this way clang will be compatible with gcc and they can both use
> -ftrivial-auto-var-init=zero.

Exactly, i.e. `-enable` is the one getting removed, but the wording
within it (`knowing-it-will-be-removed-from-clang`) meant that the
`-ftrivial...=zero` one was the one that would be removed, not
`-enable` itself (even if now what will happen is the opposite than
what the option suggested originally).

So when I read the commit message, the "this additional option ... is
going to be removed in the future (as the name of the option
suggests)" sounded wrong, since the name did not suggest that, but
rather that `-ftrivial...=zero` would be removed, not itself (since
that was the original plan).

If I understood the story properly, that is. :)

Thanks for taking a look!

Cheers,
Miguel