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

["Linux regression tracking (Thorsten Leemhuis)" <regressions@leemhuis.info>]

On 12.03.23 15:37, Guenter Roeck wrote:
> 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

Ahh, great, thx.

Ciao, Thorsten