RE: [PATCH v3 5/5] target/hexagon: Remove unreachable

[<ltaylorsimpson@gmail.com>]

> -----Original Message-----
> From: Brian Cain <brian.cain@oss.qualcomm.com>
> Sent: Wednesday, April 16, 2025 4:22 PM
> To: ltaylorsimpson@gmail.com; 'Matheus Tavares Bernardino'
> <matheus.bernardino@oss.qualcomm.com>
> Cc: qemu-devel@nongnu.org; richard.henderson@linaro.org;
> philmd@linaro.org; ale@rev.ng; anjo@rev.ng;
> marco.liebel@oss.qualcomm.com; alex.bennee@linaro.org;
> quic_mburton@quicinc.com; sidneym@quicinc.com
> Subject: Re: [PATCH v3 5/5] target/hexagon: Remove unreachable
> 
> 
> On 4/14/2025 3:59 PM, ltaylorsimpson@gmail.com wrote:
> >
> >> -----Original Message-----
> >> From: Matheus Tavares Bernardino
> >> <matheus.bernardino@oss.qualcomm.com>
> >> Sent: Monday, April 14, 2025 12:10 PM
> >> To: ltaylorsimpson@gmail.com
> >> Cc: brian.cain@oss.qualcomm.com; qemu-devel@nongnu.org;
> >> richard.henderson@linaro.org; philmd@linaro.org;
> >> matheus.bernardino@oss.qualcomm.com; ale@rev.ng; anjo@rev.ng;
> >> marco.liebel@oss.qualcomm.com; alex.bennee@linaro.org;
> >> quic_mburton@quicinc.com; sidneym@quicinc.com
> >> Subject: RE: [PATCH v3 5/5] target/hexagon: Remove unreachable
> >>
> >> On Mon, 14 Apr 2025 11:19:38 -0600 <ltaylorsimpson@gmail.com> wrote:
> >>>> -----Original Message-----
> >>>> From: Brian Cain <brian.cain@oss.qualcomm.com>
> >>>> Sent: Monday, April 7, 2025 1:27 PM
> >>>> To: qemu-devel@nongnu.org
> >>>> Cc: brian.cain@oss.qualcomm.com; richard.henderson@linaro.org;
> >>>> philmd@linaro.org; matheus.bernardino@oss.qualcomm.com;
> >> ale@rev.ng;
> >>>> anjo@rev.ng; marco.liebel@oss.qualcomm.com;
> >>>> ltaylorsimpson@gmail.com; alex.bennee@linaro.org;
> >>>> quic_mburton@quicinc.com; sidneym@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.cain@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
> >
> >