* Re: linux-next: Tree for Dec 6 (objtool, lots in btrfs) [not found] <20191206135406.563336e7@canb.auug.org.au> @ 2019-12-06 16:17 ` Randy Dunlap 2019-12-11 13:49 ` David Sterba 0 siblings, 1 reply; 18+ messages in thread From: Randy Dunlap @ 2019-12-06 16:17 UTC (permalink / raw) To: Stephen Rothwell, Linux Next Mailing List Cc: Linux Kernel Mailing List, Linux Btrfs On 12/5/19 6:54 PM, Stephen Rothwell wrote: > Hi all, > > Please do not add any material for v5.6 to your linux-next included > trees until after v5.5-rc1 has been released. > > Changes since 20191204: > on x86_64: fs/btrfs/ctree.o: warning: objtool: btrfs_search_slot()+0x2d4: unreachable instruction fs/btrfs/extent-tree.o: warning: objtool: btrfs_get_extent_inline_ref_type()+0x161: unreachable instruction fs/btrfs/disk-io.o: warning: objtool: btrfs_init_fs_root()+0x1d9: unreachable instruction fs/btrfs/transaction.o: warning: objtool: btrfs_trans_release_metadata()+0x96: unreachable instruction fs/btrfs/inode.o: warning: objtool: btrfs_retry_endio()+0x77: unreachable instruction fs/btrfs/file.o: warning: objtool: __btrfs_drop_extents()+0x4aa: unreachable instruction fs/btrfs/extent_map.o: warning: objtool: mergable_maps()+0x100: unreachable instruction fs/btrfs/xattr.o: warning: objtool: btrfs_setxattr()+0x86: unreachable instruction fs/btrfs/extent_io.o: warning: objtool: __process_pages_contig()+0x1af: unreachable instruction fs/btrfs/volumes.o: warning: objtool: find_live_mirror.isra.23()+0x49: unreachable instruction fs/btrfs/ioctl.o: warning: objtool: btrfs_clone()+0xcc9: unreachable instruction fs/btrfs/free-space-cache.o: warning: objtool: search_bitmap()+0xca: unreachable instruction fs/btrfs/tree-log.o: warning: objtool: btrfs_log_all_xattrs()+0x204: unreachable instruction fs/btrfs/compression.o: warning: objtool: end_compressed_bio_read()+0xb6: unreachable instruction fs/btrfs/delayed-ref.o: warning: objtool: insert_delayed_ref.isra.6()+0x23e: unreachable instruction fs/btrfs/relocation.o: warning: objtool: add_tree_block.isra.26()+0x26e: unreachable instruction fs/btrfs/scrub.o: warning: objtool: scrub_find_csum()+0x1eb: unreachable instruction fs/btrfs/send.o: warning: objtool: inconsistent_snapshot_error()+0x53: unreachable instruction fs/btrfs/dev-replace.o: warning: objtool: btrfs_dev_replace_by_ioctl()+0x70e: unreachable instruction fs/btrfs/raid56.o: warning: objtool: raid56_parity_recover()+0x262: unreachable instruction fs/btrfs/free-space-tree.o: warning: objtool: btrfs_search_prev_slot.constprop.6()+0x2b: unreachable instruction fs/btrfs/block-group.o: warning: objtool: btrfs_start_trans_remove_block_group()+0x6a: unreachable instruction fs/btrfs/ref-verify.o: warning: objtool: add_tree_block()+0x15e: unreachable instruction samples/ftrace/ftrace-direct.o: warning: objtool: .text+0x0: unreachable instruction samples/ftrace/ftrace-direct-too.o: warning: objtool: .text+0x0: unreachable instruction samples/ftrace/ftrace-direct-modify.o: warning: objtool: .text+0x0: unreachable instruction kernel/exit.o: warning: objtool: __x64_sys_exit_group()+0x21: unreachable instruction kernel/cred.o: warning: objtool: prepare_creds()+0x2c3: unreachable instruction net/core/skbuff.o: warning: objtool: skb_push()+0x6d: unreachable instruction drivers/gpu/drm/ttm/ttm_bo.o: warning: objtool: ttm_bo_del_from_lru()+0xed: unreachable instruction gcc (SUSE Linux) 7.4.1 20190905 [gcc-7-branch revision 275407] -- ~Randy Reported-by: Randy Dunlap <rdunlap@infradead.org> ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: linux-next: Tree for Dec 6 (objtool, lots in btrfs) 2019-12-06 16:17 ` linux-next: Tree for Dec 6 (objtool, lots in btrfs) Randy Dunlap @ 2019-12-11 13:49 ` David Sterba 2019-12-11 16:21 ` Randy Dunlap 0 siblings, 1 reply; 18+ messages in thread From: David Sterba @ 2019-12-11 13:49 UTC (permalink / raw) To: Randy Dunlap Cc: Stephen Rothwell, Linux Next Mailing List, Linux Kernel Mailing List, Linux Btrfs On Fri, Dec 06, 2019 at 08:17:30AM -0800, Randy Dunlap wrote: > On 12/5/19 6:54 PM, Stephen Rothwell wrote: > > Hi all, > > > > Please do not add any material for v5.6 to your linux-next included > > trees until after v5.5-rc1 has been released. > > > > Changes since 20191204: > > > > on x86_64: > > fs/btrfs/ctree.o: warning: objtool: btrfs_search_slot()+0x2d4: unreachable instruction Can somebody enlighten me what is one supposed to do to address the warnings? Function names reported in the list contain our ASSERT macro that conditionally calls BUG() that I believe is what could cause the unreachable instructions but I don't see how. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/btrfs/ctree.h#n3113 __cold static inline void assfail(const char *expr, const char *file, int line) { if (IS_ENABLED(CONFIG_BTRFS_ASSERT)) { pr_err("assertion failed: %s, in %s:%d\n", expr, file, line); BUG(); } } #define ASSERT(expr) \ (likely(expr) ? (void)0 : assfail(#expr, __FILE__, __LINE__)) ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: linux-next: Tree for Dec 6 (objtool, lots in btrfs) 2019-12-11 13:49 ` David Sterba @ 2019-12-11 16:21 ` Randy Dunlap 2019-12-12 18:47 ` Josh Poimboeuf 0 siblings, 1 reply; 18+ messages in thread From: Randy Dunlap @ 2019-12-11 16:21 UTC (permalink / raw) To: dsterba, Stephen Rothwell, Linux Next Mailing List, Linux Kernel Mailing List, Linux Btrfs, Peter Zijlstra, Josh Poimboeuf [oops, forgot to add Josh and PeterZ] On 12/11/19 5:49 AM, David Sterba wrote: > On Fri, Dec 06, 2019 at 08:17:30AM -0800, Randy Dunlap wrote: >> On 12/5/19 6:54 PM, Stephen Rothwell wrote: >>> Hi all, >>> >>> Please do not add any material for v5.6 to your linux-next included >>> trees until after v5.5-rc1 has been released. >>> >>> Changes since 20191204: >>> >> >> on x86_64: >> >> fs/btrfs/ctree.o: warning: objtool: btrfs_search_slot()+0x2d4: unreachable instruction > > Can somebody enlighten me what is one supposed to do to address the > warnings? Function names reported in the list contain our ASSERT macro > that conditionally calls BUG() that I believe is what could cause the > unreachable instructions but I don't see how. > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/btrfs/ctree.h#n3113 > > __cold > static inline void assfail(const char *expr, const char *file, int line) > { > if (IS_ENABLED(CONFIG_BTRFS_ASSERT)) { > pr_err("assertion failed: %s, in %s:%d\n", expr, file, line); > BUG(); > } > } > > #define ASSERT(expr) \ > (likely(expr) ? (void)0 : assfail(#expr, __FILE__, __LINE__)) > -- ~Randy Reported-by: Randy Dunlap <rdunlap@infradead.org> ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: linux-next: Tree for Dec 6 (objtool, lots in btrfs) 2019-12-11 16:21 ` Randy Dunlap @ 2019-12-12 18:47 ` Josh Poimboeuf 2019-12-12 20:25 ` Randy Dunlap 0 siblings, 1 reply; 18+ messages in thread From: Josh Poimboeuf @ 2019-12-12 18:47 UTC (permalink / raw) To: Randy Dunlap Cc: dsterba, Stephen Rothwell, Linux Next Mailing List, Linux Kernel Mailing List, Linux Btrfs, Peter Zijlstra On Wed, Dec 11, 2019 at 08:21:38AM -0800, Randy Dunlap wrote: > [oops, forgot to add Josh and PeterZ] > > On 12/11/19 5:49 AM, David Sterba wrote: > > On Fri, Dec 06, 2019 at 08:17:30AM -0800, Randy Dunlap wrote: > >> On 12/5/19 6:54 PM, Stephen Rothwell wrote: > >>> Hi all, > >>> > >>> Please do not add any material for v5.6 to your linux-next included > >>> trees until after v5.5-rc1 has been released. > >>> > >>> Changes since 20191204: > >>> > >> > >> on x86_64: > >> > >> fs/btrfs/ctree.o: warning: objtool: btrfs_search_slot()+0x2d4: unreachable instruction > > > > Can somebody enlighten me what is one supposed to do to address the > > warnings? Function names reported in the list contain our ASSERT macro > > that conditionally calls BUG() that I believe is what could cause the > > unreachable instructions but I don't see how. > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/btrfs/ctree.h#n3113 > > > > __cold > > static inline void assfail(const char *expr, const char *file, int line) > > { > > if (IS_ENABLED(CONFIG_BTRFS_ASSERT)) { > > pr_err("assertion failed: %s, in %s:%d\n", expr, file, line); > > BUG(); > > } > > } > > > > #define ASSERT(expr) \ > > (likely(expr) ? (void)0 : assfail(#expr, __FILE__, __LINE__)) > > Randy, can you share one of the btrfs .o files? I'm not able to recreate. -- Josh ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: linux-next: Tree for Dec 6 (objtool, lots in btrfs) 2019-12-12 18:47 ` Josh Poimboeuf @ 2019-12-12 20:25 ` Randy Dunlap [not found] ` <ba2a7a9b-933b-d4e4-8970-85b6c1291fca@infradead.org> 0 siblings, 1 reply; 18+ messages in thread From: Randy Dunlap @ 2019-12-12 20:25 UTC (permalink / raw) To: Josh Poimboeuf Cc: dsterba, Stephen Rothwell, Linux Next Mailing List, Linux Kernel Mailing List, Linux Btrfs, Peter Zijlstra On 12/12/19 10:47 AM, Josh Poimboeuf wrote: > On Wed, Dec 11, 2019 at 08:21:38AM -0800, Randy Dunlap wrote: >> [oops, forgot to add Josh and PeterZ] >> >> On 12/11/19 5:49 AM, David Sterba wrote: >>> On Fri, Dec 06, 2019 at 08:17:30AM -0800, Randy Dunlap wrote: >>>> On 12/5/19 6:54 PM, Stephen Rothwell wrote: >>>>> Hi all, >>>>> >>>>> Please do not add any material for v5.6 to your linux-next included >>>>> trees until after v5.5-rc1 has been released. >>>>> >>>>> Changes since 20191204: >>>>> >>>> >>>> on x86_64: >>>> >>>> fs/btrfs/ctree.o: warning: objtool: btrfs_search_slot()+0x2d4: unreachable instruction >>> >>> Can somebody enlighten me what is one supposed to do to address the >>> warnings? Function names reported in the list contain our ASSERT macro >>> that conditionally calls BUG() that I believe is what could cause the >>> unreachable instructions but I don't see how. >>> >>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/btrfs/ctree.h#n3113 >>> >>> __cold >>> static inline void assfail(const char *expr, const char *file, int line) >>> { >>> if (IS_ENABLED(CONFIG_BTRFS_ASSERT)) { >>> pr_err("assertion failed: %s, in %s:%d\n", expr, file, line); >>> BUG(); >>> } >>> } >>> >>> #define ASSERT(expr) \ >>> (likely(expr) ? (void)0 : assfail(#expr, __FILE__, __LINE__)) >>> > > Randy, can you share one of the btrfs .o files? I'm not able to > recreate. > Hm. I'll have to try to recreate this. I no longer have files from 20191206 (lack of space). I'll let you know if/when I can recreate it. -- ~Randy ^ permalink raw reply [flat|nested] 18+ messages in thread
[parent not found: <ba2a7a9b-933b-d4e4-8970-85b6c1291fca@infradead.org>]
* Re: linux-next: Tree for Dec 6 (objtool, lots in btrfs) [not found] ` <ba2a7a9b-933b-d4e4-8970-85b6c1291fca@infradead.org> @ 2019-12-13 23:50 ` Josh Poimboeuf 2019-12-14 0:04 ` Randy Dunlap 0 siblings, 1 reply; 18+ messages in thread From: Josh Poimboeuf @ 2019-12-13 23:50 UTC (permalink / raw) To: Randy Dunlap Cc: dsterba, Stephen Rothwell, Linux Next Mailing List, Linux Kernel Mailing List, Linux Btrfs, Peter Zijlstra On Fri, Dec 13, 2019 at 03:03:11PM -0800, Randy Dunlap wrote: > On 12/12/19 12:25 PM, Randy Dunlap wrote: > > On 12/12/19 10:47 AM, Josh Poimboeuf wrote: > >> On Wed, Dec 11, 2019 at 08:21:38AM -0800, Randy Dunlap wrote: > >>> [oops, forgot to add Josh and PeterZ] > >>> > >>> On 12/11/19 5:49 AM, David Sterba wrote: > >>>> On Fri, Dec 06, 2019 at 08:17:30AM -0800, Randy Dunlap wrote: > >>>>> On 12/5/19 6:54 PM, Stephen Rothwell wrote: > >>>>>> Hi all, > >>>>>> > >>>>>> Please do not add any material for v5.6 to your linux-next included > >>>>>> trees until after v5.5-rc1 has been released. > >>>>>> > >>>>>> Changes since 20191204: > >>>>>> > >>>>> > >>>>> on x86_64: > >>>>> > >>>>> fs/btrfs/ctree.o: warning: objtool: btrfs_search_slot()+0x2d4: unreachable instruction > >>>> > >>>> Can somebody enlighten me what is one supposed to do to address the > >>>> warnings? Function names reported in the list contain our ASSERT macro > >>>> that conditionally calls BUG() that I believe is what could cause the > >>>> unreachable instructions but I don't see how. > >>>> > >>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/btrfs/ctree.h#n3113 > >>>> > >>>> __cold > >>>> static inline void assfail(const char *expr, const char *file, int line) > >>>> { > >>>> if (IS_ENABLED(CONFIG_BTRFS_ASSERT)) { > >>>> pr_err("assertion failed: %s, in %s:%d\n", expr, file, line); > >>>> BUG(); > >>>> } > >>>> } > >>>> > >>>> #define ASSERT(expr) \ > >>>> (likely(expr) ? (void)0 : assfail(#expr, __FILE__, __LINE__)) > >>>> > >> > >> Randy, can you share one of the btrfs .o files? I'm not able to > >> recreate. > >> > > > > Hm. I'll have to try to recreate this. I no longer have files from 20191206 > > (lack of space). > > > > I'll let you know if/when I can recreate it. > > OK, 40 builds later, I have reproduced it. > > I am attaching one of the btrfs .o files and the kernel config file (FTR). > (gzipped) > Let me know if you want more of the .o files. Thanks. This is arguably a compiler bug, but the below produces better code generation by adding a noreturn annotation. I think GCC gets tripped up by the IS_ENABLED conditional and can't always tell that assfail (sic) doesn't return. BTW, I'm on my way out the door for a week of much-needed PTO but I can handle this patch (and several others I have pending which were reported by you) when I get back. diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h index b2e8fd8a8e59..bbd68520f5f1 100644 --- a/fs/btrfs/ctree.h +++ b/fs/btrfs/ctree.h @@ -3110,14 +3110,16 @@ do { \ rcu_read_unlock(); \ } while (0) -__cold +#ifdef CONFIG_BTRFS_ASSERT +__cold __unlikely static inline void assfail(const char *expr, const char *file, int line) { - if (IS_ENABLED(CONFIG_BTRFS_ASSERT)) { - pr_err("assertion failed: %s, in %s:%d\n", expr, file, line); - BUG(); - } + pr_err("assertion failed: %s, in %s:%d\n", expr, file, line); + BUG(); } +#else +static inline void assfail(const char *expr, const char *file, int line) {} +#endif #define ASSERT(expr) \ (likely(expr) ? (void)0 : assfail(#expr, __FILE__, __LINE__)) ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: linux-next: Tree for Dec 6 (objtool, lots in btrfs) 2019-12-13 23:50 ` Josh Poimboeuf @ 2019-12-14 0:04 ` Randy Dunlap 2019-12-14 5:45 ` Josh Poimboeuf 0 siblings, 1 reply; 18+ messages in thread From: Randy Dunlap @ 2019-12-14 0:04 UTC (permalink / raw) To: Josh Poimboeuf Cc: dsterba, Stephen Rothwell, Linux Next Mailing List, Linux Kernel Mailing List, Linux Btrfs, Peter Zijlstra On 12/13/19 3:50 PM, Josh Poimboeuf wrote: > On Fri, Dec 13, 2019 at 03:03:11PM -0800, Randy Dunlap wrote: >> On 12/12/19 12:25 PM, Randy Dunlap wrote: >>> On 12/12/19 10:47 AM, Josh Poimboeuf wrote: >>>> On Wed, Dec 11, 2019 at 08:21:38AM -0800, Randy Dunlap wrote: >>>>> [oops, forgot to add Josh and PeterZ] >>>>> >>>>> On 12/11/19 5:49 AM, David Sterba wrote: >>>>>> On Fri, Dec 06, 2019 at 08:17:30AM -0800, Randy Dunlap wrote: >>>>>>> On 12/5/19 6:54 PM, Stephen Rothwell wrote: >>>>>>>> Hi all, >>>>>>>> >>>>>>>> Please do not add any material for v5.6 to your linux-next included >>>>>>>> trees until after v5.5-rc1 has been released. >>>>>>>> >>>>>>>> Changes since 20191204: >>>>>>>> >>>>>>> >>>>>>> on x86_64: >>>>>>> >>>>>>> fs/btrfs/ctree.o: warning: objtool: btrfs_search_slot()+0x2d4: unreachable instruction >>>>>> >>>>>> Can somebody enlighten me what is one supposed to do to address the >>>>>> warnings? Function names reported in the list contain our ASSERT macro >>>>>> that conditionally calls BUG() that I believe is what could cause the >>>>>> unreachable instructions but I don't see how. >>>>>> >>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/btrfs/ctree.h#n3113 >>>>>> >>>>>> __cold >>>>>> static inline void assfail(const char *expr, const char *file, int line) >>>>>> { >>>>>> if (IS_ENABLED(CONFIG_BTRFS_ASSERT)) { >>>>>> pr_err("assertion failed: %s, in %s:%d\n", expr, file, line); >>>>>> BUG(); >>>>>> } >>>>>> } >>>>>> >>>>>> #define ASSERT(expr) \ >>>>>> (likely(expr) ? (void)0 : assfail(#expr, __FILE__, __LINE__)) >>>>>> >>>> >>>> Randy, can you share one of the btrfs .o files? I'm not able to >>>> recreate. >>>> >>> >>> Hm. I'll have to try to recreate this. I no longer have files from 20191206 >>> (lack of space). >>> >>> I'll let you know if/when I can recreate it. >> >> OK, 40 builds later, I have reproduced it. >> >> I am attaching one of the btrfs .o files and the kernel config file (FTR). >> (gzipped) >> Let me know if you want more of the .o files. > > Thanks. This is arguably a compiler bug, but the below produces better > code generation by adding a noreturn annotation. I think GCC gets > tripped up by the IS_ENABLED conditional and can't always tell that > assfail (sic) doesn't return. > > BTW, I'm on my way out the door for a week of much-needed PTO but I can > handle this patch (and several others I have pending which were reported > by you) when I get back. Sure, no hurry. Have a good one. > diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h > index b2e8fd8a8e59..bbd68520f5f1 100644 > --- a/fs/btrfs/ctree.h > +++ b/fs/btrfs/ctree.h > @@ -3110,14 +3110,16 @@ do { \ > rcu_read_unlock(); \ > } while (0) > > -__cold > +#ifdef CONFIG_BTRFS_ASSERT > +__cold __unlikely what provides __unlikely? It is causing build errors. and if I remove the "__unlikely", I still see the objtool warnings (unreachable instructions). > static inline void assfail(const char *expr, const char *file, int line) > { > - if (IS_ENABLED(CONFIG_BTRFS_ASSERT)) { > - pr_err("assertion failed: %s, in %s:%d\n", expr, file, line); > - BUG(); > - } > + pr_err("assertion failed: %s, in %s:%d\n", expr, file, line); > + BUG(); > } > +#else > +static inline void assfail(const char *expr, const char *file, int line) {} > +#endif > > #define ASSERT(expr) \ > (likely(expr) ? (void)0 : assfail(#expr, __FILE__, __LINE__)) > -- ~Randy ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: linux-next: Tree for Dec 6 (objtool, lots in btrfs) 2019-12-14 0:04 ` Randy Dunlap @ 2019-12-14 5:45 ` Josh Poimboeuf [not found] ` <fe1e0318-9b74-7ae0-07bd-d7a6c908e79a@infradead.org> 2019-12-17 15:29 ` David Sterba 0 siblings, 2 replies; 18+ messages in thread From: Josh Poimboeuf @ 2019-12-14 5:45 UTC (permalink / raw) To: Randy Dunlap Cc: dsterba, Stephen Rothwell, Linux Next Mailing List, Linux Kernel Mailing List, Linux Btrfs, Peter Zijlstra On Fri, Dec 13, 2019 at 04:04:58PM -0800, Randy Dunlap wrote: > > diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h > > index b2e8fd8a8e59..bbd68520f5f1 100644 > > --- a/fs/btrfs/ctree.h > > +++ b/fs/btrfs/ctree.h > > @@ -3110,14 +3110,16 @@ do { \ > > rcu_read_unlock(); \ > > } while (0) > > > > -__cold > > +#ifdef CONFIG_BTRFS_ASSERT > > +__cold __unlikely > > what provides __unlikely? It is causing build errors. > > and if I remove the "__unlikely", I still see the objtool warnings > (unreachable instructions). Ha, not sure how that happened... Should be __noreturn instead of __unlikely. Cheers... diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h index b2e8fd8a8e59..398bd010dfc5 100644 --- a/fs/btrfs/ctree.h +++ b/fs/btrfs/ctree.h @@ -3110,14 +3110,16 @@ do { \ rcu_read_unlock(); \ } while (0) -__cold +#ifdef CONFIG_BTRFS_ASSERT +__cold __noreturn static inline void assfail(const char *expr, const char *file, int line) { - if (IS_ENABLED(CONFIG_BTRFS_ASSERT)) { - pr_err("assertion failed: %s, in %s:%d\n", expr, file, line); - BUG(); - } + pr_err("assertion failed: %s, in %s:%d\n", expr, file, line); + BUG(); } +#else +static inline void assfail(const char *expr, const char *file, int line) {} +#endif #define ASSERT(expr) \ (likely(expr) ? (void)0 : assfail(#expr, __FILE__, __LINE__)) ^ permalink raw reply related [flat|nested] 18+ messages in thread
[parent not found: <fe1e0318-9b74-7ae0-07bd-d7a6c908e79a@infradead.org>]
* Re: linux-next: Tree for Dec 6 (objtool, lots in btrfs) [not found] ` <fe1e0318-9b74-7ae0-07bd-d7a6c908e79a@infradead.org> @ 2019-12-17 15:25 ` David Sterba 2020-01-17 17:26 ` Josh Poimboeuf 0 siblings, 1 reply; 18+ messages in thread From: David Sterba @ 2019-12-17 15:25 UTC (permalink / raw) To: Randy Dunlap Cc: Josh Poimboeuf, Stephen Rothwell, Linux Next Mailing List, Linux Kernel Mailing List, Linux Btrfs, Peter Zijlstra On Fri, Dec 13, 2019 at 11:05:18PM -0800, Randy Dunlap wrote: > OK, that fixes most of them, but still leaves these 2: > > btrfs006.out:fs/btrfs/extent_io.o: warning: objtool: __set_extent_bit()+0x536: unreachable instruction Hard to read from the assembly what C statement is it referring to. I think there are also several functions inlined, I don't see anything suspicious inside __set_extent_bit itself. > btrfs006.out:fs/btrfs/relocation.o: warning: objtool: add_tree_block()+0x501: unreachable instruction Probably also heavily inlined, the function has like 50 lines, a few non-trivial function calls but the offset in the warning suggests a larger size. While browsing the callees I noticed that both have in common a function that is supposed to print and stop at fatal errors. They're extent_io_tree_panic (extent_io.c) and backref_tree_panic (relocation.c). Both call btrfs_panic which is a macro: 3239 #define btrfs_panic(fs_info, errno, fmt, args...) \ 3240 do { \ 3241 __btrfs_panic(fs_info, __func__, __LINE__, errno, fmt, ##args); \ 3242 BUG(); \ 3243 } while (0) There are no conditionals and BUG has the __noreturn annotation (unreachable()) so all is in place and I don't have better ideas what's causing the reports. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: linux-next: Tree for Dec 6 (objtool, lots in btrfs) 2019-12-17 15:25 ` David Sterba @ 2020-01-17 17:26 ` Josh Poimboeuf 2020-01-17 20:28 ` Marco Elver 0 siblings, 1 reply; 18+ messages in thread From: Josh Poimboeuf @ 2020-01-17 17:26 UTC (permalink / raw) To: David Sterba Cc: Randy Dunlap, Stephen Rothwell, Linux Next Mailing List, Linux Kernel Mailing List, Linux Btrfs, Peter Zijlstra, Marco Elver On Tue, Dec 17, 2019 at 04:25:11PM +0100, David Sterba wrote: > On Fri, Dec 13, 2019 at 11:05:18PM -0800, Randy Dunlap wrote: > > OK, that fixes most of them, but still leaves these 2: > > > > btrfs006.out:fs/btrfs/extent_io.o: warning: objtool: __set_extent_bit()+0x536: unreachable instruction > > Hard to read from the assembly what C statement is it referring to. I > think there are also several functions inlined, I don't see anything > suspicious inside __set_extent_bit itself. > > > btrfs006.out:fs/btrfs/relocation.o: warning: objtool: add_tree_block()+0x501: unreachable instruction > > Probably also heavily inlined, the function has like 50 lines, a few > non-trivial function calls but the offset in the warning suggests a > larger size. > > While browsing the callees I noticed that both have in common a function > that is supposed to print and stop at fatal errors. They're > extent_io_tree_panic (extent_io.c) and backref_tree_panic > (relocation.c). Both call btrfs_panic which is a macro: > > 3239 #define btrfs_panic(fs_info, errno, fmt, args...) \ > 3240 do { \ > 3241 __btrfs_panic(fs_info, __func__, __LINE__, errno, fmt, ##args); \ > 3242 BUG(); \ > 3243 } while (0) > > There are no conditionals and BUG has the __noreturn annotation > (unreachable()) so all is in place and I don't have better ideas what's > causing the reports. I think KCSAN is somehow disabling GCC's detection of implicit noreturn functions -- or at least some calls to them. So GCC is inserting dead code after the calls. BUG() uses __builtin_unreachable(), so GCC should know better. If this is specific to KCSAN then I might just disable these warnings for KCSAN configs. -- Josh ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: linux-next: Tree for Dec 6 (objtool, lots in btrfs) 2020-01-17 17:26 ` Josh Poimboeuf @ 2020-01-17 20:28 ` Marco Elver 2020-01-17 21:26 ` Josh Poimboeuf 0 siblings, 1 reply; 18+ messages in thread From: Marco Elver @ 2020-01-17 20:28 UTC (permalink / raw) To: Josh Poimboeuf Cc: David Sterba, Randy Dunlap, Stephen Rothwell, Linux Next Mailing List, Linux Kernel Mailing List, Linux Btrfs, Peter Zijlstra On Fri, 17 Jan 2020 at 18:26, Josh Poimboeuf <jpoimboe@redhat.com> wrote: > > On Tue, Dec 17, 2019 at 04:25:11PM +0100, David Sterba wrote: > > On Fri, Dec 13, 2019 at 11:05:18PM -0800, Randy Dunlap wrote: > > > OK, that fixes most of them, but still leaves these 2: > > > > > > btrfs006.out:fs/btrfs/extent_io.o: warning: objtool: __set_extent_bit()+0x536: unreachable instruction > > > > Hard to read from the assembly what C statement is it referring to. I > > think there are also several functions inlined, I don't see anything > > suspicious inside __set_extent_bit itself. > > > > > btrfs006.out:fs/btrfs/relocation.o: warning: objtool: add_tree_block()+0x501: unreachable instruction > > > > Probably also heavily inlined, the function has like 50 lines, a few > > non-trivial function calls but the offset in the warning suggests a > > larger size. > > > > While browsing the callees I noticed that both have in common a function > > that is supposed to print and stop at fatal errors. They're > > extent_io_tree_panic (extent_io.c) and backref_tree_panic > > (relocation.c). Both call btrfs_panic which is a macro: > > > > 3239 #define btrfs_panic(fs_info, errno, fmt, args...) \ > > 3240 do { \ > > 3241 __btrfs_panic(fs_info, __func__, __LINE__, errno, fmt, ##args); \ > > 3242 BUG(); \ > > 3243 } while (0) > > > > There are no conditionals and BUG has the __noreturn annotation > > (unreachable()) so all is in place and I don't have better ideas what's > > causing the reports. > > I think KCSAN is somehow disabling GCC's detection of implicit noreturn > functions -- or at least some calls to them. So GCC is inserting dead > code after the calls. BUG() uses __builtin_unreachable(), so GCC should > know better. > > If this is specific to KCSAN then I might just disable these warnings > for KCSAN configs. I noticed that this is also a CC_OPTIMIZE_FOR_SIZE config. I recently sent some patches to turn some inlines into __always_inlines because CC_OPTIMIZE_FOR_SIZE decides to not inline functions that should always be inlined. I noticed that 'assfail' is a 'static inline' function and you mentioned earlier that GCC seems to not be able to determine if it returns or not. If CC_OPTIMIZE_FOR_SIZE decides to not inline, then maybe this could be a problem? It could also be the compiler having some trouble here with the CC_OPTIMIZE_FOR_SIZE + KCSAN combination. Thanks, -- Marco ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: linux-next: Tree for Dec 6 (objtool, lots in btrfs) 2020-01-17 20:28 ` Marco Elver @ 2020-01-17 21:26 ` Josh Poimboeuf 2020-01-17 22:22 ` Josh Poimboeuf 0 siblings, 1 reply; 18+ messages in thread From: Josh Poimboeuf @ 2020-01-17 21:26 UTC (permalink / raw) To: Marco Elver Cc: David Sterba, Randy Dunlap, Stephen Rothwell, Linux Next Mailing List, Linux Kernel Mailing List, Linux Btrfs, Peter Zijlstra On Fri, Jan 17, 2020 at 09:28:27PM +0100, Marco Elver wrote: > On Fri, 17 Jan 2020 at 18:26, Josh Poimboeuf <jpoimboe@redhat.com> wrote: > > > > On Tue, Dec 17, 2019 at 04:25:11PM +0100, David Sterba wrote: > > > On Fri, Dec 13, 2019 at 11:05:18PM -0800, Randy Dunlap wrote: > > > > OK, that fixes most of them, but still leaves these 2: > > > > > > > > btrfs006.out:fs/btrfs/extent_io.o: warning: objtool: __set_extent_bit()+0x536: unreachable instruction > > > > > > Hard to read from the assembly what C statement is it referring to. I > > > think there are also several functions inlined, I don't see anything > > > suspicious inside __set_extent_bit itself. > > > > > > > btrfs006.out:fs/btrfs/relocation.o: warning: objtool: add_tree_block()+0x501: unreachable instruction > > > > > > Probably also heavily inlined, the function has like 50 lines, a few > > > non-trivial function calls but the offset in the warning suggests a > > > larger size. > > > > > > While browsing the callees I noticed that both have in common a function > > > that is supposed to print and stop at fatal errors. They're > > > extent_io_tree_panic (extent_io.c) and backref_tree_panic > > > (relocation.c). Both call btrfs_panic which is a macro: > > > > > > 3239 #define btrfs_panic(fs_info, errno, fmt, args...) \ > > > 3240 do { \ > > > 3241 __btrfs_panic(fs_info, __func__, __LINE__, errno, fmt, ##args); \ > > > 3242 BUG(); \ > > > 3243 } while (0) > > > > > > There are no conditionals and BUG has the __noreturn annotation > > > (unreachable()) so all is in place and I don't have better ideas what's > > > causing the reports. > > > > I think KCSAN is somehow disabling GCC's detection of implicit noreturn > > functions -- or at least some calls to them. So GCC is inserting dead > > code after the calls. BUG() uses __builtin_unreachable(), so GCC should > > know better. > > > > If this is specific to KCSAN then I might just disable these warnings > > for KCSAN configs. > > I noticed that this is also a CC_OPTIMIZE_FOR_SIZE config. I recently > sent some patches to turn some inlines into __always_inlines because > CC_OPTIMIZE_FOR_SIZE decides to not inline functions that should > always be inlined. > > I noticed that 'assfail' is a 'static inline' function and you > mentioned earlier that GCC seems to not be able to determine if it > returns or not. If CC_OPTIMIZE_FOR_SIZE decides to not inline, then > maybe this could be a problem? It could also be the compiler having > some trouble here with the CC_OPTIMIZE_FOR_SIZE + KCSAN combination. Even for a non-inlined static function, GCC typically detects when it's implicitly "noreturn", and optimizes the call sites accordingly. And that has also been true even for CC_OPTIMIZE_FOR_SIZE in the past. So something changed apparently. (KCSAN was just a guess.) -- Josh ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: linux-next: Tree for Dec 6 (objtool, lots in btrfs) 2020-01-17 21:26 ` Josh Poimboeuf @ 2020-01-17 22:22 ` Josh Poimboeuf 0 siblings, 0 replies; 18+ messages in thread From: Josh Poimboeuf @ 2020-01-17 22:22 UTC (permalink / raw) To: Marco Elver Cc: David Sterba, Randy Dunlap, Stephen Rothwell, Linux Next Mailing List, Linux Kernel Mailing List, Linux Btrfs, Peter Zijlstra On Fri, Jan 17, 2020 at 03:26:49PM -0600, Josh Poimboeuf wrote: > On Fri, Jan 17, 2020 at 09:28:27PM +0100, Marco Elver wrote: > > On Fri, 17 Jan 2020 at 18:26, Josh Poimboeuf <jpoimboe@redhat.com> wrote: > > > > > > On Tue, Dec 17, 2019 at 04:25:11PM +0100, David Sterba wrote: > > > > On Fri, Dec 13, 2019 at 11:05:18PM -0800, Randy Dunlap wrote: > > > > > OK, that fixes most of them, but still leaves these 2: > > > > > > > > > > btrfs006.out:fs/btrfs/extent_io.o: warning: objtool: __set_extent_bit()+0x536: unreachable instruction > > > > > > > > Hard to read from the assembly what C statement is it referring to. I > > > > think there are also several functions inlined, I don't see anything > > > > suspicious inside __set_extent_bit itself. > > > > > > > > > btrfs006.out:fs/btrfs/relocation.o: warning: objtool: add_tree_block()+0x501: unreachable instruction > > > > > > > > Probably also heavily inlined, the function has like 50 lines, a few > > > > non-trivial function calls but the offset in the warning suggests a > > > > larger size. > > > > > > > > While browsing the callees I noticed that both have in common a function > > > > that is supposed to print and stop at fatal errors. They're > > > > extent_io_tree_panic (extent_io.c) and backref_tree_panic > > > > (relocation.c). Both call btrfs_panic which is a macro: > > > > > > > > 3239 #define btrfs_panic(fs_info, errno, fmt, args...) \ > > > > 3240 do { \ > > > > 3241 __btrfs_panic(fs_info, __func__, __LINE__, errno, fmt, ##args); \ > > > > 3242 BUG(); \ > > > > 3243 } while (0) > > > > > > > > There are no conditionals and BUG has the __noreturn annotation > > > > (unreachable()) so all is in place and I don't have better ideas what's > > > > causing the reports. > > > > > > I think KCSAN is somehow disabling GCC's detection of implicit noreturn > > > functions -- or at least some calls to them. So GCC is inserting dead > > > code after the calls. BUG() uses __builtin_unreachable(), so GCC should > > > know better. > > > > > > If this is specific to KCSAN then I might just disable these warnings > > > for KCSAN configs. > > > > I noticed that this is also a CC_OPTIMIZE_FOR_SIZE config. I recently > > sent some patches to turn some inlines into __always_inlines because > > CC_OPTIMIZE_FOR_SIZE decides to not inline functions that should > > always be inlined. > > > > I noticed that 'assfail' is a 'static inline' function and you > > mentioned earlier that GCC seems to not be able to determine if it > > returns or not. If CC_OPTIMIZE_FOR_SIZE decides to not inline, then > > maybe this could be a problem? It could also be the compiler having > > some trouble here with the CC_OPTIMIZE_FOR_SIZE + KCSAN combination. > > Even for a non-inlined static function, GCC typically detects when it's > implicitly "noreturn", and optimizes the call sites accordingly. And > that has also been true even for CC_OPTIMIZE_FOR_SIZE in the past. So > something changed apparently. (KCSAN was just a guess.) I'm actually seeing this issue pop up recently in other places, without KCSAN enabled. So it may just be a new GCC bug (albeit a very minor one). Sorry for blaming KCSAN :-) I'll need to dig some more. The easy fix would be something like: diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c index eb8bd0258360..4db39fef3b56 100644 --- a/fs/btrfs/extent_io.c +++ b/fs/btrfs/extent_io.c @@ -655,7 +655,7 @@ alloc_extent_state_atomic(struct extent_state *prealloc) return prealloc; } -static void extent_io_tree_panic(struct extent_io_tree *tree, int err) +static void __noreturn extent_io_tree_panic(struct extent_io_tree *tree, int err) { struct inode *inode = tree->private_data; diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c index d897a8e5e430..b7a94b1739ae 100644 --- a/fs/btrfs/relocation.c +++ b/fs/btrfs/relocation.c @@ -321,7 +321,7 @@ static struct rb_node *tree_search(struct rb_root *root, u64 bytenr) return NULL; } -static void backref_tree_panic(struct rb_node *rb_node, int errno, u64 bytenr) +static void __noreturn backref_tree_panic(struct rb_node *rb_node, int errno, u64 bytenr) { struct btrfs_fs_info *fs_info = NULL; ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: linux-next: Tree for Dec 6 (objtool, lots in btrfs) 2019-12-14 5:45 ` Josh Poimboeuf [not found] ` <fe1e0318-9b74-7ae0-07bd-d7a6c908e79a@infradead.org> @ 2019-12-17 15:29 ` David Sterba 2020-01-10 19:46 ` David Sterba 1 sibling, 1 reply; 18+ messages in thread From: David Sterba @ 2019-12-17 15:29 UTC (permalink / raw) To: Josh Poimboeuf Cc: Randy Dunlap, Stephen Rothwell, Linux Next Mailing List, Linux Kernel Mailing List, Linux Btrfs, Peter Zijlstra On Fri, Dec 13, 2019 at 11:45:15PM -0600, Josh Poimboeuf wrote: > On Fri, Dec 13, 2019 at 04:04:58PM -0800, Randy Dunlap wrote: > > > diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h > > > index b2e8fd8a8e59..bbd68520f5f1 100644 > > > --- a/fs/btrfs/ctree.h > > > +++ b/fs/btrfs/ctree.h > > > @@ -3110,14 +3110,16 @@ do { \ > > > rcu_read_unlock(); \ > > > } while (0) > > > > > > -__cold > > > +#ifdef CONFIG_BTRFS_ASSERT > > > +__cold __unlikely > > > > what provides __unlikely? It is causing build errors. > > > > and if I remove the "__unlikely", I still see the objtool warnings > > (unreachable instructions). > > Ha, not sure how that happened... Should be __noreturn instead of > __unlikely. Cheers... > > diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h > index b2e8fd8a8e59..398bd010dfc5 100644 > --- a/fs/btrfs/ctree.h > +++ b/fs/btrfs/ctree.h > @@ -3110,14 +3110,16 @@ do { \ > rcu_read_unlock(); \ > } while (0) > > -__cold > +#ifdef CONFIG_BTRFS_ASSERT > +__cold __noreturn > static inline void assfail(const char *expr, const char *file, int line) > { > - if (IS_ENABLED(CONFIG_BTRFS_ASSERT)) { > - pr_err("assertion failed: %s, in %s:%d\n", expr, file, line); > - BUG(); > - } > + pr_err("assertion failed: %s, in %s:%d\n", expr, file, line); > + BUG(); > } > +#else > +static inline void assfail(const char *expr, const char *file, int line) {} > +#endif > > #define ASSERT(expr) \ > (likely(expr) ? (void)0 : assfail(#expr, __FILE__, __LINE__)) Separating the definitions by #ifdef looks ok, I'd rather do separate definitions of ASSERT too, to avoid the ternary operator. I'll send the patch. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: linux-next: Tree for Dec 6 (objtool, lots in btrfs) 2019-12-17 15:29 ` David Sterba @ 2020-01-10 19:46 ` David Sterba 2020-01-17 15:28 ` Josh Poimboeuf 0 siblings, 1 reply; 18+ messages in thread From: David Sterba @ 2020-01-10 19:46 UTC (permalink / raw) To: dsterba, Josh Poimboeuf, Randy Dunlap, Stephen Rothwell, Linux Next Mailing List, Linux Kernel Mailing List, Linux Btrfs, Peter Zijlstra On Tue, Dec 17, 2019 at 04:29:54PM +0100, David Sterba wrote: > Separating the definitions by #ifdef looks ok, I'd rather do separate > definitions of ASSERT too, to avoid the ternary operator. I'll send the > patch. Subject: [PATCH] btrfs: separate definition of assertion failure handlers There's a report where objtool detects unreachable instructions, eg.: fs/btrfs/ctree.o: warning: objtool: btrfs_search_slot()+0x2d4: unreachable instruction This seems to be a false positive due to compiler version. The cause is in the ASSERT macro implementation that does the conditional check as IS_DEFINED(CONFIG_BTRFS_ASSERT) and not an #ifdef. To avoid that, use the ifdefs directly. CC: Josh Poimboeuf <jpoimboe@redhat.com> Reported-by: Randy Dunlap <rdunlap@infradead.org> Signed-off-by: David Sterba <dsterba@suse.com> --- fs/btrfs/ctree.h | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h index 569931dd0ce5..f90b82050d2d 100644 --- a/fs/btrfs/ctree.h +++ b/fs/btrfs/ctree.h @@ -3157,17 +3157,21 @@ do { \ rcu_read_unlock(); \ } while (0) -__cold -static inline void assfail(const char *expr, const char *file, int line) +#ifdef CONFIG_BTRFS_ASSERT +__cold __noreturn +static inline void assertfail(const char *expr, const char *file, int line) { - if (IS_ENABLED(CONFIG_BTRFS_ASSERT)) { - pr_err("assertion failed: %s, in %s:%d\n", expr, file, line); - BUG(); - } + pr_err("assertion failed: %s, in %s:%d\n", expr, file, line); + BUG(); } -#define ASSERT(expr) \ - (likely(expr) ? (void)0 : assfail(#expr, __FILE__, __LINE__)) +#define ASSERT(expr) \ + (likely(expr) ? (void)0 : assertfail(#expr, __FILE__, __LINE__)) + +#else +static inline void assertfail(const char *expr, const char* file, int line) { } +#define ASSERT(expr) (void)(expr) +#endif /* * Use that for functions that are conditionally exported for sanity tests but -- ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: linux-next: Tree for Dec 6 (objtool, lots in btrfs) 2020-01-10 19:46 ` David Sterba @ 2020-01-17 15:28 ` Josh Poimboeuf 2020-01-17 16:50 ` David Sterba 0 siblings, 1 reply; 18+ messages in thread From: Josh Poimboeuf @ 2020-01-17 15:28 UTC (permalink / raw) To: David Sterba Cc: Randy Dunlap, Stephen Rothwell, Linux Next Mailing List, Linux Kernel Mailing List, Linux Btrfs, Peter Zijlstra On Fri, Jan 10, 2020 at 08:46:22PM +0100, David Sterba wrote: > On Tue, Dec 17, 2019 at 04:29:54PM +0100, David Sterba wrote: > > Separating the definitions by #ifdef looks ok, I'd rather do separate > > definitions of ASSERT too, to avoid the ternary operator. I'll send the > > patch. > > Subject: [PATCH] btrfs: separate definition of assertion failure handlers > > There's a report where objtool detects unreachable instructions, eg.: > > fs/btrfs/ctree.o: warning: objtool: btrfs_search_slot()+0x2d4: unreachable instruction > > This seems to be a false positive due to compiler version. The cause is > in the ASSERT macro implementation that does the conditional check as > IS_DEFINED(CONFIG_BTRFS_ASSERT) and not an #ifdef. > > To avoid that, use the ifdefs directly. > > CC: Josh Poimboeuf <jpoimboe@redhat.com> > Reported-by: Randy Dunlap <rdunlap@infradead.org> > Signed-off-by: David Sterba <dsterba@suse.com> > --- > fs/btrfs/ctree.h | 20 ++++++++++++-------- > 1 file changed, 12 insertions(+), 8 deletions(-) This looks quite similar to my patch, would you mind giving me attribution? > diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h > index 569931dd0ce5..f90b82050d2d 100644 > --- a/fs/btrfs/ctree.h > +++ b/fs/btrfs/ctree.h > @@ -3157,17 +3157,21 @@ do { \ > rcu_read_unlock(); \ > } while (0) > > -__cold > -static inline void assfail(const char *expr, const char *file, int line) > +#ifdef CONFIG_BTRFS_ASSERT > +__cold __noreturn > +static inline void assertfail(const char *expr, const char *file, int line) > { > - if (IS_ENABLED(CONFIG_BTRFS_ASSERT)) { > - pr_err("assertion failed: %s, in %s:%d\n", expr, file, line); > - BUG(); > - } > + pr_err("assertion failed: %s, in %s:%d\n", expr, file, line); > + BUG(); assertfail() is definitely better than "assfail", but shouldn't you update the callers so it doesn't break the build? -- Josh ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: linux-next: Tree for Dec 6 (objtool, lots in btrfs) 2020-01-17 15:28 ` Josh Poimboeuf @ 2020-01-17 16:50 ` David Sterba 2020-01-17 17:03 ` Josh Poimboeuf 0 siblings, 1 reply; 18+ messages in thread From: David Sterba @ 2020-01-17 16:50 UTC (permalink / raw) To: Josh Poimboeuf Cc: David Sterba, Randy Dunlap, Stephen Rothwell, Linux Next Mailing List, Linux Kernel Mailing List, Linux Btrfs, Peter Zijlstra On Fri, Jan 17, 2020 at 09:28:05AM -0600, Josh Poimboeuf wrote: > On Fri, Jan 10, 2020 at 08:46:22PM +0100, David Sterba wrote: > > On Tue, Dec 17, 2019 at 04:29:54PM +0100, David Sterba wrote: > > > Separating the definitions by #ifdef looks ok, I'd rather do separate > > > definitions of ASSERT too, to avoid the ternary operator. I'll send the > > > patch. > > > > Subject: [PATCH] btrfs: separate definition of assertion failure handlers > > > > There's a report where objtool detects unreachable instructions, eg.: > > > > fs/btrfs/ctree.o: warning: objtool: btrfs_search_slot()+0x2d4: unreachable instruction > > > > This seems to be a false positive due to compiler version. The cause is > > in the ASSERT macro implementation that does the conditional check as > > IS_DEFINED(CONFIG_BTRFS_ASSERT) and not an #ifdef. > > > > To avoid that, use the ifdefs directly. > > > > CC: Josh Poimboeuf <jpoimboe@redhat.com> > > Reported-by: Randy Dunlap <rdunlap@infradead.org> > > Signed-off-by: David Sterba <dsterba@suse.com> > > --- > > fs/btrfs/ctree.h | 20 ++++++++++++-------- > > 1 file changed, 12 insertions(+), 8 deletions(-) > > This looks quite similar to my patch, would you mind giving me > attribution? So Co-developed-by: or "based on patch from Josh", or something else? > > diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h > > index 569931dd0ce5..f90b82050d2d 100644 > > --- a/fs/btrfs/ctree.h > > +++ b/fs/btrfs/ctree.h > > @@ -3157,17 +3157,21 @@ do { \ > > rcu_read_unlock(); \ > > } while (0) > > > > -__cold > > -static inline void assfail(const char *expr, const char *file, int line) > > +#ifdef CONFIG_BTRFS_ASSERT > > +__cold __noreturn > > +static inline void assertfail(const char *expr, const char *file, int line) > > { > > - if (IS_ENABLED(CONFIG_BTRFS_ASSERT)) { > > - pr_err("assertion failed: %s, in %s:%d\n", expr, file, line); > > - BUG(); > > - } > > + pr_err("assertion failed: %s, in %s:%d\n", expr, file, line); > > + BUG(); > > assertfail() is definitely better than "assfail", but shouldn't you > update the callers so it doesn't break the build? I don't understand what you mean, the helper is not called directly (and build does not fail with or without CONFIG_BTRFS_ASSERT), but always as ASSERT, so I don't see what needs to be updated. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: linux-next: Tree for Dec 6 (objtool, lots in btrfs) 2020-01-17 16:50 ` David Sterba @ 2020-01-17 17:03 ` Josh Poimboeuf 0 siblings, 0 replies; 18+ messages in thread From: Josh Poimboeuf @ 2020-01-17 17:03 UTC (permalink / raw) To: David Sterba Cc: Randy Dunlap, Stephen Rothwell, Linux Next Mailing List, Linux Kernel Mailing List, Linux Btrfs, Peter Zijlstra On Fri, Jan 17, 2020 at 05:50:19PM +0100, David Sterba wrote: > On Fri, Jan 17, 2020 at 09:28:05AM -0600, Josh Poimboeuf wrote: > > On Fri, Jan 10, 2020 at 08:46:22PM +0100, David Sterba wrote: > > > On Tue, Dec 17, 2019 at 04:29:54PM +0100, David Sterba wrote: > > > > Separating the definitions by #ifdef looks ok, I'd rather do separate > > > > definitions of ASSERT too, to avoid the ternary operator. I'll send the > > > > patch. > > > > > > Subject: [PATCH] btrfs: separate definition of assertion failure handlers > > > > > > There's a report where objtool detects unreachable instructions, eg.: > > > > > > fs/btrfs/ctree.o: warning: objtool: btrfs_search_slot()+0x2d4: unreachable instruction > > > > > > This seems to be a false positive due to compiler version. The cause is > > > in the ASSERT macro implementation that does the conditional check as > > > IS_DEFINED(CONFIG_BTRFS_ASSERT) and not an #ifdef. > > > > > > To avoid that, use the ifdefs directly. > > > > > > CC: Josh Poimboeuf <jpoimboe@redhat.com> > > > Reported-by: Randy Dunlap <rdunlap@infradead.org> > > > Signed-off-by: David Sterba <dsterba@suse.com> > > > --- > > > fs/btrfs/ctree.h | 20 ++++++++++++-------- > > > 1 file changed, 12 insertions(+), 8 deletions(-) > > > > This looks quite similar to my patch, would you mind giving me > > attribution? > > So Co-developed-by: or "based on patch from Josh", or something else? Co-developed-by would be fine, thanks. > > > diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h > > > index 569931dd0ce5..f90b82050d2d 100644 > > > --- a/fs/btrfs/ctree.h > > > +++ b/fs/btrfs/ctree.h > > > @@ -3157,17 +3157,21 @@ do { \ > > > rcu_read_unlock(); \ > > > } while (0) > > > > > > -__cold > > > -static inline void assfail(const char *expr, const char *file, int line) > > > +#ifdef CONFIG_BTRFS_ASSERT > > > +__cold __noreturn > > > +static inline void assertfail(const char *expr, const char *file, int line) > > > { > > > - if (IS_ENABLED(CONFIG_BTRFS_ASSERT)) { > > > - pr_err("assertion failed: %s, in %s:%d\n", expr, file, line); > > > - BUG(); > > > - } > > > + pr_err("assertion failed: %s, in %s:%d\n", expr, file, line); > > > + BUG(); > > > > assertfail() is definitely better than "assfail", but shouldn't you > > update the callers so it doesn't break the build? > > I don't understand what you mean, the helper is not called directly (and > build does not fail with or without CONFIG_BTRFS_ASSERT), but always as > ASSERT, so I don't see what needs to be updated. Oops, you're right, I misread the patch. -- Josh ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2020-01-17 22:22 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <20191206135406.563336e7@canb.auug.org.au> 2019-12-06 16:17 ` linux-next: Tree for Dec 6 (objtool, lots in btrfs) Randy Dunlap 2019-12-11 13:49 ` David Sterba 2019-12-11 16:21 ` Randy Dunlap 2019-12-12 18:47 ` Josh Poimboeuf 2019-12-12 20:25 ` Randy Dunlap [not found] ` <ba2a7a9b-933b-d4e4-8970-85b6c1291fca@infradead.org> 2019-12-13 23:50 ` Josh Poimboeuf 2019-12-14 0:04 ` Randy Dunlap 2019-12-14 5:45 ` Josh Poimboeuf [not found] ` <fe1e0318-9b74-7ae0-07bd-d7a6c908e79a@infradead.org> 2019-12-17 15:25 ` David Sterba 2020-01-17 17:26 ` Josh Poimboeuf 2020-01-17 20:28 ` Marco Elver 2020-01-17 21:26 ` Josh Poimboeuf 2020-01-17 22:22 ` Josh Poimboeuf 2019-12-17 15:29 ` David Sterba 2020-01-10 19:46 ` David Sterba 2020-01-17 15:28 ` Josh Poimboeuf 2020-01-17 16:50 ` David Sterba 2020-01-17 17:03 ` Josh Poimboeuf
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).