* [PATCH v3 5/5] target/hexagon: Remove unreachable
2025-04-07 19:27 [PATCH v3 0/5] misc hexagon patches Brian Cain
@ 2025-04-07 19:27 ` Brian Cain
2025-04-14 17:19 ` ltaylorsimpson
0 siblings, 1 reply; 6+ messages in thread
From: Brian Cain @ 2025-04-07 19:27 UTC (permalink / raw)
To: qemu-devel
Cc: brian.cain, richard.henderson, philmd, matheus.bernardino, ale,
anjo, marco.liebel, ltaylorsimpson, alex.bennee, quic_mburton,
sidneym
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();
}
+ /*
+ * invalid/unrecognized opcode; return 1 and let gen_insn() raise an
+ * exception when it sees this empty insn.
+ */
+ return 1;
}
static void decode_add_endloop_insn(Insn *insn, int loopnum)
--
2.34.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* RE: [PATCH v3 5/5] target/hexagon: Remove unreachable
2025-04-07 19:27 ` [PATCH v3 5/5] target/hexagon: Remove unreachable Brian Cain
@ 2025-04-14 17:19 ` ltaylorsimpson
0 siblings, 0 replies; 6+ messages in thread
From: ltaylorsimpson @ 2025-04-14 17:19 UTC (permalink / raw)
To: 'Brian Cain', qemu-devel
Cc: richard.henderson, philmd, matheus.bernardino, ale, anjo,
marco.liebel, alex.bennee, quic_mburton, sidneym
> -----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?
> }
> + /*
> + * 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. A better option is to return a special value that indicates "invalid" and have decode_packet return 0 which will cause decode_and_translate_packet to generate the exception before generating the code for any other instructions in the packet.
Do you have a test case for this?
Taylor
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH v3 5/5] target/hexagon: Remove unreachable
[not found] <058801dbad61_68ff5b00_3afe1100_@gmail.com>
@ 2025-04-14 18:09 ` Matheus Tavares Bernardino
2025-04-14 20:59 ` ltaylorsimpson
0 siblings, 1 reply; 6+ messages in thread
From: Matheus Tavares Bernardino @ 2025-04-14 18:09 UTC (permalink / raw)
To: ltaylorsimpson
Cc: brian.cain, qemu-devel, richard.henderson, philmd,
matheus.bernardino, ale, anjo, marco.liebel, alex.bennee,
quic_mburton, sidneym
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.
> > }
> > + /*
> > + * 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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH v3 5/5] target/hexagon: Remove unreachable
2025-04-14 18:09 ` [PATCH v3 5/5] target/hexagon: Remove unreachable Matheus Tavares Bernardino
@ 2025-04-14 20:59 ` ltaylorsimpson
2025-04-16 22:21 ` Brian Cain
0 siblings, 1 reply; 6+ messages in thread
From: ltaylorsimpson @ 2025-04-14 20:59 UTC (permalink / raw)
To: 'Matheus Tavares Bernardino'
Cc: brian.cain, qemu-devel, richard.henderson, philmd, ale, anjo,
marco.liebel, alex.bennee, quic_mburton, sidneym
> -----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.
>
> > > }
> > > + /*
> > > + * 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.
HTH,
Taylor
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 5/5] target/hexagon: Remove unreachable
2025-04-14 20:59 ` ltaylorsimpson
@ 2025-04-16 22:21 ` Brian Cain
2025-04-16 23:27 ` ltaylorsimpson
0 siblings, 1 reply; 6+ messages in thread
From: Brian Cain @ 2025-04-16 22:21 UTC (permalink / raw)
To: ltaylorsimpson, 'Matheus Tavares Bernardino'
Cc: qemu-devel, richard.henderson, philmd, ale, anjo, marco.liebel,
alex.bennee, quic_mburton, sidneym
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?
>>
>>>> }
>>>> + /*
>>>> + * 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
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH v3 5/5] target/hexagon: Remove unreachable
2025-04-16 22:21 ` Brian Cain
@ 2025-04-16 23:27 ` ltaylorsimpson
0 siblings, 0 replies; 6+ messages in thread
From: ltaylorsimpson @ 2025-04-16 23:27 UTC (permalink / raw)
To: 'Brian Cain', 'Matheus Tavares Bernardino'
Cc: qemu-devel, richard.henderson, philmd, ale, anjo, marco.liebel,
alex.bennee, quic_mburton, sidneym
> -----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
> >
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-04-16 23:29 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <058801dbad61_68ff5b00_3afe1100_@gmail.com>
2025-04-14 18:09 ` [PATCH v3 5/5] target/hexagon: Remove unreachable Matheus Tavares Bernardino
2025-04-14 20:59 ` ltaylorsimpson
2025-04-16 22:21 ` Brian Cain
2025-04-16 23:27 ` ltaylorsimpson
2025-04-07 19:27 [PATCH v3 0/5] misc hexagon patches Brian Cain
2025-04-07 19:27 ` [PATCH v3 5/5] target/hexagon: Remove unreachable Brian Cain
2025-04-14 17:19 ` ltaylorsimpson
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).