On 4/14/2025 3:59 PM, ltaylorsimp...@gmail.com wrote:
-----Original Message-----
From: Matheus Tavares Bernardino
<matheus.bernard...@oss.qualcomm.com>
Sent: Monday, April 14, 2025 12:10 PM
To: ltaylorsimp...@gmail.com
Cc: brian.c...@oss.qualcomm.com; qemu-devel@nongnu.org;
richard.hender...@linaro.org; phi...@linaro.org;
matheus.bernard...@oss.qualcomm.com; a...@rev.ng; a...@rev.ng;
marco.lie...@oss.qualcomm.com; alex.ben...@linaro.org;
quic_mbur...@quicinc.com; sidn...@quicinc.com
Subject: RE: [PATCH v3 5/5] target/hexagon: Remove unreachable
On Mon, 14 Apr 2025 11:19:38 -0600 <ltaylorsimp...@gmail.com> wrote:
-----Original Message-----
From: Brian Cain <brian.c...@oss.qualcomm.com>
Sent: Monday, April 7, 2025 1:27 PM
To: qemu-devel@nongnu.org
Cc: brian.c...@oss.qualcomm.com; richard.hender...@linaro.org;
phi...@linaro.org; matheus.bernard...@oss.qualcomm.com;
a...@rev.ng;
a...@rev.ng; marco.lie...@oss.qualcomm.com;
ltaylorsimp...@gmail.com; alex.ben...@linaro.org;
quic_mbur...@quicinc.com; sidn...@quicinc.com
Subject: [PATCH v3 5/5] target/hexagon: Remove unreachable
We should raise an exception in the event that we encounter a packet
that can't be correctly decoded, not fault.
Signed-off-by: Brian Cain <brian.c...@oss.qualcomm.com>
---
target/hexagon/decode.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/target/hexagon/decode.c b/target/hexagon/decode.c index
b5ece60450..1db7f1950f 100644
--- a/target/hexagon/decode.c
+++ b/target/hexagon/decode.c
@@ -489,7 +489,6 @@ decode_insns(DisasContext *ctx, Insn *insn,
uint32_t
encoding)
insn->iclass = iclass_bits(encoding);
return 1;
}
- g_assert_not_reached();
} else {
uint32_t iclass = get_duplex_iclass(encoding);
unsigned int slot0_subinsn = get_slot0_subinsn(encoding);
@@ -512,6
+511,11 @@ decode_insns(DisasContext *ctx, Insn *insn, uint32_t
+encoding)
}
g_assert_not_reached();
Why leave this one rather than raising an exception?
Good point. I think this one should be removed as well. We have removed it
downstream already.
Taylor, is it satisfactory to include that update in a subsequent patch
series? Or should this one replace the second unreachable too?
}
+ /*
+ * invalid/unrecognized opcode; return 1 and let gen_insn() raise
an
+ * exception when it sees this empty insn.
+ */
+ return 1;
You should set insn->generate to NULL if you want to guarantee that
gen_insn will raise an exception.
The caller already memset's it to 0 before passing `insn` down.
Do you have a test case for this?
We do have a softmmu test for this downstream. Maybe we can adjust it for
user-mode and upstream it with this patch.
Take a look at tests/tcg/hexagon/invalid-slots.c to see how to do this in
linux-user mode. You'll also need to modify Makefile.target in that
directory.
Matheus provided a linux-user test offline. I'll include it in an
updated patch.
HTH,
Taylor
