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
