On 06/25/2017 01:59 AM, Lluís Vilanova wrote:
+static inline void translate_block_tcg_check(const DisasContextBase *db)
+{
+ if (tcg_check_temp_count()) {
+ error_report("warning: TCG temporary leaks before "TARGET_FMT_lx,
+ db->pc_next);
+ }
+}
+
+void translate_block(const TranslatorOps *ops, DisasContextBase *db,
+ CPUState *cpu, TCGv_env *tcg_cpu, TranslationBlock *tb)
tcg_cpu isn't the best name -- it doesn't reference a version of cpu. Of
course, you can always get at tcg_ctx.tcg_env, so there's no point in passing
it anyway.
+ /* Sanity-check ops */
+ if (ops->disas_insn == NULL) {
+ error_report("Missing ops->disas_insn");
+ abort();
+ }
Why? Surely an immediate crash by calling to null is just as easy to debug.
And, bikeshedding, perhaps translate_insn is a better name. On the first read
through I assumed this was related to the disassembly log.
+ while (true) {
+ CPUBreakpoint *bp;
+
+ db->num_insns++;
+ if (ops->insn_start) {
+ ops->insn_start(db, cpu);
+ }
This *must* be defined. A target cannot skip emitting insn_start or unwinding
won't work.
+
+ /* Early exit before breakpoint checks */
+ if (unlikely(db->is_jmp != DJ_NEXT)) {
+ break;
+ }
This must be done at the end of the loop, not at the beginning of the next
loop, after we've already emitted insn_start for the following insn.
That said, you already do have that check below, so what is this intended to do?
+ /* Pass breakpoint hits to target for further processing */
+ bp = NULL;
+ do {
+ bp = cpu_breakpoint_get(cpu, db->pc_next, bp);
+ if (unlikely(bp) && ops->breakpoint_check) {
+ BreakpointCheckType bp_check = ops->breakpoint_check(
Is there any point in any of these hooks being null?
An empty function in most cases, or here, one that returns BC_MISS.
+ db, cpu, bp);
+ if (bp_check == 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.
+ */
+ break;
+ } else if (bp_check == BC_HIT_TB) {
+ goto done_generating;
+ } else {
+ error_report("Unexpected BreakpointCheckType %d",
bp_check);
+ abort();
What happened to BC_MISS? And surely better structured as a switch with
default:
g_assert_not_reached();
rather than custom logging.
+#ifdef DEBUG_DISAS
+ if (qemu_loglevel_mask(CPU_LOG_TB_IN_ASM) &&
+ qemu_log_in_addr_range(db->pc_first)) {
+ int flags;
+ if (ops->disas_flags) {
+ flags = ops->disas_flags(db);
+ } else {
+ flags = 0;
+ }
+ qemu_log_lock();
+ qemu_log("----------------\n");
+ qemu_log("IN: %s\n", lookup_symbol(db->pc_first));
+ log_target_disas(cpu, db->pc_first, db->pc_next - db->pc_first, flags);
+ qemu_log("\n");
+ qemu_log_unlock();
I think the hook shouldn't be just the flags, but the whole call to
log_target_disas, at which point there isn't a reason to have a separate in a
hook for the flags. Consider the extra checks done for s390x and hppa.
r~
