Re: linux-next: Tree for Dec 6 (objtool, lots in btrfs)

Re: linux-next: Tree for Dec 6 (objtool, lots in btrfs)

[Randy Dunlap]

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>

Re: linux-next: Tree for Dec 6 (objtool, lots in btrfs)

[David Sterba]

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__))

Re: linux-next: Tree for Dec 6 (objtool, lots in btrfs)

[Randy Dunlap]

[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>

Re: linux-next: Tree for Dec 6 (objtool, lots in btrfs)

[Josh Poimboeuf]

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

Re: linux-next: Tree for Dec 6 (objtool, lots in btrfs)

[Randy Dunlap]

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

Re: linux-next: Tree for Dec 6 (objtool, lots in btrfs)

[Josh Poimboeuf]

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__))

Re: linux-next: Tree for Dec 6 (objtool, lots in btrfs)

[Randy Dunlap]

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

Re: linux-next: Tree for Dec 6 (objtool, lots in btrfs)

[Josh Poimboeuf]

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__))

Re: linux-next: Tree for Dec 6 (objtool, lots in btrfs)

[David Sterba]

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.

Re: linux-next: Tree for Dec 6 (objtool, lots in btrfs)

[Josh Poimboeuf]

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

Re: linux-next: Tree for Dec 6 (objtool, lots in btrfs)

[Marco Elver]

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

Re: linux-next: Tree for Dec 6 (objtool, lots in btrfs)

[Josh Poimboeuf]

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

Re: linux-next: Tree for Dec 6 (objtool, lots in btrfs)

[Josh Poimboeuf]

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;

Re: linux-next: Tree for Dec 6 (objtool, lots in btrfs)

[David Sterba]

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.

Re: linux-next: Tree for Dec 6 (objtool, lots in btrfs)

[David Sterba]

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
-- 

Re: linux-next: Tree for Dec 6 (objtool, lots in btrfs)

[Josh Poimboeuf]

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

Re: linux-next: Tree for Dec 6 (objtool, lots in btrfs)

[David Sterba]

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.

Re: linux-next: Tree for Dec 6 (objtool, lots in btrfs)

[Josh Poimboeuf]

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