Re: [Qemu-devel] [PATCH 04/10] disas: Support the Capstone disassembler library

[Richard Henderson <richard.henderson@linaro.org>]

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~