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

[Michael Clark]

On Thu, Mar 15, 2018 at 12:27 PM, Peter Maydell <address@hidden>
wrote:

> On 10 March 2018 at 21:25, Philippe Mathieu-Daudé <address@hidden> wrote:
> > 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;
> > }
>
> Hi -- any chance of a respin of this patch that addresses Philippe's
> review comments so we can fix it before rc0? This is causing
> my merge-build tests to fail about 50% of the time on OpenBSD
> at the moment...


I'll respin asap.

I was out earlier this week as I was at the RISC-V Hackathon at the
Embedded Linux Conference in Portland. I also have to go through the review
backlog for the post merge spec conformance and cleanup series...