I have no idea what is actually going on here, but building an x86 kernel
with CONFIG_MATOM results in countless warnings from objtool, such as
arch/x86/events/intel/ds.o: warning: objtool: intel_pmu_pebs_del()+0x43: call without frame pointer save/setup
security/keys/keyring.o: warning: objtool: keyring_read()+0x59: call without frame pointer save/setup
kernel/signal.o: warning: objtool: __dequeue_signal()+0xd8: call without frame pointer save/setup
kernel/signal.o: warning: objtool: kill_pid()+0x15: call without frame pointer save/setup
kernel/signal.o: warning: objtool: SyS_signal()+0x27: call without frame pointer save/setup
mm/page_alloc.o: warning: objtool: zone_watermark_ok_safe()+0x27: call without frame pointer save/setup
fs/exec.o: warning: objtool: read_code()+0x18: call without frame pointer save/setup
mm/swap.o: warning: objtool: get_kernel_page()+0x24: call without frame pointer save/setup
mm/swap.o: warning: objtool: pagevec_move_tail.constprop.25()+0x26: call without frame pointer save/setup
block/bio.o: warning: objtool: bio_map_kern()+0x47: call without frame pointer save/setup
arch/x86/crypto/poly1305_glue.o: warning: objtool: poly1305_simd_mult()+0x2d: call without frame pointer save/setup
crypto/skcipher.o: warning: objtool: skcipher_encrypt_ablkcipher()+0x58: call without frame pointer save/setup
crypto/skcipher.o: warning: objtool: skcipher_decrypt_ablkcipher()+0x58: call without frame pointer save/setup
fs/inode.o: warning: objtool: ilookup()+0x5d: call without frame pointer save/setup
fs/inode.o: warning: objtool: proc_nr_inodes()+0x3e: call without frame pointer save/setup
fs/namei.o: warning: objtool: lookup_one_len_unlocked()+0x21: call without frame pointer save/setup
block/elevator.o: warning: objtool: elv_rb_add()+0x5b: call without frame pointer save/setup
crypto/shash.o: warning: objtool: shash_async_init()+0x1e: call without frame pointer save/setup
crypto/shash.o: warning: objtool: shash_async_import()+0x1e: call without frame pointer save/setup
mm/vmscan.o: warning: objtool: pfmemalloc_watermark_ok()+0xb9: call without frame pointer save/setup
I have not looked at whether this is a bug in gcc or in objtool, however
I found that not using -mtune=atom reliably avoids the problem. I could
reproduce the problem with gcc versions 4.7 through 6.1.
Cc: Josh Poimboeuf <[email protected]>
Signed-off-by: Arnd Bergmann <[email protected]>
---
arch/x86/Makefile | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/Makefile b/arch/x86/Makefile
index 2d449337a360..e1dfb37d66ad 100644
--- a/arch/x86/Makefile
+++ b/arch/x86/Makefile
@@ -110,7 +110,7 @@ else
cflags-$(CONFIG_MCORE2) += \
$(call cc-option,-march=core2,$(call cc-option,-mtune=generic))
cflags-$(CONFIG_MATOM) += $(call cc-option,-march=atom) \
- $(call cc-option,-mtune=atom,$(call cc-option,-mtune=generic))
+ $(call cc-option,-mtune=generic)
cflags-$(CONFIG_GENERIC_CPU) += $(call cc-option,-mtune=generic)
KBUILD_CFLAGS += $(cflags-y)
--
2.9.0
On Mon, Oct 10, 2016 at 02:56:56PM +0200, Arnd Bergmann wrote:
> I have no idea what is actually going on here, but building an x86 kernel
> with CONFIG_MATOM results in countless warnings from objtool, such as
>
> arch/x86/events/intel/ds.o: warning: objtool: intel_pmu_pebs_del()+0x43: call without frame pointer save/setup
> security/keys/keyring.o: warning: objtool: keyring_read()+0x59: call without frame pointer save/setup
> kernel/signal.o: warning: objtool: __dequeue_signal()+0xd8: call without frame pointer save/setup
> kernel/signal.o: warning: objtool: kill_pid()+0x15: call without frame pointer save/setup
> kernel/signal.o: warning: objtool: SyS_signal()+0x27: call without frame pointer save/setup
> mm/page_alloc.o: warning: objtool: zone_watermark_ok_safe()+0x27: call without frame pointer save/setup
> fs/exec.o: warning: objtool: read_code()+0x18: call without frame pointer save/setup
> mm/swap.o: warning: objtool: get_kernel_page()+0x24: call without frame pointer save/setup
> mm/swap.o: warning: objtool: pagevec_move_tail.constprop.25()+0x26: call without frame pointer save/setup
> block/bio.o: warning: objtool: bio_map_kern()+0x47: call without frame pointer save/setup
> arch/x86/crypto/poly1305_glue.o: warning: objtool: poly1305_simd_mult()+0x2d: call without frame pointer save/setup
> crypto/skcipher.o: warning: objtool: skcipher_encrypt_ablkcipher()+0x58: call without frame pointer save/setup
> crypto/skcipher.o: warning: objtool: skcipher_decrypt_ablkcipher()+0x58: call without frame pointer save/setup
> fs/inode.o: warning: objtool: ilookup()+0x5d: call without frame pointer save/setup
> fs/inode.o: warning: objtool: proc_nr_inodes()+0x3e: call without frame pointer save/setup
> fs/namei.o: warning: objtool: lookup_one_len_unlocked()+0x21: call without frame pointer save/setup
> block/elevator.o: warning: objtool: elv_rb_add()+0x5b: call without frame pointer save/setup
> crypto/shash.o: warning: objtool: shash_async_init()+0x1e: call without frame pointer save/setup
> crypto/shash.o: warning: objtool: shash_async_import()+0x1e: call without frame pointer save/setup
> mm/vmscan.o: warning: objtool: pfmemalloc_watermark_ok()+0xb9: call without frame pointer save/setup
>
> I have not looked at whether this is a bug in gcc or in objtool, however
> I found that not using -mtune=atom reliably avoids the problem. I could
> reproduce the problem with gcc versions 4.7 through 6.1.
Thanks for reporting it. It looks like 'mtune=atom' sometimes makes a
slight change to one of the stack frame setup instructions. Instead of:
move rsp, rbp
It sometimes does:
lea (%rsp),%rbp
They're two different instructions, but they have the same result. It's
an easy fix for objtool. I'll post a patch soon.
--
Josh
>From 60c982d4d04014adb3bde1ebee6ca95320ffb213 Mon Sep 17 00:00:00 2001
Message-Id: <60c982d4d04014adb3bde1ebee6ca95320ffb213.1476150736.git.jpoimboe@redhat.com>
From: Josh Poimboeuf <[email protected]>
Date: Mon, 10 Oct 2016 15:24:01 -0500
Subject: [PATCH] objtool: support '-mtune=atom' stack frame setup instruction
Arnd reported that enabling CONFIG_MATOM results in a bunch of objtool
false positive frame pointer warnings:
arch/x86/events/intel/ds.o: warning: objtool: intel_pmu_pebs_del()+0x43: call without frame pointer save/setup
security/keys/keyring.o: warning: objtool: keyring_read()+0x59: call without frame pointer save/setup
kernel/signal.o: warning: objtool: __dequeue_signal()+0xd8: call without frame pointer save/setup
...
objtool gets confused by the fact that the '-mtune=atom' gcc option
sometimes uses 'lea (%rsp),%rbp' instead of 'mov %rsp,%rbp'. The
instructions are effectively the same, but objtool doesn't know about
the 'lea' variant.
Fix the false warnings by adding support for 'lea (%rsp),%rbp' in the
objtool decoder.
Reported-by: Arnd Bergmann <[email protected]>
Signed-off-by: Josh Poimboeuf <[email protected]>
---
tools/objtool/arch/x86/decode.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/tools/objtool/arch/x86/decode.c b/tools/objtool/arch/x86/decode.c
index c0c0b26..b63a31b 100644
--- a/tools/objtool/arch/x86/decode.c
+++ b/tools/objtool/arch/x86/decode.c
@@ -98,6 +98,15 @@ int arch_decode_instruction(struct elf *elf, struct section *sec,
*type = INSN_FP_SETUP;
break;
+ case 0x8d:
+ if (insn.rex_prefix.bytes &&
+ insn.rex_prefix.bytes[0] == 0x48 &&
+ insn.modrm.nbytes && insn.modrm.bytes[0] == 0x2c &&
+ insn.sib.nbytes && insn.sib.bytes[0] == 0x24)
+ /* lea %(rsp), %rbp */
+ *type = INSN_FP_SETUP;
+ break;
+
case 0x90:
*type = INSN_NOP;
break;
--
2.7.4
On Monday, October 10, 2016 3:23:22 PM CEST Josh Poimboeuf wrote:
>
> Thanks for reporting it. It looks like 'mtune=atom' sometimes makes a
> slight change to one of the stack frame setup instructions. Instead of:
>
> move rsp, rbp
>
> It sometimes does:
>
> lea (%rsp),%rbp
>
> They're two different instructions, but they have the same result. It's
> an easy fix for objtool. I'll post a patch soon.
>
>
Ah, good to hear. I've replaced my patch with yours in my randconfig
tests now and will let you know if there are any other warnings on
atom. I've done a few thousand x86 randconfig builds now and done private
patches for all warnings I got (I previously had fixes only for the arm
warnings). I found objtool warnings for a few files in some configurations
that do not involve -mtune=atom, maybe you can also look at what
is going on there as I have no idea for how to address them:
drivers/scsi/fnic/fnic_main.o: warning: objtool: fnic_log_q_error() falls through to next function fnic_handle_link_event()
drivers/scsi/snic/snic_res.o: warning: objtool: .text: unexpected end of section
drivers/infiniband/sw/rxe/rxe_comp.o: warning: objtool: rxe_completer()+0x2f3: sibling call from callable instruction with changed frame pointer
drivers/infiniband/sw/rxe/rxe_resp.o: warning: objtool: rxe_responder()+0x10f: sibling call from callable instruction with changed frame pointer
kernel/locking/rwsem.o: warning: objtool: down_write_killable()+0x16: call without frame pointer save/setup
I can provide additional information for reproducing them if it's
not immediately obvious what the problems are.
Thanks,
Arnd
On Tue, Oct 11, 2016 at 10:08:12AM +0200, Arnd Bergmann wrote:
> On Monday, October 10, 2016 3:23:22 PM CEST Josh Poimboeuf wrote:
> >
> > Thanks for reporting it. It looks like 'mtune=atom' sometimes makes a
> > slight change to one of the stack frame setup instructions. Instead of:
> >
> > move rsp, rbp
> >
> > It sometimes does:
> >
> > lea (%rsp),%rbp
> >
> > They're two different instructions, but they have the same result. It's
> > an easy fix for objtool. I'll post a patch soon.
> >
> >
>
> Ah, good to hear. I've replaced my patch with yours in my randconfig
> tests now and will let you know if there are any other warnings on
> atom. I've done a few thousand x86 randconfig builds now and done private
> patches for all warnings I got (I previously had fixes only for the arm
> warnings). I found objtool warnings for a few files in some configurations
> that do not involve -mtune=atom, maybe you can also look at what
> is going on there as I have no idea for how to address them:
>
> drivers/scsi/fnic/fnic_main.o: warning: objtool: fnic_log_q_error() falls through to next function fnic_handle_link_event()
> drivers/scsi/snic/snic_res.o: warning: objtool: .text: unexpected end of section
> drivers/infiniband/sw/rxe/rxe_comp.o: warning: objtool: rxe_completer()+0x2f3: sibling call from callable instruction with changed frame pointer
> drivers/infiniband/sw/rxe/rxe_resp.o: warning: objtool: rxe_responder()+0x10f: sibling call from callable instruction with changed frame pointer
> kernel/locking/rwsem.o: warning: objtool: down_write_killable()+0x16: call without frame pointer save/setup
>
> I can provide additional information for reproducing them if it's
> not immediately obvious what the problems are.
I'm really surprised the 0-day bot didn't find these. I was under the
impression that it continuously did a bunch of randconfigs.
Anyway, if you could send the configs for the warnings, that would be
very helpful.
I also happen to be working on a significant rewrite of objtool and
these configs will come in handy for making a regression suite.
--
Josh
On Tuesday, October 11, 2016 7:20:49 AM CEST Josh Poimboeuf wrote:
> On Tue, Oct 11, 2016 at 10:08:12AM +0200, Arnd Bergmann wrote:
> > On Monday, October 10, 2016 3:23:22 PM CEST Josh Poimboeuf wrote:
> > >
> > > Thanks for reporting it. It looks like 'mtune=atom' sometimes makes a
> > > slight change to one of the stack frame setup instructions. Instead of:
> > >
> > > move rsp, rbp
> > >
> > > It sometimes does:
> > >
> > > lea (%rsp),%rbp
> > >
> > > They're two different instructions, but they have the same result. It's
> > > an easy fix for objtool. I'll post a patch soon.
> > >
> > >
> >
> > Ah, good to hear. I've replaced my patch with yours in my randconfig
> > tests now and will let you know if there are any other warnings on
> > atom. I've done a few thousand x86 randconfig builds now and done private
> > patches for all warnings I got (I previously had fixes only for the arm
> > warnings). I found objtool warnings for a few files in some configurations
> > that do not involve -mtune=atom, maybe you can also look at what
> > is going on there as I have no idea for how to address them:
> >
> > drivers/scsi/fnic/fnic_main.o: warning: objtool: fnic_log_q_error() falls through to next function fnic_handle_link_event()
> > drivers/scsi/snic/snic_res.o: warning: objtool: .text: unexpected end of section
> > drivers/infiniband/sw/rxe/rxe_comp.o: warning: objtool: rxe_completer()+0x2f3: sibling call from callable instruction with changed frame pointer
> > drivers/infiniband/sw/rxe/rxe_resp.o: warning: objtool: rxe_responder()+0x10f: sibling call from callable instruction with changed frame pointer
> > kernel/locking/rwsem.o: warning: objtool: down_write_killable()+0x16: call without frame pointer save/setup
> >
> > I can provide additional information for reproducing them if it's
> > not immediately obvious what the problems are.
>
> I'm really surprised the 0-day bot didn't find these. I was under the
> impression that it continuously did a bunch of randconfigs.
>
> Anyway, if you could send the configs for the warnings, that would be
> very helpful.
>
> I also happen to be working on a significant rewrite of objtool and
> these configs will come in handy for making a regression suite.
I've attached the three .config files here, but due to the size I
don't know if they make it to the list or your inbox. Let me
know if you get them, and if you are able to reproduce the problem.
The compiler version I used is gcc-6 (Ubuntu 6.2.0-3ubuntu11~16.04)
6.2.0 20160901, and this is on top of linux-next plus a few other
patches.
Arnd
On Tue, Oct 11, 2016 at 03:30:20PM +0200, Arnd Bergmann wrote:
> I've attached the three .config files here, but due to the size I
> don't know if they make it to the list or your inbox. Let me
> know if you get them, and if you are able to reproduce the problem.
>
> The compiler version I used is gcc-6 (Ubuntu 6.2.0-3ubuntu11~16.04)
> 6.2.0 20160901, and this is on top of linux-next plus a few other
> patches.
Thanks, I got the configs, and I do see the warnings. Will
investigate...
--
Josh
(spoiler alert: another bad gcc bug which is truncating functions...)
On Tue, Oct 11, 2016 at 10:05:41AM -0500, Josh Poimboeuf wrote:
> On Tue, Oct 11, 2016 at 03:30:20PM +0200, Arnd Bergmann wrote:
> > I've attached the three .config files here, but due to the size I
> > don't know if they make it to the list or your inbox. Let me
> > know if you get them, and if you are able to reproduce the problem.
> >
> > The compiler version I used is gcc-6 (Ubuntu 6.2.0-3ubuntu11~16.04)
> > 6.2.0 20160901, and this is on top of linux-next plus a few other
> > patches.
>
> Thanks, I got the configs, and I do see the warnings. Will
> investigate...
1) 0x364C8CDB-config:
kernel/locking/rwsem.o: warning: objtool: down_write_killable()+0x16: call without frame pointer save/setup
This is a bug in kernel code in the ____down_write() macro. It doesn't
ensure there's a stack frame before the call instruction. Easy fix.
2) 0x3A1DA440-config:
drivers/infiniband/sw/rxe/rxe_comp.o: warning: objtool: rxe_completer()+0x2f4: sibling call from callable instruction with changed frame pointer
drivers/infiniband/sw/rxe/rxe_resp.o: warning: objtool: rxe_responder()+0x10f: sibling call from callable instruction with changed frame pointer
These are false positive warnings, caused by the bane of objtool's
existence, gcc switch statement jump tables. objtool needs to be made a
little smarter.
3) 0xFC244C03-config:
drivers/scsi/fnic/fnic_main.o: warning: objtool: fnic_log_q_error() falls through to next function fnic_handle_link_event()
drivers/scsi/snic/snic_res.o: warning: objtool: .text: unexpected end of section
These look like another bad gcc bug which is truncating functions:
0000000000000940 <snic_log_q_error>:
940: 55 push %rbp
941: 48 89 e5 mov %rsp,%rbp
944: 53 push %rbx
945: 48 89 fb mov %rdi,%rbx
948: e8 00 00 00 00 callq 94d <snic_log_q_error+0xd>
949: R_X86_64_PC32 __sanitizer_cov_trace_pc-0x4
94d: 8b 83 58 02 00 00 mov 0x258(%rbx),%eax
953: 85 c0 test %eax,%eax
955: 75 08 jne 95f <snic_log_q_error+0x1f>
957: e8 00 00 00 00 callq 95c <snic_log_q_error+0x1c>
958: R_X86_64_PC32 __sanitizer_cov_trace_pc-0x4
95c: 5b pop %rbx
95d: 5d pop %rbp
95e: c3 retq
95f: e8 00 00 00 00 callq 964 <snic_log_q_error+0x24>
960: R_X86_64_PC32 __sanitizer_cov_trace_pc-0x4
964: 48 8b 83 10 1c 00 00 mov 0x1c10(%rbx),%rax
96b: 48 8d 78 50 lea 0x50(%rax),%rdi
96f: e8 00 00 00 00 callq 974 <snic_log_q_error+0x34>
970: R_X86_64_PC32 ioread32-0x4
974: 83 bb 58 02 00 00 01 cmpl $0x1,0x258(%rbx)
97b: 76 da jbe 957 <snic_log_q_error+0x17>
97d: e8 00 00 00 00 callq 982 <snic_log_q_error+0x42>
97e: R_X86_64_PC32 __sanitizer_cov_trace_pc-0x4
[end of file]
Notice how it just falls off the end of the function. We had a similar
bug before:
https://lkml.kernel.org/r/20160413033649.7r3msnmo3trtq47z@treble
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70646
I'm not sure yet if this is the same gcc bug or a different one. Maybe
it's related to the new GCC_PLUGIN_SANCOV?
--
Josh
On Tuesday, October 11, 2016 10:51:46 AM CEST Josh Poimboeuf wrote:
>
> 3) 0xFC244C03-config:
> drivers/scsi/fnic/fnic_main.o: warning: objtool: fnic_log_q_error() falls through to next function fnic_handle_link_event()
> drivers/scsi/snic/snic_res.o: warning: objtool: .text: unexpected end of section
>
> These look like another bad gcc bug which is truncating functions:
Same bug for both of them?
>
> 0000000000000940 <snic_log_q_error>:
> 940: 55 push %rbp
> 941: 48 89 e5 mov %rsp,%rbp
> 944: 53 push %rbx
> 945: 48 89 fb mov %rdi,%rbx
> 948: e8 00 00 00 00 callq 94d <snic_log_q_error+0xd>
> 949: R_X86_64_PC32 __sanitizer_cov_trace_pc-0x4
> 94d: 8b 83 58 02 00 00 mov 0x258(%rbx),%eax
> 953: 85 c0 test %eax,%eax
> 955: 75 08 jne 95f <snic_log_q_error+0x1f>
> 957: e8 00 00 00 00 callq 95c <snic_log_q_error+0x1c>
> 958: R_X86_64_PC32 __sanitizer_cov_trace_pc-0x4
> 95c: 5b pop %rbx
> 95d: 5d pop %rbp
> 95e: c3 retq
> 95f: e8 00 00 00 00 callq 964 <snic_log_q_error+0x24>
> 960: R_X86_64_PC32 __sanitizer_cov_trace_pc-0x4
> 964: 48 8b 83 10 1c 00 00 mov 0x1c10(%rbx),%rax
> 96b: 48 8d 78 50 lea 0x50(%rax),%rdi
> 96f: e8 00 00 00 00 callq 974 <snic_log_q_error+0x34>
> 970: R_X86_64_PC32 ioread32-0x4
> 974: 83 bb 58 02 00 00 01 cmpl $0x1,0x258(%rbx)
> 97b: 76 da jbe 957 <snic_log_q_error+0x17>
> 97d: e8 00 00 00 00 callq 982 <snic_log_q_error+0x42>
> 97e: R_X86_64_PC32 __sanitizer_cov_trace_pc-0x4
>
> [end of file]
>
> Notice how it just falls off the end of the function. We had a similar
> bug before:
>
> https://lkml.kernel.org/r/20160413033649.7r3msnmo3trtq47z@treble
I remember that nightmare :(
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70646
>
> I'm not sure yet if this is the same gcc bug or a different one. Maybe
> it's related to the new GCC_PLUGIN_SANCOV?
I've reduced one of the test cases to this now:
/* gcc-6 -O2 -fno-strict-aliasing -fno-reorder-blocks -fno-omit-frame-pointer -Wno-pointer-sign -fsanitize-coverage=trace-pc -Wall -Werror -c snic_res.c -o snic_res.o */
typedef int spinlock_t;
extern unsigned int ioread32(void *);
struct vnic_wq_ctrl {
unsigned int error_status;
};
struct vnic_wq {
struct vnic_wq_ctrl *ctrl;
} mempool_t;
struct snic {
unsigned int wq_count;
__attribute__ ((__aligned__)) struct vnic_wq wq[1];
spinlock_t wq_lock[1];
};
unsigned int snic_log_q_error_err_status;
void snic_log_q_error(struct snic *snic)
{
unsigned int i;
for (i = 0; i < snic->wq_count; i++)
snic_log_q_error_err_status =
ioread32(&snic->wq[i].ctrl->error_status);
}
which gets compiled into
0000000000000000 <snic_log_q_error>:
0: 55 push %rbp
1: 48 89 e5 mov %rsp,%rbp
4: 53 push %rbx
5: 48 89 fb mov %rdi,%rbx
8: 48 83 ec 08 sub $0x8,%rsp
c: e8 00 00 00 00 callq 11 <snic_log_q_error+0x11>
d: R_X86_64_PC32 __sanitizer_cov_trace_pc-0x4
11: 8b 03 mov (%rbx),%eax
13: 85 c0 test %eax,%eax
15: 75 11 jne 28 <snic_log_q_error+0x28>
17: 48 83 c4 08 add $0x8,%rsp
1b: 5b pop %rbx
1c: 5d pop %rbp
1d: e9 00 00 00 00 jmpq 22 <snic_log_q_error+0x22>
1e: R_X86_64_PC32 __sanitizer_cov_trace_pc-0x4
22: 66 0f 1f 44 00 00 nopw 0x0(%rax,%rax,1)
28: e8 00 00 00 00 callq 2d <snic_log_q_error+0x2d>
29: R_X86_64_PC32 __sanitizer_cov_trace_pc-0x4
2d: 48 8b 7b 10 mov 0x10(%rbx),%rdi
31: e8 00 00 00 00 callq 36 <snic_log_q_error+0x36>
32: R_X86_64_PC32 ioread32-0x4
36: 89 05 00 00 00 00 mov %eax,0x0(%rip) # 3c <snic_log_q_error+0x3c>
38: R_X86_64_PC32 snic_log_q_error_err_status-0x4
3c: 83 3b 01 cmpl $0x1,(%rbx)
3f: 76 d6 jbe 17 <snic_log_q_error+0x17>
41: e8 00 00 00 00 callq 46 <snic_log_q_error+0x46>
42: R_X86_64_PC32 __sanitizer_cov_trace_pc-0x4
Arnd
On Tue, Oct 11, 2016 at 10:38:42PM +0200, Arnd Bergmann wrote:
> I've reduced one of the test cases to this now:
>
> /* gcc-6 -O2 -fno-strict-aliasing -fno-reorder-blocks -fno-omit-frame-pointer -Wno-pointer-sign -fsanitize-coverage=trace-pc -Wall -Werror -c snic_res.c -o snic_res.o */
> typedef int spinlock_t;
> extern unsigned int ioread32(void *);
> struct vnic_wq_ctrl {
> unsigned int error_status;
> };
> struct vnic_wq {
> struct vnic_wq_ctrl *ctrl;
> } mempool_t;
> struct snic {
> unsigned int wq_count;
> __attribute__ ((__aligned__)) struct vnic_wq wq[1];
> spinlock_t wq_lock[1];
> };
> unsigned int snic_log_q_error_err_status;
> void snic_log_q_error(struct snic *snic)
> {
> unsigned int i;
> for (i = 0; i < snic->wq_count; i++)
> snic_log_q_error_err_status =
> ioread32(&snic->wq[i].ctrl->error_status);
> }
>
> which gets compiled into
>
> 0000000000000000 <snic_log_q_error>:
> 0: 55 push %rbp
> 1: 48 89 e5 mov %rsp,%rbp
> 4: 53 push %rbx
> 5: 48 89 fb mov %rdi,%rbx
> 8: 48 83 ec 08 sub $0x8,%rsp
> c: e8 00 00 00 00 callq 11 <snic_log_q_error+0x11>
> d: R_X86_64_PC32 __sanitizer_cov_trace_pc-0x4
> 11: 8b 03 mov (%rbx),%eax
> 13: 85 c0 test %eax,%eax
> 15: 75 11 jne 28 <snic_log_q_error+0x28>
> 17: 48 83 c4 08 add $0x8,%rsp
> 1b: 5b pop %rbx
> 1c: 5d pop %rbp
> 1d: e9 00 00 00 00 jmpq 22 <snic_log_q_error+0x22>
> 1e: R_X86_64_PC32 __sanitizer_cov_trace_pc-0x4
> 22: 66 0f 1f 44 00 00 nopw 0x0(%rax,%rax,1)
> 28: e8 00 00 00 00 callq 2d <snic_log_q_error+0x2d>
> 29: R_X86_64_PC32 __sanitizer_cov_trace_pc-0x4
> 2d: 48 8b 7b 10 mov 0x10(%rbx),%rdi
> 31: e8 00 00 00 00 callq 36 <snic_log_q_error+0x36>
> 32: R_X86_64_PC32 ioread32-0x4
> 36: 89 05 00 00 00 00 mov %eax,0x0(%rip) # 3c <snic_log_q_error+0x3c>
> 38: R_X86_64_PC32 snic_log_q_error_err_status-0x4
> 3c: 83 3b 01 cmpl $0x1,(%rbx)
> 3f: 76 d6 jbe 17 <snic_log_q_error+0x17>
> 41: e8 00 00 00 00 callq 46 <snic_log_q_error+0x46>
> 42: R_X86_64_PC32 __sanitizer_cov_trace_pc-0x4
>
Thanks! I'll open a gcc bug.
--
Josh
On Tue, Oct 11, 2016 at 10:38:42PM +0200, Arnd Bergmann wrote:
> On Tuesday, October 11, 2016 10:51:46 AM CEST Josh Poimboeuf wrote:
> > Notice how it just falls off the end of the function. We had a similar
> > bug before:
> >
> > https://lkml.kernel.org/r/20160413033649.7r3msnmo3trtq47z@treble
>
> I remember that nightmare :(
>
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70646
> >
> > I'm not sure yet if this is the same gcc bug or a different one. Maybe
> > it's related to the new GCC_PLUGIN_SANCOV?
>
> I've reduced one of the test cases to this now:
>
> /* gcc-6 -O2 -fno-strict-aliasing -fno-reorder-blocks -fno-omit-frame-pointer -Wno-pointer-sign -fsanitize-coverage=trace-pc -Wall -Werror -c snic_res.c -o snic_res.o */
> typedef int spinlock_t;
> extern unsigned int ioread32(void *);
> struct vnic_wq_ctrl {
> unsigned int error_status;
> };
> struct vnic_wq {
> struct vnic_wq_ctrl *ctrl;
> } mempool_t;
> struct snic {
> unsigned int wq_count;
> __attribute__ ((__aligned__)) struct vnic_wq wq[1];
> spinlock_t wq_lock[1];
> };
> unsigned int snic_log_q_error_err_status;
> void snic_log_q_error(struct snic *snic)
> {
> unsigned int i;
> for (i = 0; i < snic->wq_count; i++)
> snic_log_q_error_err_status =
> ioread32(&snic->wq[i].ctrl->error_status);
> }
>
> which gets compiled into
>
> 0000000000000000 <snic_log_q_error>:
> 0: 55 push %rbp
> 1: 48 89 e5 mov %rsp,%rbp
> 4: 53 push %rbx
> 5: 48 89 fb mov %rdi,%rbx
> 8: 48 83 ec 08 sub $0x8,%rsp
> c: e8 00 00 00 00 callq 11 <snic_log_q_error+0x11>
> d: R_X86_64_PC32 __sanitizer_cov_trace_pc-0x4
> 11: 8b 03 mov (%rbx),%eax
> 13: 85 c0 test %eax,%eax
> 15: 75 11 jne 28 <snic_log_q_error+0x28>
> 17: 48 83 c4 08 add $0x8,%rsp
> 1b: 5b pop %rbx
> 1c: 5d pop %rbp
> 1d: e9 00 00 00 00 jmpq 22 <snic_log_q_error+0x22>
> 1e: R_X86_64_PC32 __sanitizer_cov_trace_pc-0x4
> 22: 66 0f 1f 44 00 00 nopw 0x0(%rax,%rax,1)
> 28: e8 00 00 00 00 callq 2d <snic_log_q_error+0x2d>
> 29: R_X86_64_PC32 __sanitizer_cov_trace_pc-0x4
> 2d: 48 8b 7b 10 mov 0x10(%rbx),%rdi
> 31: e8 00 00 00 00 callq 36 <snic_log_q_error+0x36>
> 32: R_X86_64_PC32 ioread32-0x4
> 36: 89 05 00 00 00 00 mov %eax,0x0(%rip) # 3c <snic_log_q_error+0x3c>
> 38: R_X86_64_PC32 snic_log_q_error_err_status-0x4
> 3c: 83 3b 01 cmpl $0x1,(%rbx)
> 3f: 76 d6 jbe 17 <snic_log_q_error+0x17>
> 41: e8 00 00 00 00 callq 46 <snic_log_q_error+0x46>
> 42: R_X86_64_PC32 __sanitizer_cov_trace_pc-0x4
I opened a bug:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77966
--
Josh
On 10/13/2016 02:46 PM, Josh Poimboeuf wrote:
> On Tue, Oct 11, 2016 at 10:38:42PM +0200, Arnd Bergmann wrote:
>> On Tuesday, October 11, 2016 10:51:46 AM CEST Josh Poimboeuf wrote:
>>> Notice how it just falls off the end of the function. We had a similar
>>> bug before:
>>>
>>> https://lkml.kernel.org/r/20160413033649.7r3msnmo3trtq47z@treble
>>
>> I remember that nightmare :(
>>
>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70646
>>>
>>> I'm not sure yet if this is the same gcc bug or a different one. Maybe
>>> it's related to the new GCC_PLUGIN_SANCOV?
>>
>> I've reduced one of the test cases to this now:
>>
>> /* gcc-6 -O2 -fno-strict-aliasing -fno-reorder-blocks -fno-omit-frame-pointer -Wno-pointer-sign -fsanitize-coverage=trace-pc -Wall -Werror -c snic_res.c -o snic_res.o */
>> typedef int spinlock_t;
>> extern unsigned int ioread32(void *);
>> struct vnic_wq_ctrl {
>> unsigned int error_status;
>> };
>> struct vnic_wq {
>> struct vnic_wq_ctrl *ctrl;
>> } mempool_t;
>> struct snic {
>> unsigned int wq_count;
>> __attribute__ ((__aligned__)) struct vnic_wq wq[1];
>> spinlock_t wq_lock[1];
>> };
>> unsigned int snic_log_q_error_err_status;
>> void snic_log_q_error(struct snic *snic)
>> {
>> unsigned int i;
>> for (i = 0; i < snic->wq_count; i++)
>> snic_log_q_error_err_status =
>> ioread32(&snic->wq[i].ctrl->error_status);
>> }
>>
>> which gets compiled into
>>
>> 0000000000000000 <snic_log_q_error>:
>> 0: 55 push %rbp
>> 1: 48 89 e5 mov %rsp,%rbp
>> 4: 53 push %rbx
>> 5: 48 89 fb mov %rdi,%rbx
>> 8: 48 83 ec 08 sub $0x8,%rsp
>> c: e8 00 00 00 00 callq 11 <snic_log_q_error+0x11>
>> d: R_X86_64_PC32 __sanitizer_cov_trace_pc-0x4
>> 11: 8b 03 mov (%rbx),%eax
>> 13: 85 c0 test %eax,%eax
>> 15: 75 11 jne 28 <snic_log_q_error+0x28>
>> 17: 48 83 c4 08 add $0x8,%rsp
>> 1b: 5b pop %rbx
>> 1c: 5d pop %rbp
>> 1d: e9 00 00 00 00 jmpq 22 <snic_log_q_error+0x22>
>> 1e: R_X86_64_PC32 __sanitizer_cov_trace_pc-0x4
>> 22: 66 0f 1f 44 00 00 nopw 0x0(%rax,%rax,1)
>> 28: e8 00 00 00 00 callq 2d <snic_log_q_error+0x2d>
>> 29: R_X86_64_PC32 __sanitizer_cov_trace_pc-0x4
>> 2d: 48 8b 7b 10 mov 0x10(%rbx),%rdi
>> 31: e8 00 00 00 00 callq 36 <snic_log_q_error+0x36>
>> 32: R_X86_64_PC32 ioread32-0x4
>> 36: 89 05 00 00 00 00 mov %eax,0x0(%rip) # 3c <snic_log_q_error+0x3c>
>> 38: R_X86_64_PC32 snic_log_q_error_err_status-0x4
>> 3c: 83 3b 01 cmpl $0x1,(%rbx)
>> 3f: 76 d6 jbe 17 <snic_log_q_error+0x17>
>> 41: e8 00 00 00 00 callq 46 <snic_log_q_error+0x46>
>> 42: R_X86_64_PC32 __sanitizer_cov_trace_pc-0x4
>
> I opened a bug:
>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77966
>
Surprisingly, it's really "not a bug". The only way you can end up in this branch
is if you have a bug and run off the end of wq[1] array member: i.e.
if snic->wq_count >= 2. (See gcc BZ for smaller example)
It's debatable whether it's okay for gcc to just let buggy code to run off
and execute something random. It is surely surprising, and not debug-friendly.
An option to emit a crashing instruction (HLT, INT3, that sort of thing)
instead of just stopping code generation might be useful.
On Thu, Oct 13, 2016 at 07:57:41PM +0200, Denys Vlasenko wrote:
> On 10/13/2016 02:46 PM, Josh Poimboeuf wrote:
> > On Tue, Oct 11, 2016 at 10:38:42PM +0200, Arnd Bergmann wrote:
> > > 0000000000000000 <snic_log_q_error>:
> > > 0: 55 push %rbp
> > > 1: 48 89 e5 mov %rsp,%rbp
> > > 4: 53 push %rbx
> > > 5: 48 89 fb mov %rdi,%rbx
> > > 8: 48 83 ec 08 sub $0x8,%rsp
> > > c: e8 00 00 00 00 callq 11 <snic_log_q_error+0x11>
> > > d: R_X86_64_PC32 __sanitizer_cov_trace_pc-0x4
> > > 11: 8b 03 mov (%rbx),%eax
> > > 13: 85 c0 test %eax,%eax
> > > 15: 75 11 jne 28 <snic_log_q_error+0x28>
> > > 17: 48 83 c4 08 add $0x8,%rsp
> > > 1b: 5b pop %rbx
> > > 1c: 5d pop %rbp
> > > 1d: e9 00 00 00 00 jmpq 22 <snic_log_q_error+0x22>
> > > 1e: R_X86_64_PC32 __sanitizer_cov_trace_pc-0x4
> > > 22: 66 0f 1f 44 00 00 nopw 0x0(%rax,%rax,1)
> > > 28: e8 00 00 00 00 callq 2d <snic_log_q_error+0x2d>
> > > 29: R_X86_64_PC32 __sanitizer_cov_trace_pc-0x4
> > > 2d: 48 8b 7b 10 mov 0x10(%rbx),%rdi
> > > 31: e8 00 00 00 00 callq 36 <snic_log_q_error+0x36>
> > > 32: R_X86_64_PC32 ioread32-0x4
> > > 36: 89 05 00 00 00 00 mov %eax,0x0(%rip) # 3c <snic_log_q_error+0x3c>
> > > 38: R_X86_64_PC32 snic_log_q_error_err_status-0x4
> > > 3c: 83 3b 01 cmpl $0x1,(%rbx)
> > > 3f: 76 d6 jbe 17 <snic_log_q_error+0x17>
> > > 41: e8 00 00 00 00 callq 46 <snic_log_q_error+0x46>
> > > 42: R_X86_64_PC32 __sanitizer_cov_trace_pc-0x4
> >
> > I opened a bug:
> >
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77966
> >
>
> Surprisingly, it's really "not a bug". The only way you can end up in this branch
> is if you have a bug and run off the end of wq[1] array member: i.e.
> if snic->wq_count >= 2. (See gcc BZ for smaller example)
>
> It's debatable whether it's okay for gcc to just let buggy code to run off
> and execute something random. It is surely surprising, and not debug-friendly.
>
> An option to emit a crashing instruction (HLT, INT3, that sort of thing)
> instead of just stopping code generation might be useful.
Ah, you're right.
IMO it's still a gcc bug though. Instead of following a bad pointer, it
would instead start executing some random function. That takes
"undefined behavior" to a new level.
--
Josh
On Tue, Oct 11, 2016 at 10:38 PM, Arnd Bergmann <[email protected]> wrote:
> On Tuesday, October 11, 2016 10:51:46 AM CEST Josh Poimboeuf wrote:
>>
>> 3) 0xFC244C03-config:
>> drivers/scsi/fnic/fnic_main.o: warning: objtool: fnic_log_q_error() falls through to next function fnic_handle_link_event()
>> drivers/scsi/snic/snic_res.o: warning: objtool: .text: unexpected end of section
>>
>> These look like another bad gcc bug which is truncating functions:
>
> Same bug for both of them?
I ran into this one again today, after updating to the latest gcc-7.0.1:
drivers/infiniband/sw/rxe/rxe_resp.o: warning: objtool:
rxe_responder()+0xfe: sibling call from callable instruction with
changed frame pointer
Josh, did you get around to updating objtool the last time I reported it, or
is it still the same problem? If this is a new variation, I can provide more
details about the failure, otherwise I'll just ignore it for now.
Arnd
On Wed, Mar 1, 2017 at 10:34 AM, Arnd Bergmann <[email protected]> wrote:
> On Tue, Oct 11, 2016 at 10:38 PM, Arnd Bergmann <[email protected]> wrote:
>> On Tuesday, October 11, 2016 10:51:46 AM CEST Josh Poimboeuf wrote:
>>>
>>> 3) 0xFC244C03-config:
>>> drivers/scsi/fnic/fnic_main.o: warning: objtool: fnic_log_q_error() falls through to next function fnic_handle_link_event()
>>> drivers/scsi/snic/snic_res.o: warning: objtool: .text: unexpected end of section
>>>
>>> These look like another bad gcc bug which is truncating functions:
>>
>> Same bug for both of them?
>
> I ran into this one again today, after updating to the latest gcc-7.0.1:
>
> drivers/infiniband/sw/rxe/rxe_resp.o: warning: objtool:
> rxe_responder()+0xfe: sibling call from callable instruction with
> changed frame pointer
>
> Josh, did you get around to updating objtool the last time I reported it, or
> is it still the same problem? If this is a new variation, I can provide more
> details about the failure, otherwise I'll just ignore it for now.
Actually, something must have changed in gcc since last month, I also
just got a report in another file:
drivers/i2c/busses/i2c-img-scb.o: warning: objtool: img_i2c_probe()
falls through to next function img_i2c_read_fifo()
See below for the relevant snippet from the assembler output.
Arnd
# /git/arm-soc/drivers/i2c/busses/i2c-img-scb.c:1176: if
(i2c->bitrate <= timings[i].max_bitrate) {
movl 1648(%rbx), %edx # MEM[(struct img_i2c
*)_29].bitrate, _99
cmpl timings+8(%rip), %edx # timings[0].max_bitrate, _99
# /git/arm-soc/drivers/i2c/busses/i2c-img-scb.c:1171:
i2c->need_wr_rd_fence = true;
movb $1, 1652(%rbx) #, MEM[(struct img_i2c *)_29].need_wr_rd_fence
movl timings+48(%rip), %ecx # timings[1].max_bitrate, pretmp_260
# /git/arm-soc/drivers/i2c/busses/i2c-img-scb.c:1176: if
(i2c->bitrate <= timings[i].max_bitrate) {
jbe .L59 #,
cmpl %ecx, %edx # pretmp_260, _99
jbe .L60 #,
.L61:
# /git/arm-soc/drivers/i2c/busses/i2c-img-scb.c:1182:
dev_warn(i2c->adap.dev.parent,
movq 240(%rbx), %rdi # MEM[(struct img_i2c
*)_29].adap.dev.parent, MEM[(struct img_i2c *)_29].adap.dev.parent
movq $.LC12, %rsi #,
call dev_warn #
# /git/arm-soc/drivers/i2c/busses/i2c-img-scb.c:1187:
i2c->bitrate = timing.max_bitrate;
movl timings+48(%rip), %eax # MEM[(struct img_i2c_timings
*)&timings + 48B], MEM[(struct img_i2c_timings *)&timings + 48B]
movl %eax, 1648(%rbx) # MEM[(struct img_i2c_timings
*)&timings + 48B], MEM[(struct img_i2c *)_29].bitrate
.L60:
ud2
.L66:
# /git/arm-soc/drivers/i2c/busses/i2c-img-scb.c:1360:
dev_err(&pdev->dev, "can't request irq %d\n", irq);
movl %r13d, %edx # <retval>,
movq $.LC6, %rsi #,
movq %r14, %rdi # _1,
call dev_err #
# /git/arm-soc/drivers/i2c/busses/i2c-img-scb.c:1361: return ret;
movl -52(%rbp), %eax # %sfp, _62
movl %eax, %r13d # _62, <retval>
jmp .L52 #
.L65:
# /git/arm-soc/drivers/i2c/busses/i2c-img-scb.c:1341:
dev_err(&pdev->dev, "can't get irq number\n");
movq $.LC5, %rsi #,
movq %r14, %rdi # _1,
call dev_err #
# /git/arm-soc/drivers/i2c/busses/i2c-img-scb.c:1342: return irq;
jmp .L52 #
.L67:
# /git/arm-soc/drivers/i2c/busses/i2c-img-scb.c:1162:
dev_info(i2c->adap.dev.parent,
movzbl %ah, %ecx # ret, tmp150
movq 240(%rbx), %rdi # MEM[(struct img_i2c
*)_29].adap.dev.parent, MEM[(struct img_i2c *)_29].adap.dev.parent
movl %eax, %edx # ret, tmp153
movl %ecx, %r8d # tmp150, tmp150
movl %eax, %ecx # ret, tmp151
movzbl %al, %r9d # ret,
shrl $16, %ecx #, tmp151
shrl $24, %edx #, tmp153
movq $.LC11, %rsi #,
movzbl %cl, %ecx # tmp151, tmp152
# /git/arm-soc/drivers/i2c/busses/i2c-img-scb.c:1404: return ret;
movl $-22, %r13d #, <retval>
# /git/arm-soc/drivers/i2c/busses/i2c-img-scb.c:1162:
dev_info(i2c->adap.dev.parent,
call _dev_info #
# /git/arm-soc/include/linux/clk.h:210: might_sleep();
xorl %edx, %edx #
movl $210, %esi #,
movq $.LC0, %rdi #,
call __might_sleep #
xorl %edx, %edx #
movl $210, %esi #,
movq $.LC0, %rdi #,
call __might_sleep #
# /git/arm-soc/drivers/i2c/busses/i2c-img-scb.c:1404: return ret;
jmp .L52 #
.L62:
# /git/arm-soc/drivers/i2c/busses/i2c-img-scb.c:1332: return -ENOMEM;
movl $-12, %r13d #, <retval>
jmp .L52 #
.L59:
# /git/arm-soc/drivers/i2c/busses/i2c-img-scb.c:1181: if
(i2c->bitrate > timings[ARRAY_SIZE(timings) - 1].max_bitrate) {
cmpl %ecx, %edx # pretmp_260, _99
ja .L61 #,
ud2
.size img_i2c_probe, .-img_i2c_probe
.p2align 4,,15
.type img_i2c_read_fifo, @function
img_i2c_read_fifo:
1: call __fentry__
# /git/arm-soc/drivers/i2c/busses/i2c-img-scb.c:545: while (i2c->msg.len) {
cmpw $0, 1828(%rdi) #, i2c_10(D)->msg.len
# /git/arm-soc/drivers/i2c/busses/i2c-img-scb.c:544: {
pushq %rbp #
movq %rsp, %rbp #,
# /git/arm-soc/drivers/i2c/busses/i2c-img-scb.c:545: while (i2c->msg.len) {
je .L68 #,
# /git/arm-soc/arch/x86/include/asm/io.h:66: build_mmio_write(writel,
"l", unsigned int, "r", :"memory")
xorl %esi, %esi # tmp118
movl $255, %ecx #, tmp119
jmp .L71 #
On Wed, Mar 01, 2017 at 10:45:03AM +0100, Arnd Bergmann wrote:
> On Wed, Mar 1, 2017 at 10:34 AM, Arnd Bergmann <[email protected]> wrote:
> > On Tue, Oct 11, 2016 at 10:38 PM, Arnd Bergmann <[email protected]> wrote:
> >> On Tuesday, October 11, 2016 10:51:46 AM CEST Josh Poimboeuf wrote:
> >>>
> >>> 3) 0xFC244C03-config:
> >>> drivers/scsi/fnic/fnic_main.o: warning: objtool: fnic_log_q_error() falls through to next function fnic_handle_link_event()
> >>> drivers/scsi/snic/snic_res.o: warning: objtool: .text: unexpected end of section
> >>>
> >>> These look like another bad gcc bug which is truncating functions:
> >>
> >> Same bug for both of them?
> >
> > I ran into this one again today, after updating to the latest gcc-7.0.1:
> >
> > drivers/infiniband/sw/rxe/rxe_resp.o: warning: objtool:
> > rxe_responder()+0xfe: sibling call from callable instruction with
> > changed frame pointer
> >
> > Josh, did you get around to updating objtool the last time I reported it, or
> > is it still the same problem? If this is a new variation, I can provide more
> > details about the failure, otherwise I'll just ignore it for now.
>
> Actually, something must have changed in gcc since last month, I also
> just got a report in another file:
>
> drivers/i2c/busses/i2c-img-scb.o: warning: objtool: img_i2c_probe()
> falls through to next function img_i2c_read_fifo()
This one looks like it could be related to some recent objtool changes
which affect how it interprets 'ud2'. Which commit were you testing
with? Can you provide the .config file, and the object file if it's not
too big?
--
Josh
On Wed, Mar 1, 2017 at 3:31 PM, Josh Poimboeuf <[email protected]> wrote:
> On Wed, Mar 01, 2017 at 10:34:42AM +0100, Arnd Bergmann wrote:
>> On Tue, Oct 11, 2016 at 10:38 PM, Arnd Bergmann <[email protected]> wrote:
>> > On Tuesday, October 11, 2016 10:51:46 AM CEST Josh Poimboeuf wrote:
>> >>
>> >> 3) 0xFC244C03-config:
>> >> drivers/scsi/fnic/fnic_main.o: warning: objtool: fnic_log_q_error() falls through to next function fnic_handle_link_event()
>> >> drivers/scsi/snic/snic_res.o: warning: objtool: .text: unexpected end of section
>> >>
>> >> These look like another bad gcc bug which is truncating functions:
>> >
>> > Same bug for both of them?
>>
>> I ran into this one again today, after updating to the latest gcc-7.0.1:
>>
>> drivers/infiniband/sw/rxe/rxe_resp.o: warning: objtool:
>> rxe_responder()+0xfe: sibling call from callable instruction with
>> changed frame pointer
>>
>> Josh, did you get around to updating objtool the last time I reported it, or
>> is it still the same problem? If this is a new variation, I can provide more
>> details about the failure, otherwise I'll just ignore it for now.
>
> This one should have been fixed with:
>
> 3732710ff6f2 ("objtool: Improve rare switch jump table pattern detection")
It was on the current linux-next, so that commit should certainly be included.
> Can you attach the object file?
done.
Arnd
On Wed, Mar 01, 2017 at 10:34:42AM +0100, Arnd Bergmann wrote:
> On Tue, Oct 11, 2016 at 10:38 PM, Arnd Bergmann <[email protected]> wrote:
> > On Tuesday, October 11, 2016 10:51:46 AM CEST Josh Poimboeuf wrote:
> >>
> >> 3) 0xFC244C03-config:
> >> drivers/scsi/fnic/fnic_main.o: warning: objtool: fnic_log_q_error() falls through to next function fnic_handle_link_event()
> >> drivers/scsi/snic/snic_res.o: warning: objtool: .text: unexpected end of section
> >>
> >> These look like another bad gcc bug which is truncating functions:
> >
> > Same bug for both of them?
>
> I ran into this one again today, after updating to the latest gcc-7.0.1:
>
> drivers/infiniband/sw/rxe/rxe_resp.o: warning: objtool:
> rxe_responder()+0xfe: sibling call from callable instruction with
> changed frame pointer
>
> Josh, did you get around to updating objtool the last time I reported it, or
> is it still the same problem? If this is a new variation, I can provide more
> details about the failure, otherwise I'll just ignore it for now.
This one should have been fixed with:
3732710ff6f2 ("objtool: Improve rare switch jump table pattern detection")
Can you attach the object file?
--
Josh
On Wed, Mar 1, 2017 at 3:40 PM, Josh Poimboeuf <[email protected]> wrote:
> On Wed, Mar 01, 2017 at 10:45:03AM +0100, Arnd Bergmann wrote:
>> Actually, something must have changed in gcc since last month, I also
>> just got a report in another file:
>>
>> drivers/i2c/busses/i2c-img-scb.o: warning: objtool: img_i2c_probe()
>> falls through to next function img_i2c_read_fifo()
>
> This one looks like it could be related to some recent objtool changes
> which affect how it interprets 'ud2'. Which commit were you testing
> with? Can you provide the .config file, and the object file if it's not
> too big?
This is with my randconfig test series on top of latest linux-next.
I see it with the latest gcc-7.0.1 snapshot as well as an earlier gcc-7.0.0
build (20161201), but not with gcc-6.3.1
Arnd
On Wed, Mar 01, 2017 at 04:27:29PM +0100, Arnd Bergmann wrote:
> On Wed, Mar 1, 2017 at 3:40 PM, Josh Poimboeuf <[email protected]> wrote:
> > On Wed, Mar 01, 2017 at 10:45:03AM +0100, Arnd Bergmann wrote:
>
> >> Actually, something must have changed in gcc since last month, I also
> >> just got a report in another file:
> >>
> >> drivers/i2c/busses/i2c-img-scb.o: warning: objtool: img_i2c_probe()
> >> falls through to next function img_i2c_read_fifo()
> >
> > This one looks like it could be related to some recent objtool changes
> > which affect how it interprets 'ud2'. Which commit were you testing
> > with? Can you provide the .config file, and the object file if it's not
> > too big?
>
> This is with my randconfig test series on top of latest linux-next.
> I see it with the latest gcc-7.0.1 snapshot as well as an earlier gcc-7.0.0
> build (20161201), but not with gcc-6.3.1
I wonder if this is another gcc bug. gcc inserted two ud2 instructions
in img_i2c_probe() for no apparent reason. Here's one of them:
5c3: e8 00 00 00 00 callq 5c8 <img_i2c_probe+0x298>
5c4: R_X86_64_PC32 dev_warn-0x4
5c8: 8b 05 00 00 00 00 mov 0x0(%rip),%eax # 5ce <img_i2c_probe+0x29e>
5ca: R_X86_64_PC32 .data+0xec
5ce: 89 83 70 06 00 00 mov %eax,0x670(%rbx)
5d4: 0f 0b ud2
Which corresponds to the following code block:
if (i2c->bitrate > timings[ARRAY_SIZE(timings) - 1].max_bitrate) {
dev_warn(i2c->adap.dev.parent,
"requested bitrate (%u) is higher than the max bitrate supported (%u)\n",
i2c->bitrate,
timings[ARRAY_SIZE(timings) - 1].max_bitrate);
timing = timings[ARRAY_SIZE(timings) - 1];
i2c->bitrate = timing.max_bitrate;
}
I see no apparent reason for the ud2.
Can you rebuild the object with CONFIG_DEBUG_INFO and use addr2line to
see what code lines are associated with the ud2's?
--
Josh
On Wed, Mar 1, 2017 at 5:53 PM, Josh Poimboeuf <[email protected]> wrote:
> On Wed, Mar 01, 2017 at 04:27:29PM +0100, Arnd Bergmann wrote:
>> On Wed, Mar 1, 2017 at 3:40 PM, Josh Poimboeuf <[email protected]> wrote:
>> > On Wed, Mar 01, 2017 at 10:45:03AM +0100, Arnd Bergmann wrote:
>>
>> >> Actually, something must have changed in gcc since last month, I also
>> >> just got a report in another file:
>> >>
>> >> drivers/i2c/busses/i2c-img-scb.o: warning: objtool: img_i2c_probe()
>> >> falls through to next function img_i2c_read_fifo()
>> >
>> > This one looks like it could be related to some recent objtool changes
>> > which affect how it interprets 'ud2'. Which commit were you testing
>> > with? Can you provide the .config file, and the object file if it's not
>> > too big?
>>
>> This is with my randconfig test series on top of latest linux-next.
>> I see it with the latest gcc-7.0.1 snapshot as well as an earlier gcc-7.0.0
>> build (20161201), but not with gcc-6.3.1
>
> I wonder if this is another gcc bug. gcc inserted two ud2 instructions
> in img_i2c_probe() for no apparent reason. Here's one of them:
>
> 5c3: e8 00 00 00 00 callq 5c8 <img_i2c_probe+0x298>
> 5c4: R_X86_64_PC32 dev_warn-0x4
> 5c8: 8b 05 00 00 00 00 mov 0x0(%rip),%eax # 5ce <img_i2c_probe+0x29e>
> 5ca: R_X86_64_PC32 .data+0xec
> 5ce: 89 83 70 06 00 00 mov %eax,0x670(%rbx)
> 5d4: 0f 0b ud2
>
> Which corresponds to the following code block:
>
> if (i2c->bitrate > timings[ARRAY_SIZE(timings) - 1].max_bitrate) {
> dev_warn(i2c->adap.dev.parent,
> "requested bitrate (%u) is higher than the max bitrate supported (%u)\n",
> i2c->bitrate,
> timings[ARRAY_SIZE(timings) - 1].max_bitrate);
> timing = timings[ARRAY_SIZE(timings) - 1];
> i2c->bitrate = timing.max_bitrate;
> }
>
> I see no apparent reason for the ud2.
>
> Can you rebuild the object with CONFIG_DEBUG_INFO and use addr2line to
> see what code lines are associated with the ud2's?
$ addr2line -e drivers/i2c/busses/i2c-img-scb.o
0x5bc
/git/arm-soc/drivers/i2c/busses/i2c-img-scb.c:1187: i2c->bitrate =
timing.max_bitrate;
0x65d
/git/arm-soc/drivers/i2c/busses/i2c-img-scb.c:1181: if (i2c->bitrate >
timings[ARRAY_SIZE(timings) - 1].max_bitrate) {
and from the .s file with line numbers:
.type img_i2c_probe, @function
img_i2c_probe:
.LFB1968:
.loc 1 1323 0
.cfi_startproc
.LVL40:
1: call __fentry__
pushq %rbp #
.cfi_def_cfa_offset 16
.cfi_offset 6, -16
.LBB757:
.LBB758:
# /git/arm-soc/include/linux/device.h:668: return devm_kmalloc(dev,
size, gfp | __GFP_ZERO);
.file 5 "/git/arm-soc/include/linux/device.h"
.loc 5 668 0
movl $21004480, %edx #,
movl $1992, %esi #,
.LBE758:
.LBE757:
# /git/arm-soc/drivers/i2c/busses/i2c-img-scb.c:1323: {
.loc 1 1323 0
movq %rsp, %rbp #,
.cfi_def_cfa_register 6
pushq %r15 #
pushq %r14 #
.cfi_offset 15, -24
.cfi_offset 14, -32
# /git/arm-soc/drivers/i2c/busses/i2c-img-scb.c:1330: i2c =
devm_kzalloc(&pdev->dev, sizeof(struct img_i2c), GFP_KERNEL);
.loc 1 1330 0
leaq 16(%rdi), %r14 #, _1
# /git/arm-soc/drivers/i2c/busses/i2c-img-scb.c:1323: {
.loc 1 1323 0
pushq %r13 #
pushq %r12 #
pushq %rbx #
.cfi_offset 13, -40
.cfi_offset 12, -48
.cfi_offset 3, -56
movq %rdi, %r12 # pdev, pdev
subq $24, %rsp #,
# /git/arm-soc/drivers/i2c/busses/i2c-img-scb.c:1324: struct
device_node *node = pdev->dev.of_node;
.loc 1 1324 0
movq 864(%rdi), %r15 # pdev_19(D)->dev.of_node, node
.LVL41:
.LBB760:
.LBB759:
# /git/arm-soc/include/linux/device.h:668: return devm_kmalloc(dev,
size, gfp | __GFP_ZERO);
.loc 5 668 0
movq %r14, %rdi # _1,
.LVL42:
call devm_kmalloc #
.LVL43:
.LBE759:
.LBE760:
# /git/arm-soc/drivers/i2c/busses/i2c-img-scb.c:1331: if (!i2c)
.loc 1 1331 0
testq %rax, %rax # _29
je .L62 #,
# /git/arm-soc/drivers/i2c/busses/i2c-img-scb.c:1334: res =
platform_get_resource(pdev, IORESOURCE_MEM, 0);
.loc 1 1334 0
xorl %edx, %edx #
movl $512, %esi #,
movq %r12, %rdi # pdev,
movq %rax, %rbx #, _29
call platform_get_resource #
.LVL44:
# /git/arm-soc/drivers/i2c/busses/i2c-img-scb.c:1335: i2c->base =
devm_ioremap_resource(&pdev->dev, res);
.loc 1 1335 0
movq %r14, %rdi # _1,
.LVL45:
movq %rax, %rsi # res,
call devm_ioremap_resource #
.LVL46:
.LBB761:
.LBB762:
# /git/arm-soc/include/linux/err.h:35: return IS_ERR_VALUE((unsigned long)ptr);
.file 6 "/git/arm-soc/include/linux/err.h"
.loc 6 35 0
xorl %esi, %esi # tmp128
cmpq $-4096, %rax #, _2
.LBE762:
.LBE761:
# /git/arm-soc/drivers/i2c/busses/i2c-img-scb.c:1335: i2c->base =
devm_ioremap_resource(&pdev->dev, res);
.loc 1 1335 0
movq %rax, %r13 #, _2
.LBB766:
.LBB764:
# /git/arm-soc/include/linux/err.h:35: return IS_ERR_VALUE((unsigned long)ptr);
.loc 6 35 0
seta %sil #, tmp128
.LBE764:
.LBE766:
# /git/arm-soc/drivers/i2c/busses/i2c-img-scb.c:1335: i2c->base =
devm_ioremap_resource(&pdev->dev, res);
.loc 1 1335 0
movq %rax, 1624(%rbx) # _2, MEM[(struct img_i2c *)_29].base
.LBB767:
.LBB765:
.LBB763:
# /git/arm-soc/include/linux/err.h:35: return IS_ERR_VALUE((unsigned long)ptr);
.loc 6 35 0
xorl %edx, %edx #
movq $______f.2078, %rdi #,
call ftrace_likely_update #
.LVL47:
.LBE763:
.LBE765:
.LBE767:
# /git/arm-soc/drivers/i2c/busses/i2c-img-scb.c:1336: if (IS_ERR(i2c->base))
.loc 1 1336 0
cmpq $-4096, %r13 #, _2
jbe .L54 #,
# /git/arm-soc/drivers/i2c/busses/i2c-img-scb.c:1337: return PTR_ERR(i2c->base);
.loc 1 1337 0
movl 1624(%rbx), %r13d # MEM[(struct img_i2c *)_29].base, <retval>
.LVL48:
.L52:
# /git/arm-soc/drivers/i2c/busses/i2c-img-scb.c:1405: }
.loc 1 1405 0
addq $24, %rsp #,
movl %r13d, %eax # <retval>,
popq %rbx #
.cfi_remember_state
.cfi_restore 3
popq %r12 #
.cfi_restore 12
.LVL49:
popq %r13 #
.cfi_restore 13
popq %r14 #
.cfi_restore 14
popq %r15 #
.cfi_restore 15
.LVL50:
popq %rbp #
.cfi_restore 6
.cfi_def_cfa 7, 8
ret
.LVL51:
.L54:
.cfi_restore_state
# /git/arm-soc/drivers/i2c/busses/i2c-img-scb.c:1339: irq =
platform_get_irq(pdev, 0);
.loc 1 1339 0
xorl %esi, %esi #
movq %r12, %rdi # pdev,
call platform_get_irq #
.LVL52:
# /git/arm-soc/drivers/i2c/busses/i2c-img-scb.c:1340: if (irq < 0) {
.loc 1 1340 0
testl %eax, %eax # <retval>
# /git/arm-soc/drivers/i2c/busses/i2c-img-scb.c:1339: irq =
platform_get_irq(pdev, 0);
.loc 1 1339 0
movl %eax, %r13d #, <retval>
# /git/arm-soc/drivers/i2c/busses/i2c-img-scb.c:1340: if (irq < 0) {
.loc 1 1340 0
js .L65 #,
.LBB768:
.LBB769:
# /git/arm-soc/include/linux/interrupt.h:173: return
devm_request_threaded_irq(dev, irq, handler, NULL, irqflags,
.file 7 "/git/arm-soc/include/linux/interrupt.h"
.loc 7 173 0
movq (%r12), %r9 # pdev_19(D)->name,
.LBE769:
.LBE768:
# /git/arm-soc/drivers/i2c/busses/i2c-img-scb.c:1345: i2c->sys_clk =
devm_clk_get(&pdev->dev, "sys");
.loc 1 1345 0
movq $0, 1640(%rbx) #, MEM[(struct img_i2c *)_29].sys_clk
.LBB772:
.LBB770:
# /git/arm-soc/include/linux/interrupt.h:173: return
devm_request_threaded_irq(dev, irq, handler, NULL, irqflags,
.loc 7 173 0
xorl %r8d, %r8d #
.LBE770:
.LBE772:
# /git/arm-soc/drivers/i2c/busses/i2c-img-scb.c:1351: i2c->scb_clk =
devm_clk_get(&pdev->dev, "scb");
.loc 1 1351 0
movq $0, 1632(%rbx) #, MEM[(struct img_i2c *)_29].scb_clk
.LBB773:
.LBB771:
# /git/arm-soc/include/linux/interrupt.h:173: return
devm_request_threaded_irq(dev, irq, handler, NULL, irqflags,
.loc 7 173 0
xorl %ecx, %ecx #
movq %rbx, (%rsp) # _29,
movq $img_i2c_isr, %rdx #,
movl %eax, %esi # <retval>,
movq %r14, %rdi # _1,
call devm_request_threaded_irq #
.LVL53:
.LBE771:
.LBE773:
# /git/arm-soc/drivers/i2c/busses/i2c-img-scb.c:1359: if (ret) {
.loc 1 1359 0
testl %eax, %eax # _62
movl %eax, -52(%rbp) # _62, %sfp
jne .L66 #,
.LBB774:
# /git/arm-soc/drivers/i2c/busses/i2c-img-scb.c:1365:
init_timer(&i2c->check_timer);
.loc 1 1365 0
leaq 1864(%rbx), %rdi #, tmp131
xorl %esi, %esi #
movq $__key.25244, %rcx #,
movq $.LC7, %rdx #,
call init_timer_key #
.LVL54:
.LBE774:
# /git/arm-soc/drivers/i2c/busses/i2c-img-scb.c:1369: i2c->bitrate =
timings[0].max_bitrate;
.loc 1 1369 0
movl timings+8(%rip), %eax # timings[0].max_bitrate, timings[0].max_bitrate
.LBB775:
.LBB776:
.LBB777:
# /git/arm-soc/include/linux/of.h:458: int ret =
of_property_read_variable_u32_array(np, propname, out_values,
.file 8 "/git/arm-soc/include/linux/of.h"
.loc 8 458 0
leaq -44(%rbp), %rdx #, tmp161
xorl %r8d, %r8d #
.LBE777:
.LBE776:
.LBE775:
# /git/arm-soc/drivers/i2c/busses/i2c-img-scb.c:1366:
i2c->check_timer.function = img_i2c_check_timer;
.loc 1 1366 0
movq $img_i2c_check_timer, 1888(%rbx) #, MEM[(struct img_i2c
*)_29].check_timer.function
# /git/arm-soc/drivers/i2c/busses/i2c-img-scb.c:1367:
i2c->check_timer.data = (unsigned long)i2c;
.loc 1 1367 0
movq %rbx, 1896(%rbx) # _29, MEM[(struct img_i2c *)_29].check_timer.data
.LBB782:
.LBB780:
.LBB778:
# /git/arm-soc/include/linux/of.h:458: int ret =
of_property_read_variable_u32_array(np, propname, out_values,
.loc 8 458 0
movl $1, %ecx #,
movq $.LC8, %rsi #,
movq %r15, %rdi # node,
.LBE778:
.LBE780:
.LBE782:
# /git/arm-soc/drivers/i2c/busses/i2c-img-scb.c:1369: i2c->bitrate =
timings[0].max_bitrate;
.loc 1 1369 0
movl %eax, 1648(%rbx) # timings[0].max_bitrate, MEM[(struct img_i2c
*)_29].bitrate
.LBB783:
.LBB781:
.LBB779:
# /git/arm-soc/include/linux/of.h:458: int ret =
of_property_read_variable_u32_array(np, propname, out_values,
.loc 8 458 0
call of_property_read_variable_u32_array #
.LVL55:
# /git/arm-soc/include/linux/of.h:460: if (ret >= 0)
.loc 8 460 0
testl %eax, %eax # ret
js .L57 #,
.LVL56:
.LBE779:
.LBE781:
.LBE783:
# /git/arm-soc/drivers/i2c/busses/i2c-img-scb.c:1371: i2c->bitrate = val;
.loc 1 1371 0
movl -44(%rbp), %eax # val, val
.LVL57:
movl %eax, 1648(%rbx) # val, MEM[(struct img_i2c *)_29].bitrate
.LVL58:
.L57:
# /git/arm-soc/drivers/i2c/busses/i2c-img-scb.c:1379: i2c->adap.nr = pdev->id;
.loc 1 1379 0
movl 8(%r12), %eax # pdev_19(D)->id, pdev_19(D)->id
.LVL59:
.LBB784:
.LBB785:
# /git/arm-soc/include/linux/spinlock.h:288: return &lock->rlock;
.loc 4 288 0
leaq 1752(%rbx), %rdi #, tmp142
.LBE785:
.LBE784:
.LBB786:
.LBB787:
.LBB788:
# /git/arm-soc/include/linux/device.h:1033: dev->driver_data = data;
.loc 5 1033 0
movq %rbx, 512(%rbx) # _29, MEM[(struct device *)_29 + 240B].driver_data
.LBE788:
.LBE787:
.LBE786:
# /git/arm-soc/drivers/i2c/busses/i2c-img-scb.c:1374:
i2c->adap.dev.parent = &pdev->dev;
.loc 1 1374 0
movq %r14, 240(%rbx) # _1, MEM[(struct img_i2c *)_29].adap.dev.parent
# /git/arm-soc/drivers/i2c/busses/i2c-img-scb.c:1375:
i2c->adap.dev.of_node = node;
.loc 1 1375 0
movq %r15, 1088(%rbx) # node, MEM[(struct img_i2c *)_29].adap.dev.of_node
.LBB789:
# /git/arm-soc/drivers/i2c/busses/i2c-img-scb.c:1383:
spin_lock_init(&i2c->lock);
.loc 1 1383 0
movq $__key.25245, %rdx #,
.LBE789:
# /git/arm-soc/drivers/i2c/busses/i2c-img-scb.c:1376: i2c->adap.owner
= THIS_MODULE;
.loc 1 1376 0
movq $__this_module, (%rbx) #, MEM[(struct img_i2c *)_29].adap.owner
# /git/arm-soc/drivers/i2c/busses/i2c-img-scb.c:1377: i2c->adap.algo =
&img_i2c_algo;
.loc 1 1377 0
movq $img_i2c_algo, 16(%rbx) #, MEM[(struct img_i2c *)_29].adap.algo
.LBB790:
# /git/arm-soc/drivers/i2c/busses/i2c-img-scb.c:1383:
spin_lock_init(&i2c->lock);
.loc 1 1383 0
movq $.LC9, %rsi #,
.LBE790:
# /git/arm-soc/drivers/i2c/busses/i2c-img-scb.c:1379: i2c->adap.nr = pdev->id;
.loc 1 1379 0
movl %eax, 1280(%rbx) # pdev_19(D)->id, MEM[(struct img_i2c *)_29].adap.nr
# /git/arm-soc/drivers/i2c/busses/i2c-img-scb.c:1380:
snprintf(i2c->adap.name, sizeof(i2c->adap.name), "IMG SCB I2C");
.loc 1 1380 0
movabsq $2324494381979487561, %rax #, tmp162
# /git/arm-soc/drivers/i2c/busses/i2c-img-scb.c:1378: i2c->adap.retries = 5;
.loc 1 1378 0
movl $5, 236(%rbx) #, MEM[(struct img_i2c *)_29].adap.retries
# /git/arm-soc/drivers/i2c/busses/i2c-img-scb.c:1380:
snprintf(i2c->adap.name, sizeof(i2c->adap.name), "IMG SCB I2C");
.loc 1 1380 0
movq %rax, 1284(%rbx) # tmp162, MEM[(void *)_29 + 1284B]
movl $4403785, 1292(%rbx) #, MEM[(void *)_29 + 1284B]
.LBB791:
.LBB792:
# /git/arm-soc/drivers/i2c/busses/i2c-img-scb.c:443: i2c->mode = mode;
.loc 1 443 0
movl $0, 1848(%rbx) #, MEM[(struct img_i2c *)_29].mode
# /git/arm-soc/drivers/i2c/busses/i2c-img-scb.c:444: i2c->int_enable =
img_i2c_int_enable_by_mode[mode];
.loc 1 444 0
movq $0, 1852(%rbx) #, MEM[(unsigned int *)_29 + 1852B]
.LBE792:
.LBE791:
.LBB793:
# /git/arm-soc/drivers/i2c/busses/i2c-img-scb.c:1383:
spin_lock_init(&i2c->lock);
.loc 1 1383 0
call __raw_spin_lock_init #
.LVL60:
.LBE793:
.LBB794:
.LBB795:
.LBB796:
# /git/arm-soc/include/linux/completion.h:79: init_waitqueue_head(&x->wait);
.file 9 "/git/arm-soc/include/linux/completion.h"
.loc 9 79 0
leaq 1664(%rbx), %rdi #, tmp144
.LBE796:
# /git/arm-soc/include/linux/completion.h:78: x->done = 0;
.loc 9 78 0
movl $0, 1656(%rbx) #, MEM[(struct completion *)_29 + 1656B].done
.LBB797:
# /git/arm-soc/include/linux/completion.h:79: init_waitqueue_head(&x->wait);
.loc 9 79 0
movq $__key.8818, %rdx #,
movq $.LC10, %rsi #,
call __init_waitqueue_head #
.LVL61:
.LBE797:
.LBE795:
.LBE794:
.LBB798:
.LBB799:
.LBB800:
.LBB801:
# /git/arm-soc/include/linux/clk.h:191: might_sleep();
.loc 2 191 0
xorl %edx, %edx #
.LBE801:
.LBE800:
.LBE799:
.LBE798:
.LBB805:
.LBB806:
.LBB807:
# /git/arm-soc/include/linux/device.h:1033: dev->driver_data = data;
.loc 5 1033 0
movq %rbx, 288(%r12) # _29, MEM[(struct device *)pdev_19(D) + 16B].driver_data
.LBE807:
.LBE806:
.LBE805:
.LBB808:
.LBB804:
.LBB803:
.LBB802:
# /git/arm-soc/include/linux/clk.h:191: might_sleep();
.loc 2 191 0
movl $191, %esi #,
movq $.LC0, %rdi #,
call __might_sleep #
.LVL62:
.LBE802:
.LBE803:
.LBE804:
.LBE808:
.LBB809:
.LBB810:
.LBB811:
.LBB812:
.LBB813:
.LBB814:
xorl %edx, %edx #
movl $191, %esi #,
movq $.LC0, %rdi #,
call __might_sleep #
.LVL63:
.LBE814:
.LBE813:
.LBE812:
.LBE811:
.LBB815:
.LBB816:
# /git/arm-soc/drivers/i2c/busses/i2c-img-scb.c:418: return
readl(i2c->base + offset);
.loc 1 418 0
movq 1624(%rbx), %rax # MEM[(struct img_i2c *)_29].base, MEM[(struct
img_i2c *)_29].base
.LBB817:
.LBB818:
# /git/arm-soc/arch/x86/include/asm/io.h:58: build_mmio_read(readl,
"l", unsigned int, "=r", :"memory")
.loc 3 58 0
#APP
# 58 "/git/arm-soc/arch/x86/include/asm/io.h" 1
movl 128(%rax),%eax # MEM[(volatile unsigned int *)_81], ret
# 0 "" 2
.LVL64:
#NO_APP
.LBE818:
.LBE817:
.LBE816:
.LBE815:
# /git/arm-soc/drivers/i2c/busses/i2c-img-scb.c:1161: if ((rev &
0x00ffffff) < 0x00020200) {
.loc 1 1161 0
movl %eax, %edx # ret, tmp147
andl $16777215, %edx #, tmp147
cmpl $131583, %edx #, tmp147
jbe .L67 #,
# /git/arm-soc/drivers/i2c/busses/i2c-img-scb.c:1176: if (i2c->bitrate
<= timings[i].max_bitrate) {
.loc 1 1176 0
movl 1648(%rbx), %edx # MEM[(struct img_i2c *)_29].bitrate, _99
cmpl timings+8(%rip), %edx # timings[0].max_bitrate, _99
# /git/arm-soc/drivers/i2c/busses/i2c-img-scb.c:1171:
i2c->need_wr_rd_fence = true;
.loc 1 1171 0
movb $1, 1652(%rbx) #, MEM[(struct img_i2c *)_29].need_wr_rd_fence
movl timings+48(%rip), %ecx # timings[1].max_bitrate, pretmp_260
# /git/arm-soc/drivers/i2c/busses/i2c-img-scb.c:1176: if (i2c->bitrate
<= timings[i].max_bitrate) {
.loc 1 1176 0
jbe .L59 #,
cmpl %ecx, %edx # pretmp_260, _99
jbe .L60 #,
.L61:
.LBB819:
# /git/arm-soc/drivers/i2c/busses/i2c-img-scb.c:1182:
dev_warn(i2c->adap.dev.parent,
.loc 1 1182 0
movq 240(%rbx), %rdi # MEM[(struct img_i2c *)_29].adap.dev.parent,
MEM[(struct img_i2c *)_29].adap.dev.parent
movq $.LC12, %rsi #,
call dev_warn #
.LVL65:
# /git/arm-soc/drivers/i2c/busses/i2c-img-scb.c:1187: i2c->bitrate =
timing.max_bitrate;
.loc 1 1187 0
movl timings+48(%rip), %eax # MEM[(struct img_i2c_timings *)&timings +
48B], MEM[(struct img_i2c_timings *)&timings + 48B]
movl %eax, 1648(%rbx) # MEM[(struct img_i2c_timings *)&timings + 48B],
MEM[(struct img_i2c *)_29].bitrate
.LVL66:
.L60:
ud2
.LVL67:
.L66:
.LBE819:
.LBE810:
.LBE809:
On Wed, Mar 1, 2017 at 5:53 PM, Josh Poimboeuf <[email protected]> wrote:
> On Wed, Mar 01, 2017 at 04:27:29PM +0100, Arnd Bergmann wrote:
> I see no apparent reason for the ud2.
It's the possible division by zero. This change would avoid the ud2:
diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-scb.c
index db8e8b40569d..a2b09c518225 100644
--- a/drivers/i2c/busses/i2c-img-scb.c
+++ b/drivers/i2c/busses/i2c-img-scb.c
@@ -1196,6 +1196,8 @@ static int img_i2c_init(struct img_i2c *i2c)
clk_khz /= prescale;
/* Setup the clock increment value */
+ if (clk_khz < 1)
+ clk_khz = 1;
inc = (256 * 16 * bitrate_khz) / clk_khz;
/*
Arnd
On Wed, Mar 01, 2017 at 11:42:54PM +0100, Arnd Bergmann wrote:
> On Wed, Mar 1, 2017 at 5:53 PM, Josh Poimboeuf <[email protected]> wrote:
> > On Wed, Mar 01, 2017 at 04:27:29PM +0100, Arnd Bergmann wrote:
>
> > I see no apparent reason for the ud2.
>
> It's the possible division by zero. This change would avoid the ud2:
>
> diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-scb.c
> index db8e8b40569d..a2b09c518225 100644
> --- a/drivers/i2c/busses/i2c-img-scb.c
> +++ b/drivers/i2c/busses/i2c-img-scb.c
> @@ -1196,6 +1196,8 @@ static int img_i2c_init(struct img_i2c *i2c)
> clk_khz /= prescale;
>
> /* Setup the clock increment value */
> + if (clk_khz < 1)
> + clk_khz = 1;
> inc = (256 * 16 * bitrate_khz) / clk_khz;
>
> /*
Ok, I see what gcc is doing.
clk_khz = clk_get_rate(i2c->scb_clk) / 1000;
...
inc = (256 * 16 * bitrate_khz) / clk_khz;
Because CONFIG_HAVE_CLK isn't set, clk_get_rate() returns 0, which means
clk_khz is always zero, so the last statement *always* results in a
divide-by-zero. So that looks like a bug in the code.
However, I'm baffled by how gcc handles it. Instead of:
a) reporting a compile-time warning/error; or
b) letting the #DE (divide error) exception happen;
it inserts a 'ud2', resulting in a #UD (invalid opcode). Why?!?
--
Josh
* Josh Poimboeuf <[email protected]> wrote:
> On Wed, Mar 01, 2017 at 11:42:54PM +0100, Arnd Bergmann wrote:
> > On Wed, Mar 1, 2017 at 5:53 PM, Josh Poimboeuf <[email protected]> wrote:
> > > On Wed, Mar 01, 2017 at 04:27:29PM +0100, Arnd Bergmann wrote:
> >
> > > I see no apparent reason for the ud2.
> >
> > It's the possible division by zero. This change would avoid the ud2:
> >
> > diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-scb.c
> > index db8e8b40569d..a2b09c518225 100644
> > --- a/drivers/i2c/busses/i2c-img-scb.c
> > +++ b/drivers/i2c/busses/i2c-img-scb.c
> > @@ -1196,6 +1196,8 @@ static int img_i2c_init(struct img_i2c *i2c)
> > clk_khz /= prescale;
> >
> > /* Setup the clock increment value */
> > + if (clk_khz < 1)
> > + clk_khz = 1;
> > inc = (256 * 16 * bitrate_khz) / clk_khz;
> >
> > /*
>
> Ok, I see what gcc is doing.
>
> clk_khz = clk_get_rate(i2c->scb_clk) / 1000;
> ...
> inc = (256 * 16 * bitrate_khz) / clk_khz;
>
> Because CONFIG_HAVE_CLK isn't set, clk_get_rate() returns 0, which means
> clk_khz is always zero, so the last statement *always* results in a
> divide-by-zero. So that looks like a bug in the code.
>
> However, I'm baffled by how gcc handles it. Instead of:
>
> a) reporting a compile-time warning/error; or
>
> b) letting the #DE (divide error) exception happen;
>
> it inserts a 'ud2', resulting in a #UD (invalid opcode). Why?!?
Well, technically an invalid opcode is shorter code than generating an (integer)
division by zero exception, right?
Thanks,
Ingo
* Josh Poimboeuf <[email protected]> wrote:
> On Thu, Mar 02, 2017 at 07:31:39AM +0100, Ingo Molnar wrote:
> >
> > * Josh Poimboeuf <[email protected]> wrote:
> >
> > > On Wed, Mar 01, 2017 at 11:42:54PM +0100, Arnd Bergmann wrote:
> > > > On Wed, Mar 1, 2017 at 5:53 PM, Josh Poimboeuf <[email protected]> wrote:
> > > > > On Wed, Mar 01, 2017 at 04:27:29PM +0100, Arnd Bergmann wrote:
> > > >
> > > > > I see no apparent reason for the ud2.
> > > >
> > > > It's the possible division by zero. This change would avoid the ud2:
> > > >
> > > > diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-scb.c
> > > > index db8e8b40569d..a2b09c518225 100644
> > > > --- a/drivers/i2c/busses/i2c-img-scb.c
> > > > +++ b/drivers/i2c/busses/i2c-img-scb.c
> > > > @@ -1196,6 +1196,8 @@ static int img_i2c_init(struct img_i2c *i2c)
> > > > clk_khz /= prescale;
> > > >
> > > > /* Setup the clock increment value */
> > > > + if (clk_khz < 1)
> > > > + clk_khz = 1;
> > > > inc = (256 * 16 * bitrate_khz) / clk_khz;
> > > >
> > > > /*
> > >
> > > Ok, I see what gcc is doing.
> > >
> > > clk_khz = clk_get_rate(i2c->scb_clk) / 1000;
> > > ...
> > > inc = (256 * 16 * bitrate_khz) / clk_khz;
> > >
> > > Because CONFIG_HAVE_CLK isn't set, clk_get_rate() returns 0, which means
> > > clk_khz is always zero, so the last statement *always* results in a
> > > divide-by-zero. So that looks like a bug in the code.
> > >
> > > However, I'm baffled by how gcc handles it. Instead of:
> > >
> > > a) reporting a compile-time warning/error; or
> > >
> > > b) letting the #DE (divide error) exception happen;
> > >
> > > it inserts a 'ud2', resulting in a #UD (invalid opcode). Why?!?
> >
> > Well, technically an invalid opcode is shorter code than generating an (integer)
> > division by zero exception, right?
>
> What does that matter if it's the wrong behavior?
Well, both terminate the program, and it's obvious if you look at it with a
debugger what happened, right?
Thanks,
Ingo
On Thu, Mar 02, 2017 at 07:31:39AM +0100, Ingo Molnar wrote:
>
> * Josh Poimboeuf <[email protected]> wrote:
>
> > On Wed, Mar 01, 2017 at 11:42:54PM +0100, Arnd Bergmann wrote:
> > > On Wed, Mar 1, 2017 at 5:53 PM, Josh Poimboeuf <[email protected]> wrote:
> > > > On Wed, Mar 01, 2017 at 04:27:29PM +0100, Arnd Bergmann wrote:
> > >
> > > > I see no apparent reason for the ud2.
> > >
> > > It's the possible division by zero. This change would avoid the ud2:
> > >
> > > diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-scb.c
> > > index db8e8b40569d..a2b09c518225 100644
> > > --- a/drivers/i2c/busses/i2c-img-scb.c
> > > +++ b/drivers/i2c/busses/i2c-img-scb.c
> > > @@ -1196,6 +1196,8 @@ static int img_i2c_init(struct img_i2c *i2c)
> > > clk_khz /= prescale;
> > >
> > > /* Setup the clock increment value */
> > > + if (clk_khz < 1)
> > > + clk_khz = 1;
> > > inc = (256 * 16 * bitrate_khz) / clk_khz;
> > >
> > > /*
> >
> > Ok, I see what gcc is doing.
> >
> > clk_khz = clk_get_rate(i2c->scb_clk) / 1000;
> > ...
> > inc = (256 * 16 * bitrate_khz) / clk_khz;
> >
> > Because CONFIG_HAVE_CLK isn't set, clk_get_rate() returns 0, which means
> > clk_khz is always zero, so the last statement *always* results in a
> > divide-by-zero. So that looks like a bug in the code.
> >
> > However, I'm baffled by how gcc handles it. Instead of:
> >
> > a) reporting a compile-time warning/error; or
> >
> > b) letting the #DE (divide error) exception happen;
> >
> > it inserts a 'ud2', resulting in a #UD (invalid opcode). Why?!?
>
> Well, technically an invalid opcode is shorter code than generating an (integer)
> division by zero exception, right?
What does that matter if it's the wrong behavior?
--
Josh
On Thu, Mar 02, 2017 at 02:46:29PM +0100, Ingo Molnar wrote:
>
> * Josh Poimboeuf <[email protected]> wrote:
>
> > On Thu, Mar 02, 2017 at 07:31:39AM +0100, Ingo Molnar wrote:
> > >
> > > * Josh Poimboeuf <[email protected]> wrote:
> > >
> > > > On Wed, Mar 01, 2017 at 11:42:54PM +0100, Arnd Bergmann wrote:
> > > > > On Wed, Mar 1, 2017 at 5:53 PM, Josh Poimboeuf <[email protected]> wrote:
> > > > > > On Wed, Mar 01, 2017 at 04:27:29PM +0100, Arnd Bergmann wrote:
> > > > >
> > > > > > I see no apparent reason for the ud2.
> > > > >
> > > > > It's the possible division by zero. This change would avoid the ud2:
> > > > >
> > > > > diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-scb.c
> > > > > index db8e8b40569d..a2b09c518225 100644
> > > > > --- a/drivers/i2c/busses/i2c-img-scb.c
> > > > > +++ b/drivers/i2c/busses/i2c-img-scb.c
> > > > > @@ -1196,6 +1196,8 @@ static int img_i2c_init(struct img_i2c *i2c)
> > > > > clk_khz /= prescale;
> > > > >
> > > > > /* Setup the clock increment value */
> > > > > + if (clk_khz < 1)
> > > > > + clk_khz = 1;
> > > > > inc = (256 * 16 * bitrate_khz) / clk_khz;
> > > > >
> > > > > /*
> > > >
> > > > Ok, I see what gcc is doing.
> > > >
> > > > clk_khz = clk_get_rate(i2c->scb_clk) / 1000;
> > > > ...
> > > > inc = (256 * 16 * bitrate_khz) / clk_khz;
> > > >
> > > > Because CONFIG_HAVE_CLK isn't set, clk_get_rate() returns 0, which means
> > > > clk_khz is always zero, so the last statement *always* results in a
> > > > divide-by-zero. So that looks like a bug in the code.
> > > >
> > > > However, I'm baffled by how gcc handles it. Instead of:
> > > >
> > > > a) reporting a compile-time warning/error; or
> > > >
> > > > b) letting the #DE (divide error) exception happen;
> > > >
> > > > it inserts a 'ud2', resulting in a #UD (invalid opcode). Why?!?
> > >
> > > Well, technically an invalid opcode is shorter code than generating an (integer)
> > > division by zero exception, right?
> >
> > What does that matter if it's the wrong behavior?
>
> Well, both terminate the program, and it's obvious if you look at it with a
> debugger what happened, right?
If it were obvious, we wouldn't be having this discussion :-)
The only thing obvious to me was that gcc mysteriously removed a bunch
of code and replaced it with a 'ud2' instruction in the middle of the
function for no apparent reason.
--
Josh
* Josh Poimboeuf <[email protected]> wrote:
> > > > Well, technically an invalid opcode is shorter code than generating an
> > > > (integer) division by zero exception, right?
> > >
> > > What does that matter if it's the wrong behavior?
> >
> > Well, both terminate the program, and it's obvious if you look at it with a
> > debugger what happened, right?
>
> If it were obvious, we wouldn't be having this discussion :-)
Touche ;-)
> The only thing obvious to me was that gcc mysteriously removed a bunch of code
> and replaced it with a 'ud2' instruction in the middle of the function for no
> apparent reason.
I don't know what their motivation was, but if it's not a bug, if it was done
intentionally, then I'd guess it's roughly the argument I made: in simple
testcases it can be argued to be a code size improvement, plus it's probably
allowed by the letter of the compiler standards (program termination behavior is
notoriously platform dependent and thus vaguely specified) - but for real-life
code I very much agree that it's a step backward in generated code quality...
Thanks,
Ingo
On Wed, Mar 01, 2017 at 04:21:36PM +0100, Arnd Bergmann wrote:
> On Wed, Mar 1, 2017 at 3:31 PM, Josh Poimboeuf <[email protected]> wrote:
> > On Wed, Mar 01, 2017 at 10:34:42AM +0100, Arnd Bergmann wrote:
> >> On Tue, Oct 11, 2016 at 10:38 PM, Arnd Bergmann <[email protected]> wrote:
> >> > On Tuesday, October 11, 2016 10:51:46 AM CEST Josh Poimboeuf wrote:
> >> >>
> >> >> 3) 0xFC244C03-config:
> >> >> drivers/scsi/fnic/fnic_main.o: warning: objtool: fnic_log_q_error() falls through to next function fnic_handle_link_event()
> >> >> drivers/scsi/snic/snic_res.o: warning: objtool: .text: unexpected end of section
> >> >>
> >> >> These look like another bad gcc bug which is truncating functions:
> >> >
> >> > Same bug for both of them?
> >>
> >> I ran into this one again today, after updating to the latest gcc-7.0.1:
> >>
> >> drivers/infiniband/sw/rxe/rxe_resp.o: warning: objtool:
> >> rxe_responder()+0xfe: sibling call from callable instruction with
> >> changed frame pointer
> >>
> >> Josh, did you get around to updating objtool the last time I reported it, or
> >> is it still the same problem? If this is a new variation, I can provide more
> >> details about the failure, otherwise I'll just ignore it for now.
> >
> > This one should have been fixed with:
> >
> > 3732710ff6f2 ("objtool: Improve rare switch jump table pattern detection")
>
> It was on the current linux-next, so that commit should certainly be included.
>
> > Can you attach the object file?
Here's the preliminary fix for this one (still needs more testing):
diff --git a/tools/objtool/builtin-check.c b/tools/objtool/builtin-check.c
index bd12eb1..7b718bb 100644
--- a/tools/objtool/builtin-check.c
+++ b/tools/objtool/builtin-check.c
@@ -806,11 +806,20 @@ static struct rela *find_switch_table(struct objtool_file *file,
insn->jump_dest->offset > orig_insn->offset))
break;
+ /* look for a rela which references .rodata */
text_rela = find_rela_by_dest_range(insn->sec, insn->offset,
insn->len);
- if (text_rela && text_rela->sym == file->rodata->sym)
- return find_rela_by_dest(file->rodata,
- text_rela->addend);
+ if (!text_rela || text_rela->sym != file->rodata->sym)
+ continue;
+
+ /*
+ * Make sure the .rodata address isn't associated with a
+ * symbol. gcc jump tables are anonymous data.
+ */
+ if (find_symbol_containing(file->rodata, text_rela->addend))
+ continue;
+
+ return find_rela_by_dest(file->rodata, text_rela->addend);
}
return NULL;
diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c
index 0d7983a..d897702 100644
--- a/tools/objtool/elf.c
+++ b/tools/objtool/elf.c
@@ -85,6 +85,18 @@ struct symbol *find_symbol_by_offset(struct section *sec, unsigned long offset)
return NULL;
}
+struct symbol *find_symbol_containing(struct section *sec, unsigned long offset)
+{
+ struct symbol *sym;
+
+ list_for_each_entry(sym, &sec->symbol_list, list)
+ if (sym->type != STT_SECTION &&
+ offset >= sym->offset && offset < sym->offset + sym->len)
+ return sym;
+
+ return NULL;
+}
+
struct rela *find_rela_by_dest_range(struct section *sec, unsigned long offset,
unsigned int len)
{
diff --git a/tools/objtool/elf.h b/tools/objtool/elf.h
index aa1ff65..731973e 100644
--- a/tools/objtool/elf.h
+++ b/tools/objtool/elf.h
@@ -79,6 +79,7 @@ struct elf {
struct elf *elf_open(const char *name);
struct section *find_section_by_name(struct elf *elf, const char *name);
struct symbol *find_symbol_by_offset(struct section *sec, unsigned long offset);
+struct symbol *find_symbol_containing(struct section *sec, unsigned long offset);
struct rela *find_rela_by_dest(struct section *sec, unsigned long offset);
struct rela *find_rela_by_dest_range(struct section *sec, unsigned long offset,
unsigned int len);
On Thu, Mar 2, 2017 at 7:25 PM, Josh Poimboeuf <[email protected]> wrote:
>> It was on the current linux-next, so that commit should certainly be included.
>>
>> > Can you attach the object file?
>
> Here's the preliminary fix for this one (still needs more testing):
It fixes the warning for me.
Arnd
On Thu, Mar 2, 2017 at 11:57 PM, Josh Poimboeuf <[email protected]> wrote:
>
> Arnd Bergmann reported a (false positive) objtool warning:
>
> drivers/infiniband/sw/rxe/rxe_resp.o: warning: objtool: rxe_responder()+0xfe: sibling call from callable instruction with changed frame pointer
>
> The issue is in find_switch_table(). It tries to find a switch
> statement's jump table by walking backwards from an indirect jump
> instruction, looking for a relocation to the .rodata section. In this
> case it stopped walking prematurely: the first .rodata relocation it
> encountered was for a variable (resp_state_name) instead of a jump
> table, so it just assumed there wasn't a jump table.
>
> The fix is to ignore any .rodata relocation which refers to an ELF
> object symbol. This works because the jump tables are anonymous and
> have no symbols associated with them.
>
> Reported-by: Arnd Bergmann <[email protected]>
> Fixes: 3732710ff6f2 ("objtool: Improve rare switch jump table pattern detection")
> Signed-off-by: Josh Poimboeuf <[email protected]>
Tested-by: Arnd Bergmann <[email protected]>
On Thu, Mar 2, 2017 at 2:03 AM, Josh Poimboeuf <[email protected]> wrote:
> On Wed, Mar 01, 2017 at 11:42:54PM +0100, Arnd Bergmann wrote:
>> On Wed, Mar 1, 2017 at 5:53 PM, Josh Poimboeuf <[email protected]> wrote:
>> > On Wed, Mar 01, 2017 at 04:27:29PM +0100, Arnd Bergmann wrote:
>>
>> > I see no apparent reason for the ud2.
>>
>> It's the possible division by zero. This change would avoid the ud2:
>>
>> diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-scb.c
>> index db8e8b40569d..a2b09c518225 100644
>> --- a/drivers/i2c/busses/i2c-img-scb.c
>> +++ b/drivers/i2c/busses/i2c-img-scb.c
>> @@ -1196,6 +1196,8 @@ static int img_i2c_init(struct img_i2c *i2c)
>> clk_khz /= prescale;
>>
>> /* Setup the clock increment value */
>> + if (clk_khz < 1)
>> + clk_khz = 1;
>> inc = (256 * 16 * bitrate_khz) / clk_khz;
>>
>> /*
>
> Ok, I see what gcc is doing.
>
> clk_khz = clk_get_rate(i2c->scb_clk) / 1000;
> ...
> inc = (256 * 16 * bitrate_khz) / clk_khz;
>
> Because CONFIG_HAVE_CLK isn't set, clk_get_rate() returns 0, which means
> clk_khz is always zero, so the last statement *always* results in a
> divide-by-zero. So that looks like a bug in the code.
>
> However, I'm baffled by how gcc handles it. Instead of:
>
> a) reporting a compile-time warning/error; or
>
> b) letting the #DE (divide error) exception happen;
>
> it inserts a 'ud2', resulting in a #UD (invalid opcode). Why?!?
Just FYI, I found another one like this:
0000000000000000 <hibvt_pwm_get_state>:
0: e8 00 00 00 00 callq 5 <hibvt_pwm_get_state+0x5>
1: R_X86_64_PC32 __fentry__-0x4
5: 8b 46 10 mov 0x10(%rsi),%eax
8: 55 push %rbp
9: 48 89 e5 mov %rsp,%rbp
c: c1 e0 05 shl $0x5,%eax
f: 48 03 47 48 add 0x48(%rdi),%rax
13: 8b 00 mov (%rax),%eax
15: 0f 0b ud2
17: 66 0f 1f 84 00 00 00 nopw 0x0(%rax,%rax,1)
1e: 00 00
static inline unsigned long clk_get_rate(struct clk *clk)
{
return 0;
}
static void hibvt_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
struct pwm_state *state)
{
struct hibvt_pwm_chip *hi_pwm_chip = to_hibvt_pwm_chip(chip);
void __iomem *base;
u32 freq, value;
freq = div_u64(clk_get_rate(hi_pwm_chip->clk), 1000000);
base = hi_pwm_chip->base;
value = readl(base + PWM_CFG0_ADDR(pwm->hwpwm));
state->period = div_u64(value * 1000, freq);
value = readl(base + PWM_CFG1_ADDR(pwm->hwpwm));
state->duty_cycle = div_u64(value * 1000, freq);
value = readl(base + PWM_CTRL_ADDR(pwm->hwpwm));
state->enabled = (PWM_ENABLE_MASK & value);
}
Arnd
On Thu, Mar 02, 2017 at 11:49:49PM +0100, Arnd Bergmann wrote:
> On Thu, Mar 2, 2017 at 2:03 AM, Josh Poimboeuf <[email protected]> wrote:
> > On Wed, Mar 01, 2017 at 11:42:54PM +0100, Arnd Bergmann wrote:
> >> On Wed, Mar 1, 2017 at 5:53 PM, Josh Poimboeuf <[email protected]> wrote:
> >> > On Wed, Mar 01, 2017 at 04:27:29PM +0100, Arnd Bergmann wrote:
> >>
> >> > I see no apparent reason for the ud2.
> >>
> >> It's the possible division by zero. This change would avoid the ud2:
> >>
> >> diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-scb.c
> >> index db8e8b40569d..a2b09c518225 100644
> >> --- a/drivers/i2c/busses/i2c-img-scb.c
> >> +++ b/drivers/i2c/busses/i2c-img-scb.c
> >> @@ -1196,6 +1196,8 @@ static int img_i2c_init(struct img_i2c *i2c)
> >> clk_khz /= prescale;
> >>
> >> /* Setup the clock increment value */
> >> + if (clk_khz < 1)
> >> + clk_khz = 1;
> >> inc = (256 * 16 * bitrate_khz) / clk_khz;
> >>
> >> /*
> >
> > Ok, I see what gcc is doing.
> >
> > clk_khz = clk_get_rate(i2c->scb_clk) / 1000;
> > ...
> > inc = (256 * 16 * bitrate_khz) / clk_khz;
> >
> > Because CONFIG_HAVE_CLK isn't set, clk_get_rate() returns 0, which means
> > clk_khz is always zero, so the last statement *always* results in a
> > divide-by-zero. So that looks like a bug in the code.
> >
> > However, I'm baffled by how gcc handles it. Instead of:
> >
> > a) reporting a compile-time warning/error; or
> >
> > b) letting the #DE (divide error) exception happen;
> >
> > it inserts a 'ud2', resulting in a #UD (invalid opcode). Why?!?
>
> Just FYI, I found another one like this:
>
> 0000000000000000 <hibvt_pwm_get_state>:
> 0: e8 00 00 00 00 callq 5 <hibvt_pwm_get_state+0x5>
> 1: R_X86_64_PC32 __fentry__-0x4
> 5: 8b 46 10 mov 0x10(%rsi),%eax
> 8: 55 push %rbp
> 9: 48 89 e5 mov %rsp,%rbp
> c: c1 e0 05 shl $0x5,%eax
> f: 48 03 47 48 add 0x48(%rdi),%rax
> 13: 8b 00 mov (%rax),%eax
> 15: 0f 0b ud2
> 17: 66 0f 1f 84 00 00 00 nopw 0x0(%rax,%rax,1)
> 1e: 00 00
>
> static inline unsigned long clk_get_rate(struct clk *clk)
> {
> return 0;
> }
>
> static void hibvt_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> struct pwm_state *state)
> {
> struct hibvt_pwm_chip *hi_pwm_chip = to_hibvt_pwm_chip(chip);
> void __iomem *base;
> u32 freq, value;
>
> freq = div_u64(clk_get_rate(hi_pwm_chip->clk), 1000000);
> base = hi_pwm_chip->base;
>
> value = readl(base + PWM_CFG0_ADDR(pwm->hwpwm));
> state->period = div_u64(value * 1000, freq);
>
> value = readl(base + PWM_CFG1_ADDR(pwm->hwpwm));
> state->duty_cycle = div_u64(value * 1000, freq);
>
> value = readl(base + PWM_CTRL_ADDR(pwm->hwpwm));
> state->enabled = (PWM_ENABLE_MASK & value);
> }
I assume '-Wdiv-by-zero' is enabled and gcc isn't showing the "division
by zero" warning for either of these? The 'ud2' is guaranteed to
trigger since the function has no branches. Surely at least the missing
warning is a gcc bug.
The good news is objtool is flushing these out, albeit with a confusing
message.
--
Josh
Arnd Bergmann reported a (false positive) objtool warning:
drivers/infiniband/sw/rxe/rxe_resp.o: warning: objtool: rxe_responder()+0xfe: sibling call from callable instruction with changed frame pointer
The issue is in find_switch_table(). It tries to find a switch
statement's jump table by walking backwards from an indirect jump
instruction, looking for a relocation to the .rodata section. In this
case it stopped walking prematurely: the first .rodata relocation it
encountered was for a variable (resp_state_name) instead of a jump
table, so it just assumed there wasn't a jump table.
The fix is to ignore any .rodata relocation which refers to an ELF
object symbol. This works because the jump tables are anonymous and
have no symbols associated with them.
Reported-by: Arnd Bergmann <[email protected]>
Fixes: 3732710ff6f2 ("objtool: Improve rare switch jump table pattern detection")
Signed-off-by: Josh Poimboeuf <[email protected]>
---
tools/objtool/builtin-check.c | 15 ++++++++++++---
tools/objtool/elf.c | 12 ++++++++++++
tools/objtool/elf.h | 1 +
3 files changed, 25 insertions(+), 3 deletions(-)
diff --git a/tools/objtool/builtin-check.c b/tools/objtool/builtin-check.c
index 5fc52ee..c2a8518 100644
--- a/tools/objtool/builtin-check.c
+++ b/tools/objtool/builtin-check.c
@@ -805,11 +805,20 @@ static struct rela *find_switch_table(struct objtool_file *file,
insn->jump_dest->offset > orig_insn->offset))
break;
+ /* look for a relocation which references .rodata */
text_rela = find_rela_by_dest_range(insn->sec, insn->offset,
insn->len);
- if (text_rela && text_rela->sym == file->rodata->sym)
- return find_rela_by_dest(file->rodata,
- text_rela->addend);
+ if (!text_rela || text_rela->sym != file->rodata->sym)
+ continue;
+
+ /*
+ * Make sure the .rodata address isn't associated with a
+ * symbol. gcc jump tables are anonymous data.
+ */
+ if (find_symbol_containing(file->rodata, text_rela->addend))
+ continue;
+
+ return find_rela_by_dest(file->rodata, text_rela->addend);
}
return NULL;
diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c
index 0d7983a..d897702 100644
--- a/tools/objtool/elf.c
+++ b/tools/objtool/elf.c
@@ -85,6 +85,18 @@ struct symbol *find_symbol_by_offset(struct section *sec, unsigned long offset)
return NULL;
}
+struct symbol *find_symbol_containing(struct section *sec, unsigned long offset)
+{
+ struct symbol *sym;
+
+ list_for_each_entry(sym, &sec->symbol_list, list)
+ if (sym->type != STT_SECTION &&
+ offset >= sym->offset && offset < sym->offset + sym->len)
+ return sym;
+
+ return NULL;
+}
+
struct rela *find_rela_by_dest_range(struct section *sec, unsigned long offset,
unsigned int len)
{
diff --git a/tools/objtool/elf.h b/tools/objtool/elf.h
index aa1ff65..731973e 100644
--- a/tools/objtool/elf.h
+++ b/tools/objtool/elf.h
@@ -79,6 +79,7 @@ struct elf {
struct elf *elf_open(const char *name);
struct section *find_section_by_name(struct elf *elf, const char *name);
struct symbol *find_symbol_by_offset(struct section *sec, unsigned long offset);
+struct symbol *find_symbol_containing(struct section *sec, unsigned long offset);
struct rela *find_rela_by_dest(struct section *sec, unsigned long offset);
struct rela *find_rela_by_dest_range(struct section *sec, unsigned long offset,
unsigned int len);
--
2.7.4
On Fri, Mar 3, 2017 at 12:05 AM, Josh Poimboeuf <[email protected]> wrote:
> I assume '-Wdiv-by-zero' is enabled and gcc isn't showing the "division
> by zero" warning for either of these? The 'ud2' is guaranteed to
> trigger since the function has no branches. Surely at least the missing
> warning is a gcc bug.
>
> The good news is objtool is flushing these out, albeit with a confusing
> message.
I'm still not sure if it's intentional or not. I've reduced the test case to the
simple
static inline int return0(void) { return 0; }
int provoke_div0_warning(void) { return 1 / return0(); }
which does not generate a compile-time warning, but will generate
an unconditional runtime warning if built with -fsanitze=integer-divide-by-zero.
Arnd
I opened requests on both gcc and llvm, but it looks like there is no
easy way to get a warning here:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79828
https://bugs.llvm.org/show_bug.cgi?id=32126
Arnd