Re: Overriding -Werror

[Mark D Rustad]

Brian,

On Aug 15, 2014, at 12:33 PM, Brian Norris <computersforpeace@xxxxxxxxx> wrote:

> Hi,
> 
> On Fri, Aug 15, 2014 at 02:30:49AM -0700, Jeff Kirsher wrote:
>> Funny that you bring this up because I have ~60 patches in my queue to
>> resolve several thousand of these warnings.  Half of the patches
>> actually resolve warnings that can be resolved and the other half
>> implement compiler diagnostic control macros to help silence warnings.
>> All these were the work of a co-worker Mark Rustad, below is the patch
>> he put together to implement the compiler diagnostic control macros.
> 
> While fixing warnings is usually a good thing (at least when done right;
> there are plenty of ways to fight with the compiler over silly things,
> but that's another discussion),

I have said at some presentations on the subject that resolving warnings is not something you want an intern to do.

> I think that my issue is still
> orthogonal to the one you're addressing. In my estimation, it is
> impossible to guarantee that the entire kernel (including drivers) will
> build without any warnings, across all levels of warning verbosity.
> Thus, even with a valiant effort to fix or annotate all warnings, we
> still won't get to the point where I can build 'make ARCH=mips W=1', if
> -Werror is active.

Actually, some years ago, I got a MIPS Linux kernel to compile clean with even more warnings than W=12 provides. It can be done, but it certainly is not a state that is required and cannot be maintained across all configurations, architectures and compiler versions. This is the real world.

> Besides, when testing *new* code, it's even more likely to have new
> warnings, and I'd like to see as many as possible, without -Werror
> getting in the way.

I have to say that I rather like -Werror. One thing that not a lot of people are aware of is that you can selectively allow some warnings. -Wno-error=shadow would allow shadow warnings to be reported without being treated as errors.

> So I still think -Werror is fundamentally wrong in some cases, and I
> would like to pursue some approach like in my original post.
> 
> BTW, for a little more context: I realize the output of 'make W=[123]'
> may not be very useful on its own, sometimes, but it's actually pretty
> useful to quickly catch potential issues in new code, by diff'ing the
> warnings in the before/after build logs. In this case, it's not helpful
> at all if the first build "fails" due to dubious warnings. I'm doing
> this in the context of Aiaiai [1]. Right now, I have to keep around a
> few local patches to remove -Werror from arch/{mips,sh}.

The problem is that when a kernel build throws over 125,000 warnings, it just becomes completely useless. That was what kind of set me off. I did wind up pushing this rock further up the hill than I really meant to. Still, I got the build under 1,400 warnings, and I now know how to address most of them in a systematic way.

>> commit 7b9aace02b2405f0714bc08c424b72e6962f1c2e
>> Author: Mark Rustad <mark.d.rustad@xxxxxxxxx>
>> Date:   Fri Aug 15 01:43:44 2014 -0700
>> 
>>    compiler: Add diagnostic control macros
>> 
>>    Add macros to control diagnostic messages where needed. These
>>    are used to silence warning messages that are expected, normal
>>    and do not indicate any sort of problem. Reducing the stream
>>    of messages in this way helps possible problems to stand out.
>> 
>>    The macros provided are:
>>        DIAG_PUSH() - to save diagnostic settings
>>        DIAG_POP() - to restore diagnostic settings
>>        DIAG_IGNORE(option) - to ignore a particular warning
>>        DIAG_GCC_IGNORE(option) - DIAG_IGNORE for gcc only
>>        DIAG_CLANG_IGNORE(option) - DIAG_IGNORE for clang only
>> 
>>    Signed-off-by: Mark Rustad <mark.d.rustad@xxxxxxxxx>
>> 
>> diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h
>> index d1e49d5..039b112 100644
>> --- a/include/linux/compiler-clang.h
>> +++ b/include/linux/compiler-clang.h
>> @@ -10,3 +10,29 @@
>> #undef uninitialized_var
>> #define uninitialized_var(x) x = *(&(x))
>> #endif
>> +
>> +/*
>> + * Provide macros to manipulate diagnostic messages when possible.
>> + * DIAG_PUSH pushes the diagnostic settings
>> + * DIAG_POP pops the diagnostic settings
>> + * DIAG_IGNORE(x) changes the given diagnostic setting to ignore
>> + *
>> + * Example:
>> + *    DIAG_PUSH DIAG_IGNORE(aggregate-return)
>> + *    struct timespec ns_to_timespec(const s64 nsec)
>> + *    {
>> + *        ...
>> + *    }
>> + *    DIAG_POP
>> + *
>> + * Will prevent the warning on compilation of the function. Other
>> + * steps are necessary to do the same thing for the call sites.
>> + */
> 
> While I do not want to disparage your/Mark's work here, my first thought
> about this kind of annotation is that it seems to be a pretty big burden
> to have to annotate all code with these sorts of things.

I wouldn't suggest annotating everything. However note that the annotations can serve as a notice that something has been analyzed and deemed ok. That can be useful as long as that is really true. I wouldn't take new code from a new developer that included such annotations.

> While it could
> help for some core parts of the kernel that can be closely scrutinized
> and defended against false/useless warnings, from my perspective it
> seems like people are bound to get it wrong when it comes to the long
> tail of scattered device drivers.

Yes, it is VERY valuable for addressing warnings thrown by core kernel includes. Just a few static inlines with warnings produced about 50,000 of those 125,000 warnings.

> But I'm not really the right voice for that topic. Feel free to send
> your work and see what the rest of the community thinks.

I look forward to the discussion. I have no illusion about all my patches going in. In fact there are instances where I used the diagnostic control macros to silence a bunch of warnings because it made for a much smaller patch - a patch to resolve them would have had to replace an entire table, so I guess I hope to get a request to do that. The exchanges I have had with the few patches that have been sent thus far have been productive, so I hope for more like that.

I do think that the diagnostic control macros serve a useful purpose if used well. I agree that they can be abused, like anything else, but some rules about their use should help. Maybe checkpatch should warn on patches that add them...

-- 
Mark Rustad, MRustad@xxxxxxxxx

Attachment: signature.asc
Description: Message signed with OpenPGP using GPGMail