linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] trivial: Fix several compilation errors/warnings with GCC12
@ 2022-04-14 15:08 Christophe de Dinechin
  2022-04-14 15:08 ` [PATCH 1/3] sched/headers: Fix compilation error with GCC 12 Christophe de Dinechin
                   ` (3 more replies)
  0 siblings, 4 replies; 18+ messages in thread
From: Christophe de Dinechin @ 2022-04-14 15:08 UTC (permalink / raw)
  To: trivial
  Cc: Ben Segall, Michael S. Tsirkin, Andrew Morton, Steven Rostedt,
	Ingo Molnar, Mel Gorman, Dietmar Eggemann, Vincent Guittot,
	Paolo Bonzini, Daniel Bristot de Oliveira, Jason Wang,
	virtualization, linux-kernel, Zhen Lei, Christophe de Dinechin,
	Juri Lelli, Peter Zijlstra

Compiling with GCC 12 using defconfig generates a number of build errors\r
due to new warnings, notably array-bounds checks. Some of these warnings appear\r
legitimate and relatively easy to fix.\r
\r
Note that this series is not sufficient for a clean build yet. There are\r
in particular a number of warnings reported by the array-bounds check\r
that appear bogus, like:\r
\r
| In function ‘__native_read_cr3’,\r
|     inlined from ‘__read_cr3’\r
|         at ./arch/x86/include/asm/special_insns.h:169:9,\r
|     inlined from ‘read_cr3_pa’\r
|         at ./arch/x86/include/asm/processor.h:252:9,\r
|     inlined from ‘relocate_restore_code’\r
|         at arch/x86/power/hibernate.c:165:17:\r
| ./arch/x86/include/asm/special_insns.h:48:9: error:\r
|    array subscript 0 is outside array bounds of ‘unsigned int[0]’\r
|    [-Werror=array-bounds]\r
|    48 | asm volatile("mov %%cr3,%0\n\t" : "=r" (val) : __FORCE_ORDER);\r
|       | ^~~\r
| cc1: all warnings being treated as errors\r
\r
The error above is for an instruction that does not obviously address any\r
C array, in particular since the asm constraint is "=r" and not "=rm".\r
\r
Consequently, the series here only addresses a few low hanging fruits that\r
appear legitimate and relatively easy to fix.\r
\r
Christophe de Dinechin (3):\r
  sched/headers: Fix compilation error with GCC 12\r
  nodemask.h: Fix compilation error with GCC12\r
  virtio-pci: Use cpumask_available to fix compilation error\r
\r
 drivers/virtio/virtio_pci_common.c |  2 +-\r
 include/linux/nodemask.h           | 13 ++++++-------\r
 kernel/sched/sched.h               | 11 +++++++++--\r
 3 files changed, 16 insertions(+), 10 deletions(-)\r
\r
-- \r
2.35.1\r
\r


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH 1/3] sched/headers: Fix compilation error with GCC 12
  2022-04-14 15:08 [PATCH 0/3] trivial: Fix several compilation errors/warnings with GCC12 Christophe de Dinechin
@ 2022-04-14 15:08 ` Christophe de Dinechin
  2022-04-14 15:21   ` Peter Zijlstra
  2022-04-14 15:08 ` [PATCH 2/3] nodemask.h: Fix compilation error with GCC12 Christophe de Dinechin
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 18+ messages in thread
From: Christophe de Dinechin @ 2022-04-14 15:08 UTC (permalink / raw)
  To: trivial
  Cc: Ben Segall, Michael S. Tsirkin, Andrew Morton, Steven Rostedt,
	Ingo Molnar, Mel Gorman, Dietmar Eggemann, Vincent Guittot,
	Paolo Bonzini, Daniel Bristot de Oliveira, Jason Wang,
	virtualization, linux-kernel, Zhen Lei, Christophe de Dinechin,
	Juri Lelli, Peter Zijlstra

With gcc version 12.0.1 20220401 (Red Hat 12.0.1-0) (GCC), the following
errors are reported in sched.h when building after `make defconfig`:

|   CC      kernel/sched/core.o
| In file included from kernel/sched/core.c:81:
| kernel/sched/core.c: In function ‘set_rq_online.part.0’:
| kernel/sched/sched.h:2197:52: error: array subscript -1 is outside
|  array bounds of ‘struct sched_class[44343134792571037]’
|  [-Werror=array-bounds]
|  2197 | #define sched_class_lowest  (__begin_sched_classes - 1)
|       |                                                    ^
| kernel/sched/sched.h:2200:41: note: in definition of macro
|  ‘for_class_range’
|  2200 |         for (class = (_from); class != (_to); class--)
|       |                                         ^~~
| kernel/sched/sched.h:2203:53: note: in expansion of macro
|  ‘sched_class_lowest’
|  2203 |for_class_range(class, sched_class_highest, sched_class_lowest)
|       |                                            ^~~~~~~~~~~~~~~~~~
| kernel/sched/core.c:9115:17: note: in expansion of macro
|  ‘for_each_class’
|  9115 |                 for_each_class(class) {
|       |                 ^~~~~~~~~~~~~~
| kernel/sched/sched.h:2193:27: note: at offset -208 into object
|   ‘__begin_sched_classes’ of size [0, 9223372036854775807]
|  2193 | extern struct sched_class __begin_sched_classes[];
|       |                           ^~~~~~~~~~~~~~~~~~~~~
| kernel/sched/core.c: In function ‘set_rq_offline.part.0’:
| kernel/sched/sched.h:2197:52: error: array subscript -1 is outside
|   array bounds of ‘struct sched_class[44343134792571037]’
|   [-Werror=array-bounds]
|  2197 | #define sched_class_lowest  (__begin_sched_classes - 1)
|       |                                                    ^
| kernel/sched/sched.h:2200:41: note: in definition of macro
|  ‘for_class_range’
|  2200 |         for (class = (_from); class != (_to); class--)
|       |                                         ^~~
| kernel/sched/sched.h:2203:53: note: in expansion of macro
|  ‘sched_class_lowest’
|  2203 |for_class_range(class, sched_class_highest, sched_class_lowest)
|       |                                            ^~~~~~~~~~~~~~~~~~
| kernel/sched/core.c:9127:17: note: in expansion of macro
|  ‘for_each_class’
|  9127 |                 for_each_class(class) {
|       |                 ^~~~~~~~~~~~~~
| kernel/sched/sched.h:2193:27: note: at offset -208 into object
|  ‘__begin_sched_classes’ of size [0, 9223372036854775807]
|  2193 | extern struct sched_class __begin_sched_classes[];
|       |                           ^~~~~~~~~~~~~~~~~~~~~
| In function ‘__pick_next_task’,
|     inlined from ‘pick_next_task’ at kernel/sched/core.c:6204:9,
|     inlined from ‘__schedule’ at kernel/sched/core.c:6352:9:
| kernel/sched/sched.h:2197:52: error: array subscript -1 is outside
|  array bounds of ‘struct sched_class[44343134792571037]’
|  [-Werror=array-bounds]
|  2197 | #define sched_class_lowest  (__begin_sched_classes - 1)
|       |                                                    ^
| kernel/sched/sched.h:2200:41: note: in definition of macro
|  ‘for_class_range’
|  2200 |         for (class = (_from); class != (_to); class--)
|       |                                         ^~~
| kernel/sched/sched.h:2203:53: note: in expansion of macro
|  ‘sched_class_lowest’
|  2203 |for_class_range(class, sched_class_highest, sched_class_lowest)
|       |                                            ^~~~~~~~~~~~~~~~~~
| kernel/sched/core.c:5711:9: note: in expansion of macro
|  ‘for_each_class’
|  5711 |         for_each_class(class) {
|       |         ^~~~~~~~~~~~~~
| kernel/sched/sched.h: In function ‘__schedule’:
| kernel/sched/sched.h:2193:27: note: at offset -208 into object
|  ‘__begin_sched_classes’ of size [0, 9223372036854775807]
|  2193 | extern struct sched_class __begin_sched_classes[];
|       |                           ^~~~~~~~~~~~~~~~~~~~~
| cc1: all warnings being treated as errors

Rewrite the definitions of sched_class_highest and for_class_range to
avoid this error as follows:

1/ The sched_class_highest is rewritten to be relative to
  __begin_sched_classes, so that GCC sees it as being part of the
  __begin_sched_classes array and not a distinct __end_sched_classes
  array.

2/ The for_class_range macro is modified to replace the comparison with
  an out-of-bound pointer __begin_sched_classes - 1 with an equivalent,
  but in-bounds comparison.

In that specific case, I believe that the GCC analysis is correct and
potentially valuable for other arrays, so it makes sense to keep it
enabled.

Signed-off-by: Christophe de Dinechin <christophe@dinechin.org>
Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
---
 kernel/sched/sched.h | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index 8dccb34eb190..6350fbc7418d 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -2193,11 +2193,18 @@ const struct sched_class name##_sched_class \
 extern struct sched_class __begin_sched_classes[];
 extern struct sched_class __end_sched_classes[];
 
-#define sched_class_highest (__end_sched_classes - 1)
+/*
+ * sched_class_highests is really __end_sched_classes - 1, but written in a way
+ * that makes it clear that it is within __begin_sched_classes[] and not outside
+ * of __end_sched_classes[].
+ */
+#define sched_class_highest (__begin_sched_classes + \
+			     (__end_sched_classes - __begin_sched_classes - 1))
 #define sched_class_lowest  (__begin_sched_classes - 1)
 
+/* The + 1 below places the pointers within the range of their array */
 #define for_class_range(class, _from, _to) \
-	for (class = (_from); class != (_to); class--)
+	for (class = (_from); class + 1 != (_to) + 1; class--)
 
 #define for_each_class(class) \
 	for_class_range(class, sched_class_highest, sched_class_lowest)
-- 
2.35.1


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH 2/3] nodemask.h: Fix compilation error with GCC12
  2022-04-14 15:08 [PATCH 0/3] trivial: Fix several compilation errors/warnings with GCC12 Christophe de Dinechin
  2022-04-14 15:08 ` [PATCH 1/3] sched/headers: Fix compilation error with GCC 12 Christophe de Dinechin
@ 2022-04-14 15:08 ` Christophe de Dinechin
  2022-04-14 15:23   ` Peter Zijlstra
  2022-04-14 15:08 ` [PATCH 3/3] virtio-pci: Use cpumask_available to fix compilation error Christophe de Dinechin
  2022-05-15 21:24 ` [PATCH 0/3] trivial: Fix several compilation errors/warnings with GCC12 Davidlohr Bueso
  3 siblings, 1 reply; 18+ messages in thread
From: Christophe de Dinechin @ 2022-04-14 15:08 UTC (permalink / raw)
  To: trivial
  Cc: Ben Segall, Michael S. Tsirkin, Andrew Morton, Steven Rostedt,
	Ingo Molnar, Mel Gorman, Dietmar Eggemann, Vincent Guittot,
	Paolo Bonzini, Daniel Bristot de Oliveira, Jason Wang,
	virtualization, linux-kernel, Zhen Lei, Christophe de Dinechin,
	Juri Lelli, Peter Zijlstra

With gcc version 12.0.1 20220401 (Red Hat 12.0.1-0), building with
defconfig results in the following compilation error:

|   CC      mm/swapfile.o
| mm/swapfile.c: In function ‘setup_swap_info’:
| mm/swapfile.c:2291:47: error: array subscript -1 is below array bounds
|  of ‘struct plist_node[]’ [-Werror=array-bounds]
|  2291 |                                 p->avail_lists[i].prio = 1;
|       |                                 ~~~~~~~~~~~~~~^~~
| In file included from mm/swapfile.c:16:
| ./include/linux/swap.h:292:27: note: while referencing ‘avail_lists’
|   292 |         struct plist_node avail_lists[]; /*
|       |                           ^~~~~~~~~~~

This is due to the compiler detecting that the mask in
node_states[__state] could theoretically be zero, which would lead to
first_node() returning -1 through find_first_bit.

I believe that the warning/error is legitimate. I first tried
adding a test to check that the node mask is not emtpy, since a
similar test exists in the case where MAX_NUMNODES == 1.

However, adding the if statement causes other warnings to appear in
for_each_cpu_node_but, because it introduces a dangling else
ambiguity. And unfortunately, GCC is not smart enough to detect that the
added test makes the case where (node) == -1 impossible, so it still
complains with the same message.

This is why I settled on replacing that with a harmless, but relatively
useless (node) >= 0 test. Based on the warning for the dangling else, I
also decided to fix the case where MAX_NUMNODES == 1 by moving the
condition inside the for loop. It will still only be tested once. This
ensures that the meaning of an else following for_each_node_mask
or derivatives would not silently have a different meaning depending on
the configuration.

Signed-off-by: Christophe de Dinechin <christophe@dinechin.org>
Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
---
 include/linux/nodemask.h | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/include/linux/nodemask.h b/include/linux/nodemask.h
index 567c3ddba2c4..c6199dbe2591 100644
--- a/include/linux/nodemask.h
+++ b/include/linux/nodemask.h
@@ -375,14 +375,13 @@ static inline void __nodes_fold(nodemask_t *dstp, const nodemask_t *origp,
 }
 
 #if MAX_NUMNODES > 1
-#define for_each_node_mask(node, mask)			\
-	for ((node) = first_node(mask);			\
-		(node) < MAX_NUMNODES;			\
-		(node) = next_node((node), (mask)))
+#define for_each_node_mask(node, mask)				    \
+	for ((node) = first_node(mask);				    \
+	     (node >= 0) && (node) < MAX_NUMNODES;		    \
+	     (node) = next_node((node), (mask)))
 #else /* MAX_NUMNODES == 1 */
-#define for_each_node_mask(node, mask)			\
-	if (!nodes_empty(mask))				\
-		for ((node) = 0; (node) < 1; (node)++)
+#define for_each_node_mask(node, mask)                                  \
+	for ((node) = 0; (node) < 1 && !nodes_empty(mask); (node)++)
 #endif /* MAX_NUMNODES */
 
 /*
-- 
2.35.1


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH 3/3] virtio-pci: Use cpumask_available to fix compilation error
  2022-04-14 15:08 [PATCH 0/3] trivial: Fix several compilation errors/warnings with GCC12 Christophe de Dinechin
  2022-04-14 15:08 ` [PATCH 1/3] sched/headers: Fix compilation error with GCC 12 Christophe de Dinechin
  2022-04-14 15:08 ` [PATCH 2/3] nodemask.h: Fix compilation error with GCC12 Christophe de Dinechin
@ 2022-04-14 15:08 ` Christophe de Dinechin
  2022-04-15  8:48   ` Michael S. Tsirkin
  2022-05-15 21:24 ` [PATCH 0/3] trivial: Fix several compilation errors/warnings with GCC12 Davidlohr Bueso
  3 siblings, 1 reply; 18+ messages in thread
From: Christophe de Dinechin @ 2022-04-14 15:08 UTC (permalink / raw)
  To: trivial
  Cc: Ben Segall, Michael S. Tsirkin, Andrew Morton, Steven Rostedt,
	Ingo Molnar, Mel Gorman, Dietmar Eggemann, Vincent Guittot,
	Paolo Bonzini, Daniel Bristot de Oliveira, Jason Wang,
	virtualization, linux-kernel, Zhen Lei, Christophe de Dinechin,
	Juri Lelli, Peter Zijlstra

With GCC 12 and defconfig, we get the following error:

|   CC      drivers/virtio/virtio_pci_common.o
| drivers/virtio/virtio_pci_common.c: In function ‘vp_del_vqs’:
| drivers/virtio/virtio_pci_common.c:257:29: error: the comparison will
|  always evaluate as ‘true’ for the pointer operand in
|  ‘vp_dev->msix_affinity_masks + (sizetype)((long unsigned int)i * 8)’
|  must not be NULL [-Werror=address]
|   257 |                         if (vp_dev->msix_affinity_masks[i])
|       |                             ^~~~~~

This happens in the case where CONFIG_CPUMASK_OFFSTACK is not defined,
since we typedef cpumask_var_t as an array. The compiler is essentially
complaining that an array pointer cannot be NULL. This is not a very
important warning, but there is a function called cpumask_available that
seems to be defined just for that case, so the fix is easy.

Signed-off-by: Christophe de Dinechin <christophe@dinechin.org>
Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
---
 drivers/virtio/virtio_pci_common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
index d724f676608b..5c44a2f13c93 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -254,7 +254,7 @@ void vp_del_vqs(struct virtio_device *vdev)
 
 	if (vp_dev->msix_affinity_masks) {
 		for (i = 0; i < vp_dev->msix_vectors; i++)
-			if (vp_dev->msix_affinity_masks[i])
+			if (cpumask_available(vp_dev->msix_affinity_masks[i]))
 				free_cpumask_var(vp_dev->msix_affinity_masks[i]);
 	}
 
-- 
2.35.1


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/3] sched/headers: Fix compilation error with GCC 12
  2022-04-14 15:08 ` [PATCH 1/3] sched/headers: Fix compilation error with GCC 12 Christophe de Dinechin
@ 2022-04-14 15:21   ` Peter Zijlstra
  2022-04-14 20:30     ` Andrew Morton
                       ` (2 more replies)
  0 siblings, 3 replies; 18+ messages in thread
From: Peter Zijlstra @ 2022-04-14 15:21 UTC (permalink / raw)
  To: Christophe de Dinechin
  Cc: trivial, Ben Segall, Michael S. Tsirkin, Andrew Morton,
	Steven Rostedt, Ingo Molnar, Mel Gorman, Dietmar Eggemann,
	Vincent Guittot, Paolo Bonzini, Daniel Bristot de Oliveira,
	Jason Wang, virtualization, linux-kernel, Zhen Lei, Juri Lelli

On Thu, Apr 14, 2022 at 05:08:53PM +0200, Christophe de Dinechin wrote:
> With gcc version 12.0.1 20220401 (Red Hat 12.0.1-0) (GCC), the following
> errors are reported in sched.h when building after `make defconfig`:

<snip tons of noise>

> Rewrite the definitions of sched_class_highest and for_class_range to
> avoid this error as follows:
> 
> 1/ The sched_class_highest is rewritten to be relative to
>   __begin_sched_classes, so that GCC sees it as being part of the
>   __begin_sched_classes array and not a distinct __end_sched_classes
>   array.
> 
> 2/ The for_class_range macro is modified to replace the comparison with
>   an out-of-bound pointer __begin_sched_classes - 1 with an equivalent,
>   but in-bounds comparison.
> 
> In that specific case, I believe that the GCC analysis is correct and
> potentially valuable for other arrays, so it makes sense to keep it
> enabled.
> 
> Signed-off-by: Christophe de Dinechin <christophe@dinechin.org>
> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
> ---
>  kernel/sched/sched.h | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> index 8dccb34eb190..6350fbc7418d 100644
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -2193,11 +2193,18 @@ const struct sched_class name##_sched_class \
>  extern struct sched_class __begin_sched_classes[];
>  extern struct sched_class __end_sched_classes[];
>  
> -#define sched_class_highest (__end_sched_classes - 1)
> +/*
> + * sched_class_highests is really __end_sched_classes - 1, but written in a way
> + * that makes it clear that it is within __begin_sched_classes[] and not outside
> + * of __end_sched_classes[].
> + */
> +#define sched_class_highest (__begin_sched_classes + \
> +			     (__end_sched_classes - __begin_sched_classes - 1))
>  #define sched_class_lowest  (__begin_sched_classes - 1)
>  
> +/* The + 1 below places the pointers within the range of their array */
>  #define for_class_range(class, _from, _to) \
> -	for (class = (_from); class != (_to); class--)
> +	for (class = (_from); class + 1 != (_to) + 1; class--)

Urgh, so now we get less readable code, just because GCC is being
stupid?

What's wrong with negative array indexes? memory is memory, stuff works.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/3] nodemask.h: Fix compilation error with GCC12
  2022-04-14 15:08 ` [PATCH 2/3] nodemask.h: Fix compilation error with GCC12 Christophe de Dinechin
@ 2022-04-14 15:23   ` Peter Zijlstra
  0 siblings, 0 replies; 18+ messages in thread
From: Peter Zijlstra @ 2022-04-14 15:23 UTC (permalink / raw)
  To: Christophe de Dinechin
  Cc: trivial, Ben Segall, Michael S. Tsirkin, Andrew Morton,
	Steven Rostedt, Ingo Molnar, Mel Gorman, Dietmar Eggemann,
	Vincent Guittot, Paolo Bonzini, Daniel Bristot de Oliveira,
	Jason Wang, virtualization, linux-kernel, Zhen Lei, Juri Lelli

On Thu, Apr 14, 2022 at 05:08:54PM +0200, Christophe de Dinechin wrote:

> diff --git a/include/linux/nodemask.h b/include/linux/nodemask.h
> index 567c3ddba2c4..c6199dbe2591 100644
> --- a/include/linux/nodemask.h
> +++ b/include/linux/nodemask.h
> @@ -375,14 +375,13 @@ static inline void __nodes_fold(nodemask_t *dstp, const nodemask_t *origp,
>  }
>  
>  #if MAX_NUMNODES > 1
> -#define for_each_node_mask(node, mask)			\
> -	for ((node) = first_node(mask);			\
> -		(node) < MAX_NUMNODES;			\
> -		(node) = next_node((node), (mask)))
> +#define for_each_node_mask(node, mask)				    \
> +	for ((node) = first_node(mask);				    \
> +	     (node >= 0) && (node) < MAX_NUMNODES;		    \
> +	     (node) = next_node((node), (mask)))
>  #else /* MAX_NUMNODES == 1 */
> -#define for_each_node_mask(node, mask)			\
> -	if (!nodes_empty(mask))				\
> -		for ((node) = 0; (node) < 1; (node)++)
> +#define for_each_node_mask(node, mask)                                  \
> +	for ((node) = 0; (node) < 1 && !nodes_empty(mask); (node)++)
>  #endif /* MAX_NUMNODES */

Again, less readable code :/ And why?

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/3] sched/headers: Fix compilation error with GCC 12
  2022-04-14 15:21   ` Peter Zijlstra
@ 2022-04-14 20:30     ` Andrew Morton
  2022-04-17 15:52       ` Peter Zijlstra
  2022-04-25 14:23       ` Christophe de Dinechin
  2022-04-17 13:27     ` David Laight
  2022-04-25 14:07     ` Christophe de Dinechin
  2 siblings, 2 replies; 18+ messages in thread
From: Andrew Morton @ 2022-04-14 20:30 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Christophe de Dinechin, trivial, Ben Segall, Michael S. Tsirkin,
	Steven Rostedt, Ingo Molnar, Mel Gorman, Dietmar Eggemann,
	Vincent Guittot, Paolo Bonzini, Daniel Bristot de Oliveira,
	Jason Wang, virtualization, linux-kernel, Zhen Lei, Juri Lelli

On Thu, 14 Apr 2022 17:21:01 +0200 Peter Zijlstra <peterz@infradead.org> wrote:

> > +/* The + 1 below places the pointers within the range of their array */
> >  #define for_class_range(class, _from, _to) \
> > -	for (class = (_from); class != (_to); class--)
> > +	for (class = (_from); class + 1 != (_to) + 1; class--)
> 
> Urgh, so now we get less readable code, just because GCC is being
> stupid?
> 
> What's wrong with negative array indexes? memory is memory, stuff works.

What's more, C is C.  Glorified assembly language in which people do odd
stuff.

But this is presumably a released gcc version and we need to do
something.  And presumably, we need to do a backportable something, so
people can compile older kernels with gcc-12.

Is it possible to suppress just this warning with a gcc option?  And if
so, are we confident that this warning will never be useful in other
places in the kernel?

If no||no then we'll need to add workarounds such as these?

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 3/3] virtio-pci: Use cpumask_available to fix compilation error
  2022-04-14 15:08 ` [PATCH 3/3] virtio-pci: Use cpumask_available to fix compilation error Christophe de Dinechin
@ 2022-04-15  8:48   ` Michael S. Tsirkin
  2022-04-28  9:48     ` Christophe Marie Francois Dupont de Dinechin
  0 siblings, 1 reply; 18+ messages in thread
From: Michael S. Tsirkin @ 2022-04-15  8:48 UTC (permalink / raw)
  To: Christophe de Dinechin
  Cc: trivial, Ben Segall, Andrew Morton, Steven Rostedt, Ingo Molnar,
	Mel Gorman, Dietmar Eggemann, Vincent Guittot, Paolo Bonzini,
	Daniel Bristot de Oliveira, Jason Wang, virtualization,
	linux-kernel, Zhen Lei, Juri Lelli, Peter Zijlstra,
	Murilo Opsfelder Araujo

On Thu, Apr 14, 2022 at 05:08:55PM +0200, Christophe de Dinechin wrote:
> With GCC 12 and defconfig, we get the following error:
> 
> |   CC      drivers/virtio/virtio_pci_common.o
> | drivers/virtio/virtio_pci_common.c: In function ‘vp_del_vqs’:
> | drivers/virtio/virtio_pci_common.c:257:29: error: the comparison will
> |  always evaluate as ‘true’ for the pointer operand in
> |  ‘vp_dev->msix_affinity_masks + (sizetype)((long unsigned int)i * 8)’
> |  must not be NULL [-Werror=address]
> |   257 |                         if (vp_dev->msix_affinity_masks[i])
> |       |                             ^~~~~~
> 
> This happens in the case where CONFIG_CPUMASK_OFFSTACK is not defined,
> since we typedef cpumask_var_t as an array. The compiler is essentially
> complaining that an array pointer cannot be NULL. This is not a very
> important warning, but there is a function called cpumask_available that
> seems to be defined just for that case, so the fix is easy.
> 
> Signed-off-by: Christophe de Dinechin <christophe@dinechin.org>
> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>

There was an alternate patch proposed for this by
Murilo Opsfelder Araujo. What do you think about that approach?


> ---
>  drivers/virtio/virtio_pci_common.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> index d724f676608b..5c44a2f13c93 100644
> --- a/drivers/virtio/virtio_pci_common.c
> +++ b/drivers/virtio/virtio_pci_common.c
> @@ -254,7 +254,7 @@ void vp_del_vqs(struct virtio_device *vdev)
>  
>  	if (vp_dev->msix_affinity_masks) {
>  		for (i = 0; i < vp_dev->msix_vectors; i++)
> -			if (vp_dev->msix_affinity_masks[i])
> +			if (cpumask_available(vp_dev->msix_affinity_masks[i]))
>  				free_cpumask_var(vp_dev->msix_affinity_masks[i]);
>  	}
>  
> -- 
> 2.35.1


^ permalink raw reply	[flat|nested] 18+ messages in thread

* RE: [PATCH 1/3] sched/headers: Fix compilation error with GCC 12
  2022-04-14 15:21   ` Peter Zijlstra
  2022-04-14 20:30     ` Andrew Morton
@ 2022-04-17 13:27     ` David Laight
  2022-04-25 14:07     ` Christophe de Dinechin
  2 siblings, 0 replies; 18+ messages in thread
From: David Laight @ 2022-04-17 13:27 UTC (permalink / raw)
  To: 'Peter Zijlstra', Christophe de Dinechin
  Cc: trivial, Ben Segall, Michael S. Tsirkin, Andrew Morton,
	Steven Rostedt, Ingo Molnar, Mel Gorman, Dietmar Eggemann,
	Vincent Guittot, Paolo Bonzini, Daniel Bristot de Oliveira,
	Jason Wang, virtualization, linux-kernel, Zhen Lei, Juri Lelli

From: Peter Zijlstra
> Sent: 14 April 2022 16:21
...
> <snip tons of noise>
> 
..
> > -#define sched_class_highest (__end_sched_classes - 1)
> > +/*
> > + * sched_class_highests is really __end_sched_classes - 1, but written in a way
> > + * that makes it clear that it is within __begin_sched_classes[] and not outside
> > + * of __end_sched_classes[].
> > + */
> > +#define sched_class_highest (__begin_sched_classes + \
> > +			     (__end_sched_classes - __begin_sched_classes - 1))
> >  #define sched_class_lowest  (__begin_sched_classes - 1)
> >
> > +/* The + 1 below places the pointers within the range of their array */
> >  #define for_class_range(class, _from, _to) \
> > -	for (class = (_from); class != (_to); class--)
> > +	for (class = (_from); class + 1 != (_to) + 1; class--)

That is still technically broken because you are still calculating
the address of array[-1] - even though it is probably optimised out.

> Urgh, so now we get less readable code, just because GCC is being
> stupid?
> 
> What's wrong with negative array indexes? memory is memory, stuff works.

Consider segmented x86 where malloc() always returns {segment:0..segment:size).
Pointer arithmetic will only change the offset.
So &array[-1] is likely to be greater than &array[0].
So it has never been valid C to create pointers to before a data item.

OTOH I've NFI why gcc and clang have started generating warnings for
portability issues that really don't affect mainstream systems.

I'm just waiting for them to warn about memset(p, 0 sizeof *p) when
the structure contains pointers - because the NULL pointer doesn't
have to be the all-zero bit pattern.
The only reason (int)&(struct foo *)0->member is non-portable is because
NULL might not be 0.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/3] sched/headers: Fix compilation error with GCC 12
  2022-04-14 20:30     ` Andrew Morton
@ 2022-04-17 15:52       ` Peter Zijlstra
  2022-04-20 18:45         ` Kees Cook
  2022-04-25 14:23       ` Christophe de Dinechin
  1 sibling, 1 reply; 18+ messages in thread
From: Peter Zijlstra @ 2022-04-17 15:52 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Christophe de Dinechin, trivial, Ben Segall, Michael S. Tsirkin,
	Steven Rostedt, Ingo Molnar, Mel Gorman, Dietmar Eggemann,
	Vincent Guittot, Paolo Bonzini, Daniel Bristot de Oliveira,
	Jason Wang, virtualization, linux-kernel, Zhen Lei, Juri Lelli

On Thu, Apr 14, 2022 at 01:30:50PM -0700, Andrew Morton wrote:
> On Thu, 14 Apr 2022 17:21:01 +0200 Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > > +/* The + 1 below places the pointers within the range of their array */
> > >  #define for_class_range(class, _from, _to) \
> > > -	for (class = (_from); class != (_to); class--)
> > > +	for (class = (_from); class + 1 != (_to) + 1; class--)
> > 
> > Urgh, so now we get less readable code, just because GCC is being
> > stupid?
> > 
> > What's wrong with negative array indexes? memory is memory, stuff works.
> 
> What's more, C is C.  Glorified assembly language in which people do odd
> stuff.
> 
> But this is presumably a released gcc version and we need to do
> something.  And presumably, we need to do a backportable something, so
> people can compile older kernels with gcc-12.
> 
> Is it possible to suppress just this warning with a gcc option?  And if
> so, are we confident that this warning will never be useful in other
> places in the kernel?
> 
> If no||no then we'll need to add workarounds such as these?

-Wno-array-bounds

Is the obvious fix-all cure. The thing is, I want to hear if this new
warning has any actual use or is just crack induced madness like many of
the warnings we turn off.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/3] sched/headers: Fix compilation error with GCC 12
  2022-04-17 15:52       ` Peter Zijlstra
@ 2022-04-20 18:45         ` Kees Cook
  2022-04-21  7:32           ` Peter Zijlstra
  0 siblings, 1 reply; 18+ messages in thread
From: Kees Cook @ 2022-04-20 18:45 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Andrew Morton, Christophe de Dinechin, trivial, Ben Segall,
	Michael S. Tsirkin, Steven Rostedt, Ingo Molnar, Mel Gorman,
	Dietmar Eggemann, Vincent Guittot, Paolo Bonzini,
	Daniel Bristot de Oliveira, Jason Wang, virtualization,
	linux-kernel, Zhen Lei, Juri Lelli

On Sun, Apr 17, 2022 at 05:52:05PM +0200, Peter Zijlstra wrote:
> On Thu, Apr 14, 2022 at 01:30:50PM -0700, Andrew Morton wrote:
> > On Thu, 14 Apr 2022 17:21:01 +0200 Peter Zijlstra <peterz@infradead.org> wrote:
> > 
> > > > +/* The + 1 below places the pointers within the range of their array */
> > > >  #define for_class_range(class, _from, _to) \
> > > > -	for (class = (_from); class != (_to); class--)
> > > > +	for (class = (_from); class + 1 != (_to) + 1; class--)
> > > 
> > > Urgh, so now we get less readable code, just because GCC is being
> > > stupid?
> > > 
> > > What's wrong with negative array indexes? memory is memory, stuff works.
> > 
> > What's more, C is C.  Glorified assembly language in which people do odd
> > stuff.
> > 
> > But this is presumably a released gcc version and we need to do
> > something.  And presumably, we need to do a backportable something, so
> > people can compile older kernels with gcc-12.
> > 
> > Is it possible to suppress just this warning with a gcc option?  And if
> > so, are we confident that this warning will never be useful in other
> > places in the kernel?
> > 
> > If no||no then we'll need to add workarounds such as these?
> 
> -Wno-array-bounds

Please no; we just spent two years fixing all the old non-flexible array
definitions and so many other things fixed for this to be enable because
it finds actual flaws (but we turned it off when it was introduced
because of how much sloppy old code we had).

> Is the obvious fix-all cure. The thing is, I want to hear if this new
> warning has any actual use or is just crack induced madness like many of
> the warnings we turn off.

Yes, it finds real flaws. And also yes, it is rather opinionated about
some "tricks" that have worked in C, but frankly, most of those tricks
end up being weird/accidentally-correct and aren't great for long-term
readability or robustness. Though I'm not speaking specifically to this
proposed patch; I haven't looked closely at it yet.

I quickly went back through commits; here's a handful I found:

20314bacd2f9 ("staging: r8188eu: Fix PPPoE tag insertion on little endian systems")
dcf500065fab ("net: bnxt_ptp: fix compilation error")
5f7dc7d48c94 ("octeontx2-af: fix array bound error")
c7d58971dbea ("ALSA: mixart: Reduce size of mixart_timer_notify")
b3f1dd52c991 ("ARM: vexpress/spc: Avoid negative array index when !SMP")
a2151490cc6c ("drm/dp: Fix OOB read when handling Post Cursor2 register")
d4da1f27396f ("drm/dp: Fix off-by-one in register cache size")
47307c31d90a ("crypto: octeontx2 - Avoid stack variable overflow")
a6501e4b380f ("eeprom: at25: Restore missing allocation")
33ce7f2f95ca ("drm/imx: imx-ldb: fix out of bounds array access warning")
f051ae4f6c73 ("Input: cyapa_gen6 - fix out-of-bounds stack access")
f3217d6f2f7a ("firmware: xilinx: fix out-of-bounds access")
8a03447dd311 ("rtw88: fix subscript above array bounds compiler warning")
ad82a928eb58 ("s390/perf: fix gcc 8 array-bounds warning")
6038aa532a22 ("nvme: target: fix buffer overflow")
50a0d71a5d20 ("cros_ec: fix nul-termination for firmware build info")
43d15c201313 ("staging: rtl8822be: Keep array subscript no lower than zero")

The important part is that with this enabled now, we won't get _new_
problems introduced. Making the C code clear enough that the compiler
can understand the intent, though, can be a little annoying, but makes
things much easier to automatically check. Getting our code-base arranged
so the toolchain can actually do the analysis is well worth it.

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/3] sched/headers: Fix compilation error with GCC 12
  2022-04-20 18:45         ` Kees Cook
@ 2022-04-21  7:32           ` Peter Zijlstra
  0 siblings, 0 replies; 18+ messages in thread
From: Peter Zijlstra @ 2022-04-21  7:32 UTC (permalink / raw)
  To: Kees Cook
  Cc: Andrew Morton, Christophe de Dinechin, trivial, Ben Segall,
	Michael S. Tsirkin, Steven Rostedt, Ingo Molnar, Mel Gorman,
	Dietmar Eggemann, Vincent Guittot, Paolo Bonzini,
	Daniel Bristot de Oliveira, Jason Wang, virtualization,
	linux-kernel, Zhen Lei, Juri Lelli

On Wed, Apr 20, 2022 at 11:45:05AM -0700, Kees Cook wrote:

> > -Wno-array-bounds
> 
> Please no; we just spent two years fixing all the old non-flexible array
> definitions and so many other things fixed for this to be enable because
> it finds actual flaws (but we turned it off when it was introduced
> because of how much sloppy old code we had).
> 
> > Is the obvious fix-all cure. The thing is, I want to hear if this new
> > warning has any actual use or is just crack induced madness like many of
> > the warnings we turn off.
> 
> Yes, it finds real flaws. And also yes, it is rather opinionated about
> some "tricks" that have worked in C, but frankly, most of those tricks
> end up being weird/accidentally-correct and aren't great for long-term
> readability or robustness. Though I'm not speaking specifically to this
> proposed patch; I haven't looked closely at it yet.

So the whole access outside object is UB thing in C is complete rubbish
from an OS perspective. The memory is there and there are geniune uses
for it.

And so far, the patches I've seen for it make the code actively worse.
So we need a sane annotation to tell the compiler to shut up already
without making the code an unreadable mess.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/3] sched/headers: Fix compilation error with GCC 12
  2022-04-14 15:21   ` Peter Zijlstra
  2022-04-14 20:30     ` Andrew Morton
  2022-04-17 13:27     ` David Laight
@ 2022-04-25 14:07     ` Christophe de Dinechin
  2022-05-19 11:16       ` Peter Zijlstra
  2 siblings, 1 reply; 18+ messages in thread
From: Christophe de Dinechin @ 2022-04-25 14:07 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Christophe de Dinechin, trivial, Ben Segall, Michael S. Tsirkin,
	Andrew Morton, Steven Rostedt, Ingo Molnar, Mel Gorman,
	Dietmar Eggemann, Vincent Guittot, Paolo Bonzini,
	Daniel Bristot de Oliveira, Jason Wang, virtualization,
	linux-kernel, Zhen Lei, Juri Lelli



> On 14 Apr 2022, at 17:21, Peter Zijlstra <peterz@infradead.org> wrote:
> 
> On Thu, Apr 14, 2022 at 05:08:53PM +0200, Christophe de Dinechin wrote:
>> With gcc version 12.0.1 20220401 (Red Hat 12.0.1-0) (GCC), the following
>> errors are reported in sched.h when building after `make defconfig`:
> 
> <snip tons of noise>

I don’t mind removing the detailed error message.
What do others think?

> 
>> Rewrite the definitions of sched_class_highest and for_class_range to
>> avoid this error as follows:
>> 
>> 1/ The sched_class_highest is rewritten to be relative to
>>  __begin_sched_classes, so that GCC sees it as being part of the
>>  __begin_sched_classes array and not a distinct __end_sched_classes
>>  array.
>> 
>> 2/ The for_class_range macro is modified to replace the comparison with
>>  an out-of-bound pointer __begin_sched_classes - 1 with an equivalent,
>>  but in-bounds comparison.
>> 
>> In that specific case, I believe that the GCC analysis is correct and
>> potentially valuable for other arrays, so it makes sense to keep it
>> enabled.
>> 
>> Signed-off-by: Christophe de Dinechin <christophe@dinechin.org>
>> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
>> ---
>> kernel/sched/sched.h | 11 +++++++++--
>> 1 file changed, 9 insertions(+), 2 deletions(-)
>> 
>> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
>> index 8dccb34eb190..6350fbc7418d 100644
>> --- a/kernel/sched/sched.h
>> +++ b/kernel/sched/sched.h
>> @@ -2193,11 +2193,18 @@ const struct sched_class name##_sched_class \
>> extern struct sched_class __begin_sched_classes[];
>> extern struct sched_class __end_sched_classes[];
>> 
>> -#define sched_class_highest (__end_sched_classes - 1)
>> +/*
>> + * sched_class_highests is really __end_sched_classes - 1, but written in a way
>> + * that makes it clear that it is within __begin_sched_classes[] and not outside
>> + * of __end_sched_classes[].
>> + */
>> +#define sched_class_highest (__begin_sched_classes + \
>> +			     (__end_sched_classes - __begin_sched_classes - 1))
>> #define sched_class_lowest  (__begin_sched_classes - 1)
>> 
>> +/* The + 1 below places the pointers within the range of their array */
>> #define for_class_range(class, _from, _to) \
>> -	for (class = (_from); class != (_to); class--)
>> +	for (class = (_from); class + 1 != (_to) + 1; class--)
> 
> Urgh, so now we get less readable code,

You consider the original code readable? It actually relies on a
precise layout that is not enforced in this code, not even documented,
but actually enforced by the linker script.

> just because GCC is being
> stupid?

I think that GCC is actually remarkably smart there. It tells you
that you are building pointers to A[] from B[], when there is a legit
way to say that the pointer is in A[] (which is what my patch does)

> What's wrong with negative array indexes? memory is memory, stuff works.

What’s wrong is that the compiler cannot prove theorems anymore.
These theorems are used to optimise code. When you write -1[B], the
compiler cannot optimise based on knowing this refers to A[B-A-1].

While at first, you might think that disabling a warning is a win, what comes next
is the compiler optimizing in a way you did not anticipate, mysterious bugs showing up,
and/or having to turn off some potentially useful optimisation.



^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/3] sched/headers: Fix compilation error with GCC 12
  2022-04-14 20:30     ` Andrew Morton
  2022-04-17 15:52       ` Peter Zijlstra
@ 2022-04-25 14:23       ` Christophe de Dinechin
  1 sibling, 0 replies; 18+ messages in thread
From: Christophe de Dinechin @ 2022-04-25 14:23 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Peter Zijlstra, Christophe de Dinechin, trivial, Ben Segall,
	Michael S. Tsirkin, Steven Rostedt, Ingo Molnar, Mel Gorman,
	Dietmar Eggemann, Vincent Guittot, Paolo Bonzini,
	Daniel Bristot de Oliveira, Jason Wang, virtualization,
	linux-kernel, Zhen Lei, Juri Lelli



> On 14 Apr 2022, at 22:30, Andrew Morton <akpm@linux-foundation.org> wrote:
> 
> On Thu, 14 Apr 2022 17:21:01 +0200 Peter Zijlstra <peterz@infradead.org> wrote:
> 
>>> +/* The + 1 below places the pointers within the range of their array */
>>> #define for_class_range(class, _from, _to) \
>>> -	for (class = (_from); class != (_to); class--)
>>> +	for (class = (_from); class + 1 != (_to) + 1; class--)
>> 
>> Urgh, so now we get less readable code, just because GCC is being
>> stupid?
>> 
>> What's wrong with negative array indexes? memory is memory, stuff works.
> 
> What's more, C is C.  Glorified assembly language in which people do odd
> stuff.

Notably since the advent of clang, we moved a bit beyond glorified assembly language.
There is no 1 on 1 correspondence between what you write and the generated
assembly anymore, by a long shot. I’m sure you know that ;-), but that’s an
opportunity to plug Jason Turner’s conference on writing a C64 pong game using C++17
(https://www.youtube.com/watch?v=zBkNBP00wJE). That demonstrates,
in a funny way, just how far compilers go these days to massage your code.

> 
> But this is presumably a released gcc version and we need to do
> something.  And presumably, we need to do a backportable something, so
> people can compile older kernels with gcc-12.

Hmm, I must admit I had not considered the backporting implications.

> 
> Is it possible to suppress just this warning with a gcc option? And if
> so, are we confident that this warning will never be useful in other
> places in the kernel?

I would advise against it, and not just because of warnings.

With GCC’s ability to track pointers to individual C objects, you can expect
that they will soon start optimising based on that collected knowledge.

An example of useful optimisation based on that knowledge is to
avoid memory reloads, The idea is that a write in array B[] does
not force you to reload all data you already fetched from array A[].
But that requires the compiler to prove that pointers to A[] stay in A[]
and that you don’t purposely build negative indexes from B[] or
anything weird like that.


> If no||no then we'll need to add workarounds such as these?

It is definitely possible to silence that warning. I would still recommend
adding this kind of changes, which I would personally describe more as
“accurate description intended of memory accesses” rather than “workarounds”.
To me, it’s on the same level as putting memory fences, for example.

> 


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 3/3] virtio-pci: Use cpumask_available to fix compilation error
  2022-04-15  8:48   ` Michael S. Tsirkin
@ 2022-04-28  9:48     ` Christophe Marie Francois Dupont de Dinechin
  2022-04-28 11:06       ` Michael S. Tsirkin
  0 siblings, 1 reply; 18+ messages in thread
From: Christophe Marie Francois Dupont de Dinechin @ 2022-04-28  9:48 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: Christophe de Dinechin, trivial, Ben Segall, Andrew Morton,
	Steven Rostedt, Ingo Molnar, Mel Gorman, Dietmar Eggemann,
	Vincent Guittot, Paolo Bonzini, Daniel Bristot de Oliveira,
	Jason Wang, virtualization, linux-kernel, Zhen Lei, Juri Lelli,
	Peter Zijlstra, Murilo Opsfelder Araujo



> On 15 Apr 2022, at 10:48, Michael S. Tsirkin <mst@redhat.com> wrote:
> 
> On Thu, Apr 14, 2022 at 05:08:55PM +0200, Christophe de Dinechin wrote:
>> With GCC 12 and defconfig, we get the following error:
>> 
>> |   CC      drivers/virtio/virtio_pci_common.o
>> | drivers/virtio/virtio_pci_common.c: In function ‘vp_del_vqs’:
>> | drivers/virtio/virtio_pci_common.c:257:29: error: the comparison will
>> |  always evaluate as ‘true’ for the pointer operand in
>> |  ‘vp_dev->msix_affinity_masks + (sizetype)((long unsigned int)i * 8)’
>> |  must not be NULL [-Werror=address]
>> |   257 |                         if (vp_dev->msix_affinity_masks[i])
>> |       |                             ^~~~~~
>> 
>> This happens in the case where CONFIG_CPUMASK_OFFSTACK is not defined,
>> since we typedef cpumask_var_t as an array. The compiler is essentially
>> complaining that an array pointer cannot be NULL. This is not a very
>> important warning, but there is a function called cpumask_available that
>> seems to be defined just for that case, so the fix is easy.
>> 
>> Signed-off-by: Christophe de Dinechin <christophe@dinechin.org>
>> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
> 
> There was an alternate patch proposed for this by
> Murilo Opsfelder Araujo. What do you think about that approach?

I responded on the other thread, but let me share the response here:

[to muriloo@linux.ibm.com]
Apologies for the delay in responding, broken laptop…

In the case where CONFIG_CPUMASK_OFFSTACK is not defined, we have:

	typedef struct cpumask cpumask_var_t[1];

So that vp_dev->msix_affinity_masks[i] is statically not null (that’s the warning)
but also a static pointer, so not kfree-safe IMO.

> 
> 
>> ---
>> drivers/virtio/virtio_pci_common.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
>> index d724f676608b..5c44a2f13c93 100644
>> --- a/drivers/virtio/virtio_pci_common.c
>> +++ b/drivers/virtio/virtio_pci_common.c
>> @@ -254,7 +254,7 @@ void vp_del_vqs(struct virtio_device *vdev)
>> 
>> 	if (vp_dev->msix_affinity_masks) {
>> 		for (i = 0; i < vp_dev->msix_vectors; i++)
>> -			if (vp_dev->msix_affinity_masks[i])
>> +			if (cpumask_available(vp_dev->msix_affinity_masks[i]))
>> 				free_cpumask_var(vp_dev->msix_affinity_masks[i]);
>> 	}
>> 
>> -- 
>> 2.35.1
> 


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 3/3] virtio-pci: Use cpumask_available to fix compilation error
  2022-04-28  9:48     ` Christophe Marie Francois Dupont de Dinechin
@ 2022-04-28 11:06       ` Michael S. Tsirkin
  0 siblings, 0 replies; 18+ messages in thread
From: Michael S. Tsirkin @ 2022-04-28 11:06 UTC (permalink / raw)
  To: Christophe Marie Francois Dupont de Dinechin
  Cc: Christophe de Dinechin, trivial, Ben Segall, Andrew Morton,
	Steven Rostedt, Ingo Molnar, Mel Gorman, Dietmar Eggemann,
	Vincent Guittot, Paolo Bonzini, Daniel Bristot de Oliveira,
	Jason Wang, virtualization, linux-kernel, Zhen Lei, Juri Lelli,
	Peter Zijlstra, Murilo Opsfelder Araujo

On Thu, Apr 28, 2022 at 11:48:01AM +0200, Christophe Marie Francois Dupont de Dinechin wrote:
> 
> 
> > On 15 Apr 2022, at 10:48, Michael S. Tsirkin <mst@redhat.com> wrote:
> > 
> > On Thu, Apr 14, 2022 at 05:08:55PM +0200, Christophe de Dinechin wrote:
> >> With GCC 12 and defconfig, we get the following error:
> >> 
> >> |   CC      drivers/virtio/virtio_pci_common.o
> >> | drivers/virtio/virtio_pci_common.c: In function ‘vp_del_vqs’:
> >> | drivers/virtio/virtio_pci_common.c:257:29: error: the comparison will
> >> |  always evaluate as ‘true’ for the pointer operand in
> >> |  ‘vp_dev->msix_affinity_masks + (sizetype)((long unsigned int)i * 8)’
> >> |  must not be NULL [-Werror=address]
> >> |   257 |                         if (vp_dev->msix_affinity_masks[i])
> >> |       |                             ^~~~~~
> >> 
> >> This happens in the case where CONFIG_CPUMASK_OFFSTACK is not defined,
> >> since we typedef cpumask_var_t as an array. The compiler is essentially
> >> complaining that an array pointer cannot be NULL. This is not a very
> >> important warning, but there is a function called cpumask_available that
> >> seems to be defined just for that case, so the fix is easy.
> >> 
> >> Signed-off-by: Christophe de Dinechin <christophe@dinechin.org>
> >> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
> > 
> > There was an alternate patch proposed for this by
> > Murilo Opsfelder Araujo. What do you think about that approach?
> 
> I responded on the other thread, but let me share the response here:
> 
> [to muriloo@linux.ibm.com]
> Apologies for the delay in responding, broken laptop…
> 
> In the case where CONFIG_CPUMASK_OFFSTACK is not defined, we have:
> 
> 	typedef struct cpumask cpumask_var_t[1];
> 
> So that vp_dev->msix_affinity_masks[i] is statically not null (that’s the warning)
> but also a static pointer, so not kfree-safe IMO.


Not sure I understand what you are saying here.

> > 
> > 
> >> ---
> >> drivers/virtio/virtio_pci_common.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >> 
> >> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> >> index d724f676608b..5c44a2f13c93 100644
> >> --- a/drivers/virtio/virtio_pci_common.c
> >> +++ b/drivers/virtio/virtio_pci_common.c
> >> @@ -254,7 +254,7 @@ void vp_del_vqs(struct virtio_device *vdev)
> >> 
> >> 	if (vp_dev->msix_affinity_masks) {
> >> 		for (i = 0; i < vp_dev->msix_vectors; i++)
> >> -			if (vp_dev->msix_affinity_masks[i])
> >> +			if (cpumask_available(vp_dev->msix_affinity_masks[i]))
> >> 				free_cpumask_var(vp_dev->msix_affinity_masks[i]);
> >> 	}
> >> 
> >> -- 
> >> 2.35.1
> > 


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 0/3] trivial: Fix several compilation errors/warnings with GCC12
  2022-04-14 15:08 [PATCH 0/3] trivial: Fix several compilation errors/warnings with GCC12 Christophe de Dinechin
                   ` (2 preceding siblings ...)
  2022-04-14 15:08 ` [PATCH 3/3] virtio-pci: Use cpumask_available to fix compilation error Christophe de Dinechin
@ 2022-05-15 21:24 ` Davidlohr Bueso
  3 siblings, 0 replies; 18+ messages in thread
From: Davidlohr Bueso @ 2022-05-15 21:24 UTC (permalink / raw)
  To: Christophe de Dinechin
  Cc: trivial, Ben Segall, Michael S. Tsirkin, Andrew Morton,
	Steven Rostedt, Ingo Molnar, Mel Gorman, Dietmar Eggemann,
	Vincent Guittot, Paolo Bonzini, Daniel Bristot de Oliveira,
	Jason Wang, virtualization, linux-kernel, Zhen Lei, Juri Lelli,
	Peter Zijlstra, torvalds

Hello - What is the status of this? Currently gcc 12 (tumbleweed) is unable to
build Linus' latest because of splats in the scheduler headers...

Thanks,
Davidlohr

On Thu, 14 Apr 2022, Christophe de Dinechin wrote:

>Compiling with GCC 12 using defconfig generates a number of build errors
>due to new warnings, notably array-bounds checks. Some of these warnings appear
>legitimate and relatively easy to fix.
>
>Note that this series is not sufficient for a clean build yet. There are
>in particular a number of warnings reported by the array-bounds check
>that appear bogus, like:
>
>| In function ???__native_read_cr3???,
>|     inlined from ???__read_cr3???
>|         at ./arch/x86/include/asm/special_insns.h:169:9,
>|     inlined from ???read_cr3_pa???
>|         at ./arch/x86/include/asm/processor.h:252:9,
>|     inlined from ???relocate_restore_code???
>|         at arch/x86/power/hibernate.c:165:17:
>| ./arch/x86/include/asm/special_insns.h:48:9: error:
>|    array subscript 0 is outside array bounds of ???unsigned int[0]???
>|    [-Werror=array-bounds]
>|    48 | asm volatile("mov %%cr3,%0\n\t" : "=r" (val) : __FORCE_ORDER);
>|       | ^~~
>| cc1: all warnings being treated as errors
>
>The error above is for an instruction that does not obviously address any
>C array, in particular since the asm constraint is "=r" and not "=rm".
>
>Consequently, the series here only addresses a few low hanging fruits that
>appear legitimate and relatively easy to fix.
>
>Christophe de Dinechin (3):
>  sched/headers: Fix compilation error with GCC 12
>  nodemask.h: Fix compilation error with GCC12
>  virtio-pci: Use cpumask_available to fix compilation error
>
> drivers/virtio/virtio_pci_common.c |  2 +-
> include/linux/nodemask.h           | 13 ++++++-------
> kernel/sched/sched.h               | 11 +++++++++--
> 3 files changed, 16 insertions(+), 10 deletions(-)
>
>--
>2.35.1
>
>

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/3] sched/headers: Fix compilation error with GCC 12
  2022-04-25 14:07     ` Christophe de Dinechin
@ 2022-05-19 11:16       ` Peter Zijlstra
  0 siblings, 0 replies; 18+ messages in thread
From: Peter Zijlstra @ 2022-05-19 11:16 UTC (permalink / raw)
  To: Christophe de Dinechin
  Cc: Christophe de Dinechin, trivial, Ben Segall, Michael S. Tsirkin,
	Andrew Morton, Steven Rostedt, Ingo Molnar, Mel Gorman,
	Dietmar Eggemann, Vincent Guittot, Paolo Bonzini,
	Daniel Bristot de Oliveira, Jason Wang, virtualization,
	linux-kernel, Zhen Lei, Juri Lelli

On Mon, Apr 25, 2022 at 04:07:43PM +0200, Christophe de Dinechin wrote:

> >> extern struct sched_class __begin_sched_classes[];
> >> extern struct sched_class __end_sched_classes[];
> >> 
> >> -#define sched_class_highest (__end_sched_classes - 1)
> >> +/*
> >> + * sched_class_highests is really __end_sched_classes - 1, but written in a way
> >> + * that makes it clear that it is within __begin_sched_classes[] and not outside
> >> + * of __end_sched_classes[].
> >> + */
> >> +#define sched_class_highest (__begin_sched_classes + \
> >> +			     (__end_sched_classes - __begin_sched_classes - 1))
> >> #define sched_class_lowest  (__begin_sched_classes - 1)
> >> 
> >> +/* The + 1 below places the pointers within the range of their array */
> >> #define for_class_range(class, _from, _to) \
> >> -	for (class = (_from); class != (_to); class--)
> >> +	for (class = (_from); class + 1 != (_to) + 1; class--)
> > 
> > Urgh, so now we get less readable code,
> 
> You consider the original code readable? 

Yeah, because: x + y - x - 1 == y - 1, so wth would you want to write it
with the x on. That's just silly.

> It actually relies on a
> precise layout that is not enforced in this code, not even documented,
> but actually enforced by the linker script.

It has a comment pointing at the linker script, and we have:

	/* Make sure the linker didn't screw up */
	BUG_ON(&idle_sched_class + 1 != &fair_sched_class ||
	       &fair_sched_class + 1 != &rt_sched_class ||
	       &rt_sched_class + 1   != &dl_sched_class);
#ifdef CONFIG_SMP
	BUG_ON(&dl_sched_class + 1 != &stop_sched_class);
#endif

On boot to verify the layout is as we expect.

> > just because GCC is being
> > stupid?
> 
> I think that GCC is actually remarkably smart there. It tells you
> that you are building pointers to A[] from B[], when there is a legit
> way to say that the pointer is in A[] (which is what my patch does)

We build with -fno-strict-aliasing, it must not assume anything like
that, unless restrict is used.

In this case, they're not two objects but the same one. Just because
linker script can't really get us a sensible array definition.

> > What's wrong with negative array indexes? memory is memory, stuff works.
> 
> What’s wrong is that the compiler cannot prove theorems anymore.
> These theorems are used to optimise code. When you write -1[B], the
> compiler cannot optimise based on knowing this refers to A[B-A-1].
> 
> While at first, you might think that disabling a warning is a win,
> what comes next is the compiler optimizing in a way you did not
> anticipate, mysterious bugs showing up, and/or having to turn off some
> potentially useful optimisation.

We're usually fairly quick to call a compiler broken if doesn't do what
we want it to. Dodgy optimizations go out the window real fast.

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2022-05-19 11:16 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-14 15:08 [PATCH 0/3] trivial: Fix several compilation errors/warnings with GCC12 Christophe de Dinechin
2022-04-14 15:08 ` [PATCH 1/3] sched/headers: Fix compilation error with GCC 12 Christophe de Dinechin
2022-04-14 15:21   ` Peter Zijlstra
2022-04-14 20:30     ` Andrew Morton
2022-04-17 15:52       ` Peter Zijlstra
2022-04-20 18:45         ` Kees Cook
2022-04-21  7:32           ` Peter Zijlstra
2022-04-25 14:23       ` Christophe de Dinechin
2022-04-17 13:27     ` David Laight
2022-04-25 14:07     ` Christophe de Dinechin
2022-05-19 11:16       ` Peter Zijlstra
2022-04-14 15:08 ` [PATCH 2/3] nodemask.h: Fix compilation error with GCC12 Christophe de Dinechin
2022-04-14 15:23   ` Peter Zijlstra
2022-04-14 15:08 ` [PATCH 3/3] virtio-pci: Use cpumask_available to fix compilation error Christophe de Dinechin
2022-04-15  8:48   ` Michael S. Tsirkin
2022-04-28  9:48     ` Christophe Marie Francois Dupont de Dinechin
2022-04-28 11:06       ` Michael S. Tsirkin
2022-05-15 21:24 ` [PATCH 0/3] trivial: Fix several compilation errors/warnings with GCC12 Davidlohr Bueso

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).