Date:   Sat, 11 Feb 2023 10:04:16 +0100
From:   Andrea Righi <andrea.righi@canonical.com>
To:     Kees Cook <kees@kernel.org>
Cc:     Miguel Ojeda <ojeda@kernel.org>,
        Alex Gaynor <alex.gaynor@gmail.com>,
        Wedson Almeida Filho <wedsonaf@gmail.com>,
        Boqun Feng <boqun.feng@gmail.com>, Gary Guo <gary@garyguo.net>,
        bjorn3_gh@protonmail.com, Kees Cook <keescook@chromium.org>,
        rust-for-linux@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] rust: allow to use INIT_STACK_ALL_ZERO
Message-ID: <Y+daEOZgaiF3IMTX@righiandr-XPS-13-7390>
References: <20230210215141.108958-1-andrea.righi@canonical.com>
 <8178319F-4188-4F8E-AD8D-DCC392E26DBF@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <8178319F-4188-4F8E-AD8D-DCC392E26DBF@kernel.org>
Precedence: bulk

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