Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220302234400.782002-1-nathan@kernel.org>
In-Reply-To: <20220302234400.782002-1-nathan@kernel.org>
From:   Masahiro Yamada <masahiroy@kernel.org>
Date:   Fri, 4 Mar 2022 20:16:00 +0900
Message-ID: <CAK7LNARAPqTaO0Zho0VFib6kxjfpbnvrX-ZwcVPQgES5T8z4qQ@mail.gmail.com>
Subject: Re: [PATCH] kbuild: Allow a suffix with $(LLVM)
To:     Nathan Chancellor <nathan@kernel.org>
Cc:     Nick Desaulniers <ndesaulniers@google.com>,
        Shuah Khan <shuah@kernel.org>, llvm@lists.linux.dev,
        Linux Kbuild mailing list <linux-kbuild@vger.kernel.org>,
        "open list:DOCUMENTATION" <linux-doc@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@vger.kernel.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Kees Cook <keescook@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Mar 3, 2022 at 8:47 AM Nathan Chancellor <nathan@kernel.org> wrote:
>
> The LLVM make variable allows a developer to quickly switch between the
> GNU and LLVM tools. However, it does not handle versioned binaries, such
> as the ones shipped by Debian, as LLVM=1 just defines the tool variables
> with the unversioned binaries.
>
> There was some discussion during the review of the patch that introduces
> LLVM=1 around versioned binaries, ultimately coming to the conclusion
> that developers can just add the folder that contains the unversioned
> binaries to their PATH, as Debian's versioned suffixed binaries are
> really just symlinks to the unversioned binaries in /usr/lib/llvm-#/bin:
>
> $ realpath /usr/bin/clang-14
> /usr/lib/llvm-14/bin/clang
>
> $ PATH=/usr/lib/llvm-14/bin:$PATH make ... LLVM=1
>
> However, that can be cumbersome to developers who are constantly testing
> series with different toolchains and versions. It is simple enough to
> support these versioned binaries directly in the Kbuild system by
> allowing the developer to specify the version suffix with LLVM=, which
> is shorter than the above suggestion:
>
> $ make ... LLVM=-14
>
> It does not change the meaning of LLVM=1 (which will continue to use
> unversioned binaries) and it does not add too much additional complexity
> to the existing $(LLVM) code, while allowing developers to quickly test
> their series with different versions of the whole LLVM suite of tools.
>
> Link: https://lore.kernel.org/r/20200317215515.226917-1-ndesaulniers@google.com/
> Link: https://lore.kernel.org/r/20220224151322.072632223@infradead.org/
> Suggested-by: Peter Zijlstra <peterz@infradead.org>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
> Signed-off-by: Nathan Chancellor <nathan@kernel.org>
> ---
>
> RFC -> v1: https://lore.kernel.org/r/Yh%2FegU1LZudfrgVy@dev-arch.thelio-3990X/
>
> * Tidy up commit message slightly.
>
> * Add tags.
>
> * Add links to prior discussions for context.
>
> * Add change to tools/testing/selftests/lib.mk.
>
> I would like for this to go through the Kbuild tree, please ack as
> necessary.
>
>  Documentation/kbuild/llvm.rst  |  7 +++++++
>  Makefile                       | 24 ++++++++++++++----------
>  tools/scripts/Makefile.include | 20 ++++++++++++--------
>  tools/testing/selftests/lib.mk |  6 +++++-
>  4 files changed, 38 insertions(+), 19 deletions(-)
>
> diff --git a/Documentation/kbuild/llvm.rst b/Documentation/kbuild/llvm.rst
> index d32616891dcf..5805a8473a36 100644
> --- a/Documentation/kbuild/llvm.rst
> +++ b/Documentation/kbuild/llvm.rst
> @@ -60,6 +60,13 @@ They can be enabled individually. The full list of the parameters: ::
>           OBJCOPY=llvm-objcopy OBJDUMP=llvm-objdump READELF=llvm-readelf \
>           HOSTCC=clang HOSTCXX=clang++ HOSTAR=llvm-ar HOSTLD=ld.lld
>
> +If your LLVM tools have a suffix and you prefer to test an explicit version rather
> +than the unsuffixed executables, use ``LLVM=<suffix>``. For example: ::
> +
> +       make LLVM=-14
> +
> +will use ``clang-14``, ``ld.lld-14``, etc.
> +
>  The integrated assembler is enabled by default. You can pass ``LLVM_IAS=0`` to
>  disable it.


Perhaps, it might be worth mentioning the difference between
LLVM=1 and LLVM=<suffix>

The current behavior is,
any value other than '1' is regarded as a suffix.


> diff --git a/Makefile b/Makefile
> index a82095c69fdd..963840c00eae 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -424,8 +424,12 @@ HOST_LFS_LDFLAGS := $(shell getconf LFS_LDFLAGS 2>/dev/null)
>  HOST_LFS_LIBS := $(shell getconf LFS_LIBS 2>/dev/null)
>
>  ifneq ($(LLVM),)
> -HOSTCC = clang
> -HOSTCXX        = clang++
> +ifneq ($(LLVM),1)
> +LLVM_SFX := $(LLVM)
> +endif

I am OK with this, but please note LLVM=0
uses 'clang0', 'ld.lld0' instead of disabling
LLVM explicitly.

This might be a small surprise because LLVM_IAS=0
is used to disable the integrated assembler.


If you want handle LLVM=<suffix>
only when <suffix> start with a hyphen,
you can do like this:

ifneq ($(filter -%, $(LLVM)),)
LLVM_SFX := $(LLVM)
endif


In the future, If somebody requests to support
    make LLVM=/path/to/my/own/llvm/dir/
to use llvm tools in that path,
we can expand the code like this:


# "LLVM=foo/bar/" is a syntax sugar of "LLVM=1 LLVM_PFX=foo/bar"
# "LLVM=-foo" is a syntax sugar of "LLVM=1 LLVM_SFX=-foo"

ifneq ($(filter %/, $(LLVM)),)
LLVM_PFX := $(LLVM)
else ifneq ($(filter -%, $(LLVM)),)
LLVM_SFX := $(LLVM)
endif


Lastly, I personally prefer to fully spell LLVM_SUFFIX
as Nick originally suggested:
https://lkml.org/lkml/2020/3/17/1477


-- 
Best Regards
Masahiro Yamada