Re: This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations)

2016-04-19 Thread Ingo Molnar 

* Josh Poimboeuf  wrote:

> On Sat, Apr 16, 2016 at 11:03:32AM +0200, Ingo Molnar wrote:
> > 
> > * Josh Poimboeuf  wrote:
> > 
> > > > I don't think we know yet if there's a reliable way to turn the bug off.
> > > > 
> > > > Also, according to the gcc guys, this bug won't always result in a
> > > > truncated function, and may sometimes just make some inline function
> > > > call sites disappear:
> > > > 
> > > >   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70646#c14
> > > > 
> > > > though I haven't been able to confirm that experimentally.  But if it's
> > > > true, that means that objtool won't be able to detect all cases of the
> > > > bug and some function calls may just silently disappear!
> > > > 
> > > > There's a lot of activity in the bug now, so hopefully they'll be able
> > > > to tell us soon if there's a reliable way to avoid it and/or detect it.
> > > > 
> > > > BTW, Denys posted a workaround patch for the qla2 code:
> > > > 
> > > >   
> > > > https://lkml.kernel.org/r/1460716583-15673-1-git-send-email-dvlas...@redhat.com
> > > 
> > > Martin Jambor wrote a succinct summary of the conditions needed for this
> > > bug:
> > > 
> > >   "This bug can occur when an inlineable function containing a call to
> > >   __builtin_constant_p, which checks a parameter or a value it
> > >   references and a (possibly indirect) caller of the function actually
> > >   passes a constant, but stores it using a type of a different size."
> > > 
> > > So to prevent it from happening elsewhere in the kernel, it sounds like
> > > we'd have to either remove all uses of __builtin_constant_p() or disable
> > > inlining completely.
> > > 
> > > There's also no reliable way to detect the bug has occurred, though
> > > objtool will detect it in cases when the function gets truncated.
> > 
> > So it appears to me that due to the hard to detect nature of the GCC bug 
> > the fix 
> > will probably be backported by them, so I think we should be fine with 
> > relying on 
> > objtool to detect weird code sequences in the kernel, and should work 
> > around 
> > specific instances of the bug.
> 
> I agree.  So how should we work around the bug in this case?  There have
> been several suggestions:
> 
> - change wwn_to_u64() to __always_inline
> 
> - change qla2x00_get_host_fabric_name() to skip the unnecessary call to
>   wwn_to_u64()
> 
> - revert one of the two commits:
>   bc27fb68aaad ("include/uapi/linux/byteorder, swab: force inlining of some 
> byteswap operations")
>   ef3fb2422ffe ("scsi: fc: use get/put_unaligned64 for wwn access")

The first option sounds like the best one by far: it does a change that is 
related 
to the GCC bug (tweaks inlining), has near zero impact and does not revert 
other 
useful progress.

Thanks,

Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations)

2016-04-18 Thread Arnd Bergmann 
On Monday 18 April 2016 08:39:32 Josh Poimboeuf wrote:
> 
> I agree.  So how should we work around the bug in this case?  There have
> been several suggestions:
> 
> - change wwn_to_u64() to __always_inline
> 
> - change qla2x00_get_host_fabric_name() to skip the unnecessary call to
>   wwn_to_u64()
> 
> - revert one of the two commits:
>   bc27fb68aaad ("include/uapi/linux/byteorder, swab: force inlining of some 
> byteswap operations")
>   ef3fb2422ffe ("scsi: fc: use get/put_unaligned64 for wwn access")

What about the patch to change get_unaligned_be64() that I posted?

I think we want to merge that anyway, I just don't know if that helps
with this particular problem as well.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations)

2016-04-18 Thread Arnd Bergmann 
On Monday 18 April 2016 09:12:41 Josh Poimboeuf wrote:
> On Mon, Apr 18, 2016 at 04:07:51PM +0200, Arnd Bergmann wrote:
> > On Monday 18 April 2016 08:39:32 Josh Poimboeuf wrote:
> > > 
> > > I agree.  So how should we work around the bug in this case?  There have
> > > been several suggestions:
> > > 
> > > - change wwn_to_u64() to __always_inline
> > > 
> > > - change qla2x00_get_host_fabric_name() to skip the unnecessary call to
> > >   wwn_to_u64()
> > > 
> > > - revert one of the two commits:
> > >   bc27fb68aaad ("include/uapi/linux/byteorder, swab: force inlining of 
> > > some byteswap operations")
> > >   ef3fb2422ffe ("scsi: fc: use get/put_unaligned64 for wwn access")
> > 
> > What about the patch to change get_unaligned_be64() that I posted?
> > 
> > I think we want to merge that anyway, I just don't know if that helps
> > with this particular problem as well.
> 
> I replied to your other email about that -- it doesn't seem to help this
> issue.
> 

Ok, I see. I had problems with my mail server last week, your reply
must have been a victim of that as I never saw it (found it on the
web archive now).

I'd vote for the wwn_to_u64 change then as it should prevent the
same thing from happining in other drivers. I would prefer not to
see ef3fb2422ffe reverted, as that works around another gcc-6 bug
on ARM.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations)

2016-04-18 Thread Josh Poimboeuf 
On Mon, Apr 18, 2016 at 04:07:51PM +0200, Arnd Bergmann wrote:
> On Monday 18 April 2016 08:39:32 Josh Poimboeuf wrote:
> > 
> > I agree.  So how should we work around the bug in this case?  There have
> > been several suggestions:
> > 
> > - change wwn_to_u64() to __always_inline
> > 
> > - change qla2x00_get_host_fabric_name() to skip the unnecessary call to
> >   wwn_to_u64()
> > 
> > - revert one of the two commits:
> >   bc27fb68aaad ("include/uapi/linux/byteorder, swab: force inlining of some 
> > byteswap operations")
> >   ef3fb2422ffe ("scsi: fc: use get/put_unaligned64 for wwn access")
> 
> What about the patch to change get_unaligned_be64() that I posted?
> 
> I think we want to merge that anyway, I just don't know if that helps
> with this particular problem as well.

I replied to your other email about that -- it doesn't seem to help this
issue.

-- 
Josh
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations)

2016-04-18 Thread Josh Poimboeuf 
On Sat, Apr 16, 2016 at 11:03:32AM +0200, Ingo Molnar wrote:
> 
> * Josh Poimboeuf  wrote:
> 
> > > I don't think we know yet if there's a reliable way to turn the bug off.
> > > 
> > > Also, according to the gcc guys, this bug won't always result in a
> > > truncated function, and may sometimes just make some inline function
> > > call sites disappear:
> > > 
> > >   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70646#c14
> > > 
> > > though I haven't been able to confirm that experimentally.  But if it's
> > > true, that means that objtool won't be able to detect all cases of the
> > > bug and some function calls may just silently disappear!
> > > 
> > > There's a lot of activity in the bug now, so hopefully they'll be able
> > > to tell us soon if there's a reliable way to avoid it and/or detect it.
> > > 
> > > BTW, Denys posted a workaround patch for the qla2 code:
> > > 
> > >   
> > > https://lkml.kernel.org/r/1460716583-15673-1-git-send-email-dvlas...@redhat.com
> > 
> > Martin Jambor wrote a succinct summary of the conditions needed for this
> > bug:
> > 
> >   "This bug can occur when an inlineable function containing a call to
> >   __builtin_constant_p, which checks a parameter or a value it
> >   references and a (possibly indirect) caller of the function actually
> >   passes a constant, but stores it using a type of a different size."
> > 
> > So to prevent it from happening elsewhere in the kernel, it sounds like
> > we'd have to either remove all uses of __builtin_constant_p() or disable
> > inlining completely.
> > 
> > There's also no reliable way to detect the bug has occurred, though
> > objtool will detect it in cases when the function gets truncated.
> 
> So it appears to me that due to the hard to detect nature of the GCC bug the 
> fix 
> will probably be backported by them, so I think we should be fine with 
> relying on 
> objtool to detect weird code sequences in the kernel, and should work around 
> specific instances of the bug.

I agree.  So how should we work around the bug in this case?  There have
been several suggestions:

- change wwn_to_u64() to __always_inline

- change qla2x00_get_host_fabric_name() to skip the unnecessary call to
  wwn_to_u64()

- revert one of the two commits:
  bc27fb68aaad ("include/uapi/linux/byteorder, swab: force inlining of some 
byteswap operations")
  ef3fb2422ffe ("scsi: fc: use get/put_unaligned64 for wwn access")


-- 
Josh
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations)

2016-04-18 Thread Josh Poimboeuf 
On Sat, Apr 16, 2016 at 09:42:37AM +0200, Arnd Bergmann wrote:
> On Friday 15 April 2016 07:45:19 Ingo Molnar wrote:
> > 
> > * Denys Vlasenko  wrote:
> > 
> > > > In fact, the following patch seems to fix it:
> > > > 
> > > > diff --git a/include/scsi/scsi_transport_fc.h 
> > > > b/include/scsi/scsi_transport_fc.h
> > > > index bf66ea6..56b9e81 100644
> > > > --- a/include/scsi/scsi_transport_fc.h
> > > > +++ b/include/scsi/scsi_transport_fc.h
> > > > @@ -796,7 +796,7 @@ fc_remote_port_chkready(struct fc_rport *rport)
> > > > return result;
> > > >  }
> > > >  
> > > > -static inline u64 wwn_to_u64(u8 *wwn)
> > > > +static __always_inline u64 wwn_to_u64(u8 *wwn)
> > > >  {
> > > > return get_unaligned_be64(wwn);
> > > >  }
> > > 
> > > It is not a guarantee.
> > 
> > Of course it's a workaround - but is there any deterministic way to turn 
> > off this 
> > GCC bug (by activating some GCC command line switch), or do we have to live 
> > with 
> > objtool warning about this GCC?
> > 
> > Which, by the way, is pretty cool!
> 
> I have done a patch for the asm-generic/unaligned handling recently that
> reworks the implementation to avoid an ARM specific bug (gcc uses certain
> CPU instructions that require aligned data when we tell it that unaligned
> data is not).
> 
> It changes the code enough that the gcc bug might not be triggered any more,
> aside from generating far superior code in some cases.

I tried this patch, but unfortunately it doesn't make the gcc bug go
away.

-- 
Josh
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations)

2016-04-16 Thread Ingo Molnar 

* Josh Poimboeuf  wrote:

> > I don't think we know yet if there's a reliable way to turn the bug off.
> > 
> > Also, according to the gcc guys, this bug won't always result in a
> > truncated function, and may sometimes just make some inline function
> > call sites disappear:
> > 
> >   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70646#c14
> > 
> > though I haven't been able to confirm that experimentally.  But if it's
> > true, that means that objtool won't be able to detect all cases of the
> > bug and some function calls may just silently disappear!
> > 
> > There's a lot of activity in the bug now, so hopefully they'll be able
> > to tell us soon if there's a reliable way to avoid it and/or detect it.
> > 
> > BTW, Denys posted a workaround patch for the qla2 code:
> > 
> >   
> > https://lkml.kernel.org/r/1460716583-15673-1-git-send-email-dvlas...@redhat.com
> 
> Martin Jambor wrote a succinct summary of the conditions needed for this
> bug:
> 
>   "This bug can occur when an inlineable function containing a call to
>   __builtin_constant_p, which checks a parameter or a value it
>   references and a (possibly indirect) caller of the function actually
>   passes a constant, but stores it using a type of a different size."
> 
> So to prevent it from happening elsewhere in the kernel, it sounds like
> we'd have to either remove all uses of __builtin_constant_p() or disable
> inlining completely.
> 
> There's also no reliable way to detect the bug has occurred, though
> objtool will detect it in cases when the function gets truncated.

So it appears to me that due to the hard to detect nature of the GCC bug the 
fix 
will probably be backported by them, so I think we should be fine with relying 
on 
objtool to detect weird code sequences in the kernel, and should work around 
specific instances of the bug.

Thanks,

Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations)

2016-04-16 Thread Arnd Bergmann 
On Friday 15 April 2016 07:45:19 Ingo Molnar wrote:
> 
> * Denys Vlasenko  wrote:
> 
> > > In fact, the following patch seems to fix it:
> > > 
> > > diff --git a/include/scsi/scsi_transport_fc.h 
> > > b/include/scsi/scsi_transport_fc.h
> > > index bf66ea6..56b9e81 100644
> > > --- a/include/scsi/scsi_transport_fc.h
> > > +++ b/include/scsi/scsi_transport_fc.h
> > > @@ -796,7 +796,7 @@ fc_remote_port_chkready(struct fc_rport *rport)
> > >   return result;
> > >  }
> > >  
> > > -static inline u64 wwn_to_u64(u8 *wwn)
> > > +static __always_inline u64 wwn_to_u64(u8 *wwn)
> > >  {
> > >   return get_unaligned_be64(wwn);
> > >  }
> > 
> > It is not a guarantee.
> 
> Of course it's a workaround - but is there any deterministic way to turn off 
> this 
> GCC bug (by activating some GCC command line switch), or do we have to live 
> with 
> objtool warning about this GCC?
> 
> Which, by the way, is pretty cool!

I have done a patch for the asm-generic/unaligned handling recently that
reworks the implementation to avoid an ARM specific bug (gcc uses certain
CPU instructions that require aligned data when we tell it that unaligned
data is not).

It changes the code enough that the gcc bug might not be triggered any more,
aside from generating far superior code in some cases.

I thought I had submitted that patch before, but I can't find a version
with a proper changelog any more now, so I probably haven't. However, I did
all the research to show that it only makes things better on ARM and x86
except in cases where the gcc inliner happens to pick a different set of
functions to be inline (these have a 50:50 chance of better vs worse, the
result on average seems to be the same).

Arnd

commit 752b719f6675be02a3dd29fe5d92b2f380b5743d
Author: Arnd Bergmann 
Date:   Fri Mar 4 16:15:20 2016 +0100

asm-generic: always use struct based unaligned access

Signed-off-by: Arnd Bergmann 

diff --git a/include/asm-generic/unaligned.h b/include/asm-generic/unaligned.h
index 1ac097279db1..e8f5523eeb0a 100644
--- a/include/asm-generic/unaligned.h
+++ b/include/asm-generic/unaligned.h
@@ -3,29 +3,19 @@
 
 /*
  * This is the most generic implementation of unaligned accesses
- * and should work almost anywhere.
+ * and should work almost anywhere, we trust that the compiler
+ * knows how to handle unaligned accesses.
  */
 #include 
 
-/* Set by the arch if it can handle unaligned accesses in hardware. */
-#ifdef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
-# include 
-#endif
+#include 
+#include 
+#include 
 
 #if defined(__LITTLE_ENDIAN)
-# ifndef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
-#  include 
-#  include 
-# endif
-# include 
 # define get_unaligned __get_unaligned_le
 # define put_unaligned __put_unaligned_le
 #elif defined(__BIG_ENDIAN)
-# ifndef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
-#  include 
-#  include 
-# endif
-# include 
 # define get_unaligned __get_unaligned_be
 # define put_unaligned __put_unaligned_be
 #else
diff --git a/include/linux/unaligned/be_struct.h 
b/include/linux/unaligned/be_struct.h
index 132415836c50..9ab8c53bb3fe 100644
--- a/include/linux/unaligned/be_struct.h
+++ b/include/linux/unaligned/be_struct.h
@@ -2,35 +2,36 @@
 #define _LINUX_UNALIGNED_BE_STRUCT_H
 
 #include 
+#include 
 
 static inline u16 get_unaligned_be16(const void *p)
 {
-   return __get_unaligned_cpu16((const u8 *)p);
+   return be16_to_cpu((__le16 __force)__get_unaligned_cpu16((const u8 
*)p));
 }
 
 static inline u32 get_unaligned_be32(const void *p)
 {
-   return __get_unaligned_cpu32((const u8 *)p);
+   return be32_to_cpu((__le32 __force)__get_unaligned_cpu32((const u8 
*)p));
 }
 
 static inline u64 get_unaligned_be64(const void *p)
 {
-   return __get_unaligned_cpu64((const u8 *)p);
+   return be64_to_cpu((__le64 __force)__get_unaligned_cpu64((const u8 
*)p));
 }
 
 static inline void put_unaligned_be16(u16 val, void *p)
 {
-   __put_unaligned_cpu16(val, p);
+   __put_unaligned_cpu16((u16 __force)cpu_to_be16(val), p);
 }
 
 static inline void put_unaligned_be32(u32 val, void *p)
 {
-   __put_unaligned_cpu32(val, p);
+   __put_unaligned_cpu32((u32 __force)cpu_to_be32(val), p);
 }
 
 static inline void put_unaligned_be64(u64 val, void *p)
 {
-   __put_unaligned_cpu64(val, p);
+   __put_unaligned_cpu64((u64 __force)cpu_to_be64(val), p);
 }
 
 #endif /* _LINUX_UNALIGNED_BE_STRUCT_H */
diff --git a/include/linux/unaligned/le_struct.h 
b/include/linux/unaligned/le_struct.h
index 088c4572faa8..64171ad0b100 100644
--- a/include/linux/unaligned/le_struct.h
+++ b/include/linux/unaligned/le_struct.h
@@ -2,35 +2,36 @@
 #define _LINUX_UNALIGNED_LE_STRUCT_H
 
 #include 
+#include 
 
 static inline u16 get_unaligned_le16(const void *p)
 {
-   return __get_unaligned_cpu16((const u8 *)p);
+   return le16_to_cpu((__le16 __force)__get_unaligned_cpu16((const u8 
*)p));
 }
 
 static inline u32 get_unaligned_le32(const void

Re: This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations)

2016-04-15 Thread Josh Poimboeuf 
On Fri, Apr 15, 2016 at 08:47:45AM -0500, Josh Poimboeuf wrote:
> On Fri, Apr 15, 2016 at 07:45:19AM +0200, Ingo Molnar wrote:
> > 
> > * Denys Vlasenko  wrote:
> > 
> > > > In fact, the following patch seems to fix it:
> > > > 
> > > > diff --git a/include/scsi/scsi_transport_fc.h 
> > > > b/include/scsi/scsi_transport_fc.h
> > > > index bf66ea6..56b9e81 100644
> > > > --- a/include/scsi/scsi_transport_fc.h
> > > > +++ b/include/scsi/scsi_transport_fc.h
> > > > @@ -796,7 +796,7 @@ fc_remote_port_chkready(struct fc_rport *rport)
> > > > return result;
> > > >  }
> > > >  
> > > > -static inline u64 wwn_to_u64(u8 *wwn)
> > > > +static __always_inline u64 wwn_to_u64(u8 *wwn)
> > > >  {
> > > > return get_unaligned_be64(wwn);
> > > >  }
> > > 
> > > It is not a guarantee.
> > 
> > Of course it's a workaround - but is there any deterministic way to turn 
> > off this 
> > GCC bug (by activating some GCC command line switch), or do we have to live 
> > with 
> > objtool warning about this GCC?
> 
> I don't think we know yet if there's a reliable way to turn the bug off.
> 
> Also, according to the gcc guys, this bug won't always result in a
> truncated function, and may sometimes just make some inline function
> call sites disappear:
> 
>   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70646#c14
> 
> though I haven't been able to confirm that experimentally.  But if it's
> true, that means that objtool won't be able to detect all cases of the
> bug and some function calls may just silently disappear!
> 
> There's a lot of activity in the bug now, so hopefully they'll be able
> to tell us soon if there's a reliable way to avoid it and/or detect it.
> 
> BTW, Denys posted a workaround patch for the qla2 code:
> 
>   
> https://lkml.kernel.org/r/1460716583-15673-1-git-send-email-dvlas...@redhat.com

Martin Jambor wrote a succinct summary of the conditions needed for this
bug:

  "This bug can occur when an inlineable function containing a call to
  __builtin_constant_p, which checks a parameter or a value it
  references and a (possibly indirect) caller of the function actually
  passes a constant, but stores it using a type of a different size."

So to prevent it from happening elsewhere in the kernel, it sounds like
we'd have to either remove all uses of __builtin_constant_p() or disable
inlining completely.

There's also no reliable way to detect the bug has occurred, though
objtool will detect it in cases when the function gets truncated.

-- 
Josh
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations)

2016-04-15 Thread Josh Poimboeuf 
On Fri, Apr 15, 2016 at 07:45:19AM +0200, Ingo Molnar wrote:
> 
> * Denys Vlasenko  wrote:
> 
> > > In fact, the following patch seems to fix it:
> > > 
> > > diff --git a/include/scsi/scsi_transport_fc.h 
> > > b/include/scsi/scsi_transport_fc.h
> > > index bf66ea6..56b9e81 100644
> > > --- a/include/scsi/scsi_transport_fc.h
> > > +++ b/include/scsi/scsi_transport_fc.h
> > > @@ -796,7 +796,7 @@ fc_remote_port_chkready(struct fc_rport *rport)
> > >   return result;
> > >  }
> > >  
> > > -static inline u64 wwn_to_u64(u8 *wwn)
> > > +static __always_inline u64 wwn_to_u64(u8 *wwn)
> > >  {
> > >   return get_unaligned_be64(wwn);
> > >  }
> > 
> > It is not a guarantee.
> 
> Of course it's a workaround - but is there any deterministic way to turn off 
> this 
> GCC bug (by activating some GCC command line switch), or do we have to live 
> with 
> objtool warning about this GCC?

I don't think we know yet if there's a reliable way to turn the bug off.

Also, according to the gcc guys, this bug won't always result in a
truncated function, and may sometimes just make some inline function
call sites disappear:

  https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70646#c14

though I haven't been able to confirm that experimentally.  But if it's
true, that means that objtool won't be able to detect all cases of the
bug and some function calls may just silently disappear!

There's a lot of activity in the bug now, so hopefully they'll be able
to tell us soon if there's a reliable way to avoid it and/or detect it.

BTW, Denys posted a workaround patch for the qla2 code:

  
https://lkml.kernel.org/r/1460716583-15673-1-git-send-email-dvlas...@redhat.com


-- 
Josh
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations)

2016-04-14 Thread Ingo Molnar 

* Denys Vlasenko  wrote:

> > In fact, the following patch seems to fix it:
> > 
> > diff --git a/include/scsi/scsi_transport_fc.h 
> > b/include/scsi/scsi_transport_fc.h
> > index bf66ea6..56b9e81 100644
> > --- a/include/scsi/scsi_transport_fc.h
> > +++ b/include/scsi/scsi_transport_fc.h
> > @@ -796,7 +796,7 @@ fc_remote_port_chkready(struct fc_rport *rport)
> > return result;
> >  }
> >  
> > -static inline u64 wwn_to_u64(u8 *wwn)
> > +static __always_inline u64 wwn_to_u64(u8 *wwn)
> >  {
> > return get_unaligned_be64(wwn);
> >  }
> 
> It is not a guarantee.

Of course it's a workaround - but is there any deterministic way to turn off 
this 
GCC bug (by activating some GCC command line switch), or do we have to live 
with 
objtool warning about this GCC?

Which, by the way, is pretty cool!

Thanks,

Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations)

2016-04-14 Thread Josh Poimboeuf 
On Thu, Apr 14, 2016 at 05:29:06PM +0200, Denys Vlasenko wrote:
> On 04/13/2016 07:10 PM, Josh Poimboeuf wrote:
> >> From the disassembly of drivers/scsi/qla2xxx/qla_attr.o:
> >>
> >> 2f53 :
> >> 2f53:   55  push   %rbp
> >> 2f54:   48 89 e5mov%rsp,%rbp
> >>
> >> 2f57 :
> >> 2f57:   55  push   %rbp
> >> 2f58:   b9 e8 00 00 00  mov$0xe8,%ecx
> >> 2f5d:   48 89 e5mov%rsp,%rbp
> >> ...
> >>
> >> Note that qla2x00_get_host_fabric_name() is inexplicably
> >> truncated after
> >> setting up the frame pointer.  It falls through to the next
> >> function, which is
> >> very wrong.
> >
> > Wow, that's ... interesting.
> >
> >
> >> I can recreate it with either gcc 5.3.1 or gcc 6.0 on
> >> linus/master with
> >> the .config from the above link.
> >>
> >> The call chain which appears to trigger the problem is:
> >>
> >> qla2x00_get_host_fabric_name()
> >>   wwn_to_u64()
> >> get_unaligned_be64()
> >>   be64_to_cpup()
> >> __be64_to_cpup() <- changed to __always_inline by this
> >> patch
> >>
> >> It occurs with the combination of the following two recent
> >> commits:
> >>
> >> - bc27fb68aaad ("include/uapi/linux/byteorder, swab: force
> >> inlining of some byteswap operations")
> >> - ef3fb2422ffe ("scsi: fc: use get/put_unaligned64 for wwn
> >> access")
> >>
> >> I can confirm that reverting either patch makes the problem go
> >> away.
> >> I'm planning on opening a gcc bug tomorrow.
> >
> >
> > Note that if CONFIG_OPTIMIZE_INLINING is not set, _all_ "inline"
> > keywords are in fact __always_inline, so the bug must be
> > triggering
> > even without the patch.
> 
>  Makes sense in theory, but the bug doesn't actually trigger when I
>  revert the patch and set CONFIG_OPTIMIZE_INLINING=n.
> 
>  Perhaps even more surprising, it doesn't trigger *with* the patch
>  and
>  CONFIG_OPTIMIZE_INLINING=n.
> >>>
> >>> [ Adding James to CC since this bug affects scsi. ]
> >>>
> >>> Here's the gcc bug:
> >>>
> >>>   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70646
> >>>
> >>
> >>
> >> Actually, adding me doesn't help, I've added linux-scsi.  The summary
> >> is that there's a but in gcc-5.3.1 which is miscompiling qla_attr.c ...
> >> this means we're going to have to ask the compiler version of reported
> >> crashes.
> > 
> > The bug isn't specific to a compiler version.  I've seen it with gcc
> > 5.3.1 and gcc 6.0.  I haven't tried any older versions.  And the gcc bug
> > hasn't been resolved (or even investigated) yet.
> > 
> > The bug is triggered by a combination of the above two commits from the
> > 4.6 merge window, so presumably we'd need to revert one of them to avoid
> > crashes in 4.6.
> 
> The bug is indeed in the compiler. 4.9 and all later versions are affected.
> gcc bugzilla now has a reproducer. In abridged form:
> 
> 
> static inline __attribute__((always_inline)) u64 __swab64p(const u64 *p)
> {
>  return (__builtin_constant_p((u64)(*p)) ? ((u64)( (((u64)(*p) & 
> (u64)0x00ffULL) << 56) | (((u64)(*p) & 
> (u64)0xff00ULL) << 40) | (((u64)(*p) & 
> (u64)0x00ffULL) << 24) | (((u64)(*p) & 
> (u64)0xff00ULL) << 8) | (((u64)(*p) & (u64)0x00ffULL) 
> >> 8) | (((u64)(*p) & (u64)0xff00ULL) >> 24) | (((u64)(*p) & 
> (u64)0x00ffULL) >> 40) | (((u64)(*p) & 
> (u64)0xff00ULL) >> 56))) : __builtin_bswap64(*p));
> }
> static inline u64 wwn_to_u64(void *wwn)
> {
>  return __swab64p(wwn);
> }
> static void qla2x00_get_host_fabric_name(struct Scsi_Host *shost)
> {
>  scsi_qla_host_t *vha = shost_priv(shost);
>  u8 node_name[8] = { 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF};
>  u64 fabric_name = wwn_to_u64(node_name);
>  if (vha->device_flags & 0x1)
>   fabric_name = wwn_to_u64(vha->fabric_node_name);
>  (((struct fc_host_attrs *)(shost)->shost_data)->fabric_name) = fabric_name;
> }

Nice work with the reproducer!

> Two (or more, there were more before simplification) levels of inlining
> are necessary for bug to trigger in this example (folding to one level
> makes it go away). "__attribute__((always_inline))" is necessary too.
> 
> 
> Since we have lots of __always_inline anyway, this bug has a potential
> to miscompile kernels regardless of CONFIG_OPTIMIZE_INLINING setting,
> and with or without the patches mentioned above (they just happen
> to create a reliable reproducer).

Well, but setting !CONFIG_OPTIMIZE_INLINING makes the problem go away
for some reason.  It seems like the bug requires wwn_to_u64() being
out-of-line and __swab64p() being in-line.

In fact, the following patch seems to fix it:

diff --git

Re: This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations)

2016-04-14 Thread Denys Vlasenko 
On 04/13/2016 07:10 PM, Josh Poimboeuf wrote:
>> From the disassembly of drivers/scsi/qla2xxx/qla_attr.o:
>>
>> 2f53 :
>> 2f53:   55  push   %rbp
>> 2f54:   48 89 e5mov%rsp,%rbp
>>
>> 2f57 :
>> 2f57:   55  push   %rbp
>> 2f58:   b9 e8 00 00 00  mov$0xe8,%ecx
>> 2f5d:   48 89 e5mov%rsp,%rbp
>> ...
>>
>> Note that qla2x00_get_host_fabric_name() is inexplicably
>> truncated after
>> setting up the frame pointer.  It falls through to the next
>> function, which is
>> very wrong.
>
> Wow, that's ... interesting.
>
>
>> I can recreate it with either gcc 5.3.1 or gcc 6.0 on
>> linus/master with
>> the .config from the above link.
>>
>> The call chain which appears to trigger the problem is:
>>
>> qla2x00_get_host_fabric_name()
>>   wwn_to_u64()
>> get_unaligned_be64()
>>   be64_to_cpup()
>> __be64_to_cpup() <- changed to __always_inline by this
>> patch
>>
>> It occurs with the combination of the following two recent
>> commits:
>>
>> - bc27fb68aaad ("include/uapi/linux/byteorder, swab: force
>> inlining of some byteswap operations")
>> - ef3fb2422ffe ("scsi: fc: use get/put_unaligned64 for wwn
>> access")
>>
>> I can confirm that reverting either patch makes the problem go
>> away.
>> I'm planning on opening a gcc bug tomorrow.
>
>
> Note that if CONFIG_OPTIMIZE_INLINING is not set, _all_ "inline"
> keywords are in fact __always_inline, so the bug must be
> triggering
> even without the patch.

 Makes sense in theory, but the bug doesn't actually trigger when I
 revert the patch and set CONFIG_OPTIMIZE_INLINING=n.

 Perhaps even more surprising, it doesn't trigger *with* the patch
 and
 CONFIG_OPTIMIZE_INLINING=n.
>>>
>>> [ Adding James to CC since this bug affects scsi. ]
>>>
>>> Here's the gcc bug:
>>>
>>>   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70646
>>>
>>
>>
>> Actually, adding me doesn't help, I've added linux-scsi.  The summary
>> is that there's a but in gcc-5.3.1 which is miscompiling qla_attr.c ...
>> this means we're going to have to ask the compiler version of reported
>> crashes.
> 
> The bug isn't specific to a compiler version.  I've seen it with gcc
> 5.3.1 and gcc 6.0.  I haven't tried any older versions.  And the gcc bug
> hasn't been resolved (or even investigated) yet.
> 
> The bug is triggered by a combination of the above two commits from the
> 4.6 merge window, so presumably we'd need to revert one of them to avoid
> crashes in 4.6.

The bug is indeed in the compiler. 4.9 and all later versions are affected.
gcc bugzilla now has a reproducer. In abridged form:


static inline __attribute__((always_inline)) u64 __swab64p(const u64 *p)
{
 return (__builtin_constant_p((u64)(*p)) ? ((u64)( (((u64)(*p) & 
(u64)0x00ffULL) << 56) | (((u64)(*p) & (u64)0xff00ULL) 
<< 40) | (((u64)(*p) & (u64)0x00ffULL) << 24) | (((u64)(*p) & 
(u64)0xff00ULL) << 8) | (((u64)(*p) & (u64)0x00ffULL) 
>> 8) | (((u64)(*p) & (u64)0xff00ULL) >> 24) | (((u64)(*p) & 
(u64)0x00ffULL) >> 40) | (((u64)(*p) & (u64)0xff00ULL) 
>> 56))) : __builtin_bswap64(*p));
}
static inline u64 wwn_to_u64(void *wwn)
{
 return __swab64p(wwn);
}
static void qla2x00_get_host_fabric_name(struct Scsi_Host *shost)
{
 scsi_qla_host_t *vha = shost_priv(shost);
 u8 node_name[8] = { 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF};
 u64 fabric_name = wwn_to_u64(node_name);
 if (vha->device_flags & 0x1)
  fabric_name = wwn_to_u64(vha->fabric_node_name);
 (((struct fc_host_attrs *)(shost)->shost_data)->fabric_name) = fabric_name;
}


Two (or more, there were more before simplification) levels of inlining
are necessary for bug to trigger in this example (folding to one level
makes it go away). "__attribute__((always_inline))" is necessary too.


Since we have lots of __always_inline anyway, this bug has a potential
to miscompile kernels regardless of CONFIG_OPTIMIZE_INLINING setting,
and with or without the patches mentioned above (they just happen
to create a reliable reproducer).

Since it was not detected for two years since gcc 4.9 release,
it must be triggering quite rarely.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations)

2016-04-13 Thread Josh Poimboeuf 
On Wed, Apr 13, 2016 at 09:55:09AM -0700, James Bottomley wrote:
> On Wed, 2016-04-13 at 10:15 -0500, Josh Poimboeuf wrote:
> > On Wed, Apr 13, 2016 at 07:36:07AM -0500, Josh Poimboeuf wrote:
> > > On Wed, Apr 13, 2016 at 02:12:25PM +0200, Denys Vlasenko wrote:
> > > > On 04/13/2016 05:36 AM, Josh Poimboeuf wrote:
> > > > > On Thu, Feb 04, 2016 at 08:45:35PM +0100, Denys Vlasenko wrote:
> > > > > > Sometimes gcc mysteriously doesn't inline
> > > > > > very small functions we expect to be inlined. See
> > > > > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66122
> > > > > > 
> > > > > > With this .config:
> > > > > > http://busybox.net/~vda/kernel_config_OPTIMIZE_INLINING_and_O
> > > > > > s,
> > > > > > the following functions get deinlined many times.
> > > > > > Examples of disassembly:
> > > > > > 
> > > > > >  (12 copies, 51 calls):
> > > > > >66 8b 07mov(%rdi),%ax
> > > > > >55  push   %rbp
> > > > > >48 89 e5mov%rsp,%rbp
> > > > > >86 e0   xchg   %ah,%al
> > > > > >5d  pop%rbp
> > > > > >c3  retq
> > > > ...
> > > > > > This patch fixes this via s/inline/__always_inline/.
> > > > > > Code size decrease after the patch is ~4.5k:
> > > > > > 
> > > > > > text data  bss   dec hex filename
> > > > > > 92202377 20826112 36417536 149446025 8e85d89 vmlinux
> > > > > > 92197848 20826112 36417536 149441496 8e84bd8
> > > > > > vmlinux5_swap_after
> > > > > > 
> > > > > > Signed-off-by: Denys Vlasenko 
> > > > > > Cc: Ingo Molnar 
> > > > > > Cc: Thomas Graf 
> > > > > > Cc: Peter Zijlstra 
> > > > > > Cc: David Rientjes 
> > > > > > Cc: Andrew Morton 
> > > > > > Cc: linux-ker...@vger.kernel.org
> > > > > > ---
> > > > > >  include/uapi/linux/byteorder/big_endian.h| 24
> > > > > > 
> > > > > >  include/uapi/linux/byteorder/little_endian.h | 24
> > > > > > 
> > > > > >  include/uapi/linux/swab.h| 10 +-
> > > > > >  3 files changed, 29 insertions(+), 29 deletions(-)
> > > > > 
> > > > > Hi,
> > > > > 
> > > > > This patch seems to trigger a gcc bug which can produce corrupt
> > > > > code.  I
> > > > > discovered it when investigating an objtool warning reported by
> > > > > kbuild
> > > > > bot:
> > > > > 
> > > > >   http://www.spinics.net/lists/linux-scsi/msg95481.html
> > > > > 
> > > > > From the disassembly of drivers/scsi/qla2xxx/qla_attr.o:
> > > > > 
> > > > > 2f53 :
> > > > > 2f53:   55  push   %rbp
> > > > > 2f54:   48 89 e5mov%rsp,%rbp
> > > > > 
> > > > > 2f57 :
> > > > > 2f57:   55  push   %rbp
> > > > > 2f58:   b9 e8 00 00 00  mov$0xe8,%ecx
> > > > > 2f5d:   48 89 e5mov%rsp,%rbp
> > > > > ...
> > > > > 
> > > > > Note that qla2x00_get_host_fabric_name() is inexplicably
> > > > > truncated after
> > > > > setting up the frame pointer.  It falls through to the next
> > > > > function, which is
> > > > > very wrong.
> > > > 
> > > > Wow, that's ... interesting.
> > > > 
> > > > 
> > > > > I can recreate it with either gcc 5.3.1 or gcc 6.0 on
> > > > > linus/master with
> > > > > the .config from the above link.
> > > > > 
> > > > > The call chain which appears to trigger the problem is:
> > > > > 
> > > > > qla2x00_get_host_fabric_name()
> > > > >   wwn_to_u64()
> > > > > get_unaligned_be64()
> > > > >   be64_to_cpup()
> > > > > __be64_to_cpup() <- changed to __always_inline by this
> > > > > patch
> > > > > 
> > > > > It occurs with the combination of the following two recent
> > > > > commits:
> > > > > 
> > > > > - bc27fb68aaad ("include/uapi/linux/byteorder, swab: force
> > > > > inlining of some byteswap operations")
> > > > > - ef3fb2422ffe ("scsi: fc: use get/put_unaligned64 for wwn
> > > > > access")
> > > > > 
> > > > > I can confirm that reverting either patch makes the problem go
> > > > > away.
> > > > > I'm planning on opening a gcc bug tomorrow.
> > > > 
> > > > 
> > > > Note that if CONFIG_OPTIMIZE_INLINING is not set, _all_ "inline"
> > > > keywords are in fact __always_inline, so the bug must be
> > > > triggering
> > > > even without the patch.
> > > 
> > > Makes sense in theory, but the bug doesn't actually trigger when I
> > > revert the patch and set CONFIG_OPTIMIZE_INLINING=n.
> > > 
> > > Perhaps even more surprising, it doesn't trigger *with* the patch
> > > and
> > > CONFIG_OPTIMIZE_INLINING=n.
> > 
> > [ Adding James to CC since this bug affects scsi. ]
> > 
> > Here's the gcc bug:
> > 
> >   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70646
> > 
> 
> 
> Actually, adding me doesn't help, I've added linux-scsi.  The

Re: This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations)

2016-04-13 Thread James Bottomley 
On Wed, 2016-04-13 at 10:15 -0500, Josh Poimboeuf wrote:
> On Wed, Apr 13, 2016 at 07:36:07AM -0500, Josh Poimboeuf wrote:
> > On Wed, Apr 13, 2016 at 02:12:25PM +0200, Denys Vlasenko wrote:
> > > On 04/13/2016 05:36 AM, Josh Poimboeuf wrote:
> > > > On Thu, Feb 04, 2016 at 08:45:35PM +0100, Denys Vlasenko wrote:
> > > > > Sometimes gcc mysteriously doesn't inline
> > > > > very small functions we expect to be inlined. See
> > > > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66122
> > > > > 
> > > > > With this .config:
> > > > > http://busybox.net/~vda/kernel_config_OPTIMIZE_INLINING_and_O
> > > > > s,
> > > > > the following functions get deinlined many times.
> > > > > Examples of disassembly:
> > > > > 
> > > > >  (12 copies, 51 calls):
> > > > >66 8b 07mov(%rdi),%ax
> > > > >55  push   %rbp
> > > > >48 89 e5mov%rsp,%rbp
> > > > >86 e0   xchg   %ah,%al
> > > > >5d  pop%rbp
> > > > >c3  retq
> > > ...
> > > > > This patch fixes this via s/inline/__always_inline/.
> > > > > Code size decrease after the patch is ~4.5k:
> > > > > 
> > > > > text data  bss   dec hex filename
> > > > > 92202377 20826112 36417536 149446025 8e85d89 vmlinux
> > > > > 92197848 20826112 36417536 149441496 8e84bd8
> > > > > vmlinux5_swap_after
> > > > > 
> > > > > Signed-off-by: Denys Vlasenko 
> > > > > Cc: Ingo Molnar 
> > > > > Cc: Thomas Graf 
> > > > > Cc: Peter Zijlstra 
> > > > > Cc: David Rientjes 
> > > > > Cc: Andrew Morton 
> > > > > Cc: linux-ker...@vger.kernel.org
> > > > > ---
> > > > >  include/uapi/linux/byteorder/big_endian.h| 24
> > > > > 
> > > > >  include/uapi/linux/byteorder/little_endian.h | 24
> > > > > 
> > > > >  include/uapi/linux/swab.h| 10 +-
> > > > >  3 files changed, 29 insertions(+), 29 deletions(-)
> > > > 
> > > > Hi,
> > > > 
> > > > This patch seems to trigger a gcc bug which can produce corrupt
> > > > code.  I
> > > > discovered it when investigating an objtool warning reported by
> > > > kbuild
> > > > bot:
> > > > 
> > > >   http://www.spinics.net/lists/linux-scsi/msg95481.html
> > > > 
> > > > From the disassembly of drivers/scsi/qla2xxx/qla_attr.o:
> > > > 
> > > > 2f53 :
> > > > 2f53:   55  push   %rbp
> > > > 2f54:   48 89 e5mov%rsp,%rbp
> > > > 
> > > > 2f57 :
> > > > 2f57:   55  push   %rbp
> > > > 2f58:   b9 e8 00 00 00  mov$0xe8,%ecx
> > > > 2f5d:   48 89 e5mov%rsp,%rbp
> > > > ...
> > > > 
> > > > Note that qla2x00_get_host_fabric_name() is inexplicably
> > > > truncated after
> > > > setting up the frame pointer.  It falls through to the next
> > > > function, which is
> > > > very wrong.
> > > 
> > > Wow, that's ... interesting.
> > > 
> > > 
> > > > I can recreate it with either gcc 5.3.1 or gcc 6.0 on
> > > > linus/master with
> > > > the .config from the above link.
> > > > 
> > > > The call chain which appears to trigger the problem is:
> > > > 
> > > > qla2x00_get_host_fabric_name()
> > > >   wwn_to_u64()
> > > > get_unaligned_be64()
> > > >   be64_to_cpup()
> > > > __be64_to_cpup() <- changed to __always_inline by this
> > > > patch
> > > > 
> > > > It occurs with the combination of the following two recent
> > > > commits:
> > > > 
> > > > - bc27fb68aaad ("include/uapi/linux/byteorder, swab: force
> > > > inlining of some byteswap operations")
> > > > - ef3fb2422ffe ("scsi: fc: use get/put_unaligned64 for wwn
> > > > access")
> > > > 
> > > > I can confirm that reverting either patch makes the problem go
> > > > away.
> > > > I'm planning on opening a gcc bug tomorrow.
> > > 
> > > 
> > > Note that if CONFIG_OPTIMIZE_INLINING is not set, _all_ "inline"
> > > keywords are in fact __always_inline, so the bug must be
> > > triggering
> > > even without the patch.
> > 
> > Makes sense in theory, but the bug doesn't actually trigger when I
> > revert the patch and set CONFIG_OPTIMIZE_INLINING=n.
> > 
> > Perhaps even more surprising, it doesn't trigger *with* the patch
> > and
> > CONFIG_OPTIMIZE_INLINING=n.
> 
> [ Adding James to CC since this bug affects scsi. ]
> 
> Here's the gcc bug:
> 
>   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70646
> 


Actually, adding me doesn't help, I've added linux-scsi.  The summary
is that there's a but in gcc-5.3.1 which is miscompiling qla_attr.c ...
this means we're going to have to ask the compiler version of reported
crashes.

James

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo