| Message ID | 3130b0553b15518e3bef6d14c80280beed0f5ff9.1406850006.git.josh@joshtriplett.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 31 Jul 2014 16:47:23 -0700 Josh Triplett <josh@joshtriplett.org> wrote: > GCC 4.10 and newer, and Sparse, supports > __attribute__((designated_init)), which marks a structure as requiring > a designated initializer rather than a positional one. This helps > reduce churn and errors when used with _ops structures and similar > structures designed for future extension. > > Add a wrapper __designated_init, which turns into > __attribute__((designated_init)) for Sparse or sufficiently new GCC. > Enable the corresponding warning as an error. > > The following semantic patch can help mark structures containing > function pointers as requiring designated initializers: > > @@ > identifier I, f; > type T; > @@ > > struct I { > ... > T (*f)(...); > ... > } > + __designated_init hm, dunno about this. I think that the kernel should always use designated initializers everywhere. Perhaps there are a few special cases where positional initializers provide a superior result but I'm not sure where those might be. In which case what we should do is to teach sparse to warn about positional initializers then go fix them all up (lol). After that process is complete, this __designated_init tag would be just noise. To support this perhaps a sparse tag would be needed which says "positional initializers are OK here". This way we're adding the annotation to the exceptional cases, not to the common cases. -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Aug 01, 2014 at 01:45:29PM -0700, Andrew Morton wrote: > On Thu, 31 Jul 2014 16:47:23 -0700 Josh Triplett <josh@joshtriplett.org> wrote: > > > GCC 4.10 and newer, and Sparse, supports > > __attribute__((designated_init)), which marks a structure as requiring > > a designated initializer rather than a positional one. This helps > > reduce churn and errors when used with _ops structures and similar > > structures designed for future extension. > > > > Add a wrapper __designated_init, which turns into > > __attribute__((designated_init)) for Sparse or sufficiently new GCC. > > Enable the corresponding warning as an error. > > > > The following semantic patch can help mark structures containing > > function pointers as requiring designated initializers: > > > > @@ > > identifier I, f; > > type T; > > @@ > > > > struct I { > > ... > > T (*f)(...); > > ... > > } > > + __designated_init > > hm, dunno about this. > > I think that the kernel should always use designated initializers > everywhere. Perhaps there are a few special cases where positional > initializers provide a superior result but I'm not sure where those > might be. > > In which case what we should do is to teach sparse to warn about > positional initializers then go fix them all up (lol). After that > process is complete, this __designated_init tag would be just noise. > > To support this perhaps a sparse tag would be needed which says > "positional initializers are OK here". This way we're adding the > annotation to the exceptional cases, not to the common cases. Before submitting this series, I actually used coccinelle to automatically add __designated_init to nearly a thousand structures (just in include/linux/ alone), and did a build with that. Even just adding it to structures with function pointers, some cases produce a *huge* number of warnings. I think it might make sense to work our way through those warnings, but "go fix them all up" would be a gargantuan effort. (For an idea of scale: there were more warnings about positional initialization than everything other warning combined, by a substantial factor, and that's just from changing a few hundred structures.) I'm not at all convinced that we want to universally enforce designated initializers *yet*. They make sense for _ops structures and other structures that contain function pointers (which is a small subset), but for many other structures they just add a large amount of noise to initialization. If we could say "automatically add __designated_init to structures matching these criteria", that could work, but "all structures" not so much. (For instance, a structure with a single field, or two fields of incompatible types with an intuitive ordering, doesn't gain much value from designated initialization.) Now, some cases may make sense to fix despite the large volume. For instance, many users of dmi_system_id initialize it positionally, and shouldn't; I've already written patches for quite a few of them, with many more to go. But, for instance, obs_kernel_param seems much less worthwhile to fix, because we shouldn't add any new instances of it, and we likely won't ever extend it. The value in fixing this issue mostly comes not with the current code, but with future changes to the structure. (And, as always happens with this kind of change, we'll end up with a few holdouts who don't want to change their code, which will stop us from eliminating the warning completely.) In the course of introducing this change, we'll end up fixing a large number of positional init warnings. If in the course of doing so, it starts making sense to enforce it everywhere, it seems easy enough to sed away all the __designated_init tags. But in terms of noise, the actual additions of __designated_init will pale in comparison to the patches fixing positional initializers. Finally, by migrating over to this incrementally, structure by structure, we can safely make the warning an error, which we could not do if we applied it to every struct immediately. - Josh Triplett -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Makefile b/Makefile index f6a7794..83773c2 100644 --- a/Makefile +++ b/Makefile @@ -744,6 +744,9 @@ KBUILD_CFLAGS += $(call cc-option,-Werror=strict-prototypes) # Prohibit date/time macros, which would make the build non-deterministic KBUILD_CFLAGS += $(call cc-option,-Werror=date-time) +# Disallow positional initialization of designated structs +KBUILD_CFLAGS += $(call cc-option,-Werror=designated-init) + # use the deterministic mode of AR if available KBUILD_ARFLAGS := $(call ar-option,D) diff --git a/include/linux/compiler-gcc4.h b/include/linux/compiler-gcc4.h index 2507fd2..5cd3c26 100644 --- a/include/linux/compiler-gcc4.h +++ b/include/linux/compiler-gcc4.h @@ -85,4 +85,8 @@ #if GCC_VERSION >= 40800 || (defined(__powerpc__) && GCC_VERSION >= 40600) #define __HAVE_BUILTIN_BSWAP16__ #endif + +#if GCC_VERSION >= 41000 || defined(__CHECKER__) +#define __designated_init __attribute__((designated_init)) +#endif #endif /* CONFIG_ARCH_USE_BUILTIN_BSWAP */ diff --git a/include/linux/compiler.h b/include/linux/compiler.h index d5ad7b1..c2334b2 100644 --- a/include/linux/compiler.h +++ b/include/linux/compiler.h @@ -266,6 +266,11 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect); #define __always_inline inline #endif +/* Marks a struct as requiring designated initializers, never positional. */ +#ifndef __designated_init +#define __designated_init +#endif + #endif /* __KERNEL__ */ /*
GCC 4.10 and newer, and Sparse, supports __attribute__((designated_init)), which marks a structure as requiring a designated initializer rather than a positional one. This helps reduce churn and errors when used with _ops structures and similar structures designed for future extension. Add a wrapper __designated_init, which turns into __attribute__((designated_init)) for Sparse or sufficiently new GCC. Enable the corresponding warning as an error. The following semantic patch can help mark structures containing function pointers as requiring designated initializers: @@ identifier I, f; type T; @@ struct I { ... T (*f)(...); ... } + __designated_init ; Signed-off-by: Josh Triplett <josh@joshtriplett.org> --- Makefile | 3 +++ include/linux/compiler-gcc4.h | 4 ++++ include/linux/compiler.h | 5 +++++ 3 files changed, 12 insertions(+)