regression: missing UC_HOOK_MEM

[boborjan2]

It seems that after 4f417c3 (having also 6974b53 applied), in rare cases, we have some UC_HOOK_MEMs gone missing. They are neither repetitions nor unaligned. Unfortunately I do not have a simple reproduction scenario. Target is x86.

[wtdcode]

Weird, how can I help to reproduce?

[boborjan2]

Yes, I don't just know, I am trying to narrow it down.

[wtdcode]

If possible, do as follows:

Build a debug build with -DUNICORN_LOGGING=on -DCMAKE_BUILD_TYPE=Debug, and run your programs with UNICORN_LOG_LEVEL=7 environment. Note this prints out every translation buffer and can be very very verbose. Redirecting output to a file is highly recommended.

Attach the translation buffers, registers contexts and original instructions around the addresses where hooks are missing. I will try to see which instructions cause this.

[boborjan2]

Ok, thanks, I will make a trace. What I know so far is that we have ~ 1700 mem read hooks in this regression. After the patch, we get only 246 (the first 246). It seems like the mem hook is completely shut off after a point. We don't receive the hooks from the same ip or target address either (where we had had hits before).
The execution flow however is the same, we reach the exit point properly, instruction count is exactly the same in both cases.

[wtdcode]

I just notice a better way to debug.

In store_helper and load_helper, if possible could you fprintf(stderr, "%x %x %x\n", addr, val, uc->size_recur_mem) ? I think it is the culprit.

[boborjan2]

loads.zip

I made a run both before and after the patch. Note: the after-patch is actually the 2.1.3 release.

[boborjan2]

Here is what happens:

1. store_helper() gets called with an unaligned address (size 4) that gets serviced by a recursive call to handle the 4 bytes separately. size_recur_mem is set to 4.
2. the recursively called store_helper() handles the 1-byte write using notdirty_write()
3. tb_invalidate_phys_page_fast() is called, then tb_invalidate_phys_page_range__locked()
4. at the end of tb_invalidate_phys_page_range__locked(), as current_tb_modified is set, cpu_loop_exit_noexc() gets called that exits by a siglongjmp().

As size_recur_mem is set to 4, no UC_HOOK_MEMs are called any more in the session.

1. A possible solution would be to add size_recur_mem as a parameter to store_helper() and do the recursion fully on stack.

2. Is this OK that only a halfword (or 1 byte) is written in a 4-byte store? It seems that something is off. From the logs it is obvious that the store is never finished, execution is continued at the next op.

[PhilippTakacs]

Hi thanks for this good explanation. Can you try following patch:

diff --git a/qemu/accel/tcg/cpu-exec.c b/qemu/accel/tcg/cpu-exec.c
index 09148f1d..06e0e1fa 100644
--- a/qemu/accel/tcg/cpu-exec.c
+++ b/qemu/accel/tcg/cpu-exec.c
@@ -610,6 +610,7 @@ int cpu_exec(struct uc_struct *uc, CPUState *cpu)
             if (unlikely(cpu->exit_request)) {
                 continue;
             }
+            uc->size_recur_mem = 0;
             cpu_loop_exec_tb(cpu, tb, &last_tb, &tb_exit);
             /* Try to align the host and virtual clocks
                if the guest is in advance */
diff --git a/qemu/softmmu/cpus.c b/qemu/softmmu/cpus.c
index afb8bbed..22511ac7 100644
--- a/qemu/softmmu/cpus.c
+++ b/qemu/softmmu/cpus.c
@@ -93,7 +93,6 @@ static int tcg_cpu_exec(struct uc_struct *uc)
         //                  (cpu->singlestep_enabled & SSTEP_NOTIMER) == 0);
         if (cpu_can_run(cpu)) {
             uc->quit_request = false;
-            uc->size_recur_mem = 0;
             r = cpu_exec(uc, cpu);
 
             // quit current TB but continue emulating?

[boborjan2]

Hi, thanks. I confirm it does the trick.

I gather it's an exception so it's just fine the store is not done (?). Btw how does this align to real-cpu behavior, does a word-sized write end up in a half-word write if there is an exception in the middle?

[boborjan2]

Another: shouldn't uc->size_recur_mem reset in the positive branch of sigsetjmp()? Imo it should be reset if we exit the recursion in any case.

[PhilippTakacs]

Now that you know how this happen, can you write a minimal reporducer?

Another: shouldn't uc->size_recur_mem reset in the positive branch of sigsetjmp()? Imo it should be reset if we exit the recursion in any case.

I have put it just before executing a translation block, this way it's allways set to 0 when a jump in a tb is happen. IHMO this is the best way preventing bugs like this. I need to read the code a bit more to understand what possible side effects are and what might get wrong with this.

Resetting size_recur_mem only in the positive branch of sigsetjmp() might miss some cases (i.e. when break_translation_loop() is called by a read of the IP register from a mem hook).

[wtdcode]

A PR is also highly welcome! And thanks @PhilippTakacs for the quick patch.

…

________________________________ From: PhilippTakacs ***@***.***> Sent: Tuesday, April 8, 2025 6:16:30 PM To: unicorn-engine/unicorn ***@***.***> Cc: lazymio ***@***.***>; Comment ***@***.***> Subject: Re: [unicorn-engine/unicorn] regression: missing UC_HOOK_MEM (Issue #2141) Now that you know how this happen, can you write a minimal reporducer? Another: shouldn't uc->size_recur_mem reset in the positive branch of sigsetjmp()? Imo it should be reset if we exit the recursion in any case. I have put it just before executing a translation block, this way it's allways set to 0 when a jump in a tb is happen. IHMO this is the best way preventing bugs like this. I need to read the code a bit more to understand what possible side effects are and what might get wrong with this. Resetting size_recur_mem only in the positive branch of sigsetjmp() might miss some cases (i.e. when break_translation_loop() is called by a read of the IP register from a mem hook). ― Reply to this email directly, view it on GitHub<#2141 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AHJULO2RCULVQ5V64RZN5Z32YOOX5AVCNFSM6AAAAAB2JNGHCOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDOOBVHE2DGNRRGU>. You are receiving this because you commented.Message ID: ***@***.***> [https://avatars.githubusercontent.com/u/76390863?s=20&v=4]PhilippTakacs left a comment (unicorn-engine/unicorn#2141)<#2141 (comment)> Now that you know how this happen, can you write a minimal reporducer? Another: shouldn't uc->size_recur_mem reset in the positive branch of sigsetjmp()? Imo it should be reset if we exit the recursion in any case. I have put it just before executing a translation block, this way it's allways set to 0 when a jump in a tb is happen. IHMO this is the best way preventing bugs like this. I need to read the code a bit more to understand what possible side effects are and what might get wrong with this. Resetting size_recur_mem only in the positive branch of sigsetjmp() might miss some cases (i.e. when break_translation_loop() is called by a read of the IP register from a mem hook). ― Reply to this email directly, view it on GitHub<#2141 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AHJULO2RCULVQ5V64RZN5Z32YOOX5AVCNFSM6AAAAAB2JNGHCOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDOOBVHE2DGNRRGU>. You are receiving this because you commented.Message ID: ***@***.***>

[boborjan2]

My concern is: will execution always touch the reset of size_recur_mem in the while loop in case we exit by sigsetjmp()?

I try to come up with a minimal test case.

[PhilippTakacs]

My concern is: will execution always touch the reset of size_recur_mem in the while loop in case we exit by sigsetjmp()?

This is exactly why I don't put it on the exit condition. I want to reset it before starting the emulation, independent why the loop stopped or if it stopped at all. So it's save to assume that on emulation start it's reset. With my last fix I haven't put this deep enough, so now I want to search for other exit conditions and check if they might skipping this reset.

[boborjan2]

2. Is this OK that only a halfword (or 1 byte) is written in a 4-byte store? It seems that something is off. From the logs it is obvious that the store is never finished, execution is continued at the next op.

I made a test on windows using VirtualProtect to make a page readonly. An unaligned 32-bit access to the end of the r/w area (i.e. last writeable page + 4094) causes access exception and I confirm the value is not written partly.

This means that the loop handling unaligned writes in store_helper() breaks compatibility on x86 (perhaps on other architectures as well).