Linux: -Wignored-attributes in fs/exofs/*

Created on 14 Sep 2018 · 13Comments · Source: ClangBuiltLinux/linux

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?

-Wignored-attributes [BUG] linux [FIXED][LINUX] 5.1

Source

nickdesaulniers

All 13 comments

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.

stephenhines on 14 Sep 2018

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).

nickdesaulniers on 14 Sep 2018

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:

https://github.com/nathanchance/linux/commit/01a98ac074fed41e9256b0f98f3a21ada8f22b8c

https://github.com/nathanchance/linux/commit/7ccf8c9b3ba45bff3e5c5da1c26ba21e707b51e4