[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 04/10] disas: Support the Capstone disassembler
From: |
Richard Henderson |
Subject: |
Re: [Qemu-devel] [PATCH 04/10] disas: Support the Capstone disassembler library |
Date: |
Fri, 15 Sep 2017 09:58:19 -0700 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 |
On 09/14/2017 09:46 PM, Philippe Mathieu-Daudé wrote:
>> +static bool cap_disas(disassemble_info *info, uint64_t pc, size_t size)
>
> I'd rather use:
>
> ..,, target_ulong code, ...
>> +{
>
> uint64_t pc = (uint64_t)code;
Why?
>
>> + bool ret = false;
>
> Isn't it cleaner to have a stubs/disas_capstone.c?
>
>> +#ifdef CONFIG_CAPSTONE
If this one function warranted a separate file of its own, maybe, but certainly
not without that.
>> + cap_mode += (info->endian == BFD_ENDIAN_BIG ? CS_MODE_BIG_ENDIAN
>> + : CS_MODE_LITTLE_ENDIAN);
>> +
>
> assert(size); ?
The existing disassembler doesn't have it. So, no?
> err = cs_open(info->cap_arch, cap_mode, &handle);
> if (err != CS_ERR_OK) {
> (*info->fprintf_func)(info->stream, "Capstone: %s\n",
> cs_strerror(err));
No. Or not without extra limits. We don't want 100k instances of this error
as we dump all of the hunks for "-d in_asm".
We'd be assuming the distro didn't do anything silly wrt compiling with "diet"
mode or with only the host cpu enabled. I really don't know what to do about
such an eventuality except continue to fall back to the binutils code.
>> + cbuf = buf = g_malloc(size);
>
> if (buf == NULL) {
> goto err2;
> }
g_malloc cannot fail.
>
> cs_free(insn, 1);
> err2:
>
>> + g_free(buf);
>> + err1:
Oops, yes.
>> +
>> + if (s.info.cap_arch >= 0 && cap_disas(&s.info, (uintptr_t)code, size)) {
>
> (target_ulong)(uintptr_t)code, ?
Why? Even if I did change the type of the argument?
The extra cast would be implied by the language.
>> + /* ??? Capstone requires that we copy the data into a host-addressable
>> + buffer first and has no call-back to read more. Therefore we need
>> + an estimate of buffer size. This will work for most RISC, but we'll
>> + need to figure out something else for variable-length ISAs. */
>> + if (s.info.cap_arch >= 0 && cap_disas(&s.info, pc, 4 * nb_insn)) {
>
> .., MIN(16 * nb_insn, TARGET_PAGE_SIZE))) ?
That's no better than what I have; it simply prints more. I need a *real*
solution. It will probably involve not re-using cap_disas but writing code
just for the monitor.
r~
- [Qemu-devel] [PATCH 02/10] target/ppc: Convert to disas_set_info hook, (continued)
- [Qemu-devel] [PATCH 02/10] target/ppc: Convert to disas_set_info hook, Richard Henderson, 2017/09/14
- [Qemu-devel] [PATCH 05/10] target/i386: Support Capstone in disas_set_info, Richard Henderson, 2017/09/14
- [Qemu-devel] [PATCH 01/10] target/i386: Convert to disas_set_info hook, Richard Henderson, 2017/09/14
- [Qemu-devel] [PATCH 03/10] disas: Remove unused flags arguments, Richard Henderson, 2017/09/14
- [Qemu-devel] [PATCH 07/10] target/ppc: Support Capstone in disas_set_info, Richard Henderson, 2017/09/14
- [Qemu-devel] [PATCH 04/10] disas: Support the Capstone disassembler library, Richard Henderson, 2017/09/14
- [Qemu-devel] [PATCH 06/10] target/arm: Support Capstone in disas_set_info, Richard Henderson, 2017/09/14
- [Qemu-devel] [PATCH 08/10] target/s390x: Support Capstone in disas_set_info, Richard Henderson, 2017/09/14
- [Qemu-devel] [PATCH 10/10] target/mips: Support Capstone in disas_set_info, Richard Henderson, 2017/09/14
- [Qemu-devel] [PATCH 09/10] target/sparc: Support Capstone in disas_set_info, Richard Henderson, 2017/09/14
- Re: [Qemu-devel] [PATCH 00/10] Support the Capstone disassembler, Philippe Mathieu-Daudé, 2017/09/15