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