-Wignored-attributes in fs/exofs/*

[nickdesaulniers]

few cases of:

  CC      fs/exofs/ore.o
In file included from fs/exofs/ore.c:30:
In file included from fs/exofs/ore_raid.h:16:
In file included from ./include/scsi/osd_ore.h:25:
In file included from ./include/scsi/osd_initiator.h:18:
./include/scsi/osd_types.h:31:21: warning: 'weak' attribute only applies to variables, functions, and classes [-Wignored-attributes]
static const struct __weak osd_obj_id osd_root_object = {0, 0};
                    ^
./include/linux/compiler_types.h:211:33: note: expanded from macro '__weak'
#define __weak                  __attribute__((weak))
                                               ^

In file included from fs/exofs/inode.c:36:
In file included from fs/exofs/exofs.h:41:
fs/exofs/common.h:186:21: warning: 'weak' attribute only applies to variables, functions, and classes [-Wignored-attributes]
static const struct __weak osd_attr g_attr_inode_data = ATTR_DEF(
                    ^
./include/linux/compiler_types.h:211:33: note: expanded from macro '__weak'
#define __weak                  __attribute__((weak))
                                               ^

I think it should be straightforward to remove the __weak, but it would be interesting if clang was actually wrong and gcc supported __attribute__((weak)) on structs. I wonder what gcc's docs says about that attribute and structs?

[stephenhines]

https://godbolt.org/z/txZ1bd shows that it is the positioning of the attribute that Clang is complaining about here. If you move it after the actual structure name, Clang doesn't generate a diagnostic. I'm not sure whether this is something that is wrong with Clang or the code as written.

[nickdesaulniers]

There are many uses of __weak in the kernel on functions, which makes finding the cases for __weak structs like searching for a needle in a haystack. This may be an isolated case that is a simple fix to the kernel sources, but I'm curious if GCC supports the arbitrary positioning (or if it's silently dropping the variable attribute on the floor).

[nathanchance]

This is the best I could come up with:

$ rg "__weak" | rg "struct" | rg ";"
include/scsi/osd_types.h:static const struct __weak osd_obj_id osd_root_object = {0, 0};
drivers/pci/pci-driver.c:struct dev_pm_ops __weak pcibios_pm_ops;
drivers/pci/pci-sysfs.c:int __weak pci_create_resource_files(struct pci_dev *dev) { return 0; }
drivers/pci/pci-sysfs.c:void __weak pci_remove_resource_files(struct pci_dev *dev) { return; }
kernel/bpf/core.c:const struct bpf_func_proto bpf_map_lookup_elem_proto __weak;
kernel/bpf/core.c:const struct bpf_func_proto bpf_map_update_elem_proto __weak;
kernel/bpf/core.c:const struct bpf_func_proto bpf_map_delete_elem_proto __weak;
kernel/bpf/core.c:const struct bpf_func_proto bpf_get_prandom_u32_proto __weak;
kernel/bpf/core.c:const struct bpf_func_proto bpf_get_smp_processor_id_proto __weak;
kernel/bpf/core.c:const struct bpf_func_proto bpf_get_numa_node_id_proto __weak;
kernel/bpf/core.c:const struct bpf_func_proto bpf_ktime_get_ns_proto __weak;
kernel/bpf/core.c:const struct bpf_func_proto bpf_get_current_pid_tgid_proto __weak;
kernel/bpf/core.c:const struct bpf_func_proto bpf_get_current_uid_gid_proto __weak;
kernel/bpf/core.c:const struct bpf_func_proto bpf_get_current_comm_proto __weak;
kernel/bpf/core.c:const struct bpf_func_proto bpf_sock_map_update_proto __weak;
kernel/bpf/core.c:const struct bpf_func_proto bpf_sock_hash_update_proto __weak;
kernel/bpf/core.c:const struct bpf_func_proto bpf_get_current_cgroup_id_proto __weak;
kernel/bpf/core.c:const struct bpf_func_proto bpf_get_local_storage_proto __weak;
tools/perf/util/unwind-libunwind.c:struct unwind_libunwind_ops __weak *local_unwind_libunwind_ops;
tools/perf/util/unwind-libunwind.c:struct unwind_libunwind_ops __weak *x86_32_unwind_libunwind_ops;
tools/perf/util/unwind-libunwind.c:struct unwind_libunwind_ops __weak *arm64_unwind_libunwind_ops;
arch/arc/kernel/smp.c:struct plat_smp_ops  __weak plat_smp_ops;
arch/mips/oprofile/common.c:extern struct op_mips_model op_model_mipsxx_ops __weak;
arch/mips/oprofile/common.c:extern struct op_mips_model op_model_loongson2_ops __weak;
arch/mips/oprofile/common.c:extern struct op_mips_model op_model_loongson3_ops __weak;
$ rg "__weak" | rg "struct" | rg "="
include/scsi/osd_types.h:static const struct __weak osd_obj_id osd_root_object = {0, 0};
fs/exofs/common.h:static const struct __weak osd_attr g_attr_inode_data = ATTR_DEF(
tools/perf/tests/builtin-test.c:struct test __weak arch_tests[] = {
tools/perf/util/perf_regs.c:const struct sample_reg __weak sample_reg_masks[] = {

The only two who deviate from the accepted pattern are include/scsi/osd_types.h and fs/exofs/common.h, which are the ones generating the warnings. Clang supports either struct type __weak as show by Stephen's example or struct type name __weak; (verified in the same manner).

I believe Nick's correct that GCC is silently dropping the attribute as well because this diff

diff --git a/fs/exofs/common.h b/fs/exofs/common.h
index 7d88ef566213..b7e7da4037a1 100644
--- a/fs/exofs/common.h
+++ b/fs/exofs/common.h
@@ -183,7 +183,7 @@ struct exofs_fcb {
 #define EXOFS_INO_ATTR_SIZE    sizeof(struct exofs_fcb)
 
 /* This is the Attribute the fcb is stored in */
-static const struct __weak osd_attr g_attr_inode_data = ATTR_DEF(
+static const struct osd_attr __weak g_attr_inode_data = ATTR_DEF(
        EXOFS_APAGE_FS_DATA,
        EXOFS_ATTR_INODE_DATA,
        EXOFS_INO_ATTR_SIZE);
diff --git a/include/scsi/osd_types.h b/include/scsi/osd_types.h
index 48e8a165e136..67ec374bcf7b 100644
--- a/include/scsi/osd_types.h
+++ b/include/scsi/osd_types.h
@@ -28,7 +28,7 @@ struct osd_obj_id {
        osd_id id;
 };
 
-static const struct __weak osd_obj_id osd_root_object = {0, 0};
+static const struct osd_obj_id __weak osd_root_object = {0, 0};
 
 struct osd_attr {
        u32 attr_page;

massively fails on both GCC:

In file included from ./include/scsi/osd_initiator.h:18,
                 from ./include/scsi/osd_ore.h:25,
                 from fs/exofs/exofs.h:39,
                 from fs/exofs/dir.c:35:
./include/scsi/osd_types.h:31:39: error: weak declaration of ‘osd_root_object’ must be public
 static const struct osd_obj_id __weak osd_root_object = {0, 0};
                                       ^~~~~~~~~~~~~~~
In file included from fs/exofs/exofs.h:41,  
                 from fs/exofs/dir.c:35:
fs/exofs/common.h:186:30: error: weak declaration of ‘g_attr_inode_data’ must be public
 static const struct osd_attr g_attr_inode_data __weak = ATTR_DEF(
                              ^~~~~~~~~~~~~~~~~
make[1]: *** [scripts/Makefile.build:296: fs/exofs/dir.o] Error 1
make: *** [Makefile:1513: _module_fs/exofs] Error 2

and Clang:

In file included from fs/exofs/namei.c:34:
In file included from fs/exofs/exofs.h:41:
fs/exofs/common.h:186:30: error: weak declaration cannot have internal linkage
static const struct osd_attr __weak g_attr_inode_data = ATTR_DEF(
                             ^
./include/linux/compiler_types.h:214:33: note: expanded from macro '__weak'
#define __weak                  __attribute__((weak))
                                               ^
49 warnings and 2 errors generated.

If GCC were accepting it, we should be seeing the same errors without that diff, no? I'll post the following patches removing __weak in the morning if there are no objections:

nathanchance/pi-kernel@01a98ac

nathanchance/pi-kernel@7ccf8c9

[stephenhines]

Nice patches! Thanks for checking through the sources to take care of these.

[nathanchance]

Patches sent:

https://lore.kernel.org/lkml/20180930205126.25840-1-natechancellor@gmail.com/

https://lore.kernel.org/lkml/20180930205448.26205-1-natechancellor@gmail.com/

[nickdesaulniers]

@nathanchance can you please re-ping these threads?

[nathanchance]

Sure, I'll do it now.

[nathanchance]

Patch resent: https://lore.kernel.org/lkml/20190126064201.24625-1-natechancellor@gmail.com/

[nathanchance]

I forgot the scsi warnings were folded into this issue too. That patch has been resent as well: https://lore.kernel.org/lkml/20190126064740.12949-1-natechancellor@gmail.com/

[nickdesaulniers]

heh, good note, too.

[nathanchance]

These warnings will be fixed with the removal of the code, which has hit -next:

https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=19fcae3d4f2dd513d472055fb694d802e9a21b91

https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=80f2121380caa14895638b24b81800158c0844f2

[nickdesaulniers]

Finally!

[nathanchance]

Removal of fs/exofs and drivers/scsi/osd merged into mainline:

https://git.kernel.org/torvalds/c/80f2121380caa14895638b24b81800158c0844f2

https://git.kernel.org/torvalds/c/19fcae3d4f2dd513d472055fb694d802e9a21b91