[clang] [C] Warn on uninitialized const objects (PR #137166)

[Aaron Ballman via cfe-commits]

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