From: Nick Desaulniers <ndesaulniers@google.com> To: Nathan Chancellor <nathan@kernel.org> Cc: Masahiro Yamada <masahiroy@kernel.org>, Tom Rix <trix@redhat.com>, Nicolas Schier <nicolas@fjasle.eu>, Jonathan Corbet <corbet@lwn.net>, Paul Walmsley <paul.walmsley@sifive.com>, Palmer Dabbelt <palmer@dabbelt.com>, Albert Ou <aou@eecs.berkeley.edu>, llvm@lists.linux.dev, linux-kbuild@vger.kernel.org, linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, linux-riscv@lists.infradead.org Subject: Re: [PATCH] Documentation/llvm: refresh docs Date: Thu, 24 Aug 2023 13:32:42 -0700 [thread overview] Message-ID: <CAKwvOdkdb=dDggNNPHb08AiZNp5V-H9utgm0H+2hJmZJdO-biA@mail.gmail.com> (raw) In-Reply-To: <20230824184910.GA2015748@dev-arch.thelio-3990X> On Thu, Aug 24, 2023 at 11:49 AM Nathan Chancellor <nathan@kernel.org> wrote: > > On Thu, Aug 24, 2023 at 11:03:17AM -0700, ndesaulniers@google.com wrote: > > Link: https://github.com/ClangBuiltLinux/linux/issues/1907 [0] > > --- > > > > > > Signed-off-by: Nick Desaulniers <ndesaulniers@google.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@kernel.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
WARNING: multiple messages have this Message-ID (diff)
From: Nick Desaulniers <ndesaulniers@google.com> To: Nathan Chancellor <nathan@kernel.org> Cc: Masahiro Yamada <masahiroy@kernel.org>, Tom Rix <trix@redhat.com>, Nicolas Schier <nicolas@fjasle.eu>, Jonathan Corbet <corbet@lwn.net>, Paul Walmsley <paul.walmsley@sifive.com>, Palmer Dabbelt <palmer@dabbelt.com>, Albert Ou <aou@eecs.berkeley.edu>, llvm@lists.linux.dev, linux-kbuild@vger.kernel.org, linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, linux-riscv@lists.infradead.org Subject: Re: [PATCH] Documentation/llvm: refresh docs Date: Thu, 24 Aug 2023 13:32:42 -0700 [thread overview] Message-ID: <CAKwvOdkdb=dDggNNPHb08AiZNp5V-H9utgm0H+2hJmZJdO-biA@mail.gmail.com> (raw) In-Reply-To: <20230824184910.GA2015748@dev-arch.thelio-3990X> On Thu, Aug 24, 2023 at 11:49 AM Nathan Chancellor <nathan@kernel.org> wrote: > > On Thu, Aug 24, 2023 at 11:03:17AM -0700, ndesaulniers@google.com wrote: > > Link: https://github.com/ClangBuiltLinux/linux/issues/1907 [0] > > --- > > > > > > Signed-off-by: Nick Desaulniers <ndesaulniers@google.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@kernel.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 _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv
next prev parent reply other threads:[~2023-08-24 20:32 UTC|newest] Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top 2023-08-24 18:03 [PATCH] Documentation/llvm: refresh docs ndesaulniers 2023-08-24 18:03 ` ndesaulniers 2023-08-24 18:49 ` Nathan Chancellor 2023-08-24 18:49 ` Nathan Chancellor 2023-08-24 20:32 ` Nick Desaulniers [this message] 2023-08-24 20:32 ` Nick Desaulniers 2023-08-24 21:12 ` Nick Desaulniers 2023-08-24 21:12 ` Nick Desaulniers 2023-08-24 20:55 ` Nick Desaulniers 2023-08-24 20:55 ` Nick Desaulniers
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CAKwvOdkdb=dDggNNPHb08AiZNp5V-H9utgm0H+2hJmZJdO-biA@mail.gmail.com' \ --to=ndesaulniers@google.com \ --cc=aou@eecs.berkeley.edu \ --cc=corbet@lwn.net \ --cc=linux-doc@vger.kernel.org \ --cc=linux-kbuild@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-riscv@lists.infradead.org \ --cc=llvm@lists.linux.dev \ --cc=masahiroy@kernel.org \ --cc=nathan@kernel.org \ --cc=nicolas@fjasle.eu \ --cc=palmer@dabbelt.com \ --cc=paul.walmsley@sifive.com \ --cc=trix@redhat.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.