Re: [PATCH 1/3] sched/headers: Fix compilation error with GCC 12

Re: [PATCH 1/3] sched/headers: Fix compilation error with GCC 12

[Peter Zijlstra]

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.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 2/3] nodemask.h: Fix compilation error with GCC12

[Peter Zijlstra]

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?
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/3] sched/headers: Fix compilation error with GCC 12

[Andrew Morton]

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?
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 3/3] virtio-pci: Use cpumask_available to fix compilation error

[Michael S. Tsirkin]

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

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

RE: [PATCH 1/3] sched/headers: Fix compilation error with GCC 12

[David Laight]

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)

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/3] sched/headers: Fix compilation error with GCC 12

[Peter Zijlstra]

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.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/3] sched/headers: Fix compilation error with GCC 12

[Kees Cook]

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
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/3] sched/headers: Fix compilation error with GCC 12

[Peter Zijlstra]

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.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 3/3] virtio-pci: Use cpumask_available to fix compilation error

[Christophe Marie Francois Dupont de Dinechin]

> 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
> 

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 3/3] virtio-pci: Use cpumask_available to fix compilation error

[Michael S. Tsirkin]

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
> > 

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 0/3] trivial: Fix several compilation errors/warnings with GCC12

[Davidlohr Bueso]

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
>
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/3] sched/headers: Fix compilation error with GCC 12

[Peter Zijlstra]

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.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization