Re: [PATCH 1/2] kbuild: rust: add `CONFIG_RUSTC_LLVM_VERSION`

[Sami Tolvanen <samitolvanen@google.com>]

On Thu, Oct 10, 2024 at 01:02:17PM +0200, Alice Ryhl wrote:
> On Thu, Oct 10, 2024 at 12:55 PM Miguel Ojeda
> <miguel.ojeda.sandonis@gmail.com> wrote:
> >
> > On Thu, Oct 10, 2024 at 11:38 AM Alice Ryhl <aliceryhl@google.com> wrote:
> > >
> > > From: Gary Guo <gary@garyguo.net>
> > >
> > > Each version of Rust supports a range of LLVM versions. There are cases where
> > > we want to gate a config on the LLVM version instead of the Rust version.
> > > Normalized cfi integer tags are one example [1].
> > >
> > > For consistency with cc-version and ld-version, the new version number is added
> > > to the existing rustc-version script, rather than being added to a new script.
> > >
> > > The invocation of rustc-version is being moved from init/Kconfig to
> > > scripts/Kconfig.include to avoid invoking rustc-version.sh twice and for
> > > consistency with cc-version.
> > >
> > > Link: https://lore.kernel.org/all/20240925-cfi-norm-kasan-fix-v1-1-0328985cdf33@google.com/ [1]
> > > Signed-off-by: Gary Guo <gary@garyguo.net>
> >
> > Alice: when I apply this, I will need to add your Signed-off-by here
> > (i.e. when handling patches from others, you need to add your SoB
> > too).
> >
> > > +if output=$("$@" --version --verbose 2>/dev/null | grep LLVM); then
> > > +       set -- $output
> > > +       rustc_llvm_version=$(get_llvm_canonical_version $3)
> > > +else
> > > +       echo 0 0
> > > +       exit 1
> > > +fi
> >
> > I guess if we don't find "LLVM" in the output, something weird is
> > going on, so I guess it is reasonable not printing either here.
> > Although, in principle, we could preserve information and print at
> > least the `$rustc` one.
> >
> > Anyway, we may need to rethink this when we start supporting e.g. the
> > GCC backend, so I think it is fine as it is.
> 
> I guess we can just do
> 
> rustc_llvm_version=0
> 
> in that case?

Agreed, that should be sufficient. I'm not sure what the GCC back-end
status is, but this patch looks good to me whether you plan to fold in
this change or not. FWIW:

Reviewed-by: Sami Tolvanen <samitolvanen@google.com>

Sami