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

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 <redacted>
---
 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

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>

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:

quoted

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

On Wed, Jun 24, 2020 at 6:19 PM Geoff Levand [off-list ref] wrote:

Hi Nathan,

On 6/23/20 8:59 PM, Nathan Chancellor wrote:

quoted

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

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

Hi Nathan,

On Wed, Jun 24, 2020 at 6:02 AM Nathan Chancellor
[off-list ref] wrote:

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

On Thu, Jun 25, 2020 at 6:32 PM 'Nick Desaulniers' via Clang Built
Linux [off-list ref] wrote:

On Wed, Jun 24, 2020 at 6:19 PM Geoff Levand [off-list ref] wrote:

quoted

Hi Nathan,

On 6/23/20 8:59 PM, Nathan Chancellor wrote:

quoted

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

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
[off-list ref] wrote:

quoted

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 <redacted>
---
 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

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
[off-list ref] wrote:

quoted

        /* 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

Hi Segher,

On Mon, Jul 20, 2020 at 11:03 PM Segher Boessenkool
[off-list ref] wrote:

On Sat, Jul 18, 2020 at 09:50:50AM +0200, Geert Uytterhoeven wrote:

quoted

On Wed, Jun 24, 2020 at 6:02 AM Nathan Chancellor
[off-list ref] wrote:

quoted

        /* 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

Geert Uytterhoeven [off-list ref] writes:

On Mon, Jul 20, 2020 at 11:03 PM Segher Boessenkool
[off-list ref] wrote:

quoted

On Sat, Jul 18, 2020 at 09:50:50AM +0200, Geert Uytterhoeven wrote:

quoted

On Wed, Jun 24, 2020 at 6:02 AM Nathan Chancellor
[off-list ref] wrote:

quoted

        /* 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

Hi Michael,

On Mon, Aug 3, 2020 at 1:09 PM Michael Ellerman [off-list ref] wrote:

Geert Uytterhoeven [off-list ref] writes:

quoted

On Mon, Jul 20, 2020 at 11:03 PM Segher Boessenkool
[off-list ref] wrote:

quoted

On Sat, Jul 18, 2020 at 09:50:50AM +0200, Geert Uytterhoeven wrote:

quoted

On Wed, Jun 24, 2020 at 6:02 AM Nathan Chancellor
[off-list ref] wrote:

quoted

        /* 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