| Message ID | 1d9deaa274c13665eca60dee0ccbc4b56b506d06.1671221596.git.josef@toxicpanda.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fixup uninitialized warnings and enable extra checks | expand |
On 2022/12/17 04:15, 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. > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> Reviewed-by: Qu Wenruo <wqu@suse.com> Tested with gcc 12.2.0, no new warnings. Thanks, Qu > --- > fs/btrfs/Makefile | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/fs/btrfs/Makefile b/fs/btrfs/Makefile > index 555c962fdad6..eca995abccdf 100644 > --- a/fs/btrfs/Makefile > +++ b/fs/btrfs/Makefile > @@ -7,6 +7,7 @@ subdir-ccflags-y += -Wmissing-format-attribute > subdir-ccflags-y += -Wmissing-prototypes > subdir-ccflags-y += -Wold-style-definition > subdir-ccflags-y += -Wmissing-include-dirs > +subdir-ccflags-y += -Wmaybe-uninitialized > condflags := \ > $(call cc-option, -Wunused-but-set-variable) \ > $(call cc-option, -Wunused-const-variable) \
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. > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> This needs to be moved to the condflags section, as -Wmaybe-uninitialized is a GCC only flag, so this breaks our builds with clang on next-20221226: error: unknown warning option '-Wmaybe-uninitialized'; did you mean '-Wuninitialized'? [-Werror,-Wunknown-warning-option] I can send a patch on Tuesday unless the original commit can be amended. > --- > fs/btrfs/Makefile | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/fs/btrfs/Makefile b/fs/btrfs/Makefile > index 555c962fdad6..eca995abccdf 100644 > --- a/fs/btrfs/Makefile > +++ b/fs/btrfs/Makefile > @@ -7,6 +7,7 @@ subdir-ccflags-y += -Wmissing-format-attribute > subdir-ccflags-y += -Wmissing-prototypes > subdir-ccflags-y += -Wold-style-definition > subdir-ccflags-y += -Wmissing-include-dirs > +subdir-ccflags-y += -Wmaybe-uninitialized > condflags := \ > $(call cc-option, -Wunused-but-set-variable) \ > $(call cc-option, -Wunused-const-variable) \ > -- > 2.26.3 > >
On Mon, 26 Dec 2022 at 09:47, Nathan Chancellor <nathan@kernel.org> 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. > > > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> > > This needs to be moved to the condflags section, as > -Wmaybe-uninitialized is a GCC only flag, so this breaks our builds with > clang on next-20221226: > > error: unknown warning option '-Wmaybe-uninitialized'; did you mean '-Wuninitialized'? [-Werror,-Wunknown-warning-option] LKFT test farm also noticed these build breaks with clang on next-20221226. Regressions found on arm64, riscv, s390 and powerpc - build/clang-lkftconfig - build/clang-15-lkftconfig - build/clang-15-defconfig-40bc7ee5 - build/clang-15-defconfig - build/clang-nightly-defconfig - build/clang-15-lkftconfig-compat - build/clang-nightly-defconfig-40bc7ee5 - build/clang-nightly-lkftconfig make --silent --keep-going --jobs=8 O=/home/tuxbuild/.cache/tuxmake/builds/1/build LLVM=1 LLVM_IAS=1 ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- 'HOSTCC=sccache clang' 'CC=sccache clang' error: unknown warning option '-Wmaybe-uninitialized'; did you mean '-Wuninitialized'? [-Werror,-Wunknown-warning-option] make[4]: *** [/builds/linux/scripts/Makefile.build:252: fs/btrfs/super.o] Error 1 > I can send a patch on Tuesday unless the original commit can be amended. ref: https://qa-reports.linaro.org/lkft/linux-next-master/build/next-20221226/testrun/13818773/suite/build/test/clang-15-defconfig/details/ - Naresh
On Mon, Dec 26, 2022 at 07:34:51PM +0530, Naresh Kamboju wrote: > On Mon, 26 Dec 2022 at 09:47, Nathan Chancellor <nathan@kernel.org> 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. > > > > > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> > > > > This needs to be moved to the condflags section, as > > -Wmaybe-uninitialized is a GCC only flag, so this breaks our builds with > > clang on next-20221226: > > > > error: unknown warning option '-Wmaybe-uninitialized'; did you mean '-Wuninitialized'? [-Werror,-Wunknown-warning-option] > > LKFT test farm also noticed these build breaks with clang on next-20221226. Fixed in our branch and for-next snapshot updated so the warnings will be fixed once linux-next resumes.
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
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.
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
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
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
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
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
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.
[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.
diff --git a/fs/btrfs/Makefile b/fs/btrfs/Makefile index 555c962fdad6..eca995abccdf 100644 --- a/fs/btrfs/Makefile +++ b/fs/btrfs/Makefile @@ -7,6 +7,7 @@ subdir-ccflags-y += -Wmissing-format-attribute subdir-ccflags-y += -Wmissing-prototypes subdir-ccflags-y += -Wold-style-definition subdir-ccflags-y += -Wmissing-include-dirs +subdir-ccflags-y += -Wmaybe-uninitialized condflags := \ $(call cc-option, -Wunused-but-set-variable) \ $(call cc-option, -Wunused-const-variable) \
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. Signed-off-by: Josef Bacik <josef@toxicpanda.com> --- fs/btrfs/Makefile | 1 + 1 file changed, 1 insertion(+)