Re: [PATCH 2/2] RISC-V: re-enable gcc + rust builds

[Asuna]

> One thing - please don't send new versions
> of patchsets in response to earlier versions or other threads. It
> doesn't do you any favours with mailbox visibility.

I apologize for this, I'm pretty much new to mailing lists, so I had 
followed the step "Explicit In-Reply-To headers" [1] in doc. For future 
patches I'll send them alone instead of replying to existing threads.

[1]: 
https://www.kernel.org/doc/html/v6.9/process/submitting-patches.html#explicit-in-reply-to-headers

> Other than Zicsr/Zifencei that may need explicit handling in a dedicated
> option, the approach here seems kinda backwards.
> Individually these symbols don't actually mean what they say they do,
> which is confusing: "recognises" here is true even when it may not be
> true at all because TOOLCHAIN_HAS_FOO is not set. Why can these options
> not be removed, and instead the TOOLCHAIN_HAS_FOO options grow a
> "depends on !RUST || <condition>"?

Yes, it's kinda "backwards", which is intentional, based on the 
following considerations:

1) As mentioned in rust/Makefile, filtering flags for libclang is a 
hack, because currently bindgen only has libclang as backend, and 
ideally bindgen should support GCC so that the passed CC flags are 
supposed to be fully compatible. On the RISC-V side, I tend to think 
that version checking for extensions for libclang is also a hack, which 
could have been accomplished with just the cc-option function, ideally.

2) Rust bindgen only "generates" FFI stuff, it is not involved in the 
final assembly stage. In other words, it doesn't matter so much what 
RISC-V extensions to turn on for bindgen (although it does have a little 
impact, like some macro switches), it's more matter to CC.

Therefore, I chose not to modify the original extension config 
conditions so that if libclang doesn't support the CC flag for an 
extension, then the Rust build is not supported, rather than treating 
the extension as not supported.

Nonetheless, it occurred to me as I was writing this reply that if GCC 
implements a new extension in the future that LLVM/Clang doesn't yet 
have, this could once again lead to a break in GCC+Rust build support if 
the kernel decides to use the new extension. So it's a trade-off, you 
guys decide, I'm fine with both.

Regarding the name, initially I named it "compatible", and ended up 
changed it to "recognize" before sending the patch. If we continue on 
this path, I'm not sure what name is appropriate to use here, do you 
guys have any ideas?

> What does the libclang >= 17 requirement actually do here? Is that the
> version where llvm starts to require that Zicsr/Zifencei is set in order
> to use them? I think a comment to that effect is required if so. This
> doesn't actually need to be blocking either, should just be able to
> filter it out of march when passing to bindgen, no?

libclang >= 17 starts recognizing Zicsr/Zifencei in -march, passing them 
to -march doesn't generate an error, and passing them or not doesn't 
have any real difference. (still follows ISA before version 20190608 -- 
Zicsr/Zifencei are included in base ISA). I should have written a 
comment there to avoid confusion.

Reference commit in LLVM/Clang 22e199e6af ("[RISCV] Accept zicsr and 
zifencei command line options")
https://github.com/llvm/llvm-project/commit/22e199e6afb1263c943c0c0d4498694e15bf8a16

> What about the case where TOOLCHAIN_NEEDS_EXPLICIT_ZICSR_ZIFENCEI is not
> set at all? Currently your patch is going to block rust in that case,
> when actually nothing needs to be done at all - no part of the toolchain
> requires understanding Zicsr/Zifencei as standalone extensions in this
> case.

This is a bug, I missed this case. So it should be corrected to:

   config RUST_BINDGEN_LIBCLANG_RECOGNIZES_ZICSR_ZIFENCEI
        def_bool y
        depends on TOOLCHAIN_NEEDS_OLD_ISA_SPEC ||
   !TOOLCHAIN_NEEDS_EXPLICIT_ZICSR_ZIFENCEI ||
   RUST_BINDGEN_LIBCLANG_VERSION >= 170000


> The TOOLCHAIN_NEEDS_OLD_ISA_SPEC handling I don't remember 100% how it
> works, but if bindgen requires them to be set to use the extension
> this will return true but do nothing to add the extensions to march?
> That seems wrong to me.
> I'd be fairly amenable to disabling rust though when used in combination
> with gcc < 11.3 and gas >=2.36 since it's such a niche condition, rather
> doing work to support it. That'd be effectively an inversion of your
> first condition.

The current latest version of LLVM/Clang still does not require explicit 
Zicsr/Zifence to enable these two extensions, Clang just accepts them in 
-march and then silently ignores them.

Checking the usage of CONFIG_TOOLCHAIN_NEEDS_OLD_ISA_SPEC:

   ifdef CONFIG_TOOLCHAIN_NEEDS_OLD_ISA_SPEC
   KBUILD_CFLAGS += -Wa,-misa-spec=2.2
   KBUILD_AFLAGS += -Wa,-misa-spec=2.2
   else
   riscv-march-$(CONFIG_TOOLCHAIN_NEEDS_EXPLICIT_ZICSR_ZIFENCEI) :=
   $(riscv-march-y)_zicsr_zifencei
   endif

It just uses -Wa to force an older ISA version to GAS. So the 
RUST_BINDGEN_LIBCLANG_RECOGNIZES_ZICSR_ZIFENCEI I corrected above should 
be fine now I guess? Or would you still prefer your idea of blocking 
Rust if TOOLCHAIN_NEEDS_OLD_ISA_SPEC is true?

(To be clear, the breaking changes regarding Zicsr/Zifence are since ISA 
version 20190608, and versions 2.0, 2.1, 2.2 are older than 20190608)

The only thing I'm confused about is that according to the comment of 
TOOLCHAIN_NEEDS_EXPLICIT_ZICSR_ZIFENCEI, GCC-12.1.0 bumped the default 
ISA to 20191213, but why doesn't the depends-on have condition || 
(CC_IS_GCC && GCC_VERSION >= 120100)?

Thanks for your detailed review.