* [PATCH] arm64: insn: Add return statements after BUG_ON()
@ 2014-09-16 16:42 Mark Brown
2014-09-16 16:49 ` Will Deacon
2014-09-18 17:02 ` Catalin Marinas
0 siblings, 2 replies; 5+ messages in thread
From: Mark Brown @ 2014-09-16 16:42 UTC (permalink / raw)
To: linux-arm-kernel
Following a recent series of enhancements to the insn code the ARMv8
allnoconfig build has been generating a large number of warnings in the
form of:
arch/arm64/kernel/insn.c:689:8: warning: 'insn' may be used uninitialized in this function [-Wmaybe-uninitialized]
This is because BUG() and related macros can be compiled out so we get
execution paths which normally result in a panic compiling out to noops
instead.
I wasn't able to immediately identify a sensible return value to use in
these cases so just return 0 - this is all "should never happen" code so
hopefully it never has a practical impact.
Signed-off-by: Mark Brown <broonie@kernel.org>
---
arch/arm64/kernel/insn.c | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)
diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c
index 0668ee5..7af35f3 100644
--- a/arch/arm64/kernel/insn.c
+++ b/arch/arm64/kernel/insn.c
@@ -388,6 +388,7 @@ u32 __kprobes aarch64_insn_gen_branch_imm(unsigned long pc, unsigned long addr,
break;
default:
BUG_ON(1);
+ return 0;
}
return aarch64_insn_encode_immediate(AARCH64_INSN_IMM_26, insn,
@@ -413,6 +414,7 @@ u32 aarch64_insn_gen_comp_branch_imm(unsigned long pc, unsigned long addr,
break;
default:
BUG_ON(1);
+ return 0;
}
switch (variant) {
@@ -423,6 +425,7 @@ u32 aarch64_insn_gen_comp_branch_imm(unsigned long pc, unsigned long addr,
break;
default:
BUG_ON(1);
+ return 0;
}
insn = aarch64_insn_encode_register(AARCH64_INSN_REGTYPE_RT, insn, reg);
@@ -475,6 +478,7 @@ u32 aarch64_insn_gen_branch_reg(enum aarch64_insn_register reg,
break;
default:
BUG_ON(1);
+ return 0;
}
return aarch64_insn_encode_register(AARCH64_INSN_REGTYPE_RN, insn, reg);
@@ -497,6 +501,7 @@ u32 aarch64_insn_gen_load_store_reg(enum aarch64_insn_register reg,
break;
default:
BUG_ON(1);
+ return 0;
}
insn = aarch64_insn_encode_ldst_size(size, insn);
@@ -535,6 +540,7 @@ u32 aarch64_insn_gen_load_store_pair(enum aarch64_insn_register reg1,
break;
default:
BUG_ON(1);
+ return 0;
}
switch (variant) {
@@ -553,6 +559,7 @@ u32 aarch64_insn_gen_load_store_pair(enum aarch64_insn_register reg1,
break;
default:
BUG_ON(1);
+ return 0;
}
insn = aarch64_insn_encode_register(AARCH64_INSN_REGTYPE_RT, insn,
@@ -590,6 +597,7 @@ u32 aarch64_insn_gen_add_sub_imm(enum aarch64_insn_register dst,
break;
default:
BUG_ON(1);
+ return 0;
}
switch (variant) {
@@ -600,6 +608,7 @@ u32 aarch64_insn_gen_add_sub_imm(enum aarch64_insn_register dst,
break;
default:
BUG_ON(1);
+ return 0;
}
BUG_ON(imm & ~(SZ_4K - 1));
@@ -632,6 +641,7 @@ u32 aarch64_insn_gen_bitfield(enum aarch64_insn_register dst,
break;
default:
BUG_ON(1);
+ return 0;
}
switch (variant) {
@@ -644,6 +654,7 @@ u32 aarch64_insn_gen_bitfield(enum aarch64_insn_register dst,
break;
default:
BUG_ON(1);
+ return 0;
}
BUG_ON(immr & ~mask);
@@ -677,6 +688,7 @@ u32 aarch64_insn_gen_movewide(enum aarch64_insn_register dst,
break;
default:
BUG_ON(1);
+ return 0;
}
BUG_ON(imm & ~(SZ_64K - 1));
@@ -692,6 +704,7 @@ u32 aarch64_insn_gen_movewide(enum aarch64_insn_register dst,
break;
default:
BUG_ON(1);
+ return 0;
}
insn |= (shift >> 4) << 21;
@@ -725,6 +738,7 @@ u32 aarch64_insn_gen_add_sub_shifted_reg(enum aarch64_insn_register dst,
break;
default:
BUG_ON(1);
+ return 0;
}
switch (variant) {
@@ -737,6 +751,7 @@ u32 aarch64_insn_gen_add_sub_shifted_reg(enum aarch64_insn_register dst,
break;
default:
BUG_ON(1);
+ return 0;
}
@@ -769,6 +784,7 @@ u32 aarch64_insn_gen_data1(enum aarch64_insn_register dst,
break;
default:
BUG_ON(1);
+ return 0;
}
switch (variant) {
@@ -779,6 +795,7 @@ u32 aarch64_insn_gen_data1(enum aarch64_insn_register dst,
break;
default:
BUG_ON(1);
+ return 0;
}
insn = aarch64_insn_encode_register(AARCH64_INSN_REGTYPE_RD, insn, dst);
@@ -815,6 +832,7 @@ u32 aarch64_insn_gen_data2(enum aarch64_insn_register dst,
break;
default:
BUG_ON(1);
+ return 0;
}
switch (variant) {
@@ -825,6 +843,7 @@ u32 aarch64_insn_gen_data2(enum aarch64_insn_register dst,
break;
default:
BUG_ON(1);
+ return 0;
}
insn = aarch64_insn_encode_register(AARCH64_INSN_REGTYPE_RD, insn, dst);
@@ -852,6 +871,7 @@ u32 aarch64_insn_gen_data3(enum aarch64_insn_register dst,
break;
default:
BUG_ON(1);
+ return 0;
}
switch (variant) {
@@ -862,6 +882,7 @@ u32 aarch64_insn_gen_data3(enum aarch64_insn_register dst,
break;
default:
BUG_ON(1);
+ return 0;
}
insn = aarch64_insn_encode_register(AARCH64_INSN_REGTYPE_RD, insn, dst);
@@ -911,6 +932,7 @@ u32 aarch64_insn_gen_logical_shifted_reg(enum aarch64_insn_register dst,
break;
default:
BUG_ON(1);
+ return 0;
}
switch (variant) {
@@ -923,6 +945,7 @@ u32 aarch64_insn_gen_logical_shifted_reg(enum aarch64_insn_register dst,
break;
default:
BUG_ON(1);
+ return 0;
}
--
2.1.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH] arm64: insn: Add return statements after BUG_ON()
2014-09-16 16:42 [PATCH] arm64: insn: Add return statements after BUG_ON() Mark Brown
@ 2014-09-16 16:49 ` Will Deacon
2014-09-19 10:58 ` Catalin Marinas
2014-09-18 17:02 ` Catalin Marinas
1 sibling, 1 reply; 5+ messages in thread
From: Will Deacon @ 2014-09-16 16:49 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Sep 16, 2014 at 05:42:33PM +0100, Mark Brown wrote:
> Following a recent series of enhancements to the insn code the ARMv8
> allnoconfig build has been generating a large number of warnings in the
> form of:
>
> arch/arm64/kernel/insn.c:689:8: warning: 'insn' may be used uninitialized in this function [-Wmaybe-uninitialized]
>
> This is because BUG() and related macros can be compiled out so we get
> execution paths which normally result in a panic compiling out to noops
> instead.
>
> I wasn't able to immediately identify a sensible return value to use in
> these cases so just return 0 - this is all "should never happen" code so
> hopefully it never has a practical impact.
Hmm, I had a similar complaint when we merged the code. I'd much rather see
those BUG statements removed entirely, and have an error return code back to
the jit. However, the counter argument was that the jitted code has already
been verified at this point, so any errors really are fatal.
So, I think your patch is probably the best thing we can do without
reopening that discussion.
Will
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
> arch/arm64/kernel/insn.c | 23 +++++++++++++++++++++++
> 1 file changed, 23 insertions(+)
>
> diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c
> index 0668ee5..7af35f3 100644
> --- a/arch/arm64/kernel/insn.c
> +++ b/arch/arm64/kernel/insn.c
> @@ -388,6 +388,7 @@ u32 __kprobes aarch64_insn_gen_branch_imm(unsigned long pc, unsigned long addr,
> break;
> default:
> BUG_ON(1);
> + return 0;
> }
>
> return aarch64_insn_encode_immediate(AARCH64_INSN_IMM_26, insn,
> @@ -413,6 +414,7 @@ u32 aarch64_insn_gen_comp_branch_imm(unsigned long pc, unsigned long addr,
> break;
> default:
> BUG_ON(1);
> + return 0;
> }
>
> switch (variant) {
> @@ -423,6 +425,7 @@ u32 aarch64_insn_gen_comp_branch_imm(unsigned long pc, unsigned long addr,
> break;
> default:
> BUG_ON(1);
> + return 0;
> }
>
> insn = aarch64_insn_encode_register(AARCH64_INSN_REGTYPE_RT, insn, reg);
> @@ -475,6 +478,7 @@ u32 aarch64_insn_gen_branch_reg(enum aarch64_insn_register reg,
> break;
> default:
> BUG_ON(1);
> + return 0;
> }
>
> return aarch64_insn_encode_register(AARCH64_INSN_REGTYPE_RN, insn, reg);
> @@ -497,6 +501,7 @@ u32 aarch64_insn_gen_load_store_reg(enum aarch64_insn_register reg,
> break;
> default:
> BUG_ON(1);
> + return 0;
> }
>
> insn = aarch64_insn_encode_ldst_size(size, insn);
> @@ -535,6 +540,7 @@ u32 aarch64_insn_gen_load_store_pair(enum aarch64_insn_register reg1,
> break;
> default:
> BUG_ON(1);
> + return 0;
> }
>
> switch (variant) {
> @@ -553,6 +559,7 @@ u32 aarch64_insn_gen_load_store_pair(enum aarch64_insn_register reg1,
> break;
> default:
> BUG_ON(1);
> + return 0;
> }
>
> insn = aarch64_insn_encode_register(AARCH64_INSN_REGTYPE_RT, insn,
> @@ -590,6 +597,7 @@ u32 aarch64_insn_gen_add_sub_imm(enum aarch64_insn_register dst,
> break;
> default:
> BUG_ON(1);
> + return 0;
> }
>
> switch (variant) {
> @@ -600,6 +608,7 @@ u32 aarch64_insn_gen_add_sub_imm(enum aarch64_insn_register dst,
> break;
> default:
> BUG_ON(1);
> + return 0;
> }
>
> BUG_ON(imm & ~(SZ_4K - 1));
> @@ -632,6 +641,7 @@ u32 aarch64_insn_gen_bitfield(enum aarch64_insn_register dst,
> break;
> default:
> BUG_ON(1);
> + return 0;
> }
>
> switch (variant) {
> @@ -644,6 +654,7 @@ u32 aarch64_insn_gen_bitfield(enum aarch64_insn_register dst,
> break;
> default:
> BUG_ON(1);
> + return 0;
> }
>
> BUG_ON(immr & ~mask);
> @@ -677,6 +688,7 @@ u32 aarch64_insn_gen_movewide(enum aarch64_insn_register dst,
> break;
> default:
> BUG_ON(1);
> + return 0;
> }
>
> BUG_ON(imm & ~(SZ_64K - 1));
> @@ -692,6 +704,7 @@ u32 aarch64_insn_gen_movewide(enum aarch64_insn_register dst,
> break;
> default:
> BUG_ON(1);
> + return 0;
> }
>
> insn |= (shift >> 4) << 21;
> @@ -725,6 +738,7 @@ u32 aarch64_insn_gen_add_sub_shifted_reg(enum aarch64_insn_register dst,
> break;
> default:
> BUG_ON(1);
> + return 0;
> }
>
> switch (variant) {
> @@ -737,6 +751,7 @@ u32 aarch64_insn_gen_add_sub_shifted_reg(enum aarch64_insn_register dst,
> break;
> default:
> BUG_ON(1);
> + return 0;
> }
>
>
> @@ -769,6 +784,7 @@ u32 aarch64_insn_gen_data1(enum aarch64_insn_register dst,
> break;
> default:
> BUG_ON(1);
> + return 0;
> }
>
> switch (variant) {
> @@ -779,6 +795,7 @@ u32 aarch64_insn_gen_data1(enum aarch64_insn_register dst,
> break;
> default:
> BUG_ON(1);
> + return 0;
> }
>
> insn = aarch64_insn_encode_register(AARCH64_INSN_REGTYPE_RD, insn, dst);
> @@ -815,6 +832,7 @@ u32 aarch64_insn_gen_data2(enum aarch64_insn_register dst,
> break;
> default:
> BUG_ON(1);
> + return 0;
> }
>
> switch (variant) {
> @@ -825,6 +843,7 @@ u32 aarch64_insn_gen_data2(enum aarch64_insn_register dst,
> break;
> default:
> BUG_ON(1);
> + return 0;
> }
>
> insn = aarch64_insn_encode_register(AARCH64_INSN_REGTYPE_RD, insn, dst);
> @@ -852,6 +871,7 @@ u32 aarch64_insn_gen_data3(enum aarch64_insn_register dst,
> break;
> default:
> BUG_ON(1);
> + return 0;
> }
>
> switch (variant) {
> @@ -862,6 +882,7 @@ u32 aarch64_insn_gen_data3(enum aarch64_insn_register dst,
> break;
> default:
> BUG_ON(1);
> + return 0;
> }
>
> insn = aarch64_insn_encode_register(AARCH64_INSN_REGTYPE_RD, insn, dst);
> @@ -911,6 +932,7 @@ u32 aarch64_insn_gen_logical_shifted_reg(enum aarch64_insn_register dst,
> break;
> default:
> BUG_ON(1);
> + return 0;
> }
>
> switch (variant) {
> @@ -923,6 +945,7 @@ u32 aarch64_insn_gen_logical_shifted_reg(enum aarch64_insn_register dst,
> break;
> default:
> BUG_ON(1);
> + return 0;
> }
>
>
> --
> 2.1.0
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] arm64: insn: Add return statements after BUG_ON()
2014-09-16 16:42 [PATCH] arm64: insn: Add return statements after BUG_ON() Mark Brown
2014-09-16 16:49 ` Will Deacon
@ 2014-09-18 17:02 ` Catalin Marinas
2014-09-18 17:21 ` Mark Brown
1 sibling, 1 reply; 5+ messages in thread
From: Catalin Marinas @ 2014-09-18 17:02 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Sep 16, 2014 at 05:42:33PM +0100, Mark Brown wrote:
> Following a recent series of enhancements to the insn code the ARMv8
> allnoconfig build has been generating a large number of warnings in the
> form of:
>
> arch/arm64/kernel/insn.c:689:8: warning: 'insn' may be used uninitialized in this function [-Wmaybe-uninitialized]
>
> This is because BUG() and related macros can be compiled out so we get
> execution paths which normally result in a panic compiling out to noops
> instead.
>
> I wasn't able to immediately identify a sensible return value to use in
> these cases so just return 0 - this is all "should never happen" code so
> hopefully it never has a practical impact.
>
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
> arch/arm64/kernel/insn.c | 23 +++++++++++++++++++++++
> 1 file changed, 23 insertions(+)
>
> diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c
> index 0668ee5..7af35f3 100644
> --- a/arch/arm64/kernel/insn.c
> +++ b/arch/arm64/kernel/insn.c
> @@ -388,6 +388,7 @@ u32 __kprobes aarch64_insn_gen_branch_imm(unsigned long pc, unsigned long addr,
> break;
> default:
> BUG_ON(1);
> + return 0;
> }
Wouldn't there be less lines if we initialise insn to 0 in these
functions?
--
Catalin
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] arm64: insn: Add return statements after BUG_ON()
2014-09-18 17:02 ` Catalin Marinas
@ 2014-09-18 17:21 ` Mark Brown
0 siblings, 0 replies; 5+ messages in thread
From: Mark Brown @ 2014-09-18 17:21 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Sep 18, 2014 at 06:02:06PM +0100, Catalin Marinas wrote:
> On Tue, Sep 16, 2014 at 05:42:33PM +0100, Mark Brown wrote:
> > default:
> > BUG_ON(1);
> > + return 0;
> > }
> Wouldn't there be less lines if we initialise insn to 0 in these
> functions?
In general that's bad practice since it means that you loose the benefit
of flow analysis on that variable throughout the function - it will shut
the warning up but if someone introduces a bug later on the compiler
won't be able to tell them about it.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140918/b75f0cc9/attachment.sig>
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] arm64: insn: Add return statements after BUG_ON()
2014-09-16 16:49 ` Will Deacon
@ 2014-09-19 10:58 ` Catalin Marinas
0 siblings, 0 replies; 5+ messages in thread
From: Catalin Marinas @ 2014-09-19 10:58 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Sep 16, 2014 at 05:49:53PM +0100, Will Deacon wrote:
> On Tue, Sep 16, 2014 at 05:42:33PM +0100, Mark Brown wrote:
> > Following a recent series of enhancements to the insn code the ARMv8
> > allnoconfig build has been generating a large number of warnings in the
> > form of:
> >
> > arch/arm64/kernel/insn.c:689:8: warning: 'insn' may be used uninitialized in this function [-Wmaybe-uninitialized]
> >
> > This is because BUG() and related macros can be compiled out so we get
> > execution paths which normally result in a panic compiling out to noops
> > instead.
> >
> > I wasn't able to immediately identify a sensible return value to use in
> > these cases so just return 0 - this is all "should never happen" code so
> > hopefully it never has a practical impact.
>
> Hmm, I had a similar complaint when we merged the code. I'd much rather see
> those BUG statements removed entirely, and have an error return code back to
> the jit. However, the counter argument was that the jitted code has already
> been verified at this point, so any errors really are fatal.
It's BPF, ftrace and jump label all using the insn.c code. What's funny,
ftrace.c passes the type as true/false rather than an enum (not sure
how/when we missed this).
> So, I think your patch is probably the best thing we can do without
> reopening that discussion.
We can merge this patch for now but I would rather return an error. It
may be better if we actually return a fault generating instruction (BRK)
rather than 0 like the AARCH64_BREAK_FAULT defined here:
http://lkml.kernel.org/g/1410853730-16470-1-git-send-email-dborkman at redhat.com
--
Catalin
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-09-19 10:58 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-09-16 16:42 [PATCH] arm64: insn: Add return statements after BUG_ON() Mark Brown
2014-09-16 16:49 ` Will Deacon
2014-09-19 10:58 ` Catalin Marinas
2014-09-18 17:02 ` Catalin Marinas
2014-09-18 17:21 ` Mark Brown
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).