Hi,
while compiling the latest upstream kernel on fedora 36 which
uses gcc-12 by default, i got a lot of -Warray-bounds warnings:
(Note that this is on s390 arch)
In function ‘preempt_count’,
inlined from ‘do_one_initcall’ at init/main.c:1290:14:
./include/asm-generic/rwonce.h:44:26: warning: array subscript 0 is outside array bounds of ‘const volatile int[0]’ [-Warray-bounds]
44 | #define __READ_ONCE(x) (*(const volatile __unqual_scalar_typeof(x) *)&(x))
| ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/include/asm-generic/rwonce.h:50:9: note: in expansion of macro ‘__READ_ONCE’
50 | __READ_ONCE(x);
| ^~~~~~~~~~~
./arch/s390/include/asm/preempt.h:17:16: note: in expansion of macro ‘READ_ONCE’
17 | return READ_ONCE(S390_lowcore.preempt_count) & ~PREEMPT_NEED_RESCHED;
| ^~~~~~~~~
This is because S390_lowcore is defined as follows:
#define S390_lowcore (*((struct lowcore *) 0))
Lowcore is a 8K cpu-local memory region on s390 at fixed address 0.
The obvious 'fix' is to use absolute_pointer():
#define S390_lowcore (*((struct lowcore *)absolute_pointer(0)))
That makes the warning go away, but unfortunately the compiler no longer
knows that the memory access is fitting into a load/store with a 12 bit
displacement.
Without absolute_pointer(), reading the preempt count is just a single
instruction: 'l %r11,936'
static inline int preempt_count(void)
{
return READ_ONCE(S390_lowcore.preempt_count) & ~PREEMPT_NEED_RESCHED;
8c4: 58 b0 03 a8 l %r11,936 <--- load preempt count
8c8: b9 04 00 92 lgr %r9,%r2
int count = preempt_count();
with absolute pointer(), the compiler no longer optimizes the read to
one instruction and uses an additional base register:
static inline int preempt_count(void)
{
return READ_ONCE(S390_lowcore.preempt_count) & ~PREEMPT_NEED_RESCHED;
8c4: a7 19 00 00 lghi %r1,0 <-- use %r1 as base, load with 0
8c8: b9 04 00 92 lgr %r9,%r2
int count = preempt_count();
char msgbuf[64];
8cc: d7 3f f0 a8 f0 a8 xc 168(64,%r15),168(%r15)
8d2: 58 b0 13 a8 l %r11,936(%r1) <-- and finally add the offset and fetch
int ret;
The reason for gcc to not optimize that further is likely the asm
statement in RELOC_HIDE (located in include/linux/compiler-gcc.h)
#define RELOC_HIDE(ptr, off) \
({ \
unsigned long __ptr; \
__asm__ ("" : "=r"(__ptr) : "0"(ptr)); \
(typeof(ptr)) (__ptr + (off)); \
})
For most of the code this wouldn't be a big problem, but we're storing
information like preempt_count, current thread info, etc in lowcore
because it is the fastest way. I would like to avoid to use additional
instructions/registers just to avoid a warning.
Does anyone have an idea about a different way to make this warning go
away?
Thanks
Sven
Linus Torvalds <[email protected]> writes:
> On Thu, Apr 21, 2022 at 7:02 AM Sven Schnelle <[email protected]> wrote:
>>
>> The obvious 'fix' is to use absolute_pointer():
>>
>> #define S390_lowcore (*((struct lowcore *)absolute_pointer(0)))
>>
>> That makes the warning go away, but unfortunately the compiler no longer
>> knows that the memory access is fitting into a load/store with a 12 bit
>> displacement.
>
> In the gcc bugzilla for us needing to do these games:
>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99578
>
> one of the suggestions was "I recommend suppressing the warning either
> by #pragma GCC diagnostic or by making the pointer volatile".
>
> But READ_ONCE() should already be doing that volatile thing, so that
> suggestion may not work with gcc-12 any more.
>
> It is *possible* that gcc-12 has now special-cased the very special
> issue of a cast of the constant zero. That is how NULL was
> traditionally defined.
>
> So just out of a perverse curiosity, what happens if you do something like this:
>
> #define S390_lowcore_end ((struct lowcore *)sizeof(struct lowcore))
> #define S390_lowcore (S390_lowcore_end[-1])
>
> instead? It should get the same value in the end, but it doesn't have
> that special case of "cast an integer constant 0 to a pointer".
>
> I suspect it probably doesn't help, because gcc will still see "oh,
> you're basing this off address zero".
Yes.
> Another thing to try might be to remove the initial 16 bytes of
> padding from 'struct lowcore' (it looks like the first 20 bytes are
> not used - so leave 4 bytes of padding still), and use
>
> #define S390_lowcore (*((struct lowcore_nopad *)16))
>
> instead. Then gcc will never see that 0, and hopefully the "he's
> accessing based off a NULL pointer" logic will go away.
Looks like gcc also catches that, i get the same warning.
> Because right now, our absolute_pointer() protection against this
> horrible gcc mis-feature is literally based on hiding the value from
> the compiler with an inline asm, and by virtue of hiding the value
> then yes, gcc will have to go through a register base pointer and
> cannot see that it fits in 12 bits.
>
> .. or you just need to disable -Warray-bounds on s390.
That's what i'll do for now. I'll complain in the bugtracker entry
mentioned above, but for now it is propably easier to just disable
the warning until we have a fix.
Thanks
Sven
On Thu, Apr 21, 2022 at 7:02 AM Sven Schnelle <[email protected]> wrote:
>
> The obvious 'fix' is to use absolute_pointer():
>
> #define S390_lowcore (*((struct lowcore *)absolute_pointer(0)))
>
> That makes the warning go away, but unfortunately the compiler no longer
> knows that the memory access is fitting into a load/store with a 12 bit
> displacement.
In the gcc bugzilla for us needing to do these games:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99578
one of the suggestions was "I recommend suppressing the warning either
by #pragma GCC diagnostic or by making the pointer volatile".
But READ_ONCE() should already be doing that volatile thing, so that
suggestion may not work with gcc-12 any more.
It is *possible* that gcc-12 has now special-cased the very special
issue of a cast of the constant zero. That is how NULL was
traditionally defined.
So just out of a perverse curiosity, what happens if you do something like this:
#define S390_lowcore_end ((struct lowcore *)sizeof(struct lowcore))
#define S390_lowcore (S390_lowcore_end[-1])
instead? It should get the same value in the end, but it doesn't have
that special case of "cast an integer constant 0 to a pointer".
I suspect it probably doesn't help, because gcc will still see "oh,
you're basing this off address zero".
Another thing to try might be to remove the initial 16 bytes of
padding from 'struct lowcore' (it looks like the first 20 bytes are
not used - so leave 4 bytes of padding still), and use
#define S390_lowcore (*((struct lowcore_nopad *)16))
instead. Then gcc will never see that 0, and hopefully the "he's
accessing based off a NULL pointer" logic will go away.
Because right now, our absolute_pointer() protection against this
horrible gcc mis-feature is literally based on hiding the value from
the compiler with an inline asm, and by virtue of hiding the value
then yes, gcc will have to go through a register base pointer and
cannot see that it fits in 12 bits.
.. or you just need to disable -Warray-bounds on s390.
Linus
From: Linus Torvalds
> Sent: 21 April 2022 17:11
>
> On Thu, Apr 21, 2022 at 7:02 AM Sven Schnelle <[email protected]> wrote:
> >
> > The obvious 'fix' is to use absolute_pointer():
> >
> > #define S390_lowcore (*((struct lowcore *)absolute_pointer(0)))
> >
> > That makes the warning go away, but unfortunately the compiler no longer
> > knows that the memory access is fitting into a load/store with a 12 bit
> > displacement.
>
> In the gcc bugzilla for us needing to do these games:
>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99578
>
> one of the suggestions was "I recommend suppressing the warning either
> by #pragma GCC diagnostic or by making the pointer volatile".
>
> But READ_ONCE() should already be doing that volatile thing, so that
> suggestion may not work with gcc-12 any more.
>
> It is *possible* that gcc-12 has now special-cased the very special
> issue of a cast of the constant zero. That is how NULL was
> traditionally defined.
>
> So just out of a perverse curiosity, what happens if you do something like this:
>
> #define S390_lowcore_end ((struct lowcore *)sizeof(struct lowcore))
> #define S390_lowcore (S390_lowcore_end[-1])
>
> instead? It should get the same value in the end, but it doesn't have
> that special case of "cast an integer constant 0 to a pointer".
>
> I suspect it probably doesn't help, because gcc will still see "oh,
> you're basing this off address zero".
>
> Another thing to try might be to remove the initial 16 bytes of
> padding from 'struct lowcore' (it looks like the first 20 bytes are
> not used - so leave 4 bytes of padding still), and use
>
> #define S390_lowcore (*((struct lowcore_nopad *)16))
>
> instead. Then gcc will never see that 0, and hopefully the "he's
> accessing based off a NULL pointer" logic will go away.
>
> Because right now, our absolute_pointer() protection against this
> horrible gcc mis-feature is literally based on hiding the value from
> the compiler with an inline asm, and by virtue of hiding the value
> then yes, gcc will have to go through a register base pointer and
> cannot see that it fits in 12 bits.
I think you might be mixing up two problems.
Accessing ((foo *)0)->member is problematic because NULL might not be zero.
In which case an unexpected address is generated.
I think this is why clang really doesn't like you doing that.
Using ((foo *)(sizeof (foo))[-1].member might get around that.
I suspect the array bounds issue is caused by gcc using a size of 0
for 'I don't know the size' and then assuming it is real size later on.
That seems like a real gcc bug.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)