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
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 <[email protected]>
Signed-off-by: Christophe de Dinechin <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Christophe de Dinechin <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Christophe de Dinechin <[email protected]>
---
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
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 <[email protected]>
> Signed-off-by: Christophe de Dinechin <[email protected]>
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
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 <[email protected]>
> Signed-off-by: Christophe de Dinechin <[email protected]>
> ---
> 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.
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?
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)
> On 14 Apr 2022, at 17:21, Peter Zijlstra <[email protected]> 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 <[email protected]>
>> Signed-off-by: Christophe de Dinechin <[email protected]>
>> ---
>> 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.
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 <[email protected]> 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 <[email protected]>
> >> Signed-off-by: Christophe de Dinechin <[email protected]>
> >
> > 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 [email protected]]
> 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
> >
> On 15 Apr 2022, at 10:48, Michael S. Tsirkin <[email protected]> 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 <[email protected]>
>> Signed-off-by: Christophe de Dinechin <[email protected]>
>
> 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 [email protected]]
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
>
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
>
>
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.