Messages in this thread | | | From | "Rustad, Mark D" <> | Subject | Re: [PATCH 2/2] kernel.h: use __COUNTER__ in min and max macros to avoid -Wshadow warnings | Date | Fri, 12 Sep 2014 23:43:51 +0000 |
| |
On Sep 12, 2014, at 3:40 PM, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Thu, 11 Sep 2014 23:39:36 +0200 Michal Nazarewicz <mina86@mina86.com> wrote: > >> Because min and max macros use the same variable names no matter >> how many times they are called (or how deep the nesting of their >> calls), each time min or max calls are nested, the same variables >> are declared. This is especially noisy after min3 and max3 have >> been changed to nest min/max calls. >> >> Using __COUNTER__ solves the problem since each variable will get >> a unique number aadded to it. The code will still work even if >> the compiler does not support __COUNTER__, but then the protection >> from shadow warning won't work. >> >> The same applies to min_t and max_t macros. >> >> ... >> >> --- a/include/linux/kernel.h >> +++ b/include/linux/kernel.h >> @@ -695,15 +695,27 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { } >> #endif /* CONFIG_TRACING */ >> >> /* >> + * Preprocessor magic generating unique identifiers to avoid -Wshadow warnings >> + * used by min, max, min_t and max_t macros. cnt is __COUNTER__, op is the >> + * comparison operator; tx (ty) is type of the first (second) argument, >> + * xx (yy) is name of a temporary variable to hold the first (second) argument, >> + * and x (y) is the first (second) argument. >> + */ >> +#define _min_max_var(cnt, base) _mm_ ## cnt ## base >> +#define _min_max__(op, tx, xx, x, ty, yy, y) ({ \ >> + tx xx = (x); \ >> + ty yy = (y); \ >> + (void) (&xx == &yy); \ >> + xx op yy ? xx : yy; }) >> +#define _min_max_(cnt, op, tx, x, ty, y) \ >> + _min_max__(op, tx, _min_max_var(cnt, a), x, ty, _min_max_var(cnt, b), y) >> +#define _min_max(...) _min_max_(__COUNTER__, __VA_ARGS__) > > The fact that __COUNTER__ is used in compiler-gcc4.h but not in > compiler-gcc3.h makes me suspicious about its availability?
AFAICT not having __COUNTER__ simply means that the shadow warnings will appear in W=2 builds, so it is no worse off than before. That is, __COUNTER__ will simply expand as __COUNTER__. No error will result from this kind of use.
> I do think that [1/2] made the code significantly worse-looking and > this one is getting crazy.
It is a little crazy, but I find that I would feel differently about it if I had come up with the idea. Actually, I am really impressed with the discoveries that arose from my original patch, including recognizing that the min3/max3 generated worse code than nested macro calls. I had never considered that possibility.
> How useful is W=2 anyway? Has anyone found > a bug using it?
Yes. But it is really hard to find anything useful when my normal kernel build throws 125000 warnings - for completely expected things - in W=2 builds. We do routinely build our drivers with W=12, but we have a special hack to ignore the large number of warnings that the kernel include files generate, while still getting warnings from our driver's code and include files. They do sometimes catch problems.
> The number of warnings in default builds is already way > too high :(
Do you mean the number of warnings enabled, or the number of warning messages being generated? I am a fan of enabling lots of warnings, but it is not effective if doing so emits thousands of messages. Back in the 2.4 kernel era, there was a time when I maintained a MIPS-based kernel that built completely cleanly with way more warnings enabled than W=12 uses today. It took work to get to that state, but we were able to maintain that for several kernel versions for our particular environment. These days, the kernel as a whole is in a much better state than we started with for that old 2.4 kernel.
I'll say that there aren't really that many warnings that are the result of nested min/max macros. There are *much* heavier hitters out there. For example "nested externs" account for tens of thousands of warnings in W=2 builds. Such noise makes using W=2 kind of ridiculous on the whole kernel. I'd like it to not be so ridiculous eventually. Some 60 patches got my build down under 1400 W=2 warnings. Then I realized that I had pushed that rock too far up the hill without getting feedback on what I was doing.
I have been working on patches that add macros to allow disabling certain warnings in select pieces of code. In that way the warning is silenced, but the macro remains as a sign to the reader that there is something special going on. Unfortunately, those macros fail miserably in the expansion of any macro called in the parameter of other macros, as the compile time asserts sometimes are. And the compile time asserts make use of a nested extern by design.
It is enough to make me want to suggest dropping the nested externs warning from W=2, but recognize that every nested extern that actually calls a function is a redundant prototype declaration that could get to be inconsistent with the function it calls. They have disentangled the include files some, but added a bit of risk and maintenance burden.
This whole area is full of judgement calls, so people will disagree. Hopefully any debate will be worthwhile. My first reaction to the patch above was that it was kind of wild, but it does well and truly fix the problem and does no harm on compilers that don't have __COUNTER__, so I am ok with it. Yes, I have compiled it.
-- Mark Rustad, Networking Division, Intel Corporation
[unhandled content-type:application/pgp-signature] | |