> -----Original Message-----
> From: Brian Cain <brian.c...@oss.qualcomm.com>
> Sent: Wednesday, April 16, 2025 4:22 PM
> To: ltaylorsimp...@gmail.com; 'Matheus Tavares Bernardino'
> <matheus.bernard...@oss.qualcomm.com>
> Cc: qemu-devel@nongnu.org; richard.hender...@linaro.org;
> phi...@linaro.org; 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 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?
If you just remove that line, it will fall through to the "return 1" below.
> >>
> >>>> }
> >>>> + /*
> >>>> + * 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
> >
> >
