Breaking change without changing major version?

[mrexodia]

In https://github.com/unicorn-engine/unicorn/pull/1746 there was an update to the FAQ document:

Therefore, if you still prefer the previous paddr = vaddr simple mapping, we have a simple experimental MMU implementation that can be switched on by: uc_ctl_tlb_mode(uc, UC_TLB_VIRTUAL). With this mode, you could also add a UC_HOOK_TLB_FILL hook to manage the TLB. When a virtual address is not cached, the hook will be called. Besides, users are allowed to flush the tlb with uc_ctl_flush_tlb.

Wouldn't this be a breaking change warranting a major version bump? Existing users might get surprised by the new default behavior.

[mrexodia]

Just confirmed that restoring the old default behavior fixes the bug I mentioned here: https://github.com/unicorn-engine/unicorn/pull/1746#issuecomment-1517526868

        def __ctl_w(ctl, nr):
            return ctl | (nr << 26) | (UC_CTL_IO_WRITE << 30)
        try:
            self._uc.ctl(__ctl_w(12, 1), 1)
        except UcError:
            pass

[PhilippTakacs]

I'm not quit get which code is breaking. Can you provide a complete PoC which shows the issue?

[wtdcode]

The reason I support #1746 is that, unicorn once used a bunch of dirty and hacky tricks and I was tired of fixing the side effects introduced by them. Therefore, my long-term goal is to remove as many as possible hacks from qemu system emulation (softmmu) by exposing suitable API.

Talking about the changes of PR, the motivation is that, unicorns try to maintain a 1:1 mapping as explained but on the top of system emulation. That is to say, some memory regions are probably not written, not executable. What's worse, some regions would be even re-mapped. To solve this, unicorn hacks load_helper and write_helper and even mem_direct, which obviously doesn't cover all use cases and wastes much of my debugging time.

Regarding your question, our next release will be 2.1.0 and this advancement should be enough to reflect such compatibility changes since our main API is not changed. If you have any questions, simply provide a PoC script and we are happy to fix them before the next release, though the date to be determined.

[mrexodia]

Oh I also support the idea, it’s just that you are changing the default behavior that has been part of unicorn since the first version. Wouldn’t it be better to opt-in to such a drastic change and then at 3.x change it?

I’ll try to get a minimal poc working. Just a bit difficult because the setup is rather complex to work around unicorn bugs/oddities. It might take a while.

[wtdcode]

Oh I also support the idea, it’s just that you are changing the default behavior that has been part of unicorn since the first version. Wouldn’t it be better to opt-in to such a drastic change and then at 3.x change it?

We didn't change the default behavior. From the first version, unicorn offers a mixture of system emulation and 1:1 mapping, though not clearly stated anywhere. After this PR, we just make it a clear distinction by an extra control.

I’ll try to get a minimal poc working. Just a bit difficult because the setup is rather complex to work around unicorn bugs/oddities. It might take a while.

I could understand, but you could also share some coredumps if possible.

[mrexodia]

I pushed a standalone project that reproduces the bug here: https://github.com/mrexodia/unicorn_template/commit/7d26ebd1518fcd481d64c525ff66314058312597

[PhilippTakacs]

I get lots of errors (and warnings) when I build this. So I can't reproduce your bug.

I also don't understand what your code is supposed to do. Can you at least explain what happen?

[mrexodia]

What errors are you getting exactly? The code is just a small C++ wrapper that sets up a GDT and initializes the segments. It doesn’t get do actually emulate an instruction because it crashes with RIP==0 (presumably when the segment register is assigned).

[PhilippTakacs]

see: https://gist.github.com/PhilippTakacs/9c710916f1bde19e4871b0a8b624f661

[mrexodia]

Ah yes, the bug was only observed on Windows, I will see if it also reproduces on Linux.

[mrexodia]

I pushed a fix for the compilation errors in my code to the branch. The first error is a bug in unicorn and needs to be fixed upstream, I have no idea how the CI didn't catch it...

https://github.com/unicorn-engine/unicorn/blob/dev/include/unicorn/unicorn.h#LL248C1-L248C38

You are using an enum before it's declared, which isn't allowed when compiling in C++.

[PhilippTakacs]

Ok, now I could build and reproduce the bug. The problem is that x86 will use the load_helper on some register writes (what I have missed). Because the lookup fails unicorn tries to exit the current execution loop. Which fails because the emulation is not running. There are some details I currently not quite understand, but I'm working on it.

I'm working on a fix.

You are using an enum before it's declared

It's also not allowed in C, I'll fix this.

[mrexodia]

Awesome, thanks for checking it out!

[PhilippTakacs]

So I have now a better PoC:

#include <unicorn/unicorn.h>

int main(void)
{
    uc_engine *uc;
    uc_err e;
    uint64_t fs = 0x53;
    uc_x86_mmr gdtr = { 0, 0xfffff8076d962000, 0x57, 0 };

    uc_open(UC_ARCH_X86, UC_MODE_64, &uc);
    uc_reg_write(uc, UC_X86_REG_GDTR, &gdtr);
    e = uc_reg_write(uc, UC_X86_REG_FS, &fs);

    printf("ret: %s\n", uc_strerror(e));
}

and a fix:

diff --git a/qemu/accel/tcg/cputlb.c b/qemu/accel/tcg/cputlb.c
index 659faf3f..29c6555b 100644
--- a/qemu/accel/tcg/cputlb.c
+++ b/qemu/accel/tcg/cputlb.c
@@ -1520,21 +1520,25 @@ load_helper(CPUArchState *env, target_ulong addr, TCGMemOpIdx oi,
             mr = find_memory_region(uc, paddr);
             if (mr == NULL) {
                 uc->invalid_error = UC_ERR_MAP;
-                cpu_exit(uc->cpu);
-                // XXX(@lazymio): We have to exit early so that the target register won't be overwritten
-                //                because qemu might generate tcg code like:
-                //                       qemu_ld_i64 x0,x1,leq,8  sync: 0  dead: 0 1
-                //                where we don't have a change to recover x0 value
-                cpu_loop_exit(uc->cpu);
+                if (!uc->cpu->stopped) {
+                    cpu_exit(uc->cpu);
+                    // XXX(@lazymio): We have to exit early so that the target register won't be overwritten
+                    //                because qemu might generate tcg code like:
+                    //                       qemu_ld_i64 x0,x1,leq,8  sync: 0  dead: 0 1
+                    //                where we don't have a change to recover x0 value
+                    cpu_loop_exit(uc->cpu);
+                }
                 return 0;
             }
         } else {
             uc->invalid_addr = paddr;
             uc->invalid_error = error_code;
             // printf("***** Invalid fetch (unmapped memory) at " TARGET_FMT_lx "\n", addr);
-            cpu_exit(uc->cpu);
-            // See comments above
-            cpu_loop_exit(uc->cpu);
+            if (!uc->cpu->stopped) {
+                cpu_exit(uc->cpu);
+                // See comments above
+                cpu_loop_exit(uc->cpu);
+            }
             return 0;
         }
     }
@@ -1588,9 +1592,11 @@ load_helper(CPUArchState *env, target_ulong addr, TCGMemOpIdx oi,
                 uc->invalid_addr = paddr;
                 uc->invalid_error = UC_ERR_READ_PROT;
                 // printf("***** Invalid memory read (non-readable) at " TARGET_FMT_lx "\n", addr);
-                cpu_exit(uc->cpu);
-                // See comments above
-                cpu_loop_exit(uc->cpu);
+                if (!uc->cpu->stopped) {
+                    cpu_exit(uc->cpu);
+                    // See comments above
+                    cpu_loop_exit(uc->cpu);
+                }
                 return 0;
             }
         }
@@ -1618,9 +1624,11 @@ load_helper(CPUArchState *env, target_ulong addr, TCGMemOpIdx oi,
                 uc->invalid_addr = paddr;
                 uc->invalid_error = UC_ERR_FETCH_PROT;
                 // printf("***** Invalid fetch (non-executable) at " TARGET_FMT_lx "\n", addr);
-                cpu_exit(uc->cpu);
-                // See comments above
-                cpu_loop_exit(uc->cpu);
+                if (!uc->cpu->stopped) {
+                    cpu_exit(uc->cpu);
+                    // See comments above
+                    cpu_loop_exit(uc->cpu);
+                }
                 return 0;
             }
         }

I'm need to read a bit more and do some more testing.

[mrexodia]

Let me know if you have a branch with the fix. I can also test if there are other regressions.

[PhilippTakacs]

https://github.com/PhilippTakacs/unicorn/tree/issuevtlb

[mrexodia]

I didn't forget about this, just don't have a lot of time for testing...

[wtdcode]

I assume your issue should be solved?

We plan to release 2.1.0 recently (hope to be soooooon) regarding so many features and improvements since last release. Do you have any more concerns?