Richard Henderson writes:
> On 07/14/2017 07:26 AM, Richard Henderson wrote:
>> On 07/13/2017 11:26 PM, Lluís Vilanova wrote:
>>> Incrementally paves the way towards using the generic instruction
>>> translation
>>> loop.
>>>
>>> Signed-off-by: Lluís Vilanova <vilan...@ac.upc.edu>
>>> Reviewed-by: Richard Henderson <r...@twiddle.net>
>>> ---
>>> target/arm/translate.c | 53
>>> +++++++++++++++++++++++++++++++-----------------
>>> 1 file changed, 34 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/target/arm/translate.c b/target/arm/translate.c
>>> index b9183fc511..55bef09739 100644
>>> --- a/target/arm/translate.c
>>> +++ b/target/arm/translate.c
>>> @@ -11917,6 +11917,33 @@ static void arm_tr_insn_start(DisasContextBase
>>> *dcbase, CPUState *cpu)
>>> #endif
>>> }
>>> +static bool arm_tr_breakpoint_check(DisasContextBase *dcbase, CPUState
>>> *cpu,
>>> + const CPUBreakpoint *bp)
>>> +{
>>> + DisasContext *dc = container_of(dcbase, DisasContext, base);
>>> +
>>> + if (bp->flags & BP_CPU) {
>>> + gen_set_condexec(dc);
>>> + gen_set_pc_im(dc, dc->pc);
>>> + gen_helper_check_breakpoints(cpu_env);
>>> + /* End the TB early; it's likely not going to be executed */
>>> + dc->base.is_jmp = DISAS_UPDATE;
>>> + } else {
>>> + gen_exception_internal_insn(dc, 0, EXCP_DEBUG);
>>> + /* The address covered by the breakpoint must be
>>> + included in [tb->pc, tb->pc + tb->size) in order
>>> + to for it to be properly cleared -- thus we
>>> + increment the PC here so that the logic setting
>>> + tb->size below does the right thing. */
>>> + /* TODO: Advance PC by correct instruction length to
>>> + * avoid disassembler error messages */
>>> + dc->pc += 2;
>>> + dc->base.is_jmp = DISAS_NORETURN;
>>> + }
>>> +
>>> + return true;
>>> +}
>>> +
>>> /* generate intermediate code for basic block 'tb'. */
>>> void gen_intermediate_code(CPUState *cs, TranslationBlock *tb)
>>> {
>>> @@ -11965,28 +11992,16 @@ void gen_intermediate_code(CPUState *cs,
>>> TranslationBlock *tb)
>>> if (unlikely(!QTAILQ_EMPTY(&cs->breakpoints))) {
>>> CPUBreakpoint *bp;
>>> QTAILQ_FOREACH(bp, &cs->breakpoints, entry) {
>>> - if (bp->pc == dc->pc) {
>>> - if (bp->flags & BP_CPU) {
>>> - gen_set_condexec(dc);
>>> - gen_set_pc_im(dc, dc->pc);
>>> - gen_helper_check_breakpoints(cpu_env);
>>> - /* End the TB early; it's likely not going to be
>>> executed */
>>> - dc->base.is_jmp = DISAS_UPDATE;
>>
>> Oh I see what you're doing there in the main loop.
>> And I see that you're copying existing behaviour.
>>
>> That said, I do wonder if there's a better way.
>>
>> Looking back at the original patch (5d98bf8f), there do not
>> seem to have been any other side effects intended; simply
>> "single step" any insn for which this bp condition is met.
>>
>> Another way to handle this would be if we could adjust max_insns = num_insns.
>> That would cause the loop to exit after the current insn, with DISAS_TOO_MANY
>> if nothing else.
> Another possibility is is_jmp = DISAS_TOO_MANY, and exit the translation loop
> after the breakpoint check only for is_jmp > DISAS_TOO_MANY. That allows all
> of
> the DISAS_TARGET_N values to exit as well.
After a quick check, I see that arm uses both (DISAS_NORETURN and
DISAS_TARGET_N) to exit in different points after the breakpoint. Moxie, mips
and unicore32 use use DISAS_NORETURN, and the rest use DISAS_TARGET_N.
I really don't know if it's safe to unify into a single behaviour. I'm not sure
if some targets will need to differentiate between DISAS_NORETURN and
DISAS_TOO_MANY (e.g., in the tb_stop() hook). As I said, I'd prefer to keep the
current approach that can accommodate all cases, and trim it down afterwards if
we see it's possible. That is, unless you're sure a new proposal can correctly
cover the cases of all targets.
Thanks,
Lluis
