[PATCH] D134461: [Clang] Warn when trying to deferencing void pointers in C

Mon Sep 26 09:20:42 PDT 2022

aaron.ballman added a comment.

In D134461#3815298 <https://reviews.llvm.org/D134461#3815298>, @nathanchance wrote:

> This warning is quite noisy for the Linux kernel due to a couple of places where a `void *` is dereferenced as part of compile time checking.

Thank you for letting us know!

> Against an x86_64 allmodconfig build (which builds the majority of the code in the kernel), I see:
>
>   $ rg -c -- -Wvoid-ptr-dereference build.log
>   1588001
>
> My original commentary is available on our GitHub issue tracker (https://github.com/ClangBuiltLinux/linux/issues/1720) but I will summarize it here:
>
> The first major instance of this warning comes from the `__is_constexpr` macro, which is used a lot in common macros do fancy things at compile time (change that added this: https://git.kernel.org/linus/3c8ba0d61d04ced9f8d9ff93977995a9e4e96e91):
>
>   /*
>    * This returns a constant expression while determining if an argument is
>    * a constant expression, most importantly without evaluating the argument.
>    * Glory to Martin Uecker <Martin.Uecker at med.uni-goettingen.de>
>    */
>   #define __is_constexpr(x) \
>   	(sizeof(int) == sizeof(*(8 ? ((void *)((long)(x) * 0l)) : (int *)8)))
>
> Sample warnings:
>
>   In file included from scripts/mod/devicetable-offsets.c:3:
>   In file included from ./include/linux/mod_devicetable.h:13:
>   In file included from ./include/linux/uuid.h:12:
>   In file included from ./include/linux/string.h:253:
>   ./include/linux/fortify-string.h:159:10: warning: ISO C does not allow indirection on operand of type 'void *' [-Wvoid-ptr-dereference]
>           q_len = strlen(q);
>                   ^~~~~~~~~
>   ./include/linux/fortify-string.h:131:24: note: expanded from macro 'strlen'
>           __builtin_choose_expr(__is_constexpr(__builtin_strlen(p)),      \
>                                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   ./include/linux/const.h:12:25: note: expanded from macro '__is_constexpr'
>           (sizeof(int) == sizeof(*(8 ? ((void *)((long)(x) * 0l)) : (int *)8)))
>                                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

This one is interesting to me -- I was on the fence about whether we want to diagnose this in an unevaluated context or not. The `clang/test/Analysis/misc-ps.m` test was why I was considering it, and I eventually convinced myself that the dereference there was a harmless UB when we support pointer arithmetic on void as an extension, but UB nonetheless. Now I wonder whether we want different behavior in the `sizeof` case -- it seems odd to me that we warn by default on `sizeof(*vp)` but not `sizeof(void)`: https://godbolt.org/z/PcMrW7b5W so I think we might want to silence `sizeof(*vp)` diagnostics.

WDYT?

> In file included from arch/x86/kernel/asm-offsets.c:9:
> In file included from ./include/linux/crypto.h:20:
> In file included from ./include/linux/slab.h:15:
> In file included from ./include/linux/gfp.h:7:
> In file included from ./include/linux/mmzone.h:8:
> In file included from ./include/linux/spinlock.h:55:
> In file included from ./include/linux/preempt.h:78:
> In file included from ./arch/x86/include/asm/preempt.h:7:
> In file included from ./include/linux/thread_info.h:60:
> In file included from ./arch/x86/include/asm/thread_info.h:53:
> In file included from ./arch/x86/include/asm/cpufeature.h:5:
> In file included from ./arch/x86/include/asm/processor.h:22:
> In file included from ./arch/x86/include/asm/msr.h:11:
> In file included from ./arch/x86/include/asm/cpumask.h:5:
> In file included from ./include/linux/cpumask.h:12:
> In file included from ./include/linux/bitmap.h:9:
> ./include/linux/find.h:40:17: warning: ISO C does not allow indirection on operand of type 'void *' [-Wvoid-ptr-dereference]
>
>   val = *addr & GENMASK(size - 1, offset);
>                 ^~~~~~~~~~~~~~~~~~~~~~~~~
>
> ./include/linux/bits.h:38:3: note: expanded from macro 'GENMASK'
>
>   (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
>    ^~~~~~~~~~~~~~~~~~~~~~~~~
>
> ./include/linux/bits.h:25:3: note: expanded from macro 'GENMASK_INPUT_CHECK'
>
>   __is_constexpr((l) > (h)), (l) > (h), 0)))
>   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> ./include/linux/const.h:12:25: note: expanded from macro '__is_constexpr'
>
>   (sizeof(int) == sizeof(*(8 ? ((void *)((long)(x) * 0l)) : (int *)8)))
>                          ^
>
> ./include/linux/build_bug.h:16:62: note: expanded from macro 'BUILD_BUG_ON_ZERO'
> #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
>
>   ^
>
>   The second major instance is the type checking in `container_of` when ptr is a `void *` (change that added this: https://git.kernel.org/linus/c7acec713d14c6ce8a20154f9dfda258d6bcad3b):
>
> /**
>
> - container_of - cast a member of a structure out to the containing structure
> - @ptr:	the pointer to the member.
> - @type:	the type of the container struct this is embedded in.
> - @member:	the name of the member within the struct. * */
>
> #define container_of(ptr, type, member) ({				\
> 	void *__mptr = (void *)(ptr);					\
> 	static_assert(__same_type(*(ptr), ((type *)0)->member) ||	\
>
>   		      __same_type(*(ptr), void),			\
>   		      "pointer type mismatch in container_of()");	\
>
> ((type *)(__mptr - offsetof(type, member))); })
>
>   
>
> #define __same_type(a, b) __builtin_types_compatible_p(typeof(a), typeof(b))
>
>   

This is another interesting case involving an unevaluated operand. `typeof` doesn't evaluate its argument and so it seems somewhat defensible to allow `typeof(*vp)` for generic programming cases; it should just yield `void` as a type.

> Sample warning:
>
>   In file included from sound/soc/sof/core.c:14:
>   In file included from ./include/sound/sof.h:14:
>   ./include/linux/pci.h:600:9: warning: ISO C does not allow indirection on operand of type 'void *' [-Wvoid-ptr-dereference]
>           return container_of(priv, struct pci_host_bridge, private);
>                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   ./include/linux/container_of.h:20:21: note: expanded from macro 'container_of'
>                         __same_type(*(ptr), void),                        \
>                         ~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   ./include/linux/compiler_types.h:295:63: note: expanded from macro '__same_type'
>   #define __same_type(a, b) __builtin_types_compatible_p(typeof(a), typeof(b))
>                                                                 ^
>   ./include/linux/build_bug.h:77:50: note: expanded from macro 'static_assert'
>   #define static_assert(expr, ...) __static_assert(expr, ##__VA_ARGS__, #expr)
>                                    ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~
>   ./include/linux/build_bug.h:78:56: note: expanded from macro '__static_assert'
>   #define __static_assert(expr, msg, ...) _Static_assert(expr, msg)
>                                                          ^~~~
>
> I am aware these are quite specialized cases so warning is likely appropriate. If so, we can just disable this warning for the kernel but I figured it was worth a comment in case the diagnostic can be improved in any way.

All this said, I think in both cases we want `-pedantic` to warn on these situations even if we would silence by default. e.g.,

  void func(void *vp) {
    sizeof(void); // No warning by default, does get pedantic warning
    sizeof(*vp); // No warning by default, does get pedantic warning
    sizeof(*(vp))`; // No warning by default, does get pedantic warning
    void inner(typeof(*vp)); // No warning by default, does get pedantic warning

    (void)*vp; // Warning by default
  }

What do folks think of that idea?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134461/new/

https://reviews.llvm.org/D134461