AaronBallman wrote: > > Whhhhaaaa? `__unqual_scalar_typeof(*p)` seems to result in a > > `const`-qualified type. I'm surprised something named `unqual` would do > > that. Can you confirm I'm getting that correct? > > It looks like this is > > ```c > /* > * __unqual_scalar_typeof(x) - Declare an unqualified scalar type, leaving > * non-scalar types unchanged. > */ > /* > * Prefer C11 _Generic for better compile-times and simpler code. Note: 'char' > * is not type-compatible with 'signed char', and we define a separate case. > */ > #define __scalar_type_to_expr_cases(type) \ > unsigned type: (unsigned type)0, \ > signed type: (signed type)0 > > #define __unqual_scalar_typeof(x) typeof( \ > _Generic((x), \ > char: (char)0, \ > __scalar_type_to_expr_cases(char), \ > __scalar_type_to_expr_cases(short), \ > __scalar_type_to_expr_cases(int), \ > __scalar_type_to_expr_cases(long), \ > __scalar_type_to_expr_cases(long long), \ > default: (x))) > ``` > > from > [include/linux/compiler_types.h](https://elixir.bootlin.com/linux/v6.15-rc4/source/include/linux/compiler_types.h#L512). > I am not that familiar with `_Generic`, does this expression result in a > `const` type?
Er, yes and no are both correct answers. If passed an integer scalar, then it results in an unqualified type. If passed a non-integer scalar, it results in whatever you get from `typeof(x)`: https://godbolt.org/z/d8WMvxsdf To always strip the qualifier, you'd change the macro slightly: ``` #define __unqual_scalar_typeof(x) typeof( \ _Generic((x), \ char: (char)0, \ __scalar_type_to_expr_cases(char), \ __scalar_type_to_expr_cases(short), \ __scalar_type_to_expr_cases(int), \ __scalar_type_to_expr_cases(long), \ __scalar_type_to_expr_cases(long long), \ default: (+x))) ``` https://godbolt.org/z/vTsofhvoo That forces the rvalue conversion on `x` which strips the qualifiers so that `typeof` gets the unqualified type. Or in C23+ you could use `typeof_unqual` instead. > > Does adding `= {};` to the declarations for the dummy variable work for you? > > Yes, that appears to work to silence the warning but I have not looked to see > if this affects code generation yet. I would assume that it would not because > the results of the comparison is always dead but I could see things such as > sanitizers messing up those optimizations. > > > That looks like a correct diagnostic to me, but I admit the code is dense > > enough I may be missing something. Is there a reason why `addr` should not > > be initialized? Or is this relying on initialization through other means > > like `memcpy` into the field because the containing object is non-const? > > Yes, this code is definitely dense... My basic understanding is that > initialization and modification of `addr` is supposed to happen through > `__addr` within the internal crypto API. I see a similar situation with > `vm_flags` within `struct vm_area_struct`. > > ``` > fs/hugetlbfs/inode.c:733:24: error: default initialization of an object of > type 'struct vm_area_struct' with const member leaves the object > uninitialized and is incompatible with C++ > [-Werror,-Wdefault-const-init-unsafe] > 733 | struct vm_area_struct pseudo_vma; > | ^ > include/linux/mm_types.h:803:20: note: member 'vm_flags' declared 'const' here > 803 | const vm_flags_t vm_flags; > | ^ > 1 error generated. > ``` > > ```c > struct vm_area_struct { > ... > /* > * Flags, see mm.h. > * To modify use vm_flags_{init|reset|set|clear|mod} functions. > */ > union { > const vm_flags_t vm_flags; > vm_flags_t __private __vm_flags; > }; > ... > }; > > static long hugetlbfs_fallocate(struct file *file, int mode, loff_t offset, > loff_t len) > { > ... > struct vm_area_struct pseudo_vma; > ... > vma_init(&pseudo_vma, mm); > vm_flags_init(&pseudo_vma, VM_HUGETLB | VM_MAYSHARE | VM_SHARED); > ... > } > > static inline void vm_flags_init(struct vm_area_struct *vma, > vm_flags_t flags) > { > ACCESS_PRIVATE(vma, __vm_flags) = flags; > } > > /* sparse defines __CHECKER__; see Documentation/dev-tools/sparse.rst */ > #ifdef __CHECKER__ > ... > # define ACCESS_PRIVATE(p, member) (*((typeof((p)->member) __force *) > &(p)->member)) > #else /* __CHECKER__ */ > ... > # define ACCESS_PRIVATE(p, member) ((p)->member) > ``` > > The other few instances that I looked at either used only one or two fields > in an aggregate (not the `const` one so full initialization may be seen as > unnecessary overhead) or maybe used `memset()`/`memcpy()` to initialize the > field like [the one instance I saw in the floppy > driver](https://elixir.bootlin.com/linux/v6.15-rc4/source/drivers/block/floppy.c#L3688). > > ``` > drivers/block/floppy.c:3691:30: warning: default initialization of an object > of type 'struct compat_floppy_struct' with const member leaves the object > uninitialized and is incompatible with C++ [-Wdefault-const-init-unsafe] > 3691 | struct compat_floppy_struct v; > | ^ > include/linux/fd.h:20:23: note: member 'name' declared 'const' here > 20 | const compat_caddr_t name; > | ^ > ``` This case might be reasonable to handle differently, but I'm on the fence too. There's two cases for structure members, broadly: 1) Don't initialize the `const` field, don't ever read the `const` field. 2) Rely on the fact that you can overwrite a `const` if the top-level object was not declared `const`. In both cases, the code is valid and so the warning is a false positive. In both cases, the code is dangerous and the warning is useful. So I kind of think this is a case where we split the field diagnostic out into its own group. So we'd have `-Wdefault-const-init-field` which covers field initialization cases, and it would be grouped under `-Wdefault-const-init` which covers both fields and variables. WDYT? https://github.com/llvm/llvm-project/pull/137166 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
