Re: [PATCH 8/8] btrfs: turn on -Wmaybe-uninitialized

[Guenter Roeck <linux@roeck-us.net>]

On 2/22/23 08:38, David Sterba wrote:
> On Tue, Feb 21, 2023 at 06:59:18PM -0800, Guenter Roeck wrote:
>> On Fri, Dec 16, 2022 at 03:15:58PM -0500, Josef Bacik wrote:
>>> We had a recent bug that would have been caught by a newer compiler with
>>> -Wmaybe-uninitialized and would have saved us a month of failing tests
>>> that I didn't have time to investigate.
>>>
>>
>> Thanks to this patch, sparc64:allmodconfig and parisc:allmodconfig now
>> fail to commpile with the following error when trying to build images
>> with gcc 11.3.
>>
>> Building sparc64:allmodconfig ... failed
>> --------------
>> Error log:
>> <stdin>:1517:2: warning: #warning syscall clone3 not implemented [-Wcpp]
>> fs/btrfs/inode.c: In function 'btrfs_lookup_dentry':
>> fs/btrfs/inode.c:5730:21: error: 'location.type' may be used uninitialized [-Werror=maybe-uninitialized]
>>   5730 |         if (location.type == BTRFS_INODE_ITEM_KEY) {
>>        |             ~~~~~~~~^~~~~
>> fs/btrfs/inode.c:5719:26: note: 'location' declared here
>>   5719 |         struct btrfs_key location;
> 
> Thanks for the report, Linus warned me that there might be some fallouts
> and that the warning flag might need reverted. But before I do that I'd
> like to try to understand why the warnings happen in a code where is no
> reason for it.
> 
> I did a quick test on gcc 11.3 (on x86_64, not on sparc64 unlike you
> report), and there is no warning
> 
> gcc (SUSE Linux) 11.3.1 20220721 [revision a55184ada8e2887ca94c0ab07027617885beafc9]
> Copyright (C) 2021 Free Software Foundation, Inc.
> This is free software; see the source for copying conditions.  There is NO
> warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
> 
>    DESCEND objtool
>    CALL    scripts/checksyscalls.sh
>    CC [M]  fs/btrfs/inode.o
> 
> I.e. it's the same version, different arch and likely not the same
> config. In the function itself thre's a local variable passed by address
> to a static function in the same file.
> 
> 	struct btrfs_key location;
> 	...
> 	ret = btrfs_inode_by_name(BTRFS_I(dir), dentry, &location, &di_type);
> 
> and there it's
> 
> 	btrfs_dir_item_key_to_cpu(path->nodes[0], di, location);
> 
> which is a series of helpers to read some data and store that to the
> strucutre. At some point there's a call to read_extent_buffer() that's
> in a different file.
> 
> A local variable passed by address to external function is quite common
> so I'd expect more warnings and I don't see what's different in this
> case.

Me not either. I also don't see the problem with other architectures, only
with sparc and parisc. It doesn't have to be gcc 11.3, though; it also happens
with gcc 11.1, 11.2, 12.1, and 12.2 (tested on sparc).

Too bad that gcc doesn't tell why exactly it believes that the object
may be uninitialized. Anyway, the following change would fix the problem.

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 6c18dc9a1831..4bab8ab39948 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -5421,7 +5421,7 @@ static int btrfs_inode_by_name(struct btrfs_inode *dir, struct dentry *dentry,
                 return -ENOMEM;

         ret = fscrypt_setup_filename(&dir->vfs_inode, &dentry->d_name, 1, &fname);
-       if (ret)
+       if (ret < 0)
                 goto out;

Presumably gcc assumes that fscrypt_setup_filename() could return
a positive value.

Guenter