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

[Marek Vasut]

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 I'm fine with this patch as-is.

-- 
Best regards,
Marek Vasut