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

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

On 3/12/23 06:06, Linux regression tracking (Thorsten Leemhuis) wrote:
> On 22.02.23 18:18, Guenter Roeck wrote:
>> 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.
> 
> This discussion seems to have stalled, but from a kernelci report it
> looks like above warning still happens:
> https://lore.kernel.org/all/640bceb7.a70a0220.af8cd.146b@mx.google.com/
> 
> @btrfs developers, do you still have it on your radar?
> 
> Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
> --
> Everything you wanna know about Linux kernel regression tracking:
> https://linux-regtracking.leemhuis.info/about/#tldr
> If I did something stupid, please tell me, as explained on that page.
> 
> #regzbot poke

There was a patch:

#regzbot monitor: https://patchwork.kernel.org/project/linux-btrfs/patch/dc091588d770923c3afe779e1eb512724662db71.1678290988.git.sweettea-kernel@dorminy.me/
#regzbot fix: btrfs: fix compilation error on sparc/parisc
#regzbot ignore-activity

Guenter