Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190221222123.GC6474@magnolia> <alpine.LSU.2.11.1902222222570.1594@eggly.anvils>
 <CAHk-=wgO3MPjPpf_ARyW6zpwwPZtxXYQgMLbmj2bnbOLnR+6Cg@mail.gmail.com> <alpine.LSU.2.11.1902251214220.8973@eggly.anvils>
In-Reply-To: <alpine.LSU.2.11.1902251214220.8973@eggly.anvils>
From:   Linus Torvalds <torvalds@linux-foundation.org>
Date:   Mon, 25 Feb 2019 14:34:47 -0800
Message-ID: <CAHk-=whP-9yPAWuJDwA6+rQ-9owuYZgmrMA9AqO3EGJVefe8vg@mail.gmail.com>
Subject: Re: [PATCH] tmpfs: fix uninitialized return value in shmem_link
To:     Hugh Dickins <hughd@google.com>, Jan Hubicka <hubicka@ucw.cz>,
        rguenth@gcc.gnu.org
Cc:     "Darrick J. Wong" <darrick.wong@oracle.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Matej Kupljen <matej.kupljen@gmail.com>,
        Al Viro <viro@zeniv.linux.org.uk>,
        Dan Carpenter <dan.carpenter@oracle.com>,
        Linux List Kernel Mailing <linux-kernel@vger.kernel.org>,
        linux-fsdevel <linux-fsdevel@vger.kernel.org>,
        Linux-MM <linux-mm@kvack.org>
Content-Type: multipart/mixed; boundary="0000000000003feded0582bf8d04"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

--0000000000003feded0582bf8d04
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Mon, Feb 25, 2019 at 12:34 PM Hugh Dickins <hughd@google.com> wrote:
>
> Seems like a gcc bug? But I don't have a decent recent gcc to hand
> to submit a proper report, hope someone else can shed light on it.

I don't have a _very_ recent gcc either, but with gcc-8.2.1 the
attached test-case gives me:

   [torvalds@i7 ~]$ gcc -O2 -S -Wall test.c

with no warning, and then

   [torvalds@i7 ~]$ gcc -O2 -S -Wall -DHIDE_PROBLEM test.c
   test.c: In function =E2=80=98shmem_link=E2=80=99:
   test.c:60:9: warning: =E2=80=98ret=E2=80=99 may be used uninitialized in=
 this
function [-Wmaybe-uninitialized]
     return ret;
            ^~~

*does* show the expected warning.

So it is the presence of that

      if (ret) return ret;

that suppresses the warning.

What I *suspect* happens is

 (a) gcc sees that there is only one assignment to "ret"

 (b) in the same basic block as the assignment, there is a test
against "ret" being nonzero that goes out.

and what I think happens is that (a) causes gcc to consider that
assignment to be the defining assignment (which makes all kinds of
sense in an SSA world), and then (b) means that gcc decides that
clearly "ret" has to be zero in any case that doesn't go out due to
the if-test.

In fact, if I then look at the code generation, gcc will actually do
this (edited to be more legible):

        movl    (%rbx), %eax       <- load inode->i_nlink
        testl   %eax, %eax
        je      .L1
       ...
       ...
        call    d_instantiate
        xorl    %eax, %eax     <- explicitly zero 'ret'!
.L1:
        popq    %rbx
        popq    %rbp
        popq    %r12
        ret

so at least with my compiler, it *effectively* zeroed ret (in %rax)
anyway, and it all just _happened_ to get the right result even though
'ret' wasn't actually initialized.

Which is why it all worked just fine. And depending on how gcc works
internally, it really may not just be a random mistake of register
allocation, but really because gcc kind of _thought_ that 'ret' was
zero-initialized due to the combination of the one single assigment
and test for zero.

So it turns out that the patch to initialize to zero doesn't do
anything, probably for the same reason that gcc didn't warn about the
missing initialization. Gcc kind of added an initialization of its own
there.

I'm not entirely sure if any gcc developer would be interested in this
as a test-case, but I guess I can try to do a bugzilla.

Adding a few gcc people who have been on previous kernel gcc bugzilla
discussions, just in case they have something to add.

The gcc bugzilla is this:

    https://gcc.gnu.org/bugzilla/show_bug.cgi?id=3D89501

and I tried to make it be self-explanatory, but I wrote the bugzilla
in parallel with this email, and maybe there's some missing context
either there (or here).

                 Linus

--0000000000003feded0582bf8d04
Content-Type: text/x-c-code; charset="US-ASCII"; name="test.c"
Content-Disposition: attachment; filename="test.c"
Content-Transfer-Encoding: base64
Content-ID: <f_jskwlqay0>
X-Attachment-Id: f_jskwlqay0

LyoKICogTWluaW1hbCBmYWtlIGRlY2xhcmF0aW9ucyBvZiAia2VybmVsIiBkYXRhIHR5cGVzCiAq
LwpzdHJ1Y3Qgc3VwZXJibG9jazsKCnN0cnVjdCBpbm9kZSB7CglpbnQgaV9ubGluazsKCWludCBp
X3NpemU7CglpbnQgaV9jdGltZTsKCWludCBpX210aW1lOwoJc3RydWN0IHN1cGVyYmxvY2sgKmlf
c2I7Cn07CgpzdHJ1Y3QgZGVudHJ5IHsKCXN0cnVjdCBpbm9kZSAqZF9pbm9kZTsKfTsKCiNkZWZp
bmUgZF9pbm9kZShkZW50cnkpICgoZGVudHJ5KS0+ZF9pbm9kZSkKCmV4dGVybiBpbnQgY3VycmVu
dF90aW1lKHN0cnVjdCBpbm9kZSAqKTsKZXh0ZXJuIHZvaWQgaW5jX25saW5rKHN0cnVjdCBpbm9k
ZSAqKTsKZXh0ZXJuIHZvaWQgaWhvbGQoc3RydWN0IGlub2RlICopOwpleHRlcm4gdm9pZCBkZ2V0
KHN0cnVjdCBkZW50cnkgKik7CmV4dGVybiB2b2lkIGRfaW5zdGFudGlhdGUoc3RydWN0IGRlbnRy
eSAqLCBzdHJ1Y3QgaW5vZGUgKik7CmV4dGVybiBpbnQgc2htZW1fcmVzZXJ2ZV9pbm9kZShzdHJ1
Y3Qgc3VwZXJibG9jayAqKTsKCiNkZWZpbmUgQk9HT19ESVJFTlRfU0laRSAyMAoKLyoKICogVGhl
IGFjdHVhbCBmdW5jdGlvbiB3aGVyZSBJJ2QgaGF2ZSBleHBlY3RlZCBhIHdhcm5pbmcKICogYWJv
dXQgInJldCBtaWdodCBiZSB1c2VkIHVuaW5pdGlhbGl6ZWQiCiAqLwppbnQgc2htZW1fbGluayhz
dHJ1Y3QgZGVudHJ5ICpvbGRfZGVudHJ5LCBzdHJ1Y3QgaW5vZGUgKmRpciwKCQkgICAgICBzdHJ1
Y3QgZGVudHJ5ICpkZW50cnkpCnsKCXN0cnVjdCBpbm9kZSAqaW5vZGUgPSBkX2lub2RlKG9sZF9k
ZW50cnkpOwoJaW50IHJldDsKCgkvKgoJICogTm8gb3JkaW5hcnkgKGRpc2sgYmFzZWQpIGZpbGVz
eXN0ZW0gY291bnRzIGxpbmtzIGFzIGlub2RlczsKCSAqIGJ1dCBlYWNoIG5ldyBsaW5rIG5lZWRz
IGEgbmV3IGRlbnRyeSwgcGlubmluZyBsb3dtZW0sIGFuZAoJICogdG1wZnMgZGVudHJpZXMgY2Fu
bm90IGJlIHBydW5lZCB1bnRpbCB0aGV5IGFyZSB1bmxpbmtlZC4KCSAqIEJ1dCBpZiBhbiBPX1RN
UEZJTEUgZmlsZSBpcyBsaW5rZWQgaW50byB0aGUgdG1wZnMsIHRoZQoJICogZmlyc3QgbGluayBt
dXN0IHNraXAgdGhhdCwgdG8gZ2V0IHRoZSBhY2NvdW50aW5nIHJpZ2h0LgoJICovCglpZiAoaW5v
ZGUtPmlfbmxpbmspIHsKCQlyZXQgPSBzaG1lbV9yZXNlcnZlX2lub2RlKGlub2RlLT5pX3NiKTsK
I2lmbmRlZiBISURFX1BST0JMRU0KCQlpZiAocmV0KQoJCQlyZXR1cm4gcmV0OwojZW5kaWYKCX0K
CglkaXItPmlfc2l6ZSArPSBCT0dPX0RJUkVOVF9TSVpFOwoJaW5vZGUtPmlfY3RpbWUgPSBkaXIt
PmlfY3RpbWUgPSBkaXItPmlfbXRpbWUgPSBjdXJyZW50X3RpbWUoaW5vZGUpOwoJaW5jX25saW5r
KGlub2RlKTsKCWlob2xkKGlub2RlKTsJCS8qIE5ldyBkZW50cnkgcmVmZXJlbmNlICovCglkZ2V0
KGRlbnRyeSk7CQkvKiBFeHRyYSBwaW5uaW5nIGNvdW50IGZvciB0aGUgY3JlYXRlZCBkZW50cnkg
Ki8KCWRfaW5zdGFudGlhdGUoZGVudHJ5LCBpbm9kZSk7CglyZXR1cm4gcmV0Owp9Cg==
--0000000000003feded0582bf8d04--