Richard Henderson writes:
> On 07/07/2017 01:56 AM, Lluís Vilanova wrote:
[...]
>> +
>> + /* Instruction counting */
>> + max_insns = db->tb->cflags & CF_COUNT_MASK;
>> + if (max_insns == 0) {
>> + max_insns = CF_COUNT_MASK;
>> + }
>> + if (max_insns > TCG_MAX_INSNS) {
>> + max_insns = TCG_MAX_INSNS;
>> + }
>> + if (db->singlestep_enabled || singlestep) {
>> + max_insns = 1;
>> + }
>> +
>> + /* Start translating */
>> + gen_tb_start(db->tb);
>> + ops->tb_start(db, cpu);
> As I mentioned, we need some way to modify max_insns before the loop start.
> (For the ultimate in max_insns modifying needs, see the sh4 patch set I posted
> this morning, wherein I may *extend* max_insns in order to force it to cover a
> region to be executed atomically within one TB, within the lock held by
> cpu_exec_step_atomic.)
> Based on that, I recommend
> DisasJumpType status;
> status = ops->tb_start(db, cpu, &max_insns);
> Because in my sh4 case, tb_start might itself decide that the only way to
> handle
> the code is to raise the EXCP_ATOMIC exception and try again within
> cpu_exec_step_atomic. Which means that we would not enter the while loop at
> all. Thus,
>> + while (true) {
> while (status == DISAS_NEXT) {
>> + db->num_insns++;
>> + ops->insn_start(db, cpu);
>> +
>> + /* Early exit before breakpoint checks */
> Better comment maybe? "The insn_start hook may request early exit..."
> And, indeed, perhaps
> status = ops->insn_start(db, cpu);
> if (unlikely(status != DISAS_NEXT)) {
> break;
> }
Since other hooks can set db->is_jmp and return values (breakpoint_check), I'll
stick with db->is_jmp instead. Then tb_start can return max_insns, and generic
code can refine it with checks like single-stepping.
>> + if (unlikely(db->is_jmp != DISAS_NEXT)) {
>> + break;
>> + }
>> +
>> + /* Pass breakpoint hits to target for further processing */
>> + if (unlikely(!QTAILQ_EMPTY(&cpu->breakpoints))) {
>> + CPUBreakpoint *bp;
>> + QTAILQ_FOREACH(bp, &cpu->breakpoints, entry) {
>> + if (bp->pc == db->pc_next) {
>> + BreakpointCheckType bp_check =
>> + ops->breakpoint_check(db, cpu, bp);
>> + switch (bp_check) {
>> + case BC_MISS:
>> + /* Target ignored this breakpoint, go to next */
>> + break;
>> + case BC_HIT_INSN:
>> + /* Hit, keep translating */
>> + /*
>> + * TODO: if we're never going to have more than one
>> + * BP in a single address, we can simply use a
>> + * bool here.
>> + */
>> + goto done_breakpoints;
>> + case BC_HIT_TB:
>> + /* Hit, end TB */
>> + goto done_generating;
>> + default:
>> + g_assert_not_reached();
>> + }
>> + }
>> + }
>> + }
>> + done_breakpoints:
>> +
>> + /* Accept I/O on last instruction */
>> + if (db->num_insns == max_insns && (db->tb->cflags & CF_LAST_IO)) {
>> + gen_io_start();
>> + }
>> +
>> + /* Disassemble one instruction */
>> + db->pc_next = ops->translate_insn(db, cpu);
> I think it would be better to assign to pc_next within the hook. We don't use
> the value otherwise within the rest of the loop.
> But we do use is_jmp, immediately. So maybe better to follow the changes to
> tb_start and insn_start above.
As before, for consistency I prefer to set is_jmp instead of returning it on
hooks, since breakpoint_check() already has a return value and can set is_jmp.
Then, a future series adding tracing events to this loop uses db->pc_next in the
generic loop, so it is going to be used.
>> + }
>> +
>> + ops->tb_stop(db, cpu);
>> +
>> + if (db->tb->cflags & CF_LAST_IO) {
>> + gen_io_end();
>> + }
> You can't place this after tb_stop, as it'll never be executed. We will have
> for certain emitted the exit_tb for all paths by now.
> Just place it before tb_stop where it usually resides. In the case where some
> is_jmp value implies unreached code, and we're using icount, we'll accept the
> dead code. But in all other cases it will get executed.
Ok!
Thanks,
Lluis
