Message-ID: <CAKwvOdkdb=dDggNNPHb08AiZNp5V-H9utgm0H+2hJmZJdO-biA@mail.gmail.com>
Date:   Thu, 24 Aug 2023 13:32:42 -0700
From:   Nick Desaulniers <ndesaulniers@...gle.com>
To:     Nathan Chancellor <nathan@...nel.org>
Cc:     Masahiro Yamada <masahiroy@...nel.org>, Tom Rix <trix@...hat.com>,
        Nicolas Schier <nicolas@...sle.eu>,
        Jonathan Corbet <corbet@....net>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Palmer Dabbelt <palmer@...belt.com>,
        Albert Ou <aou@...s.berkeley.edu>, llvm@...ts.linux.dev,
        linux-kbuild@...r.kernel.org, linux-doc@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-riscv@...ts.infradead.org
Subject: Re: [PATCH] Documentation/llvm: refresh docs

On Thu, Aug 24, 2023 at 11:49 AM Nathan Chancellor <nathan@...nel.org> wrote:
>
> On Thu, Aug 24, 2023 at 11:03:17AM -0700, ndesaulniers@...gle.com wrote:
> > Link: https://github.com/ClangBuiltLinux/linux/issues/1907 [0]
> > ---
> >
> >
> > Signed-off-by: Nick Desaulniers <ndesaulniers@...gle.com>
>
> Nit: Your signed-off-by ended up below the fold, was it in your cover
> letter commit rather than your actual commit?

Heh, no. I'm restoring a machine after suffering drive corruption last
week. I just got patatt and friends re-set up, but I forgot to `git
commit --amend -s` for this patch once everything was working again.
Thanks for catching this!

>
> Aside from the relatively minor comments below, this looks like a really
> good improvement to the documentation to me. It feels like it is more
> targeting users or non-kbuild folks now, which I think is great.
>
> I trust you to address my comments as you see fit, so please carry
> forward:
>
> Reviewed-by: Nathan Chancellor <nathan@...nel.org>

Thanks for the review!

> I see a few new kernel-doc warnings from not adjusting the underlines to
> match the new length of the title:
>
>   Documentation/kbuild/llvm.rst:40: WARNING: Title underline too short.
>
>   The LLVM= argument
>   --------------
>   Documentation/kbuild/llvm.rst:40: WARNING: Title underline too short.
>
>   The LLVM= argument
>   --------------
>   Documentation/kbuild/llvm.rst:102: WARNING: Title underline too short.
>
>   The LLVM_IAS= argument
>   -----------------
>   Documentation/kbuild/llvm.rst:102: WARNING: Title underline too short.
>
>   The LLVM_IAS= argument
>   -----------------

oops! remind me of the make target to observe these?

> > -For example, to cross-compile the arm64 kernel::
> > +     make LLVM=1 LD=ld.bfd CROSS_COMPILE=s390x-linux-gnu-
>
> This should probably have ARCH=s390?

Oops! Good catch!

>
> > -     make ARCH=arm64 LLVM=1
> > +``CROSS_COMPILE`` is not used to prefix the Clang compiler binary (or
> > +corresponding LLVM utilities), but it will be for any GNU toolchain utilities.
> > +This example will invoke ``s390x-linux-gnu-ld.bfd`` as the linker, so ensure
> > +that is reachable in your ``$PATH``.
>
> I like the example as I feel like it addresses some of the fear I have had
> around recommending LLVM=1 as the initial build suggestion but 'LLVM=1
> LD=ld.bfd CROSS_COMPILE=s390x-linux-gnu-' does not compose as you describe here
> because $(LD) is not prefixed with $(CROSS_COMPILE) anywhere in Makefile. The
> non-$(LLVM) default assignment of $(LD) is '$(CROSS_COMPILE)ld' and that is
> overridden by 'LD=ld.bfd' on the command line.
>
> In other words, this should be
>
>   make ARCH=s390 LLVM=1 LD=s390x-linux-gnu-ld.bfd
>
> and have the note about CROSS_COMPILE prefixing any GNU toolchain utilities
> removed. It should problably have OBJCOPY and OBJDUMP too, as those are
> required due to https://github.com/ClangBuiltLinux/linux/issues/859 and
> https://github.com/ClangBuiltLinux/linux/issues/1530.

Ah right; I'm no longer able to repro the build failure with OBJDUMP,
though I still see warnings.

I kind of feel like we should bring back CROSS_COMPILE in some form;
having to respecify the triple is still sub-optimal.

i.e. today:
$ make LLVM=1 ARCH=s390 LD=s390x-linux-gnu-ld.bfd
OBJCOPY=s390x-linux-gnu-objcopy OBJDUMP=s390x-linux-gnu-objdump

vs

$ make LLVM=1 ARCH=s390 CROSS_COMPILE=s390x-linux-gnu- LD=ld.bfd
OBJCOPY=objcopy OBJDUMP=objdump

but perhaps that's a change for another day.

>
> > -If ``LLVM_IAS=0`` is specified, ``CROSS_COMPILE`` is also used to derive
> > -``--prefix=<path>`` to search for the GNU assembler and linker. ::
> > +The LLVM_IAS= argument
> > +-----------------
> >
> > -     make ARCH=arm64 LLVM=1 LLVM_IAS=0 CROSS_COMPILE=aarch64-linux-gnu-
> > +Clang can assemble assembler code. You can pass ``LLVM_IAS=0`` to disable this
> > +behavior and have Clang invoke the system assembler instead (or the assembler
> > +based on ``CROSS_COMPILE``). ``CROSS_COMPILE`` is necessary when ``LLVM_IAS=0``
> > +is set when cross compiling in order to set ``--prefix=`` for the compiler to
> > +find the corresponding non-integrated assembler.
>
> Thanks a lot for documenting this behavior, it is one of the most common
> issues I run into myself (adding LLVM_IAS=0 without CROSS_COMPILE) and
> maybe this note will be what I need in order to remember :)

Ah right, I'll make that clearer.

> >
> > -We provide prebuilt stable versions of LLVM on `kernel.org <https://kernel.org/pub/tools/llvm/>`_.
> > -Below are links that may be useful for building LLVM from source or procuring
> > -it through a distribution's package manager.
> > +We provide prebuilt stable versions of LLVM on `kernel.org
> > +<https://kernel.org/pub/tools/llvm/>`_. These have been optimized with profile
> > +data for building Linux kernels. Below are links that may be useful for
>
> Maybe make a note of why this matters? ", which should lower kernel
> build times compared to non-optimized LLVM toolchains."?

Ack

-- 
Thanks,
~Nick Desaulniers

Powered by blists - more mailing lists