| Message ID | 20190314234159.191745-1-dianders@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | kbuild: Make NOSTDINC_FLAGS a simply expanded variable | expand |
On Fri, Mar 15, 2019 at 8:42 AM Douglas Anderson <dianders@chromium.org> wrote: > > During a simple no-op (nothing changed) build I saw 39 invocations of > the C compiler with the argument "-print-file-name=include". We don't > need to call the C compiler 39 times for this--one time will suffice. > > Let's change NOSTDINC_FLAGS to a simply expanded variable to avoid > this since there doesn't appear to be any reason it should be > recursively expanded. > > On my build this shaved ~400 ms off my "no-op" build. > > Note that the recursive expansion seems to date back to the (really > old) commit e8f5bdb02ce0 ("[PATCH] Makefile include path ordering"). > It's a little unclear to me if the point of that patch was to switch > the variable to be recursively expanded (which it did) or to avoid > directly assigning to NOSTDINC_FLAGS (AKA to switch to +=) because > someone else (out of tree?) was setting it. I presume later since if > the only goal was to switch to recursive expansion the patch would > have just removed the ":". > > Signed-off-by: Douglas Anderson <dianders@chromium.org> Applied to linux-kbuild. Thanks. > --- > > Makefile | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/Makefile b/Makefile > index 9ef547fc7ffe..3034ba66ad51 100644 > --- a/Makefile > +++ b/Makefile > @@ -402,7 +402,7 @@ CHECK = sparse > > CHECKFLAGS := -D__linux__ -Dlinux -D__STDC__ -Dunix -D__unix__ \ > -Wbitwise -Wno-return-void -Wno-unknown-attribute $(CF) > -NOSTDINC_FLAGS = > +NOSTDINC_FLAGS := > CFLAGS_MODULE = > AFLAGS_MODULE = > LDFLAGS_MODULE = > -- > 2.21.0.360.g471c308f928-goog >
On Sat, Mar 16, 2019 at 12:27 AM Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > > On Fri, Mar 15, 2019 at 8:42 AM Douglas Anderson <dianders@chromium.org> wrote: > > > > During a simple no-op (nothing changed) build I saw 39 invocations of > > the C compiler with the argument "-print-file-name=include". We don't > > need to call the C compiler 39 times for this--one time will suffice. > > > > Let's change NOSTDINC_FLAGS to a simply expanded variable to avoid > > this since there doesn't appear to be any reason it should be > > recursively expanded. > > > > On my build this shaved ~400 ms off my "no-op" build. > > > > Note that the recursive expansion seems to date back to the (really > > old) commit e8f5bdb02ce0 ("[PATCH] Makefile include path ordering"). > > It's a little unclear to me if the point of that patch was to switch > > the variable to be recursively expanded (which it did) or to avoid > > directly assigning to NOSTDINC_FLAGS (AKA to switch to +=) because > > someone else (out of tree?) was setting it. I presume later since if > > the only goal was to switch to recursive expansion the patch would > > have just removed the ":". > > > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > Applied to linux-kbuild. > Thanks. BTW, I noticed no one else appends NOSTDINC_FLAGS. So, does it make more sense to do as follows? NOSTDINC_FLAGS := -nostdinc -isystem $(shell $(CC) -print-file-name=include) > > > > --- > > > > Makefile | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/Makefile b/Makefile > > index 9ef547fc7ffe..3034ba66ad51 100644 > > --- a/Makefile > > +++ b/Makefile > > @@ -402,7 +402,7 @@ CHECK = sparse > > > > CHECKFLAGS := -D__linux__ -Dlinux -D__STDC__ -Dunix -D__unix__ \ > > -Wbitwise -Wno-return-void -Wno-unknown-attribute $(CF) > > -NOSTDINC_FLAGS = > > +NOSTDINC_FLAGS := > > CFLAGS_MODULE = > > AFLAGS_MODULE = > > LDFLAGS_MODULE = > > -- > > 2.21.0.360.g471c308f928-goog > > > > > -- > Best Regards > Masahiro Yamada
Hi, On Fri, Mar 15, 2019 at 8:37 AM Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > > On Sat, Mar 16, 2019 at 12:27 AM Masahiro Yamada > <yamada.masahiro@socionext.com> wrote: > > > > On Fri, Mar 15, 2019 at 8:42 AM Douglas Anderson <dianders@chromium.org> wrote: > > > > > > During a simple no-op (nothing changed) build I saw 39 invocations of > > > the C compiler with the argument "-print-file-name=include". We don't > > > need to call the C compiler 39 times for this--one time will suffice. > > > > > > Let's change NOSTDINC_FLAGS to a simply expanded variable to avoid > > > this since there doesn't appear to be any reason it should be > > > recursively expanded. > > > > > > On my build this shaved ~400 ms off my "no-op" build. > > > > > > Note that the recursive expansion seems to date back to the (really > > > old) commit e8f5bdb02ce0 ("[PATCH] Makefile include path ordering"). > > > It's a little unclear to me if the point of that patch was to switch > > > the variable to be recursively expanded (which it did) or to avoid > > > directly assigning to NOSTDINC_FLAGS (AKA to switch to +=) because > > > someone else (out of tree?) was setting it. I presume later since if > > > the only goal was to switch to recursive expansion the patch would > > > have just removed the ":". > > > > > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > > > Applied to linux-kbuild. > > Thanks. > > > BTW, I noticed no one else > appends NOSTDINC_FLAGS. > > > So, does it make more sense to do as follows? > > > NOSTDINC_FLAGS := -nostdinc -isystem $(shell $(CC) -print-file-name=include) It might. ...but go back to look at commit e8f5bdb02ce0 ("[PATCH] Makefile include path ordering"). Why did they do a += there since even then nobody was setting it. Try: $ git grep NOSTDINC_FLAGS e8f5bdb02ce0~ Do you understand what was going on in that commit? -Doug
Hi, On Sat, Mar 16, 2019 at 12:41 AM Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Fri, Mar 15, 2019 at 8:37 AM Masahiro Yamada > <yamada.masahiro@socionext.com> wrote: > > > > On Sat, Mar 16, 2019 at 12:27 AM Masahiro Yamada > > <yamada.masahiro@socionext.com> wrote: > > > > > > On Fri, Mar 15, 2019 at 8:42 AM Douglas Anderson <dianders@chromium.org> wrote: > > > > > > > > During a simple no-op (nothing changed) build I saw 39 invocations of > > > > the C compiler with the argument "-print-file-name=include". We don't > > > > need to call the C compiler 39 times for this--one time will suffice. > > > > > > > > Let's change NOSTDINC_FLAGS to a simply expanded variable to avoid > > > > this since there doesn't appear to be any reason it should be > > > > recursively expanded. > > > > > > > > On my build this shaved ~400 ms off my "no-op" build. > > > > > > > > Note that the recursive expansion seems to date back to the (really > > > > old) commit e8f5bdb02ce0 ("[PATCH] Makefile include path ordering"). > > > > It's a little unclear to me if the point of that patch was to switch > > > > the variable to be recursively expanded (which it did) or to avoid > > > > directly assigning to NOSTDINC_FLAGS (AKA to switch to +=) because > > > > someone else (out of tree?) was setting it. I presume later since if > > > > the only goal was to switch to recursive expansion the patch would > > > > have just removed the ":". > > > > > > > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > > > > > Applied to linux-kbuild. > > > Thanks. > > > > > > BTW, I noticed no one else > > appends NOSTDINC_FLAGS. > > > > > > So, does it make more sense to do as follows? > > > > > > NOSTDINC_FLAGS := -nostdinc -isystem $(shell $(CC) -print-file-name=include) > > It might. ...but go back to look at commit e8f5bdb02ce0 ("[PATCH] > Makefile include path ordering"). Why did they do a += there since > even then nobody was setting it. Try: > > $ git grep NOSTDINC_FLAGS e8f5bdb02ce0~ > > > Do you understand what was going on in that commit? No, I do not understand. Maybe, it fixed a problem in downstream code since there was no directory like include/asm-xen. If you do not want to take risk, I am fine with this patch, though. > -Doug
Hi, On Fri, Mar 15, 2019 at 9:09 AM Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > > Hi, > > On Sat, Mar 16, 2019 at 12:41 AM Doug Anderson <dianders@chromium.org> wrote: > > > > Hi, > > > > On Fri, Mar 15, 2019 at 8:37 AM Masahiro Yamada > > <yamada.masahiro@socionext.com> wrote: > > > > > > On Sat, Mar 16, 2019 at 12:27 AM Masahiro Yamada > > > <yamada.masahiro@socionext.com> wrote: > > > > > > > > On Fri, Mar 15, 2019 at 8:42 AM Douglas Anderson <dianders@chromium.org> wrote: > > > > > > > > > > During a simple no-op (nothing changed) build I saw 39 invocations of > > > > > the C compiler with the argument "-print-file-name=include". We don't > > > > > need to call the C compiler 39 times for this--one time will suffice. > > > > > > > > > > Let's change NOSTDINC_FLAGS to a simply expanded variable to avoid > > > > > this since there doesn't appear to be any reason it should be > > > > > recursively expanded. > > > > > > > > > > On my build this shaved ~400 ms off my "no-op" build. > > > > > > > > > > Note that the recursive expansion seems to date back to the (really > > > > > old) commit e8f5bdb02ce0 ("[PATCH] Makefile include path ordering"). > > > > > It's a little unclear to me if the point of that patch was to switch > > > > > the variable to be recursively expanded (which it did) or to avoid > > > > > directly assigning to NOSTDINC_FLAGS (AKA to switch to +=) because > > > > > someone else (out of tree?) was setting it. I presume later since if > > > > > the only goal was to switch to recursive expansion the patch would > > > > > have just removed the ":". > > > > > > > > > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > > > > > > > Applied to linux-kbuild. > > > > Thanks. > > > > > > > > > BTW, I noticed no one else > > > appends NOSTDINC_FLAGS. > > > > > > > > > So, does it make more sense to do as follows? > > > > > > > > > NOSTDINC_FLAGS := -nostdinc -isystem $(shell $(CC) -print-file-name=include) > > > > It might. ...but go back to look at commit e8f5bdb02ce0 ("[PATCH] > > Makefile include path ordering"). Why did they do a += there since > > even then nobody was setting it. Try: > > > > $ git grep NOSTDINC_FLAGS e8f5bdb02ce0~ > > > > > > Do you understand what was going on in that commit? > > > No, I do not understand. > > Maybe, it fixed a problem in downstream code > since there was no directory like include/asm-xen. > > > If you do not want to take risk, > I am fine with this patch, though. OK, let's keep the patch I posted then. Thanks! -Doug
diff --git a/Makefile b/Makefile index 9ef547fc7ffe..3034ba66ad51 100644 --- a/Makefile +++ b/Makefile @@ -402,7 +402,7 @@ CHECK = sparse CHECKFLAGS := -D__linux__ -Dlinux -D__STDC__ -Dunix -D__unix__ \ -Wbitwise -Wno-return-void -Wno-unknown-attribute $(CF) -NOSTDINC_FLAGS = +NOSTDINC_FLAGS := CFLAGS_MODULE = AFLAGS_MODULE = LDFLAGS_MODULE =
During a simple no-op (nothing changed) build I saw 39 invocations of the C compiler with the argument "-print-file-name=include". We don't need to call the C compiler 39 times for this--one time will suffice. Let's change NOSTDINC_FLAGS to a simply expanded variable to avoid this since there doesn't appear to be any reason it should be recursively expanded. On my build this shaved ~400 ms off my "no-op" build. Note that the recursive expansion seems to date back to the (really old) commit e8f5bdb02ce0 ("[PATCH] Makefile include path ordering"). It's a little unclear to me if the point of that patch was to switch the variable to be recursively expanded (which it did) or to avoid directly assigning to NOSTDINC_FLAGS (AKA to switch to +=) because someone else (out of tree?) was setting it. I presume later since if the only goal was to switch to recursive expansion the patch would have just removed the ":". Signed-off-by: Douglas Anderson <dianders@chromium.org> --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)