| Message ID | 20220304170813.1689186-1-nathan@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] kbuild: Make $(LLVM) more flexible | expand |
On Fri, Mar 04, 2022 at 10:08:14AM -0700, Nathan Chancellor wrote: > [...] > > Update and reorder the documentation to reflect these new additions. > At the same time, notate that LLVM=0 is not the same as just omitting it > altogether, which has confused people in the past. Is it worth making LLVM=0 actually act the way it's expected to? > Link: https://lore.kernel.org/r/20200317215515.226917-1-ndesaulniers@google.com/ > Link: https://lore.kernel.org/r/20220224151322.072632223@infradead.org/ > Suggested-by: Masahiro Yamada <masahiroy@kernel.org> > Suggested-by: Peter Zijlstra <peterz@infradead.org> > Signed-off-by: Nathan Chancellor <nathan@kernel.org> Looks good; minor .rst nit below... Reviewed-by: Kees Cook <keescook@chromium.org> > [...] > -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: :: ": ::" and "::" yield the same result. I think the latter is more readable in non-rendered form. *shrug* -Kees
On Fri, Mar 04, 2022 at 10:09:03AM -0800, Kees Cook wrote: > On Fri, Mar 04, 2022 at 10:08:14AM -0700, Nathan Chancellor wrote: > > [...] > > > > Update and reorder the documentation to reflect these new additions. > > At the same time, notate that LLVM=0 is not the same as just omitting it > > altogether, which has confused people in the past. > > Is it worth making LLVM=0 actually act the way it's expected to? I don't really see the point, omitting $(LLVM) altogether is simpler. Why specify LLVM=0 if you want GNU tools, since it is the default? However, I can look into changing that in a new revision or a follow up if others disagree? > > Link: https://lore.kernel.org/r/20200317215515.226917-1-ndesaulniers@google.com/ > > Link: https://lore.kernel.org/r/20220224151322.072632223@infradead.org/ > > Suggested-by: Masahiro Yamada <masahiroy@kernel.org> > > Suggested-by: Peter Zijlstra <peterz@infradead.org> > > Signed-off-by: Nathan Chancellor <nathan@kernel.org> > > Looks good; minor .rst nit below... > > Reviewed-by: Kees Cook <keescook@chromium.org> > > > [...] > > -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: :: > > ": ::" and "::" yield the same result. I think the latter is more > readable in non-rendered form. *shrug* Ack, I'll wait for other feedback before sending v3, unless there is none and Masahiro does not mind fixing it up during application. Thanks for the review! Nathan
On Sat, Mar 5, 2022 at 3:15 AM Nathan Chancellor <nathan@kernel.org> wrote: > > On Fri, Mar 04, 2022 at 10:09:03AM -0800, Kees Cook wrote: > > On Fri, Mar 04, 2022 at 10:08:14AM -0700, Nathan Chancellor wrote: > > > [...] > > > > > > Update and reorder the documentation to reflect these new additions. > > > At the same time, notate that LLVM=0 is not the same as just omitting it > > > altogether, which has confused people in the past. > > > > Is it worth making LLVM=0 actually act the way it's expected to? > > I don't really see the point, omitting $(LLVM) altogether is simpler. > Why specify LLVM=0 if you want GNU tools, since it is the default? > However, I can look into changing that in a new revision or a follow up > if others disagree? Changing the meaning of LLVM=0 is beyond the scope of what we are trying to achieve now. I think documenting it is enough. (If we have a good reason to change it, we can. But, it should be done in a separate patch, at least)
On Fri, Mar 4, 2022 at 9:14 AM Nathan Chancellor <nathan@kernel.org> 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: Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> > + > +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. -- Thanks, ~Nick Desaulniers
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@kernel.org> 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@google.com> > > > + > > +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
On Mon, Mar 07, 2022 at 11:08:29AM -0800, Nick Desaulniers wrote: > > +``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. What happens if you say LLVM= instead of LLVM=0 ? Would that "undo" a prior LLVM=1 and use GCC instead?
On Wed, Mar 9, 2022 at 1:36 AM Matthew Wilcox <willy@infradead.org> wrote: > > On Mon, Mar 07, 2022 at 11:08:29AM -0800, Nick Desaulniers wrote: > > > +``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. > > What happens if you say LLVM= instead of LLVM=0 ? Would that "undo" > a prior LLVM=1 and use GCC instead? I think so.
On Wed, Mar 9, 2022 at 12:47 AM Nathan Chancellor <nathan@kernel.org> wrote: > > 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@kernel.org> 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! I do not have a strong opinion about this. (I just mentioned the LLVM=/path/to/llvm/ form because I guessed somebody would request this sooner or later.) If you want me to pick up this version, I will apply it with fixing up a nit pointed out by Kees (": ::" -> "::") If you want to send v3, that is fine with me as well. Please let me know your thoughts. > > > Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> > > > > > + > > > +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
On Tue, Mar 8, 2022 at 7:47 AM Nathan Chancellor <nathan@kernel.org> wrote: > > 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@kernel.org> wrote: > > > > > > +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, Then let that be reason enough. :)
On Wed, Mar 09, 2022 at 06:33:40PM +0900, Masahiro Yamada wrote: > On Wed, Mar 9, 2022 at 12:47 AM Nathan Chancellor <nathan@kernel.org> wrote: > > > > 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@kernel.org> 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! > > > I do not have a strong opinion about this. > (I just mentioned the LLVM=/path/to/llvm/ form because I guessed > somebody would request this sooner or later.) > > > If you want me to pick up this version, I will apply it with fixing up > a nit pointed out by Kees (": ::" -> "::") > > If you want to send v3, that is fine with me as well. > > Please let me know your thoughts. Given Nick's response, please pick up this revision with Kees' nit. Thank you! Cheers, Nathan > > > Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> > > > > > > > + > > > > +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 > > > > -- > Best Regards > Masahiro Yamada
On Fri, Mar 11, 2022 at 2:36 AM Nathan Chancellor <nathan@kernel.org> wrote: > > On Wed, Mar 09, 2022 at 06:33:40PM +0900, Masahiro Yamada wrote: > > On Wed, Mar 9, 2022 at 12:47 AM Nathan Chancellor <nathan@kernel.org> wrote: > > > > > > 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@kernel.org> 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! > > > > > > I do not have a strong opinion about this. > > (I just mentioned the LLVM=/path/to/llvm/ form because I guessed > > somebody would request this sooner or later.) > > > > > > If you want me to pick up this version, I will apply it with fixing up > > a nit pointed out by Kees (": ::" -> "::") > > > > If you want to send v3, that is fine with me as well. > > > > Please let me know your thoughts. > > Given Nick's response, please pick up this revision with Kees' nit. > Thank you! I fixed up ": ::" to "::", and applied to linux-kbuild. Thanks.
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. + +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``. If you only wish to use certain LLVM utilities, use their respective +make variables. + The integrated assembler is enabled by default. You can pass ``LLVM_IAS=0`` to disable it. diff --git a/Makefile b/Makefile index a82095c69fdd..b3fb32fd3906 100644 --- a/Makefile +++ b/Makefile @@ -424,8 +424,14 @@ 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 ($(filter %/,$(LLVM)),) +LLVM_PREFIX := $(LLVM) +else ifneq ($(filter -%,$(LLVM)),) +LLVM_SUFFIX := $(LLVM) +endif + +HOSTCC = $(LLVM_PREFIX)clang$(LLVM_SUFFIX) +HOSTCXX = $(LLVM_PREFIX)clang++$(LLVM_SUFFIX) else HOSTCC = gcc HOSTCXX = g++ @@ -444,14 +450,14 @@ KBUILD_HOSTLDLIBS := $(HOST_LFS_LIBS) $(HOSTLDLIBS) # Make variables (CC, etc...) CPP = $(CC) -E ifneq ($(LLVM),) -CC = clang -LD = ld.lld -AR = llvm-ar -NM = llvm-nm -OBJCOPY = llvm-objcopy -OBJDUMP = llvm-objdump -READELF = llvm-readelf -STRIP = llvm-strip +CC = $(LLVM_PREFIX)clang$(LLVM_SUFFIX) +LD = $(LLVM_PREFIX)ld.lld$(LLVM_SUFFIX) +AR = $(LLVM_PREFIX)llvm-ar$(LLVM_SUFFIX) +NM = $(LLVM_PREFIX)llvm-nm$(LLVM_SUFFIX) +OBJCOPY = $(LLVM_PREFIX)llvm-objcopy$(LLVM_SUFFIX) +OBJDUMP = $(LLVM_PREFIX)llvm-objdump$(LLVM_SUFFIX) +READELF = $(LLVM_PREFIX)llvm-readelf$(LLVM_SUFFIX) +STRIP = $(LLVM_PREFIX)llvm-strip$(LLVM_SUFFIX) else CC = $(CROSS_COMPILE)gcc LD = $(CROSS_COMPILE)ld diff --git a/tools/scripts/Makefile.include b/tools/scripts/Makefile.include index 79d102304470..b507a6a733f5 100644 --- a/tools/scripts/Makefile.include +++ b/tools/scripts/Makefile.include @@ -52,11 +52,17 @@ define allow-override endef ifneq ($(LLVM),) -$(call allow-override,CC,clang) -$(call allow-override,AR,llvm-ar) -$(call allow-override,LD,ld.lld) -$(call allow-override,CXX,clang++) -$(call allow-override,STRIP,llvm-strip) +ifneq ($(filter %/,$(LLVM)),) +LLVM_PREFIX := $(LLVM) +else ifneq ($(filter -%,$(LLVM)),) +LLVM_SUFFIX := $(LLVM) +endif + +$(call allow-override,CC,$(LLVM_PREFIX)clang$(LLVM_SUFFIX)) +$(call allow-override,AR,$(LLVM_PREFIX)llvm-ar$(LLVM_SUFFIX)) +$(call allow-override,LD,$(LLVM_PREFIX)ld.lld$(LLVM_SUFFIX)) +$(call allow-override,CXX,$(LLVM_PREFIX)clang++$(LLVM_SUFFIX)) +$(call allow-override,STRIP,$(LLVM_PREFIX)llvm-strip$(LLVM_SUFFIX)) else # Allow setting various cross-compile vars or setting CROSS_COMPILE as a prefix. $(call allow-override,CC,$(CROSS_COMPILE)gcc) @@ -69,9 +75,9 @@ endif CC_NO_CLANG := $(shell $(CC) -dM -E -x c /dev/null | grep -Fq "__clang__"; echo $$?) ifneq ($(LLVM),) -HOSTAR ?= llvm-ar -HOSTCC ?= clang -HOSTLD ?= ld.lld +HOSTAR ?= $(LLVM_PREFIX)llvm-ar$(LLVM_SUFFIX) +HOSTCC ?= $(LLVM_PREFIX)clang$(LLVM_SUFFIX) +HOSTLD ?= $(LLVM_PREFIX)ld.lld$(LLVM_SUFFIX) else HOSTAR ?= ar HOSTCC ?= gcc diff --git a/tools/testing/selftests/lib.mk b/tools/testing/selftests/lib.mk index a40add31a2e3..2a2d240cdc1b 100644 --- a/tools/testing/selftests/lib.mk +++ b/tools/testing/selftests/lib.mk @@ -1,7 +1,13 @@ # This mimics the top-level Makefile. We do it explicitly here so that this # Makefile can operate with or without the kbuild infrastructure. ifneq ($(LLVM),) -CC := clang +ifneq ($(filter %/,$(LLVM)),) +LLVM_PREFIX := $(LLVM) +else ifneq ($(filter -%,$(LLVM)),) +LLVM_SUFFIX := $(LLVM) +endif + +CC := $(LLVM_PREFIX)clang$(LLVM_SUFFIX) else CC := $(CROSS_COMPILE)gcc endif
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. Some developers may build LLVM from source but not add the binaries to their PATH, as they may not want to use that toolchain systemwide. Support those developers by allowing them to supply the directory that the LLVM tools are available in, as it is no more complex to support than the version suffix change above. $ make ... LLVM=/path/to/llvm/ Update and reorder the documentation to reflect these new additions. At the same time, notate that LLVM=0 is not the same as just omitting it altogether, which has confused people in the past. Link: https://lore.kernel.org/r/20200317215515.226917-1-ndesaulniers@google.com/ Link: https://lore.kernel.org/r/20220224151322.072632223@infradead.org/ Suggested-by: Masahiro Yamada <masahiroy@kernel.org> Suggested-by: Peter Zijlstra <peterz@infradead.org> Signed-off-by: Nathan Chancellor <nathan@kernel.org> --- v1 -> v2: https://lore.kernel.org/r/20220302234400.782002-1-nathan@kernel.org/ * Drop Kees and Nick's reviewed-by tags, as there is a substantial logic update. * Only support dash-based suffixes (Masahiro). * Support passing a path to LLVM, which will be prefixed onto the tools (Masahiro). * Reorder and update documentation for LLVM (Masahiro). * Reword commit message to reflect new changes. 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 | 31 +++++++++++++++++++++++++------ Makefile | 26 ++++++++++++++++---------- tools/scripts/Makefile.include | 22 ++++++++++++++-------- tools/testing/selftests/lib.mk | 8 +++++++- 4 files changed, 62 insertions(+), 25 deletions(-) base-commit: d42118db5e7bea017cf9d7c122c7ca3b89a51421