* [PATCH] btrfs: make ASSERT no-op in release builds @ 2025-10-30 18:23 Gladyshev Ilya 2025-10-30 21:05 ` Qu Wenruo 2025-10-31 12:18 ` kernel test robot 0 siblings, 2 replies; 5+ messages in thread From: Gladyshev Ilya @ 2025-10-30 18:23 UTC (permalink / raw) To: foxido; +Cc: Chris Mason, David Sterba, linux-btrfs, linux-kernel The current definition of `ASSERT(cond)` as `(void)(cond)` is redundant, because all checks are without side effects and don't affect code logic. However, some checks has READ_ONCE in them or other 'compiler-unfriendly' behaviour. For example, ASSERT(list_empty) in btrfs_add_dealloc_inode was compiled to redundant mov because of this. This patch replaces ASSERT with BUILD_BUG_ON_INVALID for !CONFIG_BTRFS_ASSERT builds. Signed-off-by: Gladyshev Ilya <foxido@foxido.dev> --- .o size reductions are not that big, for example on defconfig + btrfs fs/btrfs/*.o size went from 3280528 to 3277936, so compiler was pretty efficient on his own --- fs/btrfs/messages.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/btrfs/messages.h b/fs/btrfs/messages.h index 4416c165644f..f80fe40a2c2b 100644 --- a/fs/btrfs/messages.h +++ b/fs/btrfs/messages.h @@ -168,7 +168,7 @@ do { \ #endif #else -#define ASSERT(cond, args...) (void)(cond) +#define ASSERT(cond, args...) BUILD_BUG_ON_INVALID(cond) #endif #ifdef CONFIG_BTRFS_DEBUG base-commit: e53642b87a4f4b03a8d7e5f8507fc3cd0c595ea6 -- 2.51.1.dirty ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] btrfs: make ASSERT no-op in release builds 2025-10-30 18:23 [PATCH] btrfs: make ASSERT no-op in release builds Gladyshev Ilya @ 2025-10-30 21:05 ` Qu Wenruo 2025-10-31 1:08 ` David Sterba 2025-10-31 12:18 ` kernel test robot 1 sibling, 1 reply; 5+ messages in thread From: Qu Wenruo @ 2025-10-30 21:05 UTC (permalink / raw) To: Gladyshev Ilya; +Cc: Chris Mason, David Sterba, linux-btrfs, linux-kernel 在 2025/10/31 04:53, Gladyshev Ilya 写道: > The current definition of `ASSERT(cond)` as `(void)(cond)` is redundant, > because all checks are without side effects and don't affect code logic. > > However, some checks has READ_ONCE in them or other 'compiler-unfriendly' > behaviour. For example, ASSERT(list_empty) in btrfs_add_dealloc_inode > was compiled to redundant mov because of this. > > This patch replaces ASSERT with BUILD_BUG_ON_INVALID for > !CONFIG_BTRFS_ASSERT builds. > > Signed-off-by: Gladyshev Ilya <foxido@foxido.dev> > > --- > .o size reductions are not that big, for example on defconfig + btrfs > fs/btrfs/*.o size went from 3280528 to 3277936, so compiler was pretty > efficient on his own > --- > fs/btrfs/messages.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/btrfs/messages.h b/fs/btrfs/messages.h > index 4416c165644f..f80fe40a2c2b 100644 > --- a/fs/btrfs/messages.h > +++ b/fs/btrfs/messages.h > @@ -168,7 +168,7 @@ do { \ > #endif > > #else > -#define ASSERT(cond, args...) (void)(cond) > +#define ASSERT(cond, args...) BUILD_BUG_ON_INVALID(cond) And I do not think it's a good idea to use BUILD_BUG_ON_INVALID() here, most ASSERT()s are checking runtime conditions, I understand you want to avoid real code generation, but in that case there are more straightforward solutions, like "do {} while (0)" as no-op. Thanks, Qu > #endif > > #ifdef CONFIG_BTRFS_DEBUG > > base-commit: e53642b87a4f4b03a8d7e5f8507fc3cd0c595ea6 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] btrfs: make ASSERT no-op in release builds 2025-10-30 21:05 ` Qu Wenruo @ 2025-10-31 1:08 ` David Sterba 0 siblings, 0 replies; 5+ messages in thread From: David Sterba @ 2025-10-31 1:08 UTC (permalink / raw) To: Qu Wenruo Cc: Gladyshev Ilya, Chris Mason, David Sterba, linux-btrfs, linux-kernel On Fri, Oct 31, 2025 at 07:35:29AM +1030, Qu Wenruo wrote: > 在 2025/10/31 04:53, Gladyshev Ilya 写道: > > The current definition of `ASSERT(cond)` as `(void)(cond)` is redundant, > > because all checks are without side effects and don't affect code logic. > > > > However, some checks has READ_ONCE in them or other 'compiler-unfriendly' > > behaviour. For example, ASSERT(list_empty) in btrfs_add_dealloc_inode > > was compiled to redundant mov because of this. > > > > This patch replaces ASSERT with BUILD_BUG_ON_INVALID for > > !CONFIG_BTRFS_ASSERT builds. > > > > Signed-off-by: Gladyshev Ilya <foxido@foxido.dev> > > > > --- > > .o size reductions are not that big, for example on defconfig + btrfs > > fs/btrfs/*.o size went from 3280528 to 3277936, so compiler was pretty > > efficient on his own > > --- > > fs/btrfs/messages.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/fs/btrfs/messages.h b/fs/btrfs/messages.h > > index 4416c165644f..f80fe40a2c2b 100644 > > --- a/fs/btrfs/messages.h > > +++ b/fs/btrfs/messages.h > > @@ -168,7 +168,7 @@ do { \ > > #endif > > > > #else > > -#define ASSERT(cond, args...) (void)(cond) > > +#define ASSERT(cond, args...) BUILD_BUG_ON_INVALID(cond) > > And I do not think it's a good idea to use BUILD_BUG_ON_INVALID() here, > most ASSERT()s are checking runtime conditions, I understand you want to > avoid real code generation, but in that case there are more > straightforward solutions, like "do {} while (0)" as no-op. It's supposed to be no-op but also compile checked, so the do/while(0) will not do that. What BUILD_BUG_ON_INVALID is basically a sizeof(cond) so it's the right thing but the naming is confusing, we can possibly open code it. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] btrfs: make ASSERT no-op in release builds 2025-10-30 18:23 [PATCH] btrfs: make ASSERT no-op in release builds Gladyshev Ilya 2025-10-30 21:05 ` Qu Wenruo @ 2025-10-31 12:18 ` kernel test robot 2025-10-31 20:37 ` Nathan Chancellor 1 sibling, 1 reply; 5+ messages in thread From: kernel test robot @ 2025-10-31 12:18 UTC (permalink / raw) To: Gladyshev Ilya Cc: llvm, oe-kbuild-all, Chris Mason, David Sterba, linux-btrfs, linux-kernel Hi Gladyshev, kernel test robot noticed the following build warnings: [auto build test WARNING on e53642b87a4f4b03a8d7e5f8507fc3cd0c595ea6] url: https://github.com/intel-lab-lkp/linux/commits/Gladyshev-Ilya/btrfs-make-ASSERT-no-op-in-release-builds/20251031-024059 base: e53642b87a4f4b03a8d7e5f8507fc3cd0c595ea6 patch link: https://lore.kernel.org/r/20251030182322.4085697-1-foxido%40foxido.dev patch subject: [PATCH] btrfs: make ASSERT no-op in release builds config: x86_64-kexec (https://download.01.org/0day-ci/archive/20251031/202510311956.w2iYoQcn-lkp@intel.com/config) compiler: clang version 20.1.8 (https://github.com/llvm/llvm-project 87f0227cb60147a26a1eeb4fb06e3b505e9c7261) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251031/202510311956.w2iYoQcn-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202510311956.w2iYoQcn-lkp@intel.com/ All warnings (new ones prefixed by >>): >> fs/btrfs/raid56.c:302:13: warning: function 'full_page_sectors_uptodate' is not needed and will not be emitted [-Wunneeded-internal-declaration] 302 | static bool full_page_sectors_uptodate(struct btrfs_raid_bio *rbio, | ^~~~~~~~~~~~~~~~~~~~~~~~~~ 1 warning generated. vim +/full_page_sectors_uptodate +302 fs/btrfs/raid56.c 53b381b3abeb86 David Woodhouse 2013-01-29 301 d4e28d9b5f04d8 Qu Wenruo 2022-04-01 @302 static bool full_page_sectors_uptodate(struct btrfs_raid_bio *rbio, d4e28d9b5f04d8 Qu Wenruo 2022-04-01 303 unsigned int page_nr) d4e28d9b5f04d8 Qu Wenruo 2022-04-01 304 { d4e28d9b5f04d8 Qu Wenruo 2022-04-01 305 const u32 sectorsize = rbio->bioc->fs_info->sectorsize; d4e28d9b5f04d8 Qu Wenruo 2022-04-01 306 const u32 sectors_per_page = PAGE_SIZE / sectorsize; d4e28d9b5f04d8 Qu Wenruo 2022-04-01 307 int i; d4e28d9b5f04d8 Qu Wenruo 2022-04-01 308 d4e28d9b5f04d8 Qu Wenruo 2022-04-01 309 ASSERT(page_nr < rbio->nr_pages); d4e28d9b5f04d8 Qu Wenruo 2022-04-01 310 d4e28d9b5f04d8 Qu Wenruo 2022-04-01 311 for (i = sectors_per_page * page_nr; d4e28d9b5f04d8 Qu Wenruo 2022-04-01 312 i < sectors_per_page * page_nr + sectors_per_page; d4e28d9b5f04d8 Qu Wenruo 2022-04-01 313 i++) { d4e28d9b5f04d8 Qu Wenruo 2022-04-01 314 if (!rbio->stripe_sectors[i].uptodate) d4e28d9b5f04d8 Qu Wenruo 2022-04-01 315 return false; d4e28d9b5f04d8 Qu Wenruo 2022-04-01 316 } d4e28d9b5f04d8 Qu Wenruo 2022-04-01 317 return true; d4e28d9b5f04d8 Qu Wenruo 2022-04-01 318 } d4e28d9b5f04d8 Qu Wenruo 2022-04-01 319 -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] btrfs: make ASSERT no-op in release builds 2025-10-31 12:18 ` kernel test robot @ 2025-10-31 20:37 ` Nathan Chancellor 0 siblings, 0 replies; 5+ messages in thread From: Nathan Chancellor @ 2025-10-31 20:37 UTC (permalink / raw) To: kernel test robot Cc: Gladyshev Ilya, llvm, oe-kbuild-all, Chris Mason, David Sterba, linux-btrfs, linux-kernel On Fri, Oct 31, 2025 at 08:18:50PM +0800, kernel test robot wrote: > Hi Gladyshev, > > kernel test robot noticed the following build warnings: > > [auto build test WARNING on e53642b87a4f4b03a8d7e5f8507fc3cd0c595ea6] > > url: https://github.com/intel-lab-lkp/linux/commits/Gladyshev-Ilya/btrfs-make-ASSERT-no-op-in-release-builds/20251031-024059 > base: e53642b87a4f4b03a8d7e5f8507fc3cd0c595ea6 > patch link: https://lore.kernel.org/r/20251030182322.4085697-1-foxido%40foxido.dev > patch subject: [PATCH] btrfs: make ASSERT no-op in release builds > config: x86_64-kexec (https://download.01.org/0day-ci/archive/20251031/202510311956.w2iYoQcn-lkp@intel.com/config) > compiler: clang version 20.1.8 (https://github.com/llvm/llvm-project 87f0227cb60147a26a1eeb4fb06e3b505e9c7261) > reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251031/202510311956.w2iYoQcn-lkp@intel.com/reproduce) > > If you fix the issue in a separate patch/commit (i.e. not just a new version of > the same patch/commit), kindly add following tags > | Reported-by: kernel test robot <lkp@intel.com> > | Closes: https://lore.kernel.org/oe-kbuild-all/202510311956.w2iYoQcn-lkp@intel.com/ > > All warnings (new ones prefixed by >>): > > >> fs/btrfs/raid56.c:302:13: warning: function 'full_page_sectors_uptodate' is not needed and will not be emitted [-Wunneeded-internal-declaration] > 302 | static bool full_page_sectors_uptodate(struct btrfs_raid_bio *rbio, > | ^~~~~~~~~~~~~~~~~~~~~~~~~~ > 1 warning generated. Just in case it is not obvious: full_page_sectors_uptodate() is only called within an ASSERT() macro, so after this change, it is only referenced within sizeof(), so it won't be emitted in .text (which may be a bug). Presumably that is expected in this case, so I would recommend marking this as __maybe_unused to avoid the warning. Cheers, Nathan ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2025-10-31 20:37 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-10-30 18:23 [PATCH] btrfs: make ASSERT no-op in release builds Gladyshev Ilya 2025-10-30 21:05 ` Qu Wenruo 2025-10-31 1:08 ` David Sterba 2025-10-31 12:18 ` kernel test robot 2025-10-31 20:37 ` Nathan Chancellor
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox