Re: [PATCH v2] kbuild: Make $(LLVM) more flexible

[Nathan Chancellor]

On Mon, Mar 07, 2022 at 11:08:29AM -0800, Nick Desaulniers wrote:
> On Fri, Mar 4, 2022 at 9:14 AM Nathan Chancellor <nathan@xxxxxxxxxx> wrote:
> >
> > diff --git a/Documentation/kbuild/llvm.rst b/Documentation/kbuild/llvm.rst
> > index d32616891dcf..68b74416ec48 100644
> > --- a/Documentation/kbuild/llvm.rst
> > +++ b/Documentation/kbuild/llvm.rst
> > @@ -49,17 +49,36 @@ example: ::
> >  LLVM Utilities
> >  --------------
> >
> > -LLVM has substitutes for GNU binutils utilities. Kbuild supports ``LLVM=1``
> > -to enable them. ::
> > -
> > -       make LLVM=1
> > -
> > -They can be enabled individually. The full list of the parameters: ::
> > +LLVM has substitutes for GNU binutils utilities. They can be enabled individually.
> > +The full list of supported make variables: ::
> >
> >         make CC=clang LD=ld.lld AR=llvm-ar NM=llvm-nm STRIP=llvm-strip \
> >           OBJCOPY=llvm-objcopy OBJDUMP=llvm-objdump READELF=llvm-readelf \
> >           HOSTCC=clang HOSTCXX=clang++ HOSTAR=llvm-ar HOSTLD=ld.lld
> >
> > +To simplify the above command, Kbuild supports the ``LLVM`` variable: ::
> > +
> > +       make LLVM=1
> > +
> > +If your LLVM tools are not available in your PATH, you can supply their
> > +location using the LLVM variable with a trailing slash: ::
> > +
> > +       make LLVM=/path/to/llvm/
> > +
> > +which will use ``/path/to/llvm/clang``, ``/path/to/llvm/ld.lld``, etc.
> 
> I don't think we should do this; `PATH=/path/to/llvm/ make LLVM=1`
> works and (my interpretation of what) Masahiro said "if anyone asks
> for this, here's how we could do that."  I don't think I've seen an
> explicit ask for that. I'd rather LLVM= have 2 behaviors than 3, but I
> won't hold this patch up over that.  Either way:

Right, there has not been an explicit ask for the prefix support yet,
although I know I personally would use it, but I think that it is worth
doing now instead of later for a few reasons:

1. It makes path goofs easier to spot. If you do

     $ PATH=/path/to/llvm:$PATH make LLVM=1 ...

   with a path to LLVM that does not exist (maybe you are bisecting an
   issue and using a temporary build of LLVM and you forgot the path it
   was in), you fall back to the LLVM tools that are in other places in
   your PATH, which is not what the developer intended. I know that I
   have messed up bisects that way. If you did

     $ make LLVM=/path/to/llvm/

   with a path that does not exist, there will be an error much earlier:

     $ make LLVM=/this/path/does/not/exist/ defconfig
     /bin/sh: line 1: /this/path/does/not/exist/clang: No such file or directory

2. It does not take that much more code or documentation to support. It
   is the same amount of code as the suffix and the documentation is
   roughly the same amount of lines as well.

3. If we wait to implement the path-based use of $(LLVM), we have three
   "sequence" points: the initial support of $(LLVM), the suffix
   support, and the prefix support. As we are constantly working with
   various trees, it would make it harder to know what to use when. If
   we just do it in the same patch, we know 5.18+ can use both of these
   methods.

However, at the end of the day, we are a team and if you feel like we
should only have suffix support, I am more than happy to push a v3 that
does just that and we can revist prefix support in the future. Just let
me know!

> Reviewed-by: Nick Desaulniers <ndesaulniers@xxxxxxxxxx>
> 
> > +
> > +If your LLVM tools have a version suffix and you want to test with that
> > +explicit version rather than the unsuffixed executables like ``LLVM=1``, you
> > +can pass the suffix using the ``LLVM`` variable: ::
> > +
> > +       make LLVM=-14
> > +
> > +which will use ``clang-14``, ``ld.lld-14``, etc.
> > +
> > +``LLVM=0`` is not the same as omitting ``LLVM`` altogether, it will behave like
> > +``LLVM=1``.
> 
> Hmm... I can see someone's build wrappers setting LLVM=1, then them
> being surprised that appending LLVM=0 doesn't disable LLVM=1 as they
> might expect.  But Masahiro says let's fix this later which is fine.

Sure, I guess that is a reasonable case to support. I'll see if I can
come up with something that makes sense after this change lands.

Cheers,
Nathan