Re: [Qemu-devel] [PATCH v2] RISC-V: Fix riscv_isa_string, use popcount to count bits

[Philippe Mathieu-Daudé]

On 03/09/2018 10:01 PM, Michael Clark wrote:
> Logic bug caused the string size calculation for the RISC-V
> format ISA string to be small. This fix allows slack for rv128.
> 
> Cc: Palmer Dabbelt <address@hidden>
> Cc: Peter Maydell <address@hidden>
> Cc: Eric Blake <address@hidden>
> Signed-off-by: Michael Clark <address@hidden>
> ---
>  target/riscv/cpu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 4851890..1456535 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -391,7 +391,7 @@ static const TypeInfo riscv_cpu_type_info = {
>  char *riscv_isa_string(RISCVCPU *cpu)
>  {
>      int i;
> -    size_t maxlen = 5 + ctz32(cpu->env.misa);
> +    size_t maxlen = 8 + ctpop64(cpu->env.misa);

Can you document the magic 5/8?

This looks nice, but this seems to me too much optimization to save few
bytes, using sizeof(riscv_exts) is overflow-free.

Maybe this is enough and self-explanatory:

       const size_t maxlen = sizeof("rv128") + sizeof(riscv_exts);

>      char *isa_string = g_new0(char, maxlen);
>      snprintf(isa_string, maxlen, "rv%d", TARGET_LONG_BITS);

Also, if you keep the snprintf() return value, you can (naming it 'n')
simplify (also easier to review):

>      for (i = 0; i < sizeof(riscv_exts); i++) {
> 
        if (cpu->env.misa & RV(riscv_exts[i])) {
-           isa_string[strlen(isa_string)] = riscv_exts[i] - 'A' + 'a';
+           isa_string[n++] = tolower(riscv_exts[i]);
        }
    }

and simply use g_new() with:

+   isa_string[n] = '\0';

    return isa_string;
}

Regards,

Phil.