| Message ID | 20200507122127.1345392-1-yamada.masahiro@socionext.com |
|---|---|
| State | Deferred |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | kbuild: add -Werror=implicit-function-declaration | expand |
Hi Masahiro, On Thu, 7 May 2020 at 06:21, Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > > Add -Werror=implicit-function-declaration as Linux does. > > If you do not check the prototype, it may go wrong run-time. > It is better to break the build, and require to include correct > headers. > > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> > --- > > Makefile | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) NAK We already get a warning in this situation. This makes it painful for development since things that should be warnings end up being errors. So your build fails when really it should work well enough to do a test run with your new code. I don't think it has any benefit on code quality since we already detect warnings in gitlab, etc. U-Boot is set up so that warnings are reported and are easy to spot if you use 'make -s' (i.e. not buried in the output). Regards, Simon
On Fri, May 8, 2020 at 10:39 AM Simon Glass <sjg@chromium.org> wrote: > > Hi Masahiro, > > On Thu, 7 May 2020 at 06:21, Masahiro Yamada > <yamada.masahiro@socionext.com> wrote: > > > > Add -Werror=implicit-function-declaration as Linux does. > > > > If you do not check the prototype, it may go wrong run-time. > > It is better to break the build, and require to include correct > > headers. > > > > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> > > --- > > > > Makefile | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > NAK > > We already get a warning in this situation. This makes it painful for > development since things that should be warnings end up being errors. > So your build fails when really it should work well enough to do a > test run with your new code. I don't think it has any benefit on code > quality since we already detect warnings in gitlab, etc. > > U-Boot is set up so that warnings are reported and are easy to spot if > you use 'make -s' (i.e. not buried in the output). > > Regards, > Simon Linux added this flag in 2007. The intention seems to break the build earlier when a non-existing function is used. I have not seen compliant about this flag in Linux. What does it make different for U-Boot ? commit 94bed2a9c4ae980838003f5d32681eef794ecc28 Author: Dave Jones <davej@redhat.com> Date: Sun Jul 15 23:41:38 2007 -0700 Add -Werror-implicit-function-declaration Add -Werror-implicit-function-declaration This makes builds fail sooner if something is implicitly defined instead of having to wait half an hour for it to fail at the linking stage. Signed-off-by: Dave Jones <davej@redhat.com> Cc: Sam Ravnborg <sam@ravnborg.org> Signed-off-by: Andrew Morton <akpm@linux-foundation.org> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Hi Masahiro, On Thu, 7 May 2020 at 19:54, Masahiro Yamada <masahiroy@kernel.org> wrote: > > On Fri, May 8, 2020 at 10:39 AM Simon Glass <sjg@chromium.org> wrote: > > > > Hi Masahiro, > > > > On Thu, 7 May 2020 at 06:21, Masahiro Yamada > > <yamada.masahiro@socionext.com> wrote: > > > > > > Add -Werror=implicit-function-declaration as Linux does. > > > > > > If you do not check the prototype, it may go wrong run-time. > > > It is better to break the build, and require to include correct > > > headers. > > > > > > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> > > > --- > > > > > > Makefile | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > NAK > > > > We already get a warning in this situation. This makes it painful for > > development since things that should be warnings end up being errors. > > So your build fails when really it should work well enough to do a > > test run with your new code. I don't think it has any benefit on code > > quality since we already detect warnings in gitlab, etc. > > > > U-Boot is set up so that warnings are reported and are easy to spot if > > you use 'make -s' (i.e. not buried in the output). > > > > Regards, > > Simon > > > > Linux added this flag in 2007. > > The intention seems to break the build earlier > when a non-existing function is used. > > I have not seen compliant about this flag in Linux. > What does it make different for U-Boot ? Well that commit message is quite misleading. The author is presumably ignoring the warnings that come out in the compile phase. For me they come up loud and clear. I don't know why it takes half an hour to get to the link stage. My average incremental build time is under 4 seconds including the link. Finally, the warning does not tell you anything about whether the function doesn't exist. It just tells you you have left out a header file. I know how much of a pain this is, because coreboot does this. It does it partly because there is so much build output that the warnings are invisible unless they actually halt the build. Even then you have to search for what went wrong. Regards, SImon > > > > > commit 94bed2a9c4ae980838003f5d32681eef794ecc28 > Author: Dave Jones <davej@redhat.com> > Date: Sun Jul 15 23:41:38 2007 -0700 > > Add -Werror-implicit-function-declaration > > Add -Werror-implicit-function-declaration > This makes builds fail sooner if something is implicitly defined instead > of having to wait half an hour for it to fail at the linking stage. > > Signed-off-by: Dave Jones <davej@redhat.com> > Cc: Sam Ravnborg <sam@ravnborg.org> > Signed-off-by: Andrew Morton <akpm@linux-foundation.org> > Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> > > > > -- > Best Regards > Masahiro Yamada
On Thu, May 07, 2020 at 09:16:40PM -0600, Simon Glass wrote: > Hi Masahiro, > > On Thu, 7 May 2020 at 19:54, Masahiro Yamada <masahiroy@kernel.org> wrote: > > > > On Fri, May 8, 2020 at 10:39 AM Simon Glass <sjg@chromium.org> wrote: > > > > > > Hi Masahiro, > > > > > > On Thu, 7 May 2020 at 06:21, Masahiro Yamada > > > <yamada.masahiro@socionext.com> wrote: > > > > > > > > Add -Werror=implicit-function-declaration as Linux does. > > > > > > > > If you do not check the prototype, it may go wrong run-time. > > > > It is better to break the build, and require to include correct > > > > headers. > > > > > > > > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> > > > > --- > > > > > > > > Makefile | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > NAK > > > > > > We already get a warning in this situation. This makes it painful for > > > development since things that should be warnings end up being errors. > > > So your build fails when really it should work well enough to do a > > > test run with your new code. I don't think it has any benefit on code > > > quality since we already detect warnings in gitlab, etc. > > > > > > U-Boot is set up so that warnings are reported and are easy to spot if > > > you use 'make -s' (i.e. not buried in the output). > > > > > > Regards, > > > Simon > > > > > > > > Linux added this flag in 2007. > > > > The intention seems to break the build earlier > > when a non-existing function is used. > > > > I have not seen compliant about this flag in Linux. > > What does it make different for U-Boot ? > > Well that commit message is quite misleading. The author is presumably > ignoring the warnings that come out in the compile phase. For me they > come up loud and clear. I don't know why it takes half an hour to get > to the link stage. My average incremental build time is under 4 > seconds including the link. > > Finally, the warning does not tell you anything about whether the > function doesn't exist. It just tells you you have left out a header > file. > > I know how much of a pain this is, because coreboot does this. It does > it partly because there is so much build output that the warnings are > invisible unless they actually halt the build. Even then you have to > search for what went wrong. I'm not immediately sure of the right answer here. Part of the problem is that even with 'make -s' U-Boot can be horribly noisy due to device tree warnings. I assume Yamada-san ran in to a problem and was expecting the build to have failed but instead was chasing down a run-time debug until finding this. It's really easy to build with -Werror set via buildman, but a lot of people don't expect to have to use buildman (as it's not required, just a good idea), see for example the thread about building non-functional sunxi binaries. If they used buildman the non-zero exit code would have saved them the debug time. All that said, I can imagine that doing something like the include cleanup series that you do would be even harder with this on. But on the 3rd or 4th hand, adding -k to make gets those builds going along anyhow. Personally, when I see those warnings I fix them up before tossing at the hardware, but I know that's not everyones workflow.
Hi Tom, On Fri, 8 May 2020 at 12:16, Tom Rini <trini@konsulko.com> wrote: > > On Thu, May 07, 2020 at 09:16:40PM -0600, Simon Glass wrote: > > Hi Masahiro, > > > > On Thu, 7 May 2020 at 19:54, Masahiro Yamada <masahiroy@kernel.org> wrote: > > > > > > On Fri, May 8, 2020 at 10:39 AM Simon Glass <sjg@chromium.org> wrote: > > > > > > > > Hi Masahiro, > > > > > > > > On Thu, 7 May 2020 at 06:21, Masahiro Yamada > > > > <yamada.masahiro@socionext.com> wrote: > > > > > > > > > > Add -Werror=implicit-function-declaration as Linux does. > > > > > > > > > > If you do not check the prototype, it may go wrong run-time. > > > > > It is better to break the build, and require to include correct > > > > > headers. > > > > > > > > > > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> > > > > > --- > > > > > > > > > > Makefile | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > NAK > > > > > > > > We already get a warning in this situation. This makes it painful for > > > > development since things that should be warnings end up being errors. > > > > So your build fails when really it should work well enough to do a > > > > test run with your new code. I don't think it has any benefit on code > > > > quality since we already detect warnings in gitlab, etc. > > > > > > > > U-Boot is set up so that warnings are reported and are easy to spot if > > > > you use 'make -s' (i.e. not buried in the output). > > > > > > > > Regards, > > > > Simon > > > > > > > > > > > > Linux added this flag in 2007. > > > > > > The intention seems to break the build earlier > > > when a non-existing function is used. > > > > > > I have not seen compliant about this flag in Linux. > > > What does it make different for U-Boot ? > > > > Well that commit message is quite misleading. The author is presumably > > ignoring the warnings that come out in the compile phase. For me they > > come up loud and clear. I don't know why it takes half an hour to get > > to the link stage. My average incremental build time is under 4 > > seconds including the link. > > > > Finally, the warning does not tell you anything about whether the > > function doesn't exist. It just tells you you have left out a header > > file. > > > > I know how much of a pain this is, because coreboot does this. It does > > it partly because there is so much build output that the warnings are > > invisible unless they actually halt the build. Even then you have to > > search for what went wrong. > > I'm not immediately sure of the right answer here. Part of the problem > is that even with 'make -s' U-Boot can be horribly noisy due to device > tree warnings. I assume Yamada-san ran in to a problem and was Yes, I even added -y to buildman as I was so sick of that. > expecting the build to have failed but instead was chasing down a > run-time debug until finding this. It's really easy to build with > -Werror set via buildman, but a lot of people don't expect to have to > use buildman (as it's not required, just a good idea), see for example > the thread about building non-functional sunxi binaries. If they used > buildman the non-zero exit code would have saved them the debug time. > > All that said, I can imagine that doing something like the include > cleanup series that you do would be even harder with this on. But on Yes, in general. Errors make the pace of progress really slow. Warnings are not as bad. You know you need to get them them but at least you can see the whole picture of what you are doing. > the 3rd or 4th hand, adding -k to make gets those builds going along > anyhow. Yes there are lots of workarounds, but warnings are warnings for a reason. The code can still work, and you need to fix it sometime. > > Personally, when I see those warnings I fix them up before tossing at > the hardware, but I know that's not everyones workflow. Generally for me, building = writing to the hardware, so long as the build doesn't fail. Normally my build step is 'make' && 'write to board' so I just need to press reset if that is not in the script. Regards, Simon
On Sat, May 9, 2020 at 3:16 AM Tom Rini <trini@konsulko.com> wrote: > > On Thu, May 07, 2020 at 09:16:40PM -0600, Simon Glass wrote: > > Hi Masahiro, > > > > On Thu, 7 May 2020 at 19:54, Masahiro Yamada <masahiroy@kernel.org> wrote: > > > > > > On Fri, May 8, 2020 at 10:39 AM Simon Glass <sjg@chromium.org> wrote: > > > > > > > > Hi Masahiro, > > > > > > > > On Thu, 7 May 2020 at 06:21, Masahiro Yamada > > > > <yamada.masahiro@socionext.com> wrote: > > > > > > > > > > Add -Werror=implicit-function-declaration as Linux does. > > > > > > > > > > If you do not check the prototype, it may go wrong run-time. > > > > > It is better to break the build, and require to include correct > > > > > headers. > > > > > > > > > > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> > > > > > --- > > > > > > > > > > Makefile | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > NAK > > > > > > > > We already get a warning in this situation. This makes it painful for > > > > development since things that should be warnings end up being errors. > > > > So your build fails when really it should work well enough to do a > > > > test run with your new code. I don't think it has any benefit on code > > > > quality since we already detect warnings in gitlab, etc. > > > > > > > > U-Boot is set up so that warnings are reported and are easy to spot if > > > > you use 'make -s' (i.e. not buried in the output). > > > > > > > > Regards, > > > > Simon > > > > > > > > > > > > Linux added this flag in 2007. > > > > > > The intention seems to break the build earlier > > > when a non-existing function is used. > > > > > > I have not seen compliant about this flag in Linux. > > > What does it make different for U-Boot ? > > > > Well that commit message is quite misleading. The author is presumably > > ignoring the warnings that come out in the compile phase. For me they > > come up loud and clear. I don't know why it takes half an hour to get > > to the link stage. My average incremental build time is under 4 > > seconds including the link. > > > > Finally, the warning does not tell you anything about whether the > > function doesn't exist. It just tells you you have left out a header > > file. > > > > I know how much of a pain this is, because coreboot does this. It does > > it partly because there is so much build output that the warnings are > > invisible unless they actually halt the build. Even then you have to > > search for what went wrong. > > I'm not immediately sure of the right answer here. Part of the problem > is that even with 'make -s' U-Boot can be horribly noisy due to device > tree warnings. I assume Yamada-san ran in to a problem and was > expecting the build to have failed but instead was chasing down a > run-time debug until finding this. I did not run into a problem. When I was replacing <common.h> with some lighter headers, I missed some warnings ( but I noticed them after all). In Linux, if I miss to include a header, it fails to build. In U-Boot, it does not. Personally, I like to align with Linux policy, but if you are not comfortable with this patch, please feel free to ignore it. > It's really easy to build with > -Werror set via buildman, but a lot of people don't expect to have to > use buildman (as it's not required, just a good idea), see for example > the thread about building non-functional sunxi binaries. If they used > buildman the non-zero exit code would have saved them the debug time. > > All that said, I can imagine that doing something like the include > cleanup series that you do would be even harder with this on. But on > the 3rd or 4th hand, adding -k to make gets those builds going along > anyhow. > > Personally, when I see those warnings I fix them up before tossing at > the hardware, but I know that's not everyones workflow. > > -- > Tom
Hi Masahiro, On Sat, 9 May 2020 at 05:00, Masahiro Yamada <masahiroy@kernel.org> wrote: > > On Sat, May 9, 2020 at 3:16 AM Tom Rini <trini@konsulko.com> wrote: > > > > On Thu, May 07, 2020 at 09:16:40PM -0600, Simon Glass wrote: > > > Hi Masahiro, > > > > > > On Thu, 7 May 2020 at 19:54, Masahiro Yamada <masahiroy@kernel.org> wrote: > > > > > > > > On Fri, May 8, 2020 at 10:39 AM Simon Glass <sjg@chromium.org> wrote: > > > > > > > > > > Hi Masahiro, > > > > > > > > > > On Thu, 7 May 2020 at 06:21, Masahiro Yamada > > > > > <yamada.masahiro@socionext.com> wrote: > > > > > > > > > > > > Add -Werror=implicit-function-declaration as Linux does. > > > > > > > > > > > > If you do not check the prototype, it may go wrong run-time. > > > > > > It is better to break the build, and require to include correct > > > > > > headers. > > > > > > > > > > > > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> > > > > > > --- > > > > > > > > > > > > Makefile | 2 +- > > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > NAK > > > > > > > > > > We already get a warning in this situation. This makes it painful for > > > > > development since things that should be warnings end up being errors. > > > > > So your build fails when really it should work well enough to do a > > > > > test run with your new code. I don't think it has any benefit on code > > > > > quality since we already detect warnings in gitlab, etc. > > > > > > > > > > U-Boot is set up so that warnings are reported and are easy to spot if > > > > > you use 'make -s' (i.e. not buried in the output). > > > > > > > > > > Regards, > > > > > Simon > > > > > > > > > > > > > > > > Linux added this flag in 2007. > > > > > > > > The intention seems to break the build earlier > > > > when a non-existing function is used. > > > > > > > > I have not seen compliant about this flag in Linux. > > > > What does it make different for U-Boot ? > > > > > > Well that commit message is quite misleading. The author is presumably > > > ignoring the warnings that come out in the compile phase. For me they > > > come up loud and clear. I don't know why it takes half an hour to get > > > to the link stage. My average incremental build time is under 4 > > > seconds including the link. > > > > > > Finally, the warning does not tell you anything about whether the > > > function doesn't exist. It just tells you you have left out a header > > > file. > > > > > > I know how much of a pain this is, because coreboot does this. It does > > > it partly because there is so much build output that the warnings are > > > invisible unless they actually halt the build. Even then you have to > > > search for what went wrong. > > > > I'm not immediately sure of the right answer here. Part of the problem > > is that even with 'make -s' U-Boot can be horribly noisy due to device > > tree warnings. I assume Yamada-san ran in to a problem and was > > expecting the build to have failed but instead was chasing down a > > run-time debug until finding this. > > > I did not run into a problem. > > When I was replacing <common.h> with some lighter headers, > I missed some warnings ( but I noticed them after all). > > In Linux, if I miss to include a header, it fails to build. > In U-Boot, it does not. > > Personally, I like to align with Linux policy, > but if you are not comfortable with this patch, > please feel free to ignore it. I really don't understand the point of warnings if we are just going to turn them into errors. How about adding an option to tell U-Boot to use -Werror, etc.? Then those that what it can enable it. Regards, Simon
Hi Simon, On Mon, May 11, 2020 at 5:37 AM Simon Glass <sjg@chromium.org> wrote: > > Hi Masahiro, > > On Sat, 9 May 2020 at 05:00, Masahiro Yamada <masahiroy@kernel.org> wrote: > > > > On Sat, May 9, 2020 at 3:16 AM Tom Rini <trini@konsulko.com> wrote: > > > > > > On Thu, May 07, 2020 at 09:16:40PM -0600, Simon Glass wrote: > > > > Hi Masahiro, > > > > > > > > On Thu, 7 May 2020 at 19:54, Masahiro Yamada <masahiroy@kernel.org> wrote: > > > > > > > > > > On Fri, May 8, 2020 at 10:39 AM Simon Glass <sjg@chromium.org> wrote: > > > > > > > > > > > > Hi Masahiro, > > > > > > > > > > > > On Thu, 7 May 2020 at 06:21, Masahiro Yamada > > > > > > <yamada.masahiro@socionext.com> wrote: > > > > > > > > > > > > > > Add -Werror=implicit-function-declaration as Linux does. > > > > > > > > > > > > > > If you do not check the prototype, it may go wrong run-time. > > > > > > > It is better to break the build, and require to include correct > > > > > > > headers. > > > > > > > > > > > > > > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> > > > > > > > --- > > > > > > > > > > > > > > Makefile | 2 +- > > > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > > > NAK > > > > > > > > > > > > We already get a warning in this situation. This makes it painful for > > > > > > development since things that should be warnings end up being errors. > > > > > > So your build fails when really it should work well enough to do a > > > > > > test run with your new code. I don't think it has any benefit on code > > > > > > quality since we already detect warnings in gitlab, etc. > > > > > > > > > > > > U-Boot is set up so that warnings are reported and are easy to spot if > > > > > > you use 'make -s' (i.e. not buried in the output). > > > > > > > > > > > > Regards, > > > > > > Simon > > > > > > > > > > > > > > > > > > > > Linux added this flag in 2007. > > > > > > > > > > The intention seems to break the build earlier > > > > > when a non-existing function is used. > > > > > > > > > > I have not seen compliant about this flag in Linux. > > > > > What does it make different for U-Boot ? > > > > > > > > Well that commit message is quite misleading. The author is presumably > > > > ignoring the warnings that come out in the compile phase. For me they > > > > come up loud and clear. I don't know why it takes half an hour to get > > > > to the link stage. My average incremental build time is under 4 > > > > seconds including the link. > > > > > > > > Finally, the warning does not tell you anything about whether the > > > > function doesn't exist. It just tells you you have left out a header > > > > file. > > > > > > > > I know how much of a pain this is, because coreboot does this. It does > > > > it partly because there is so much build output that the warnings are > > > > invisible unless they actually halt the build. Even then you have to > > > > search for what went wrong. > > > > > > I'm not immediately sure of the right answer here. Part of the problem > > > is that even with 'make -s' U-Boot can be horribly noisy due to device > > > tree warnings. I assume Yamada-san ran in to a problem and was > > > expecting the build to have failed but instead was chasing down a > > > run-time debug until finding this. > > > > > > I did not run into a problem. > > > > When I was replacing <common.h> with some lighter headers, > > I missed some warnings ( but I noticed them after all). > > > > In Linux, if I miss to include a header, it fails to build. > > In U-Boot, it does not. > > > > Personally, I like to align with Linux policy, > > but if you are not comfortable with this patch, > > please feel free to ignore it. > > I really don't understand the point of warnings if we are just going > to turn them into errors. > > How about adding an option to tell U-Boot to use -Werror, etc.? Then > those that what it can enable it. OK. We can do it with make KCFLAGS=-Werror
On 5/11/20 3:59 AM, Masahiro Yamada wrote: > Hi Simon, > > On Mon, May 11, 2020 at 5:37 AM Simon Glass <sjg@chromium.org> wrote: >> >> Hi Masahiro, >> >> On Sat, 9 May 2020 at 05:00, Masahiro Yamada <masahiroy@kernel.org> wrote: >>> >>> On Sat, May 9, 2020 at 3:16 AM Tom Rini <trini@konsulko.com> wrote: >>>> >>>> On Thu, May 07, 2020 at 09:16:40PM -0600, Simon Glass wrote: >>>>> Hi Masahiro, >>>>> >>>>> On Thu, 7 May 2020 at 19:54, Masahiro Yamada <masahiroy@kernel.org> wrote: >>>>>> >>>>>> On Fri, May 8, 2020 at 10:39 AM Simon Glass <sjg@chromium.org> wrote: >>>>>>> >>>>>>> Hi Masahiro, >>>>>>> >>>>>>> On Thu, 7 May 2020 at 06:21, Masahiro Yamada >>>>>>> <yamada.masahiro@socionext.com> wrote: >>>>>>>> >>>>>>>> Add -Werror=implicit-function-declaration as Linux does. >>>>>>>> >>>>>>>> If you do not check the prototype, it may go wrong run-time. >>>>>>>> It is better to break the build, and require to include correct >>>>>>>> headers. >>>>>>>> >>>>>>>> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> >>>>>>>> --- >>>>>>>> >>>>>>>> Makefile | 2 +- >>>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>>> >>>>>>> NAK >>>>>>> >>>>>>> We already get a warning in this situation. This makes it painful for >>>>>>> development since things that should be warnings end up being errors. >>>>>>> So your build fails when really it should work well enough to do a >>>>>>> test run with your new code. I don't think it has any benefit on code >>>>>>> quality since we already detect warnings in gitlab, etc. >>>>>>> >>>>>>> U-Boot is set up so that warnings are reported and are easy to spot if >>>>>>> you use 'make -s' (i.e. not buried in the output). >>>>>>> >>>>>>> Regards, >>>>>>> Simon >>>>>> >>>>>> >>>>>> >>>>>> Linux added this flag in 2007. >>>>>> >>>>>> The intention seems to break the build earlier >>>>>> when a non-existing function is used. >>>>>> >>>>>> I have not seen compliant about this flag in Linux. >>>>>> What does it make different for U-Boot ? >>>>> >>>>> Well that commit message is quite misleading. The author is presumably >>>>> ignoring the warnings that come out in the compile phase. For me they >>>>> come up loud and clear. I don't know why it takes half an hour to get >>>>> to the link stage. My average incremental build time is under 4 >>>>> seconds including the link. >>>>> >>>>> Finally, the warning does not tell you anything about whether the >>>>> function doesn't exist. It just tells you you have left out a header >>>>> file. >>>>> >>>>> I know how much of a pain this is, because coreboot does this. It does >>>>> it partly because there is so much build output that the warnings are >>>>> invisible unless they actually halt the build. Even then you have to >>>>> search for what went wrong. >>>> >>>> I'm not immediately sure of the right answer here. Part of the problem >>>> is that even with 'make -s' U-Boot can be horribly noisy due to device >>>> tree warnings. I assume Yamada-san ran in to a problem and was >>>> expecting the build to have failed but instead was chasing down a >>>> run-time debug until finding this. >>> >>> >>> I did not run into a problem. >>> >>> When I was replacing <common.h> with some lighter headers, >>> I missed some warnings ( but I noticed them after all). >>> >>> In Linux, if I miss to include a header, it fails to build. >>> In U-Boot, it does not. >>> >>> Personally, I like to align with Linux policy, >>> but if you are not comfortable with this patch, >>> please feel free to ignore it. >> >> I really don't understand the point of warnings if we are just going >> to turn them into errors. >> >> How about adding an option to tell U-Boot to use -Werror, etc.? Then >> those that what it can enable it. > > > OK. We can do it with > > > make KCFLAGS=-Werror I've had a few of these failures due to implicit fn declaration, so I'd be all for adding the error by default. And if things error out and you are too lazy to spot the error, use make -k ; make -k and the error will be right there at the end. So I'm fine with this patch as-is.
Hi, On Sun, 10 May 2020 at 20:14, Marek Vasut <marex@denx.de> wrote: > > On 5/11/20 3:59 AM, Masahiro Yamada wrote: > > Hi Simon, > > > > On Mon, May 11, 2020 at 5:37 AM Simon Glass <sjg@chromium.org> wrote: > >> > >> Hi Masahiro, > >> > >> On Sat, 9 May 2020 at 05:00, Masahiro Yamada <masahiroy@kernel.org> wrote: > >>> > >>> On Sat, May 9, 2020 at 3:16 AM Tom Rini <trini@konsulko.com> wrote: > >>>> > >>>> On Thu, May 07, 2020 at 09:16:40PM -0600, Simon Glass wrote: > >>>>> Hi Masahiro, > >>>>> > >>>>> On Thu, 7 May 2020 at 19:54, Masahiro Yamada <masahiroy@kernel.org> wrote: > >>>>>> > >>>>>> On Fri, May 8, 2020 at 10:39 AM Simon Glass <sjg@chromium.org> wrote: > >>>>>>> > >>>>>>> Hi Masahiro, > >>>>>>> > >>>>>>> On Thu, 7 May 2020 at 06:21, Masahiro Yamada > >>>>>>> <yamada.masahiro@socionext.com> wrote: > >>>>>>>> > >>>>>>>> Add -Werror=implicit-function-declaration as Linux does. > >>>>>>>> > >>>>>>>> If you do not check the prototype, it may go wrong run-time. > >>>>>>>> It is better to break the build, and require to include correct > >>>>>>>> headers. > >>>>>>>> > >>>>>>>> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> > >>>>>>>> --- > >>>>>>>> > >>>>>>>> Makefile | 2 +- > >>>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>>>>>> > >>>>>>> NAK > >>>>>>> > >>>>>>> We already get a warning in this situation. This makes it painful for > >>>>>>> development since things that should be warnings end up being errors. > >>>>>>> So your build fails when really it should work well enough to do a > >>>>>>> test run with your new code. I don't think it has any benefit on code > >>>>>>> quality since we already detect warnings in gitlab, etc. > >>>>>>> > >>>>>>> U-Boot is set up so that warnings are reported and are easy to spot if > >>>>>>> you use 'make -s' (i.e. not buried in the output). > >>>>>>> > >>>>>>> Regards, > >>>>>>> Simon > >>>>>> > >>>>>> > >>>>>> > >>>>>> Linux added this flag in 2007. > >>>>>> > >>>>>> The intention seems to break the build earlier > >>>>>> when a non-existing function is used. > >>>>>> > >>>>>> I have not seen compliant about this flag in Linux. > >>>>>> What does it make different for U-Boot ? > >>>>> > >>>>> Well that commit message is quite misleading. The author is presumably > >>>>> ignoring the warnings that come out in the compile phase. For me they > >>>>> come up loud and clear. I don't know why it takes half an hour to get > >>>>> to the link stage. My average incremental build time is under 4 > >>>>> seconds including the link. > >>>>> > >>>>> Finally, the warning does not tell you anything about whether the > >>>>> function doesn't exist. It just tells you you have left out a header > >>>>> file. > >>>>> > >>>>> I know how much of a pain this is, because coreboot does this. It does > >>>>> it partly because there is so much build output that the warnings are > >>>>> invisible unless they actually halt the build. Even then you have to > >>>>> search for what went wrong. > >>>> > >>>> I'm not immediately sure of the right answer here. Part of the problem > >>>> is that even with 'make -s' U-Boot can be horribly noisy due to device > >>>> tree warnings. I assume Yamada-san ran in to a problem and was > >>>> expecting the build to have failed but instead was chasing down a > >>>> run-time debug until finding this. > >>> > >>> > >>> I did not run into a problem. > >>> > >>> When I was replacing <common.h> with some lighter headers, > >>> I missed some warnings ( but I noticed them after all). > >>> > >>> In Linux, if I miss to include a header, it fails to build. > >>> In U-Boot, it does not. > >>> > >>> Personally, I like to align with Linux policy, > >>> but if you are not comfortable with this patch, > >>> please feel free to ignore it. > >> > >> I really don't understand the point of warnings if we are just going > >> to turn them into errors. > >> > >> How about adding an option to tell U-Boot to use -Werror, etc.? Then > >> those that what it can enable it. > > > > > > OK. We can do it with > > > > > > make KCFLAGS=-Werror > > I've had a few of these failures due to implicit fn declaration, so I'd > be all for adding the error by default. And if things error out and you > are too lazy to spot the error, use make -k ; make -k and the error will > be right there at the end. So are you ignoring the warning? Is it because there are so many device-tree warnings? Should we figure out how to silence those things? > > So I'm fine with this patch as-is. Perhaps you can use Masahiro's option above? Regards, Simon
On Mon, May 11, 2020 at 02:28:48PM -0600, Simon Glass wrote: > Hi, > > On Sun, 10 May 2020 at 20:14, Marek Vasut <marex@denx.de> wrote: > > > > On 5/11/20 3:59 AM, Masahiro Yamada wrote: > > > Hi Simon, > > > > > > On Mon, May 11, 2020 at 5:37 AM Simon Glass <sjg@chromium.org> wrote: > > >> > > >> Hi Masahiro, > > >> > > >> On Sat, 9 May 2020 at 05:00, Masahiro Yamada <masahiroy@kernel.org> wrote: > > >>> > > >>> On Sat, May 9, 2020 at 3:16 AM Tom Rini <trini@konsulko.com> wrote: > > >>>> > > >>>> On Thu, May 07, 2020 at 09:16:40PM -0600, Simon Glass wrote: > > >>>>> Hi Masahiro, > > >>>>> > > >>>>> On Thu, 7 May 2020 at 19:54, Masahiro Yamada <masahiroy@kernel.org> wrote: > > >>>>>> > > >>>>>> On Fri, May 8, 2020 at 10:39 AM Simon Glass <sjg@chromium.org> wrote: > > >>>>>>> > > >>>>>>> Hi Masahiro, > > >>>>>>> > > >>>>>>> On Thu, 7 May 2020 at 06:21, Masahiro Yamada > > >>>>>>> <yamada.masahiro@socionext.com> wrote: > > >>>>>>>> > > >>>>>>>> Add -Werror=implicit-function-declaration as Linux does. > > >>>>>>>> > > >>>>>>>> If you do not check the prototype, it may go wrong run-time. > > >>>>>>>> It is better to break the build, and require to include correct > > >>>>>>>> headers. > > >>>>>>>> > > >>>>>>>> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> > > >>>>>>>> --- > > >>>>>>>> > > >>>>>>>> Makefile | 2 +- > > >>>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) > > >>>>>>> > > >>>>>>> NAK > > >>>>>>> > > >>>>>>> We already get a warning in this situation. This makes it painful for > > >>>>>>> development since things that should be warnings end up being errors. > > >>>>>>> So your build fails when really it should work well enough to do a > > >>>>>>> test run with your new code. I don't think it has any benefit on code > > >>>>>>> quality since we already detect warnings in gitlab, etc. > > >>>>>>> > > >>>>>>> U-Boot is set up so that warnings are reported and are easy to spot if > > >>>>>>> you use 'make -s' (i.e. not buried in the output). > > >>>>>>> > > >>>>>>> Regards, > > >>>>>>> Simon > > >>>>>> > > >>>>>> > > >>>>>> > > >>>>>> Linux added this flag in 2007. > > >>>>>> > > >>>>>> The intention seems to break the build earlier > > >>>>>> when a non-existing function is used. > > >>>>>> > > >>>>>> I have not seen compliant about this flag in Linux. > > >>>>>> What does it make different for U-Boot ? > > >>>>> > > >>>>> Well that commit message is quite misleading. The author is presumably > > >>>>> ignoring the warnings that come out in the compile phase. For me they > > >>>>> come up loud and clear. I don't know why it takes half an hour to get > > >>>>> to the link stage. My average incremental build time is under 4 > > >>>>> seconds including the link. > > >>>>> > > >>>>> Finally, the warning does not tell you anything about whether the > > >>>>> function doesn't exist. It just tells you you have left out a header > > >>>>> file. > > >>>>> > > >>>>> I know how much of a pain this is, because coreboot does this. It does > > >>>>> it partly because there is so much build output that the warnings are > > >>>>> invisible unless they actually halt the build. Even then you have to > > >>>>> search for what went wrong. > > >>>> > > >>>> I'm not immediately sure of the right answer here. Part of the problem > > >>>> is that even with 'make -s' U-Boot can be horribly noisy due to device > > >>>> tree warnings. I assume Yamada-san ran in to a problem and was > > >>>> expecting the build to have failed but instead was chasing down a > > >>>> run-time debug until finding this. > > >>> > > >>> > > >>> I did not run into a problem. > > >>> > > >>> When I was replacing <common.h> with some lighter headers, > > >>> I missed some warnings ( but I noticed them after all). > > >>> > > >>> In Linux, if I miss to include a header, it fails to build. > > >>> In U-Boot, it does not. > > >>> > > >>> Personally, I like to align with Linux policy, > > >>> but if you are not comfortable with this patch, > > >>> please feel free to ignore it. > > >> > > >> I really don't understand the point of warnings if we are just going > > >> to turn them into errors. > > >> > > >> How about adding an option to tell U-Boot to use -Werror, etc.? Then > > >> those that what it can enable it. > > > > > > > > > OK. We can do it with > > > > > > > > > make KCFLAGS=-Werror > > > > I've had a few of these failures due to implicit fn declaration, so I'd > > be all for adding the error by default. And if things error out and you > > are too lazy to spot the error, use make -k ; make -k and the error will > > be right there at the end. > > So are you ignoring the warning? Is it because there are so many > device-tree warnings? Should we figure out how to silence those > things? I keep wondering how many device tree warnings would get fixed with a re-sync of Linux.
diff --git a/Makefile b/Makefile index cc99873062..f8b136a07f 100644 --- a/Makefile +++ b/Makefile @@ -419,7 +419,7 @@ CHECKFLAGS := -D__linux__ -Dlinux -D__STDC__ -Dunix -D__unix__ \ KBUILD_CPPFLAGS := -D__KERNEL__ -D__UBOOT__ KBUILD_CFLAGS := -Wall -Wstrict-prototypes \ - -Wno-format-security \ + -Werror=implicit-function-declaration -Wno-format-security \ -fno-builtin -ffreestanding $(CSTD_FLAG) KBUILD_CFLAGS += -fshort-wchar -fno-strict-aliasing KBUILD_AFLAGS := -D__ASSEMBLY__
Add -Werror=implicit-function-declaration as Linux does. If you do not check the prototype, it may go wrong run-time. It is better to break the build, and require to include correct headers. Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)