Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1377489 > unrolled thread
| Started by | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| First post | 2016-04-13 05:40 +0200 |
| Last post | 2016-04-15 15:50 +0200 |
| Articles | 11 — 4 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations) Josh Poimboeuf <jpoimboe@redhat.com> - 2016-04-13 05:40 +0200
Re: This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations) Denys Vlasenko <dvlasenk@redhat.com> - 2016-04-13 14:20 +0200
Re: This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations) Josh Poimboeuf <jpoimboe@redhat.com> - 2016-04-13 14:40 +0200
Re: This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations) Josh Poimboeuf <jpoimboe@redhat.com> - 2016-04-13 17:20 +0200
Re: This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations) James Bottomley <James.Bottomley@HansenPartnership.com> - 2016-04-13 19:00 +0200
Re: This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations) Josh Poimboeuf <jpoimboe@redhat.com> - 2016-04-13 19:20 +0200
Re: This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations) Denys Vlasenko <dvlasenk@redhat.com> - 2016-04-14 17:30 +0200
Re: This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations) Josh Poimboeuf <jpoimboe@redhat.com> - 2016-04-14 18:00 +0200
Re: This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations) Denys Vlasenko <dvlasenk@redhat.com> - 2016-04-14 19:10 +0200
Re: This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations) Ingo Molnar <mingo@kernel.org> - 2016-04-15 07:50 +0200
Re: This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations) Josh Poimboeuf <jpoimboe@redhat.com> - 2016-04-15 15:50 +0200
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2016-04-13 05:40 +0200 |
| Subject | This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations) |
| Message-ID | <rnjOO-2xX-11@gated-at.bofh.it> |
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_Os,
> the following functions get deinlined many times.
> Examples of disassembly:
>
> <get_unaligned_be16> (12 copies, 51 calls):
> 66 8b 07 mov (%rdi),%ax
> 55 push %rbp
> 48 89 e5 mov %rsp,%rbp
> 86 e0 xchg %ah,%al
> 5d pop %rbp
> c3 retq
>
> <get_unaligned_be32> (12 copies, 135 calls):
> 8b 07 mov (%rdi),%eax
> 55 push %rbp
> 48 89 e5 mov %rsp,%rbp
> 0f c8 bswap %eax
> 5d pop %rbp
> c3 retq
>
> <get_unaligned_be64> (2 copies, 20 calls):
> 48 8b 07 mov (%rdi),%rax
> 55 push %rbp
> 48 89 e5 mov %rsp,%rbp
> 48 0f c8 bswap %rax
> 5d pop %rbp
> c3 retq
>
> <__swab16p> (16 copies, 146 calls):
> 55 push %rbp
> 89 f8 mov %edi,%eax
> 86 e0 xchg %ah,%al
> 48 89 e5 mov %rsp,%rbp
> 5d pop %rbp
> c3 retq
>
> <__swab32p> (43 copies, ~560 calls):
> 55 push %rbp
> 89 f8 mov %edi,%eax
> 0f c8 bswap %eax
> 48 89 e5 mov %rsp,%rbp
> 5d pop %rbp
> c3 retq
>
> <__swab64p> (21 copies, 119 calls):
> 55 push %rbp
> 48 89 f8 mov %rdi,%rax
> 48 0f c8 bswap %rax
> 48 89 e5 mov %rsp,%rbp
> 5d pop %rbp
> c3 retq
>
> <__swab32s> (6 copies, 47 calls):
> 8b 07 mov (%rdi),%eax
> 55 push %rbp
> 48 89 e5 mov %rsp,%rbp
> 0f c8 bswap %eax
> 89 07 mov %eax,(%rdi)
> 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 <dvlasenk@redhat.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Thomas Graf <tgraf@suug.ch>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: David Rientjes <rientjes@google.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: linux-kernel@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:
0000000000002f53 <qla2x00_get_host_fabric_name>:
2f53: 55 push %rbp
2f54: 48 89 e5 mov %rsp,%rbp
0000000000002f57 <qla2x00_get_fc_host_stats>:
2f57: 55 push %rbp
2f58: b9 e8 00 00 00 mov $0xe8,%ecx
2f5d: 48 89 e5 mov %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.
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.
> -static inline __u64 __be64_to_cpup(const __be64 *p)
> +static __always_inline __u64 __be64_to_cpup(const __be64 *p)
> {
> return __swab64p((__u64 *)p);
> }
--
Josh
[toc] | [next] | [standalone]
| From | Denys Vlasenko <dvlasenk@redhat.com> |
|---|---|
| Date | 2016-04-13 14:20 +0200 |
| Subject | Re: This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations) |
| Message-ID | <rnrW2-15F-15@gated-at.bofh.it> |
| In reply to | #1377489 |
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_Os,
>> the following functions get deinlined many times.
>> Examples of disassembly:
>>
>> <get_unaligned_be16> (12 copies, 51 calls):
>> 66 8b 07 mov (%rdi),%ax
>> 55 push %rbp
>> 48 89 e5 mov %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 <dvlasenk@redhat.com>
>> Cc: Ingo Molnar <mingo@kernel.org>
>> Cc: Thomas Graf <tgraf@suug.ch>
>> Cc: Peter Zijlstra <peterz@infradead.org>
>> Cc: David Rientjes <rientjes@google.com>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: linux-kernel@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:
>
> 0000000000002f53 <qla2x00_get_host_fabric_name>:
> 2f53: 55 push %rbp
> 2f54: 48 89 e5 mov %rsp,%rbp
>
> 0000000000002f57 <qla2x00_get_fc_host_stats>:
> 2f57: 55 push %rbp
> 2f58: b9 e8 00 00 00 mov $0xe8,%ecx
> 2f5d: 48 89 e5 mov %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.
[toc] | [prev] | [next] | [standalone]
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2016-04-13 14:40 +0200 |
| Subject | Re: This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations) |
| Message-ID | <rnsfp-1e6-25@gated-at.bofh.it> |
| In reply to | #1377882 |
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_Os,
> >> the following functions get deinlined many times.
> >> Examples of disassembly:
> >>
> >> <get_unaligned_be16> (12 copies, 51 calls):
> >> 66 8b 07 mov (%rdi),%ax
> >> 55 push %rbp
> >> 48 89 e5 mov %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 <dvlasenk@redhat.com>
> >> Cc: Ingo Molnar <mingo@kernel.org>
> >> Cc: Thomas Graf <tgraf@suug.ch>
> >> Cc: Peter Zijlstra <peterz@infradead.org>
> >> Cc: David Rientjes <rientjes@google.com>
> >> Cc: Andrew Morton <akpm@linux-foundation.org>
> >> Cc: linux-kernel@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:
> >
> > 0000000000002f53 <qla2x00_get_host_fabric_name>:
> > 2f53: 55 push %rbp
> > 2f54: 48 89 e5 mov %rsp,%rbp
> >
> > 0000000000002f57 <qla2x00_get_fc_host_stats>:
> > 2f57: 55 push %rbp
> > 2f58: b9 e8 00 00 00 mov $0xe8,%ecx
> > 2f5d: 48 89 e5 mov %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.
--
Josh
[toc] | [prev] | [next] | [standalone]
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2016-04-13 17:20 +0200 |
| Subject | Re: This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations) |
| Message-ID | <rnuKd-3cP-7@gated-at.bofh.it> |
| In reply to | #1377890 |
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_Os,
> > >> the following functions get deinlined many times.
> > >> Examples of disassembly:
> > >>
> > >> <get_unaligned_be16> (12 copies, 51 calls):
> > >> 66 8b 07 mov (%rdi),%ax
> > >> 55 push %rbp
> > >> 48 89 e5 mov %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 <dvlasenk@redhat.com>
> > >> Cc: Ingo Molnar <mingo@kernel.org>
> > >> Cc: Thomas Graf <tgraf@suug.ch>
> > >> Cc: Peter Zijlstra <peterz@infradead.org>
> > >> Cc: David Rientjes <rientjes@google.com>
> > >> Cc: Andrew Morton <akpm@linux-foundation.org>
> > >> Cc: linux-kernel@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:
> > >
> > > 0000000000002f53 <qla2x00_get_host_fabric_name>:
> > > 2f53: 55 push %rbp
> > > 2f54: 48 89 e5 mov %rsp,%rbp
> > >
> > > 0000000000002f57 <qla2x00_get_fc_host_stats>:
> > > 2f57: 55 push %rbp
> > > 2f58: b9 e8 00 00 00 mov $0xe8,%ecx
> > > 2f5d: 48 89 e5 mov %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
--
Josh
[toc] | [prev] | [next] | [standalone]
| From | James Bottomley <James.Bottomley@HansenPartnership.com> |
|---|---|
| Date | 2016-04-13 19:00 +0200 |
| Subject | Re: This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations) |
| Message-ID | <rnwiZ-4a7-1@gated-at.bofh.it> |
| In reply to | #1378033 |
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:
> > > > >
> > > > > <get_unaligned_be16> (12 copies, 51 calls):
> > > > > 66 8b 07 mov (%rdi),%ax
> > > > > 55 push %rbp
> > > > > 48 89 e5 mov %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 <dvlasenk@redhat.com>
> > > > > Cc: Ingo Molnar <mingo@kernel.org>
> > > > > Cc: Thomas Graf <tgraf@suug.ch>
> > > > > Cc: Peter Zijlstra <peterz@infradead.org>
> > > > > Cc: David Rientjes <rientjes@google.com>
> > > > > Cc: Andrew Morton <akpm@linux-foundation.org>
> > > > > Cc: linux-kernel@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:
> > > >
> > > > 0000000000002f53 <qla2x00_get_host_fabric_name>:
> > > > 2f53: 55 push %rbp
> > > > 2f54: 48 89 e5 mov %rsp,%rbp
> > > >
> > > > 0000000000002f57 <qla2x00_get_fc_host_stats>:
> > > > 2f57: 55 push %rbp
> > > > 2f58: b9 e8 00 00 00 mov $0xe8,%ecx
> > > > 2f5d: 48 89 e5 mov %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
[toc] | [prev] | [next] | [standalone]
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2016-04-13 19:20 +0200 |
| Subject | Re: This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations) |
| Message-ID | <rnwCm-4E2-7@gated-at.bofh.it> |
| In reply to | #1378101 |
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:
> > > > > >
> > > > > > <get_unaligned_be16> (12 copies, 51 calls):
> > > > > > 66 8b 07 mov (%rdi),%ax
> > > > > > 55 push %rbp
> > > > > > 48 89 e5 mov %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 <dvlasenk@redhat.com>
> > > > > > Cc: Ingo Molnar <mingo@kernel.org>
> > > > > > Cc: Thomas Graf <tgraf@suug.ch>
> > > > > > Cc: Peter Zijlstra <peterz@infradead.org>
> > > > > > Cc: David Rientjes <rientjes@google.com>
> > > > > > Cc: Andrew Morton <akpm@linux-foundation.org>
> > > > > > Cc: linux-kernel@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:
> > > > >
> > > > > 0000000000002f53 <qla2x00_get_host_fabric_name>:
> > > > > 2f53: 55 push %rbp
> > > > > 2f54: 48 89 e5 mov %rsp,%rbp
> > > > >
> > > > > 0000000000002f57 <qla2x00_get_fc_host_stats>:
> > > > > 2f57: 55 push %rbp
> > > > > 2f58: b9 e8 00 00 00 mov $0xe8,%ecx
> > > > > 2f5d: 48 89 e5 mov %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.
--
Josh
[toc] | [prev] | [next] | [standalone]
| From | Denys Vlasenko <dvlasenk@redhat.com> |
|---|---|
| Date | 2016-04-14 17:30 +0200 |
| Subject | Re: This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations) |
| Message-ID | <rnRns-3WP-25@gated-at.bofh.it> |
| In reply to | #1378110 |
On 04/13/2016 07:10 PM, Josh Poimboeuf wrote:
>>>>>> From the disassembly of drivers/scsi/qla2xxx/qla_attr.o:
>>>>>>
>>>>>> 0000000000002f53 <qla2x00_get_host_fabric_name>:
>>>>>> 2f53: 55 push %rbp
>>>>>> 2f54: 48 89 e5 mov %rsp,%rbp
>>>>>>
>>>>>> 0000000000002f57 <qla2x00_get_fc_host_stats>:
>>>>>> 2f57: 55 push %rbp
>>>>>> 2f58: b9 e8 00 00 00 mov $0xe8,%ecx
>>>>>> 2f5d: 48 89 e5 mov %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)0x00000000000000ffULL) << 56) | (((u64)(*p) & (u64)0x000000000000ff00ULL) << 40) | (((u64)(*p) & (u64)0x0000000000ff0000ULL) << 24) | (((u64)(*p) & (u64)0x00000000ff000000ULL) << 8) | (((u64)(*p) & (u64)0x000000ff00000000ULL) >> 8) | (((u64)(*p) & (u64)0x0000ff0000000000ULL) >> 24) | (((u64)(*p) & (u64)0x00ff000000000000ULL) >> 40) | (((u64)(*p) & (u64)0xff00000000000000ULL) >> 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.
[toc] | [prev] | [next] | [standalone]
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2016-04-14 18:00 +0200 |
| Subject | Re: This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations) |
| Message-ID | <rnRQu-4b1-1@gated-at.bofh.it> |
| In reply to | #1379021 |
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:
> >>>>>>
> >>>>>> 0000000000002f53 <qla2x00_get_host_fabric_name>:
> >>>>>> 2f53: 55 push %rbp
> >>>>>> 2f54: 48 89 e5 mov %rsp,%rbp
> >>>>>>
> >>>>>> 0000000000002f57 <qla2x00_get_fc_host_stats>:
> >>>>>> 2f57: 55 push %rbp
> >>>>>> 2f58: b9 e8 00 00 00 mov $0xe8,%ecx
> >>>>>> 2f5d: 48 89 e5 mov %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)0x00000000000000ffULL) << 56) | (((u64)(*p) & (u64)0x000000000000ff00ULL) << 40) | (((u64)(*p) & (u64)0x0000000000ff0000ULL) << 24) | (((u64)(*p) & (u64)0x00000000ff000000ULL) << 8) | (((u64)(*p) & (u64)0x000000ff00000000ULL) >> 8) | (((u64)(*p) & (u64)0x0000ff0000000000ULL) >> 24) | (((u64)(*p) & (u64)0x00ff000000000000ULL) >> 40) | (((u64)(*p) & (u64)0xff00000000000000ULL) >> 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 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);
}
> Since it was not detected for two years since gcc 4.9 release,
> it must be triggering quite rarely.
Yeah, and according to objtool this is the only occurrence of this bug
in the entire kernel tree. Thanks to the kbuild robot randconfig
builds, I think we can be pretty confident that objtool will find any
more of them if they show up.
So what do you think about working around this bug by doing something
like the above patch?
--
Josh
[toc] | [prev] | [next] | [standalone]
| From | Denys Vlasenko <dvlasenk@redhat.com> |
|---|---|
| Date | 2016-04-14 19:10 +0200 |
| Subject | Re: This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations) |
| Message-ID | <rnSWe-5pD-3@gated-at.bofh.it> |
| In reply to | #1379052 |
On 04/14/2016 05:57 PM, Josh Poimboeuf wrote:
> 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:
>>>>>>>>
>>>>>>>> 0000000000002f53 <qla2x00_get_host_fabric_name>:
>>>>>>>> 2f53: 55 push %rbp
>>>>>>>> 2f54: 48 89 e5 mov %rsp,%rbp
>>>>>>>>
>>>>>>>> 0000000000002f57 <qla2x00_get_fc_host_stats>:
>>>>>>>> 2f57: 55 push %rbp
>>>>>>>> 2f58: b9 e8 00 00 00 mov $0xe8,%ecx
>>>>>>>> 2f5d: 48 89 e5 mov %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)0x00000000000000ffULL) << 56) | (((u64)(*p) & (u64)0x000000000000ff00ULL) << 40) | (((u64)(*p) & (u64)0x0000000000ff0000ULL) << 24) | (((u64)(*p) & (u64)0x00000000ff000000ULL) << 8) | (((u64)(*p) & (u64)0x000000ff00000000ULL) >> 8) | (((u64)(*p) & (u64)0x0000ff0000000000ULL) >> 24) | (((u64)(*p) & (u64)0x00ff000000000000ULL) >> 40) | (((u64)(*p) & (u64)0xff00000000000000ULL) >> 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.
By my reading of what gcc gurus are talking there,
gcc has some new-ish machinery for discarding unreachable code.
Akin to not continuing code generation after a call to never-returning
function like exit(), but smarter (it can detect much less obvious
cases when some code path is not possible).
And it has a subtle bug. In this case, it decided that both branches
of ternary op ?: in __swab64p() are impossible, and therefore
__swab64p() is impossible.
(Which is, of course, bogus, that's why it's a bug).
Then this bogus decision was propagated through inlining
and gcc decided that entire qla2x00_get_host_fabric_name()
function is an impossible (unreachable) code path, and...
eliminated it all. Good boy :D
> 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.
[toc] | [prev] | [next] | [standalone]
| From | Ingo Molnar <mingo@kernel.org> |
|---|---|
| Date | 2016-04-15 07:50 +0200 |
| Subject | Re: This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations) |
| Message-ID | <ro4NI-6nI-1@gated-at.bofh.it> |
| In reply to | #1379116 |
* Denys Vlasenko <dvlasenk@redhat.com> 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
[toc] | [prev] | [next] | [standalone]
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2016-04-15 15:50 +0200 |
| Subject | Re: This patch triggers a bad gcc bug (was Re: [PATCH] force inlining of some byteswap operations) |
| Message-ID | <rocie-3Ln-11@gated-at.bofh.it> |
| In reply to | #1379463 |
On Fri, Apr 15, 2016 at 07:45:19AM +0200, Ingo Molnar wrote:
>
> * Denys Vlasenko <dvlasenk@redhat.com> 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 qla2xxxx code:
https://lkml.kernel.org/r/1460716583-15673-1-git-send-email-dvlasenk@redhat.com
--
Josh
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web