Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

continue in THUMB mode if CPSR register has T bit #1801

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

lowlyw
Copy link
Contributor

@lowlyw lowlyw commented Mar 9, 2023

currently only PC | 1 being set will trigger thumb mode, but actually if T bit is set in CPSR, we should run in thumb mode.

currently only PC | 1 being set will trigger thumb mode, but actually if T bit is set in CPSR, we should run in thumb mode.
@PhilippTakacs
Copy link
Contributor

There is already the function arm_cpu_set_pc in qemu/target/arm/cpu.c which does exactly the same what your code does. Also in reg_write (qemu/target/arm/unicorn_arm.c:382) is similar code. This code might miss some detail.

Can you provide a small example to show what is not correct handle?

@lowlyw
Copy link
Contributor Author

lowlyw commented Mar 9, 2023

@PhilippTakacs view this

#include <unicorn/unicorn.h>
#include <unicorn/arm.h>
#include <stdio.h>

static void hook_code(uc_engine *uc, uint64_t address, uint32_t size,
                      void *user_data)
{
    printf(">>> Tracing instruction at 0x%" PRIx64
           ", instruction size = 0x%x\n",
           address, size);
}

void run() {
    // mov r0, 9; bx r0; <some thumb instruction>
#define ARM_CODE "\x09\x00\xA0\xE3\x10\xFF\x2F\xE1\x18\x88"
    uc_engine *uc;
    uc_err err;
    uc_hook trace1;
    // start in arm mode
    err = uc_open(UC_ARCH_ARM, UC_MODE_ARM, &uc);
    if (err) {
        printf("failed uc_open!\n");
        return;
    }
    uc_mem_map(uc, 0, 2 * 1024 * 1024, UC_PROT_ALL);
    uc_mem_write(uc, 0, ARM_CODE, sizeof(ARM_CODE) - 1);
    uc_hook_add(uc, &trace1, UC_HOOK_CODE, hook_code, NULL, 0, 0xFFFFFFFF);
	uc_emu_start(uc, 0, 0 + sizeof(ARM_CODE) - 1, 0, 0);
    return;
}

void step() {
    // mov r0, 9; bx r0; <some thumb instruction>
#define ARM_CODE "\x09\x00\xA0\xE3\x10\xFF\x2F\xE1\x18\x88"
    uc_engine *uc;
    uc_err err;
    uc_hook trace1;
	int i;
    // start in arm mode
    err = uc_open(UC_ARCH_ARM, UC_MODE_ARM, &uc);
    if (err) {
        printf("failed uc_open!\n");
        return;
    }
    uc_mem_map(uc, 0, 2 * 1024 * 1024, UC_PROT_ALL);
    uc_mem_write(uc, 0, ARM_CODE, sizeof(ARM_CODE) - 1);
    uc_hook_add(uc, &trace1, UC_HOOK_CODE, hook_code, NULL, 0, 0xFFFFFFFF);
	for (i = 0; i < 3; i++) {
		uint32_t pc;
		uc_reg_read(uc, UC_ARM_REG_PC, &pc);
		uc_emu_start(uc, pc, 0 + sizeof(ARM_CODE) - 1, 0, 1);
	}
    return;
}

int main(void)
{
	printf(">>> emulation with 'run'\n");
	run();
	printf(">>> emulation with 'step'\n");
	step();
	return 0;
}

they should behave the same, do you agree?

@lowlyw
Copy link
Contributor Author

lowlyw commented Mar 9, 2023

(the current behavior is

>>> emulation with 'run'
>>> Tracing instruction at 0x0, instruction size = 0x4
>>> Tracing instruction at 0x4, instruction size = 0x4
>>> Tracing instruction at 0x8, instruction size = 0x2
>>> emulation with 'step'
>>> Tracing instruction at 0x0, instruction size = 0x4
>>> Tracing instruction at 0x4, instruction size = 0x4
>>> Tracing instruction at 0x8, instruction size = 0x4

@lowlyw
Copy link
Contributor Author

lowlyw commented Mar 9, 2023

with my PR they behave same:

>>> Tracing instruction at 0x0, instruction size = 0x4
>>> Tracing instruction at 0x4, instruction size = 0x4
>>> Tracing instruction at 0x8, instruction size = 0x2
>>> emulation with 'step'
>>> Tracing instruction at 0x0, instruction size = 0x4
>>> Tracing instruction at 0x4, instruction size = 0x4
>>> Tracing instruction at 0x8, instruction size = 0x2```

@PhilippTakacs
Copy link
Contributor

I see the problem, but I'm unsure about the solution. Because what about setting THUMB mode direct like in following example:

void step2() {
    // mov r0, 9; bx r0; <some thumb instruction>
#define ARM_CODE "\x09\x00\xA0\xE3\x10\xFF\x2F\xE1\x18\x88"
    uc_engine *uc;
    uc_err err;
    uc_hook trace1;
    uint32_t pc;
    // start in thumb mode
    err = uc_open(UC_ARCH_ARM, UC_MODE_THUMB, &uc);
    if (err) {
        printf("failed uc_open!\n");
        return;
    }
    uc_mem_map(uc, 0, 2 * 1024 * 1024, UC_PROT_ALL); 
    uc_mem_write(uc, 0, ARM_CODE, sizeof(ARM_CODE) - 1);
    uc_hook_add(uc, &trace1, UC_HOOK_CODE, hook_code, NULL, 0, 0xFFFFFFFF);
    pc = 8;
    uc_emu_start(uc, pc, 0 + sizeof(ARM_CODE) - 1, 0, 1);
    return;
}

Or what about going back to normal mode like in this example:

void step3() {
    // mov r0, 9; bx r0; <some thumb instruction>
#define ARM_CODE "\x09\x00\xA0\xE3\x10\xFF\x2F\xE1\x18\x88"
    uc_engine *uc;
    uc_err err;
    uc_hook trace1;
    int i;
    uint32_t pc;
    // start in arm mode
    err = uc_open(UC_ARCH_ARM, UC_MODE_ARM, &uc);
    if (err) {
        printf("failed uc_open!\n");
        return;
    }
    uc_mem_map(uc, 0, 2 * 1024 * 1024, UC_PROT_ALL); 
    uc_mem_write(uc, 0, ARM_CODE, sizeof(ARM_CODE) - 1);
    uc_hook_add(uc, &trace1, UC_HOOK_CODE, hook_code, NULL, 0, 0xFFFFFFFF);
    for (i = 0; i < 2; i++) {
        uc_reg_read(uc, UC_ARM_REG_PC, &pc);
        uc_emu_start(uc, pc, 0 + sizeof(ARM_CODE) - 1, 0, 1);
    }
    pc = 0;
    OK(uc_emu_start(uc, pc, 0 + sizeof(ARM_CODE) - 1, 0, 1));
    return;
}

A better solution would be to check for uc->thumb and ensure that this is in sync with env->thumb (which is currently not the case). Also add a uc_ctl mode to read and write thumb mode.

@lowlyw
Copy link
Contributor Author

lowlyw commented Mar 10, 2023

step2 my change has no effect
step3 behaves how i would expect, it is taking consideration of the THUMB bit of CPSR register. i don't understand your request in this. if you wanna have arm mode, you must CLEAR this bit.

@wtdcode
Copy link
Member

wtdcode commented Mar 12, 2023

Sorry for late on this discussion. UC_MODE_THUMB is not a good design overall because odd PC and CPSR actually implies this as you already notice. Therefore, removing it should be the clean solution though we can't do this obviously due to the compatibility. Anyway, I understand your motivation and agree with the workaround post here but:

  1. Not all arm CPU model has CPSR or even supports switching between ARM & THUMB, you probably have to check more.
  2. Please target dev branch

@PhilippTakacs
Copy link
Contributor

A bit context for my last comment. I understand the the motivation for this change, but it looks a bit inconsistent. Because there is already UC_MODE_THUMB (which is currently mostly ignored) and you can imply thumb mode by starting at an odd pc (and switch back with an even pc). If I see correct you can also switch with uc_reg_write and an even/odd pc during emulation. With this change there are more cases you need to know. I have no problem with changing the API but it should be consistent. An small function to switch between thumb and arm would be nice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants