[PATCH] powerpc/boot: Use address-of operator on section symbols

[PATCH] powerpc/boot: Use address-of operator on section symbols

[Nathan Chancellor]

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.

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 <joel@jms.id.au>
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---
 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) {
 		printf("Attached initrd image at 0x%p-0x%p\n\r",
 		       _initrd_start, _initrd_end);
 		initrd_addr = (unsigned long)_initrd_start;
@@ -152,7 +152,7 @@ static void prep_esm_blob(struct addr_range vmlinux, void *chosen)
 	unsigned long esm_blob_addr, esm_blob_size;
 
 	/* Do we have an ESM (Enter Secure Mode) blob? */
-	if (_esm_blob_end <= _esm_blob_start)
+	if (&_esm_blob_end <= &_esm_blob_start)
 		return;
 
 	printf("Attached ESM blob at 0x%p-0x%p\n\r",
diff --git a/arch/powerpc/boot/ps3.c b/arch/powerpc/boot/ps3.c
index c52552a681c5..6e4efbdb6b7c 100644
--- a/arch/powerpc/boot/ps3.c
+++ b/arch/powerpc/boot/ps3.c
@@ -127,7 +127,7 @@ void platform_init(void)
 	ps3_repository_read_rm_size(&rm_size);
 	dt_fixup_memory(0, rm_size);
 
-	if (_initrd_end > _initrd_start) {
+	if (&_initrd_end > &_initrd_start) {
 		setprop_val(chosen, "linux,initrd-start", (u32)(_initrd_start));
 		setprop_val(chosen, "linux,initrd-end", (u32)(_initrd_end));
 	}

base-commit: 3e08a95294a4fb3702bb3d35ed08028433c37fe6
-- 
2.27.0

Re: [PATCH] powerpc/boot: Use address-of operator on section symbols

[Geoff Levand]

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 <geoff@infradead.org>

Re: [PATCH] powerpc/boot: Use address-of operator on section symbols

[Nathan Chancellor]

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 <geoff@infradead.org>

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

[Nick Desaulniers]

On Wed, Jun 24, 2020 at 6:19 PM Geoff Levand <geoff@infradead.org> 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 <geoff@infradead.org>
>
>
> --

-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] powerpc/boot: Use address-of operator on section symbols

[Michael Ellerman]

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

[Geert Uytterhoeven]

Hi Nathan,

On Wed, Jun 24, 2020 at 6:02 AM Nathan Chancellor
<natechancellor@gmail.com> 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 <joel@jms.id.au>
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> ---
>  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 -- geert@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

[Arnd Bergmann]

On Thu, Jun 25, 2020 at 6:32 PM 'Nick Desaulniers' via Clang Built
Linux <clang-built-linux@googlegroups.com> wrote:
>
> On Wed, Jun 24, 2020 at 6:19 PM Geoff Levand <geoff@infradead.org> 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

[Nathan Chancellor]

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
> <natechancellor@gmail.com> 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 <joel@jms.id.au>
> > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> > ---
> >  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 <stdio.h>

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

[Segher Boessenkool]

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
> <natechancellor@gmail.com> 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

[Geert Uytterhoeven]

Hi Segher,

On Mon, Jul 20, 2020 at 11:03 PM Segher Boessenkool
<segher@kernel.crashing.org> 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
> > <natechancellor@gmail.com> 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 -- geert@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

[Michael Ellerman]

Geert Uytterhoeven <geert@linux-m68k.org> writes:
> On Mon, Jul 20, 2020 at 11:03 PM Segher Boessenkool
> <segher@kernel.crashing.org> 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
>> > <natechancellor@gmail.com> 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

[Geert Uytterhoeven]

Hi Michael,

On Mon, Aug 3, 2020 at 1:09 PM Michael Ellerman <mpe@ellerman.id.au> wrote:
> Geert Uytterhoeven <geert@linux-m68k.org> writes:
> > On Mon, Jul 20, 2020 at 11:03 PM Segher Boessenkool
> > <segher@kernel.crashing.org> 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
> >> > <natechancellor@gmail.com> 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 -- geert@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