Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1590139 > unrolled thread
| Started by | Arnd Bergmann <arnd@arndb.de> |
|---|---|
| First post | 2017-03-01 10:40 +0100 |
| Last post | 2017-03-01 16:30 +0100 |
| Articles | 20 on this page of 22 — 3 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.
Re: [PATCH] [RFC] x86: avoid -mtune=atom for objtool warnings Arnd Bergmann <arnd@arndb.de> - 2017-03-01 10:40 +0100
Re: [PATCH] [RFC] x86: avoid -mtune=atom for objtool warnings Arnd Bergmann <arnd@arndb.de> - 2017-03-01 11:00 +0100
Re: [PATCH] [RFC] x86: avoid -mtune=atom for objtool warnings Josh Poimboeuf <jpoimboe@redhat.com> - 2017-03-01 15:50 +0100
Re: [PATCH] [RFC] x86: avoid -mtune=atom for objtool warnings Josh Poimboeuf <jpoimboe@redhat.com> - 2017-03-01 18:30 +0100
Re: [PATCH] [RFC] x86: avoid -mtune=atom for objtool warnings Arnd Bergmann <arnd@arndb.de> - 2017-03-01 23:10 +0100
Re: [PATCH] [RFC] x86: avoid -mtune=atom for objtool warnings Arnd Bergmann <arnd@arndb.de> - 2017-03-01 23:50 +0100
Re: [PATCH] [RFC] x86: avoid -mtune=atom for objtool warnings Josh Poimboeuf <jpoimboe@redhat.com> - 2017-03-02 02:20 +0100
Re: [PATCH] [RFC] x86: avoid -mtune=atom for objtool warnings Ingo Molnar <mingo@kernel.org> - 2017-03-02 07:40 +0100
Re: [PATCH] [RFC] x86: avoid -mtune=atom for objtool warnings Ingo Molnar <mingo@kernel.org> - 2017-03-02 15:00 +0100
Re: [PATCH] [RFC] x86: avoid -mtune=atom for objtool warnings Josh Poimboeuf <jpoimboe@redhat.com> - 2017-03-02 15:30 +0100
Re: [PATCH] [RFC] x86: avoid -mtune=atom for objtool warnings Ingo Molnar <mingo@kernel.org> - 2017-03-02 15:50 +0100
Re: [PATCH] [RFC] x86: avoid -mtune=atom for objtool warnings Josh Poimboeuf <jpoimboe@redhat.com> - 2017-03-02 15:10 +0100
Re: [PATCH] [RFC] x86: avoid -mtune=atom for objtool warnings Arnd Bergmann <arnd@arndb.de> - 2017-03-03 00:20 +0100
Re: [PATCH] [RFC] x86: avoid -mtune=atom for objtool warnings Josh Poimboeuf <jpoimboe@redhat.com> - 2017-03-03 01:00 +0100
Re: [PATCH] [RFC] x86: avoid -mtune=atom for objtool warnings Arnd Bergmann <arnd@arndb.de> - 2017-03-03 10:30 +0100
Re: [PATCH] [RFC] x86: avoid -mtune=atom for objtool warnings Arnd Bergmann <arnd@arndb.de> - 2017-03-03 16:40 +0100
Re: [PATCH] [RFC] x86: avoid -mtune=atom for objtool warnings Arnd Bergmann <arnd@arndb.de> - 2017-03-01 16:30 +0100
Re: [PATCH] [RFC] x86: avoid -mtune=atom for objtool warnings Josh Poimboeuf <jpoimboe@redhat.com> - 2017-03-02 19:40 +0100
Re: [PATCH] [RFC] x86: avoid -mtune=atom for objtool warnings Arnd Bergmann <arnd@arndb.de> - 2017-03-03 00:00 +0100
Re: [PATCH] objtool: fix another gcc jump table detection issue Arnd Bergmann <arnd@arndb.de> - 2017-03-03 00:10 +0100
[PATCH] objtool: fix another gcc jump table detection issue Josh Poimboeuf <jpoimboe@redhat.com> - 2017-03-03 04:20 +0100
Re: [PATCH] [RFC] x86: avoid -mtune=atom for objtool warnings Josh Poimboeuf <jpoimboe@redhat.com> - 2017-03-01 16:30 +0100
Page 1 of 2 [1] 2 Next page →
| From | Arnd Bergmann <arnd@arndb.de> |
|---|---|
| Date | 2017-03-01 10:40 +0100 |
| Subject | Re: [PATCH] [RFC] x86: avoid -mtune=atom for objtool warnings |
| Message-ID | <tg8TM-179-29@gated-at.bofh.it> |
On Tue, Oct 11, 2016 at 10:38 PM, Arnd Bergmann <arnd@arndb.de> 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
[toc] | [next] | [standalone]
| From | Arnd Bergmann <arnd@arndb.de> |
|---|---|
| Date | 2017-03-01 11:00 +0100 |
| Message-ID | <tg9d8-1fv-21@gated-at.bofh.it> |
| In reply to | #1590139 |
On Wed, Mar 1, 2017 at 10:34 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tue, Oct 11, 2016 at 10:38 PM, Arnd Bergmann <arnd@arndb.de> 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 #
[toc] | [prev] | [next] | [standalone]
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2017-03-01 15:50 +0100 |
| Message-ID | <tgdJM-4r7-19@gated-at.bofh.it> |
| In reply to | #1590153 |
On Wed, Mar 01, 2017 at 10:45:03AM +0100, Arnd Bergmann wrote: > On Wed, Mar 1, 2017 at 10:34 AM, Arnd Bergmann <arnd@arndb.de> wrote: > > On Tue, Oct 11, 2016 at 10:38 PM, Arnd Bergmann <arnd@arndb.de> 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
[toc] | [prev] | [next] | [standalone]
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2017-03-01 18:30 +0100 |
| Message-ID | <tggeC-6gA-7@gated-at.bofh.it> |
| In reply to | #1590365 |
On Wed, Mar 01, 2017 at 04:27:29PM +0100, Arnd Bergmann wrote:
> On Wed, Mar 1, 2017 at 3:40 PM, Josh Poimboeuf <jpoimboe@redhat.com> 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
[toc] | [prev] | [next] | [standalone]
| From | Arnd Bergmann <arnd@arndb.de> |
|---|---|
| Date | 2017-03-01 23:10 +0100 |
| Message-ID | <tgkBA-12L-21@gated-at.bofh.it> |
| In reply to | #1590524 |
On Wed, Mar 1, 2017 at 5:53 PM, Josh Poimboeuf <jpoimboe@redhat.com> 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 <jpoimboe@redhat.com> 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:
[toc] | [prev] | [next] | [standalone]
| From | Arnd Bergmann <arnd@arndb.de> |
|---|---|
| Date | 2017-03-01 23:50 +0100 |
| Message-ID | <tgleh-1ix-9@gated-at.bofh.it> |
| In reply to | #1590524 |
On Wed, Mar 1, 2017 at 5:53 PM, Josh Poimboeuf <jpoimboe@redhat.com> 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
[toc] | [prev] | [next] | [standalone]
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2017-03-02 02:20 +0100 |
| Message-ID | <tgnzr-34a-7@gated-at.bofh.it> |
| In reply to | #1590671 |
On Wed, Mar 01, 2017 at 11:42:54PM +0100, Arnd Bergmann wrote: > On Wed, Mar 1, 2017 at 5:53 PM, Josh Poimboeuf <jpoimboe@redhat.com> 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
[toc] | [prev] | [next] | [standalone]
| From | Ingo Molnar <mingo@kernel.org> |
|---|---|
| Date | 2017-03-02 07:40 +0100 |
| Message-ID | <tgsz7-6IB-1@gated-at.bofh.it> |
| In reply to | #1590762 |
* Josh Poimboeuf <jpoimboe@redhat.com> 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 <jpoimboe@redhat.com> 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
[toc] | [prev] | [next] | [standalone]
| From | Ingo Molnar <mingo@kernel.org> |
|---|---|
| Date | 2017-03-02 15:00 +0100 |
| Message-ID | <tgzqV-2ZE-1@gated-at.bofh.it> |
| In reply to | #1590863 |
* Josh Poimboeuf <jpoimboe@redhat.com> wrote: > On Thu, Mar 02, 2017 at 07:31:39AM +0100, Ingo Molnar wrote: > > > > * Josh Poimboeuf <jpoimboe@redhat.com> 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 <jpoimboe@redhat.com> 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
[toc] | [prev] | [next] | [standalone]
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2017-03-02 15:30 +0100 |
| Message-ID | <tgzTY-3s2-23@gated-at.bofh.it> |
| In reply to | #1591090 |
On Thu, Mar 02, 2017 at 02:46:29PM +0100, Ingo Molnar wrote: > > * Josh Poimboeuf <jpoimboe@redhat.com> wrote: > > > On Thu, Mar 02, 2017 at 07:31:39AM +0100, Ingo Molnar wrote: > > > > > > * Josh Poimboeuf <jpoimboe@redhat.com> 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 <jpoimboe@redhat.com> 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
[toc] | [prev] | [next] | [standalone]
| From | Ingo Molnar <mingo@kernel.org> |
|---|---|
| Date | 2017-03-02 15:50 +0100 |
| Message-ID | <tgAdl-3As-27@gated-at.bofh.it> |
| In reply to | #1591130 |
* Josh Poimboeuf <jpoimboe@redhat.com> 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
[toc] | [prev] | [next] | [standalone]
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2017-03-02 15:10 +0100 |
| Message-ID | <tgzqV-2ZE-3@gated-at.bofh.it> |
| In reply to | #1590863 |
On Thu, Mar 02, 2017 at 07:31:39AM +0100, Ingo Molnar wrote: > > * Josh Poimboeuf <jpoimboe@redhat.com> 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 <jpoimboe@redhat.com> 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
[toc] | [prev] | [next] | [standalone]
| From | Arnd Bergmann <arnd@arndb.de> |
|---|---|
| Date | 2017-03-03 00:20 +0100 |
| Message-ID | <tgIaR-Qo-9@gated-at.bofh.it> |
| In reply to | #1590762 |
On Thu, Mar 2, 2017 at 2:03 AM, Josh Poimboeuf <jpoimboe@redhat.com> 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 <jpoimboe@redhat.com> 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
[toc] | [prev] | [next] | [standalone]
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2017-03-03 01:00 +0100 |
| Message-ID | <tgINA-13L-19@gated-at.bofh.it> |
| In reply to | #1591545 |
On Thu, Mar 02, 2017 at 11:49:49PM +0100, Arnd Bergmann wrote:
> On Thu, Mar 2, 2017 at 2:03 AM, Josh Poimboeuf <jpoimboe@redhat.com> 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 <jpoimboe@redhat.com> 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
[toc] | [prev] | [next] | [standalone]
| From | Arnd Bergmann <arnd@arndb.de> |
|---|---|
| Date | 2017-03-03 10:30 +0100 |
| Message-ID | <tgRHb-7yR-5@gated-at.bofh.it> |
| In reply to | #1591568 |
On Fri, Mar 3, 2017 at 12:05 AM, Josh Poimboeuf <jpoimboe@redhat.com> 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
[toc] | [prev] | [next] | [standalone]
| From | Arnd Bergmann <arnd@arndb.de> |
|---|---|
| Date | 2017-03-03 16:40 +0100 |
| Message-ID | <tgXtg-3ca-17@gated-at.bofh.it> |
| In reply to | #1591798 |
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
[toc] | [prev] | [next] | [standalone]
| From | Arnd Bergmann <arnd@arndb.de> |
|---|---|
| Date | 2017-03-01 16:30 +0100 |
| Message-ID | <tgemt-4Vt-3@gated-at.bofh.it> |
| In reply to | #1590139 |
[Multipart message — attachments visible in raw view] — view raw
On Wed, Mar 1, 2017 at 3:31 PM, Josh Poimboeuf <jpoimboe@redhat.com> 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 <arnd@arndb.de> 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
[toc] | [prev] | [next] | [standalone]
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2017-03-02 19:40 +0100 |
| Message-ID | <tgDNT-6hJ-7@gated-at.bofh.it> |
| In reply to | #1590405 |
On Wed, Mar 01, 2017 at 04:21:36PM +0100, Arnd Bergmann wrote:
> On Wed, Mar 1, 2017 at 3:31 PM, Josh Poimboeuf <jpoimboe@redhat.com> 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 <arnd@arndb.de> 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);
[toc] | [prev] | [next] | [standalone]
| From | Arnd Bergmann <arnd@arndb.de> |
|---|---|
| Date | 2017-03-03 00:00 +0100 |
| Message-ID | <tgHRw-ro-17@gated-at.bofh.it> |
| In reply to | #1591389 |
On Thu, Mar 2, 2017 at 7:25 PM, Josh Poimboeuf <jpoimboe@redhat.com> 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
[toc] | [prev] | [next] | [standalone]
| From | Arnd Bergmann <arnd@arndb.de> |
|---|---|
| Date | 2017-03-03 00:10 +0100 |
| Subject | Re: [PATCH] objtool: fix another gcc jump table detection issue |
| Message-ID | <tgI1c-KT-21@gated-at.bofh.it> |
| In reply to | #1591531 |
On Thu, Mar 2, 2017 at 11:57 PM, Josh Poimboeuf <jpoimboe@redhat.com> 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 <arnd@arndb.de>
> Fixes: 3732710ff6f2 ("objtool: Improve rare switch jump table pattern detection")
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Tested-by: Arnd Bergmann <arnd@arndb.de>
[toc] | [prev] | [next] | [standalone]
Page 1 of 2 [1] 2 Next page →
Back to top | Article view | linux.kernel
csiph-web