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

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

quoted

quoted

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.

quoted

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.

quoted

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