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

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

[Guenter Roeck]

Copying regzbot.

#regzbot ^introduced 1ec49744ba83
#regzbot title Build failures for sparc64:allmodconfig and parisc:allmodconfig with gcc 11.x+

On Tue, Feb 21, 2023 at 06:59:21PM -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;
> 
> Bisect log attached.
> 
> Guenter
> 
> ---
> # bad: [8bf1a529cd664c8e5268381f1e24fe67aa611dd3] Merge tag 'arm64-upstream' of git://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux
> # good: [c9c3395d5e3dcc6daee66c6908354d47bf98cb0c] Linux 6.2
> git bisect start 'HEAD' 'v6.2'
> # good: [e43efb6d713bca3855909a2f9caec280a2b0a503] dt-bindings: riscv: correct starfive visionfive 2 compatibles
> git bisect good e43efb6d713bca3855909a2f9caec280a2b0a503
> # bad: [1f2d9ffc7a5f916935749ffc6e93fb33bfe94d2f] Merge tag 'sched-core-2023-02-20' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip
> git bisect bad 1f2d9ffc7a5f916935749ffc6e93fb33bfe94d2f
> # bad: [553637f73c314c742243b8dc5ef072e9dadbe581] Merge tag 'for-6.3/dio-2023-02-16' of git://git.kernel.dk/linux
> git bisect bad 553637f73c314c742243b8dc5ef072e9dadbe581
> # good: [274978f173276c5720a3cd8d0b6047d2c0d3a684] Merge tag 'fixes_for_v6.3-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs
> git bisect good 274978f173276c5720a3cd8d0b6047d2c0d3a684
> # bad: [801fcfc5d790f4a9be2897713bd6dd08bed253f1] btrfs: raid56: add a bio_list_put helper
> git bisect bad 801fcfc5d790f4a9be2897713bd6dd08bed253f1
> # bad: [e2fd83064a9bae368ce1c88a0cb9aee64ad4e124] btrfs: skip backref walking during fiemap if we know the leaf is shared
> git bisect bad e2fd83064a9bae368ce1c88a0cb9aee64ad4e124
> # bad: [cb0922f264643f03b04352f7a04abb913c609369] btrfs: don't use size classes for zoned file systems
> git bisect bad cb0922f264643f03b04352f7a04abb913c609369
> # good: [cd30d3bc78d9acbd505d0d6a4cff3b87e40a8187] btrfs: zoned: fix uninitialized variable warning in btrfs_get_dev_zones
> git bisect good cd30d3bc78d9acbd505d0d6a4cff3b87e40a8187
> # bad: [235e1c7b872f9cf16e8a3e6050a05774b8763c58] btrfs: use a single variable to track return value for log_dir_items()
> git bisect bad 235e1c7b872f9cf16e8a3e6050a05774b8763c58
> # bad: [d31de3785047a24959eda835b0bafb1f8629f8a9] btrfs: go to matching label when cleaning em in btrfs_submit_direct
> git bisect bad d31de3785047a24959eda835b0bafb1f8629f8a9
> # bad: [1ec49744ba83f0429c5c706708610f7821a7b6f4] btrfs: turn on -Wmaybe-uninitialized
> git bisect bad 1ec49744ba83f0429c5c706708610f7821a7b6f4
> # good: [a6ca692ec22bdac5ae700e82ff575a1b5141fa40] btrfs: fix uninitialized variable warning in run_one_async_start
> git bisect good a6ca692ec22bdac5ae700e82ff575a1b5141fa40
> # first bad commit: [1ec49744ba83f0429c5c706708610f7821a7b6f4] btrfs: turn on -Wmaybe-uninitialized

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

[Guenter Roeck]

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

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

[Linux regression tracking (Thorsten Leemhuis)]

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

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

[Linux regression tracking #update (Thorsten Leemhuis)]

[TLDR: This mail in primarily relevant for Linux kernel regression
tracking. See link in footer if these mails annoy you.]

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


> 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

Thx for this, Guenter, unfortunately the fix got a different subject
when it was applied. Happens, no worries, but regzbot thus needs to get
told manually:

#regzbot fix: 10a8857a1beaa015efba7d56e06243d484549fb6
#regzbot ignore-activity

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
That page also explains what to do if mails like this annoy you.

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

[David Sterba]

On Sun, Mar 12, 2023 at 02:06:40PM +0100, 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:
> 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?

I'm aware of the warnings and that it's caused by enabling the
-Wmaybe-uninitialized warning. One has a patch, IIRC there are 2-3 more,
so either there's a fix or the commit enabling the warning will be
reverted before 6.3 final.