diff mbox

[1/5] Add __designated_init, wrapping __attribute__((designated_init))

Message ID 3130b0553b15518e3bef6d14c80280beed0f5ff9.1406850006.git.josh@joshtriplett.org (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Josh Triplett July 31, 2014, 11:47 p.m. UTC
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(+)

Comments

Andrew Morton Aug. 1, 2014, 8:45 p.m. UTC | #1
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-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Josh Triplett Aug. 1, 2014, 9:36 p.m. UTC | #2
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-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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__ */
 
 /*