* Ingo Molnar <mingo@xxxxxxxxxx> 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@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
---
include/linux/dcache.h | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/include/linux/dcache.h b/include/linux/dcache.h
index 41062093ec9b..0482c3d6f1ce 100644
--- a/include/linux/dcache.h
+++ b/include/linux/dcache.h
@@ -129,6 +129,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);
@@ -144,7 +145,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
