headers/deps: dcache: Move the ____cacheline_aligned attribute to the…

… head of the definition

* Ingo Molnar <mingo@kernel.org> wrote:

> > 1. Position of certain attributes
> >
> > In some commits, you move the cacheline_aligned attributes from after
> > the closing brace on structures to before the struct keyword, which
> > causes clang to warn (and error with CONFIG_WERROR):
> >
> > In file included from arch/arm64/kernel/asm-offsets.c:9:
> > In file included from arch/arm64/kernel/../../../kernel/sched/per_task_area_struct.h:33:
> > In file included from ./include/linux/perf_event_api.h:17:
> > In file included from ./include/linux/perf_event_types.h:41:
> > In file included from ./include/linux/ftrace.h:18:
> > In file included from ./arch/arm64/include/asm/ftrace.h:53:
> > In file included from ./include/linux/compat.h:11:
> > ./include/linux/fs_types.h:997:1: error: attribute '__aligned__' is ignored, place it after "struct" to apply attribute to type declaration [-Werror,-Wignored-attributes]
> > ____cacheline_aligned
> > ^
> > ./include/linux/cache.h:41:46: note: expanded from macro '____cacheline_aligned'
> > #define ____cacheline_aligned __attribute__((__aligned__(SMP_CACHE_BYTES)))
>
> Yeah, so this is a *really* stupid warning from Clang.
>
> Putting the attribute after 'struct' risks the hard to track down bugs when
> a <linux/cache.h> inclusion is missing, which scenario I pointed out in
> this commit:
>
>     headers/deps: dcache: Move the ____cacheline_aligned attribute to the head of the definition
>
>     When changing <linux/dcache.h> I removed the <linux/spinlock_api.h> header,
>     which caused a couple of hundred of mysterious, somewhat obscure link time errors:
>
>       ld: net/sctp/tsnmap.o:(.bss+0x0): multiple definition of `____cacheline_aligned_in_smp'; init/do_mounts_rd.o:(.bss+0x0): first defined here
>       ld: net/sctp/tsnmap.o:(.bss+0x40): multiple definition of `____cacheline_aligned'; init/do_mounts_rd.o:(.bss+0x40): first defined here
>       ld: net/sctp/debug.o:(.bss+0x0): multiple definition of `____cacheline_aligned_in_smp'; init/do_mounts_rd.o:(.bss+0x0): first defined here
>       ld: net/sctp/debug.o:(.bss+0x40): multiple definition of `____cacheline_aligned'; init/do_mounts_rd.o:(.bss+0x40): first defined here
>
>     After a bit of head-scratching, what happened is that 'struct dentry_operations'
>     has the ____cacheline_aligned attribute at the tail of the type definition -
>     which turned into a local variable definition when <linux/cache.h> was not
>     included - which <linux/spinlock_api.h> includes into <linux/dcache.h> indirectly.
>
>     There were no compile time errors, only link time errors.
>
>     Move the attribute to the head of the definition, in which case
>     a missing <linux/cache.h> inclusion creates an immediate build failure:
>
>       In file included from ./include/linux/fs.h:9,
>                        from ./include/linux/fsverity.h:14,
>                        from fs/verity/fsverity_private.h:18,
>                        from fs/verity/read_metadata.c:8:
>       ./include/linux/dcache.h:132:22: error: expected ‘;’ before ‘struct’
>         132 | ____cacheline_aligned
>             |                      ^
>             |                      ;
>         133 | struct dentry_operations {
>             | ~~~~~~
>
>     No change in functionality.
>
>     Signed-off-by: Ingo Molnar <mingo@kernel.org>
>
> Can this Clang warning be disabled?

Ok, broke out this issue into its own thread, in form of a patch submission
- so that others don't have to wade through a massive tree to find a single
commit ...

I'll of course drop these (non-essential) cleanups if the upstream policy
is to follow Clang's quirk/convention, but I find the forced attribute
tail-position a sad misfeature, due to the reasons outlined in this patch:
a straightforward build failure in case an attribute is not defined is far
preferable to spurious creation of variables with link-time warnings that
don't actually highlight the exact nature of the bug ...

Thanks,

	Ingo

=====================>
Date: Sun, 20 Jun 2021 09:41:45 +0200
Subject: [PATCH] headers/deps: dcache: Move the ____cacheline_aligned attribute to the head of the definition

When changing <linux/dcache.h> I removed the <linux/spinlock_api.h> header,
which caused a couple of hundred of mysterious, somewhat obscure link time errors:

  ld: net/sctp/tsnmap.o:(.bss+0x0): multiple definition of `____cacheline_aligned_in_smp'; init/do_mounts_rd.o:(.bss+0x0): first defined here
  ld: net/sctp/tsnmap.o:(.bss+0x40): multiple definition of `____cacheline_aligned'; init/do_mounts_rd.o:(.bss+0x40): first defined here
  ld: net/sctp/debug.o:(.bss+0x0): multiple definition of `____cacheline_aligned_in_smp'; init/do_mounts_rd.o:(.bss+0x0): first defined here
  ld: net/sctp/debug.o:(.bss+0x40): multiple definition of `____cacheline_aligned'; init/do_mounts_rd.o:(.bss+0x40): first defined here

After a bit of head-scratching, what happened is that 'struct dentry_operations'
has the ____cacheline_aligned attribute at the tail of the type definition -
which turned into a local variable definition when <linux/cache.h> was not
included - which <linux/spinlock_api.h> includes into <linux/dcache.h> indirectly.

There were no compile time errors, only link time errors.

Move the attribute to the head of the definition, in which case
a missing <linux/cache.h> inclusion creates an immediate build failure:

  In file included from ./include/linux/fs.h:9,
                   from ./include/linux/fsverity.h:14,
                   from fs/verity/fsverity_private.h:18,
                   from fs/verity/read_metadata.c:8:
  ./include/linux/dcache.h:132:22: error: expected ‘;’ before ‘struct’
    132 | ____cacheline_aligned
        |                      ^
        |                      ;
    133 | struct dentry_operations {
        | ~~~~~~

No change in functionality.

Signed-off-by: Ingo Molnar <mingo@kernel.org>

include/linux/dcache.h

@@ -134,6 +134,7 @@ enum dentry_d_lock_class
 	DENTRY_D_LOCK_NESTED
 };
 
+____cacheline_aligned
 struct dentry_operations {
 	int (*d_revalidate)(struct dentry *, unsigned int);
 	int (*d_weak_revalidate)(struct dentry *, unsigned int);
@@ -149,7 +150,7 @@ struct dentry_operations {
 	struct vfsmount *(*d_automount)(struct path *);
 	int (*d_manage)(const struct path *, bool);
 	struct dentry *(*d_real)(struct dentry *, const struct inode *);
-} ____cacheline_aligned;
+};
 
 /*
  * Locking rules for dentry_operations callbacks are to be found in