[PATCH] kbuild: add -Werror=implicit-function-declaration

[Tom Rini]

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 at 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 at chromium.org> wrote:
> > >>
> > >> Hi Masahiro,
> > >>
> > >> On Sat, 9 May 2020 at 05:00, Masahiro Yamada <masahiroy at kernel.org> wrote:
> > >>>
> > >>> On Sat, May 9, 2020 at 3:16 AM Tom Rini <trini at 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 at kernel.org> wrote:
> > >>>>>>
> > >>>>>> On Fri, May 8, 2020 at 10:39 AM Simon Glass <sjg at chromium.org> wrote:
> > >>>>>>>
> > >>>>>>> Hi Masahiro,
> > >>>>>>>
> > >>>>>>> On Thu, 7 May 2020 at 06:21, Masahiro Yamada
> > >>>>>>> <yamada.masahiro at 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 at 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.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200511/606b49f2/attachment.sig>