I discussed with the original authors who added freestanding to our
build. It turns out that it was added globally but this was just to
to workaround powerpc not compiling under clang, but they felt the
fix was appropriate globally.
Now Nick has dug up https://lkml.org/lkml/2019/8/29/1300, which
advises against freestanding. Also, I've did some research and
discovered that the original reason for using freestanding for
powerpc has been fixed here:
https://lore.kernel.org/linuxppc-dev/20191119045712.39633-3-natechancellor@gmail.com/
I'm going to remove -ffreestanding from downstream, so we don't really need
this anymore, sorry for waisting people's time.
I wonder if the freestanding fix from the aforementioned patch is really needed
though. I think that clang is actually right to point out the issue.
I don't see any reason why setjmp()/longjmp() are declared as taking longs
rather than ints. The implementation looks like it only ever propagates the
value (in longjmp) or sets it to 1 (in setjmp), and we only ever call longjmp
with integer parameters. But I'm not a PowerPC expert, so I might
be misreading the code.
So it seems that we could just remove freestanding altogether and rewrite the
code to:
From: Nick Desaulniers <ndesaulniers@google.com> Date: 2020-03-26 17:19:01
On Thu, Mar 26, 2020 at 5:38 AM Clement Courbet [off-list ref] wrote:
quoted hunk
I discussed with the original authors who added freestanding to our
build. It turns out that it was added globally but this was just to
to workaround powerpc not compiling under clang, but they felt the
fix was appropriate globally.
Now Nick has dug up https://lkml.org/lkml/2019/8/29/1300, which
advises against freestanding. Also, I've did some research and
discovered that the original reason for using freestanding for
powerpc has been fixed here:
https://lore.kernel.org/linuxppc-dev/20191119045712.39633-3-natechancellor@gmail.com/
I'm going to remove -ffreestanding from downstream, so we don't really need
this anymore, sorry for waisting people's time.
I wonder if the freestanding fix from the aforementioned patch is really needed
though. I think that clang is actually right to point out the issue.
I don't see any reason why setjmp()/longjmp() are declared as taking longs
rather than ints. The implementation looks like it only ever propagates the
value (in longjmp) or sets it to 1 (in setjmp), and we only ever call longjmp
with integer parameters. But I'm not a PowerPC expert, so I might
be misreading the code.
So it seems that we could just remove freestanding altogether and rewrite the
code to:
I'm happy to send a patch for this, and get rid of more -ffreestanding.
Opinions ?
Yes, I think the above diff and additionally cleaning up
-ffreestanding from arch/powerpc/kernel/Makefile and
arch/powerpc/xmon/Makefile would be a much better approach. If that's
good enough to fix the warning, I kind of can't believe we didn't spot
that in the original code review!
Actually, the god awful warning was:
./arch/powerpc/include/asm/setjmp.h:10:13: error: declaration of
built-in function 'setjmp' requires the declaration of the 'jmp_buf'
type, commonly provided in the header <setjmp.h>.
[-Werror,-Wincomplete-setjmp-declaration]
extern long setjmp(long *) __attribute__((returns_twice));
^
So jmp_buf was missing, wasn't used, but also the long vs int confusion.
I tested the above diff, all calls to setjmp under arch/powerpc/ just
compare the return value against 0. Callers of longjmp just pass 1
for the final parameter. So the above changes should be no functional
change.
--
Thanks,
~Nick Desaulniers
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2020-03-27 04:06:19
Clement Courbet [off-list ref] writes:
quoted hunk
I discussed with the original authors who added freestanding to our
build. It turns out that it was added globally but this was just to
to workaround powerpc not compiling under clang, but they felt the
fix was appropriate globally.
Now Nick has dug up https://lkml.org/lkml/2019/8/29/1300, which
advises against freestanding. Also, I've did some research and
discovered that the original reason for using freestanding for
powerpc has been fixed here:
https://lore.kernel.org/linuxppc-dev/20191119045712.39633-3-natechancellor@gmail.com/
I'm going to remove -ffreestanding from downstream, so we don't really need
this anymore, sorry for waisting people's time.
I wonder if the freestanding fix from the aforementioned patch is really needed
though. I think that clang is actually right to point out the issue.
I don't see any reason why setjmp()/longjmp() are declared as taking longs
rather than ints. The implementation looks like it only ever propagates the
value (in longjmp) or sets it to 1 (in setjmp), and we only ever call longjmp
with integer parameters. But I'm not a PowerPC expert, so I might
be misreading the code.
So it seems that we could just remove freestanding altogether and rewrite the
code to:
I'm happy to send a patch for this, and get rid of more -ffreestanding.
Opinions ?
If it works then it looks like a much better fix than using -ffreestanding.
Please submit a patch with a change log etc. and I'd be happy to merge
it.
cheers
I'm happy to send a patch for this, and get rid of more -ffreestanding.
Opinions ?
Pedantically, jmp_buf should be an array type. But, this will probably
work fine, and it certainly is better than what we had before.
You could do
typedef long jmp_buf[JMP_BUF_LEN];
perhaps?
Thanks,
Segher