Re: [PATCH] powerpc/boot: Use address-of operator on section symbols
Hi Michael,
On Mon, Aug 3, 2020 at 1:09 PM Michael Ellerman wrote:
> Geert Uytterhoeven writes:
> > On Mon, Jul 20, 2020 at 11:03 PM Segher Boessenkool
> > wrote:
> >> On Sat, Jul 18, 2020 at 09:50:50AM +0200, Geert Uytterhoeven wrote:
> >> > On Wed, Jun 24, 2020 at 6:02 AM Nathan Chancellor
> >> > wrote:
> >> > > /* If we have an image attached to us, it overrides anything
> >> > > * supplied by the loader. */
> >> > > - if (_initrd_end > _initrd_start) {
> >> > > + if (&_initrd_end > &_initrd_start) {
> >> >
> >> > Are you sure that fix is correct?
> >> >
> >> > extern char _initrd_start[];
> >> > extern char _initrd_end[];
> >> > extern char _esm_blob_start[];
> >> > extern char _esm_blob_end[];
> >> >
> >> > Of course the result of their comparison is a constant, as the addresses
> >> > are constant. If clangs warns about it, perhaps that warning should be
> >> > moved
> >> > to W=1?
> >> >
> >> > But adding "&" is not correct, according to C.
> >>
> >> Why not?
> >>
> >> 6.5.3.2/3
> >> The unary & operator yields the address of its operand. [...]
> >> Otherwise, the result is a pointer to the object or function designated
> >> by its operand.
> >>
> >> This is the same as using the name of an array without anything else,
> >> yes. It is a bit clearer if it would not be declared as array, perhaps,
> >> but it is correct just fine like this.
> >
> > Thanks, I stand corrected.
> >
> > Regardless, the comparison is still a comparison between two constant
> > addresses, so my fear is that the compiler will start generating
> > warnings for that in the near or distant future, making this change
> > futile.
>
> They're not constant at compile time though. So I don't think the
> compiler could (sensibly) warn about that? (surely!)
They're constant, but the compiler doesn't know their value.
That doesn't change by (not) using the address-of operator.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH] powerpc/boot: Use address-of operator on section symbols
Geert Uytterhoeven writes:
> On Mon, Jul 20, 2020 at 11:03 PM Segher Boessenkool
> wrote:
>> On Sat, Jul 18, 2020 at 09:50:50AM +0200, Geert Uytterhoeven wrote:
>> > On Wed, Jun 24, 2020 at 6:02 AM Nathan Chancellor
>> > wrote:
>> > > /* If we have an image attached to us, it overrides anything
>> > > * supplied by the loader. */
>> > > - if (_initrd_end > _initrd_start) {
>> > > + if (&_initrd_end > &_initrd_start) {
>> >
>> > Are you sure that fix is correct?
>> >
>> > extern char _initrd_start[];
>> > extern char _initrd_end[];
>> > extern char _esm_blob_start[];
>> > extern char _esm_blob_end[];
>> >
>> > Of course the result of their comparison is a constant, as the addresses
>> > are constant. If clangs warns about it, perhaps that warning should be
>> > moved
>> > to W=1?
>> >
>> > But adding "&" is not correct, according to C.
>>
>> Why not?
>>
>> 6.5.3.2/3
>> The unary & operator yields the address of its operand. [...]
>> Otherwise, the result is a pointer to the object or function designated
>> by its operand.
>>
>> This is the same as using the name of an array without anything else,
>> yes. It is a bit clearer if it would not be declared as array, perhaps,
>> but it is correct just fine like this.
>
> Thanks, I stand corrected.
>
> Regardless, the comparison is still a comparison between two constant
> addresses, so my fear is that the compiler will start generating
> warnings for that in the near or distant future, making this change
> futile.
They're not constant at compile time though. So I don't think the
compiler could (sensibly) warn about that? (surely!)
cheers
Re: [PATCH] powerpc/boot: Use address-of operator on section symbols
Hi Segher,
On Mon, Jul 20, 2020 at 11:03 PM Segher Boessenkool
wrote:
> On Sat, Jul 18, 2020 at 09:50:50AM +0200, Geert Uytterhoeven wrote:
> > On Wed, Jun 24, 2020 at 6:02 AM Nathan Chancellor
> > wrote:
> > > /* If we have an image attached to us, it overrides anything
> > > * supplied by the loader. */
> > > - if (_initrd_end > _initrd_start) {
> > > + if (&_initrd_end > &_initrd_start) {
> >
> > Are you sure that fix is correct?
> >
> > extern char _initrd_start[];
> > extern char _initrd_end[];
> > extern char _esm_blob_start[];
> > extern char _esm_blob_end[];
> >
> > Of course the result of their comparison is a constant, as the addresses
> > are constant. If clangs warns about it, perhaps that warning should be
> > moved
> > to W=1?
> >
> > But adding "&" is not correct, according to C.
>
> Why not?
>
> 6.5.3.2/3
> The unary & operator yields the address of its operand. [...]
> Otherwise, the result is a pointer to the object or function designated
> by its operand.
>
> This is the same as using the name of an array without anything else,
> yes. It is a bit clearer if it would not be declared as array, perhaps,
> but it is correct just fine like this.
Thanks, I stand corrected.
Regardless, the comparison is still a comparison between two constant
addresses, so my fear is that the compiler will start generating
warnings for that in the near or distant future, making this change
futile.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH] powerpc/boot: Use address-of operator on section symbols
Hi!
On Sat, Jul 18, 2020 at 09:50:50AM +0200, Geert Uytterhoeven wrote:
> On Wed, Jun 24, 2020 at 6:02 AM Nathan Chancellor
> wrote:
> > /* If we have an image attached to us, it overrides anything
> > * supplied by the loader. */
> > - if (_initrd_end > _initrd_start) {
> > + if (&_initrd_end > &_initrd_start) {
>
> Are you sure that fix is correct?
>
> extern char _initrd_start[];
> extern char _initrd_end[];
> extern char _esm_blob_start[];
> extern char _esm_blob_end[];
>
> Of course the result of their comparison is a constant, as the addresses
> are constant. If clangs warns about it, perhaps that warning should be moved
> to W=1?
>
> But adding "&" is not correct, according to C.
Why not?
6.5.3.2/3
The unary & operator yields the address of its operand. [...]
Otherwise, the result is a pointer to the object or function designated
by its operand.
This is the same as using the name of an array without anything else,
yes. It is a bit clearer if it would not be declared as array, perhaps,
but it is correct just fine like this.
Segher
Re: [PATCH] powerpc/boot: Use address-of operator on section symbols
On Sat, Jul 18, 2020 at 09:50:50AM +0200, Geert Uytterhoeven wrote:
> Hi Nathan,
>
> On Wed, Jun 24, 2020 at 6:02 AM Nathan Chancellor
> wrote:
> > arch/powerpc/boot/main.c:107:18: warning: array comparison always
> > evaluates to a constant [-Wtautological-compare]
> > if (_initrd_end > _initrd_start) {
> > ^
> > arch/powerpc/boot/main.c:155:20: warning: array comparison always
> > evaluates to a constant [-Wtautological-compare]
> > if (_esm_blob_end <= _esm_blob_start)
> > ^
> > 2 warnings generated.
> >
> > These are not true arrays, they are linker defined symbols, which are
> > just addresses. Using the address of operator silences the warning
> > and does not change the resulting assembly with either clang/ld.lld
> > or gcc/ld (tested with diff + objdump -Dr).
> >
> > Link: https://github.com/ClangBuiltLinux/linux/issues/212
> > Reported-by: Joel Stanley
> > Signed-off-by: Nathan Chancellor
> > ---
> > arch/powerpc/boot/main.c | 4 ++--
> > arch/powerpc/boot/ps3.c | 2 +-
> > 2 files changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/powerpc/boot/main.c b/arch/powerpc/boot/main.c
> > index a9d209135975..cae31a6e8f02 100644
> > --- a/arch/powerpc/boot/main.c
> > +++ b/arch/powerpc/boot/main.c
> > @@ -104,7 +104,7 @@ static struct addr_range prep_initrd(struct addr_range
> > vmlinux, void *chosen,
> > {
> > /* If we have an image attached to us, it overrides anything
> > * supplied by the loader. */
> > - if (_initrd_end > _initrd_start) {
> > + if (&_initrd_end > &_initrd_start) {
> >
>
> Are you sure that fix is correct?
>
> extern char _initrd_start[];
> extern char _initrd_end[];
> extern char _esm_blob_start[];
> extern char _esm_blob_end[];
>
> Of course the result of their comparison is a constant, as the addresses
> are constant. If clangs warns about it, perhaps that warning should be moved
> to W=1?
>
> But adding "&" is not correct, according to C.
>
> Gr{oetje,eeting}s,
>
> Geert
>
Hi Geert,
Yes, I have done fairly extensive testing in the past to verify that
this fix is correct.
For example:
$ cat test.c
#include
extern char _test[];
int main(void)
{
printf("_test: %p\n", _test);
printf("&_test: %p\n", &_test);
return 0;
}
$ cat test.lds
_test = .;
$ clang -Wl,-T test.lds test.c
$ ./a.out
_test: 0x204
&_test: 0x204
$ gcc -fuse-ld=lld -Wl,-T test.lds test.c
$ ./a.out
_test: 0x60a0f76301fb
&_test: 0x60a0f76301fb
I also did runtime verification in QEMU to confirm this is true when I
was testing these commits, which are already present in Linus' tree:
63174f61dfae ("kernel/extable.c: use address-of operator on section symbols")
bf2cbe044da2 ("tracing: Use address-of operator on section symbols")
8306b057a85e ("lib/dynamic_debug.c: use address-of operator on section symbols")
b0d14fc43d39 ("mm/kmemleak.c: use address-of operator on section symbols")
I did a lot of work to get this warning enabled as it can find bugs:
6def1a1d2d58 ("fanotify: Fix the checks in fanotify_fsid_equal")
79ba4f931067 ("IB/hfi1: Fix logical condition in msix_request_irq")
-Wno-tautological-compare disables a bunch of good subwarnings, as I
point out in the commit that enabled it:
afe956c577b2 ("kbuild: Enable -Wtautological-compare")
Cheers,
Nathan
Re: [PATCH] powerpc/boot: Use address-of operator on section symbols
On Thu, Jun 25, 2020 at 6:32 PM 'Nick Desaulniers' via Clang Built Linux wrote: > > On Wed, Jun 24, 2020 at 6:19 PM Geoff Levand wrote: > > > > Hi Nathan, > > > > On 6/23/20 8:59 PM, Nathan Chancellor wrote: > > > These are not true arrays, they are linker defined symbols, which are > > > just addresses. Using the address of operator silences the warning > > > and does not change the resulting assembly with either clang/ld.lld > > > or gcc/ld (tested with diff + objdump -Dr). > > > > Thanks for your patch. I tested this patch applied to v5.8-rc2 on a > > PS3 and it seems OK. > > PS3? Folks still have ones that can boot Linux? Those ers took > my Yellow Dog Linux away from me; I enjoyed depositing that settlement > check! Hopefully by now, folks have figured out how to roll back the > system firmware? I still have the PS3 from Hong Kong with original 1.5 (IIRC) firmware that I demoed at LCA2006. Haven't booted it in at least 12 years, and never used it for games or movies other than the free "Casino Royale" they sent everyone. Arnd
Re: [PATCH] powerpc/boot: Use address-of operator on section symbols
Hi Nathan,
On Wed, Jun 24, 2020 at 6:02 AM Nathan Chancellor
wrote:
> arch/powerpc/boot/main.c:107:18: warning: array comparison always
> evaluates to a constant [-Wtautological-compare]
> if (_initrd_end > _initrd_start) {
> ^
> arch/powerpc/boot/main.c:155:20: warning: array comparison always
> evaluates to a constant [-Wtautological-compare]
> if (_esm_blob_end <= _esm_blob_start)
> ^
> 2 warnings generated.
>
> These are not true arrays, they are linker defined symbols, which are
> just addresses. Using the address of operator silences the warning
> and does not change the resulting assembly with either clang/ld.lld
> or gcc/ld (tested with diff + objdump -Dr).
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/212
> Reported-by: Joel Stanley
> Signed-off-by: Nathan Chancellor
> ---
> arch/powerpc/boot/main.c | 4 ++--
> arch/powerpc/boot/ps3.c | 2 +-
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/boot/main.c b/arch/powerpc/boot/main.c
> index a9d209135975..cae31a6e8f02 100644
> --- a/arch/powerpc/boot/main.c
> +++ b/arch/powerpc/boot/main.c
> @@ -104,7 +104,7 @@ static struct addr_range prep_initrd(struct addr_range
> vmlinux, void *chosen,
> {
> /* If we have an image attached to us, it overrides anything
> * supplied by the loader. */
> - if (_initrd_end > _initrd_start) {
> + if (&_initrd_end > &_initrd_start) {
>
Are you sure that fix is correct?
extern char _initrd_start[];
extern char _initrd_end[];
extern char _esm_blob_start[];
extern char _esm_blob_end[];
Of course the result of their comparison is a constant, as the addresses
are constant. If clangs warns about it, perhaps that warning should be moved
to W=1?
But adding "&" is not correct, according to C.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH] powerpc/boot: Use address-of operator on section symbols
On Tue, 23 Jun 2020 20:59:20 -0700, Nathan Chancellor wrote:
> Clang warns:
>
> arch/powerpc/boot/main.c:107:18: warning: array comparison always
> evaluates to a constant [-Wtautological-compare]
> if (_initrd_end > _initrd_start) {
> ^
> arch/powerpc/boot/main.c:155:20: warning: array comparison always
> evaluates to a constant [-Wtautological-compare]
> if (_esm_blob_end <= _esm_blob_start)
> ^
> 2 warnings generated.
>
> [...]
Applied to powerpc/next.
[1/1] powerpc/boot: Use address-of operator on section symbols
https://git.kernel.org/powerpc/c/df4232d96e724d09e54a623362f9f610727f059f
cheers
Re: [PATCH] powerpc/boot: Use address-of operator on section symbols
On Wed, Jun 24, 2020 at 6:19 PM Geoff Levand wrote: > > Hi Nathan, > > On 6/23/20 8:59 PM, Nathan Chancellor wrote: > > These are not true arrays, they are linker defined symbols, which are > > just addresses. Using the address of operator silences the warning > > and does not change the resulting assembly with either clang/ld.lld > > or gcc/ld (tested with diff + objdump -Dr). > > Thanks for your patch. I tested this patch applied to v5.8-rc2 on a > PS3 and it seems OK. PS3? Folks still have ones that can boot Linux? Those ers took my Yellow Dog Linux away from me; I enjoyed depositing that settlement check! Hopefully by now, folks have figured out how to roll back the system firmware? > > Tested-by: Geoff Levand > > > -- -- Thanks, ~Nick Desaulniers
Re: [PATCH] powerpc/boot: Use address-of operator on section symbols
Hi Geoff, On Wed, Jun 24, 2020 at 06:18:48PM -0700, Geoff Levand wrote: > Hi Nathan, > > On 6/23/20 8:59 PM, Nathan Chancellor wrote: > > These are not true arrays, they are linker defined symbols, which are > > just addresses. Using the address of operator silences the warning > > and does not change the resulting assembly with either clang/ld.lld > > or gcc/ld (tested with diff + objdump -Dr). > > Thanks for your patch. I tested this patch applied to v5.8-rc2 on a > PS3 and it seems OK. > > Tested-by: Geoff Levand Thanks a lot for the quick response and testing, I really appreciate it! Cheers, Nathan
Re: [PATCH] powerpc/boot: Use address-of operator on section symbols
Hi Nathan, On 6/23/20 8:59 PM, Nathan Chancellor wrote: > These are not true arrays, they are linker defined symbols, which are > just addresses. Using the address of operator silences the warning > and does not change the resulting assembly with either clang/ld.lld > or gcc/ld (tested with diff + objdump -Dr). Thanks for your patch. I tested this patch applied to v5.8-rc2 on a PS3 and it seems OK. Tested-by: Geoff Levand
