Re: [PATCH v2 12/20] disas/nanomips: Replace std::string type

[Milica Lazarevic]

Thanks, it's more clear to me now! I'll modify it in the next by the suggestions.

---

From: Richard Henderson <richard.henderson@linaro.org>
Sent: Thursday, September 8, 2022 11:14 PM
To: Milica Lazarevic <Milica.Lazarevic@Syrmia.com>; thuth@redhat.com <thuth@redhat.com>
Cc: qemu-devel@nongnu.org <qemu-devel@nongnu.org>; cfontana@suse.de <cfontana@suse.de>; berrange@redhat.com <berrange@redhat.com>; pbonzini@redhat.com <pbonzini@redhat.com>; vince.delvecchio@mediatek.com <vince.delvecchio@mediatek.com>; peter.maydell@linaro.org <peter.maydell@linaro.org>; Djordje Todorovic <Djordje.Todorovic@syrmia.com>; mips32r2@gmail.com <mips32r2@gmail.com>; Dragan Mladjenovic <Dragan.Mladjenovic@syrmia.com>
Subject: Re: [PATCH v2 12/20] disas/nanomips: Replace std::string type

 

On 9/8/22 20:16, Milica Lazarevic wrote:
>     This would be better written as
>
>           char *reg_list[33];
>
>           assert(count <= 32);
>           for (c = 0; c < count; c++) {
>               bool use_gp = ...
>               uint64 this_rt = ...
>               /* glib usage below requires casting away const */
>               reg_list[c] = (char *)GPR(this_rt);
>           }
>           reg_list[count] = NULL;
>
>           return g_strjoinv(",", reg_list);
>
>
> In the implementation you suggested, there's one comma missing in the output.
> For example, instead of having:
>    > 0x802021ce: 1e12 SAVE 0x10,ra,s0
> We're having this:
>    < 0x802021ce: 1e12 SAVE 0x10ra,s0

Oh, right, because SAVE of zero registers is legal, and even useful as an adjustment to
the stack pointer.

> So, I'm assuming that there needs to exist one more concatenation between the comma and
> the result of the g_strjoinv function?
> Maybe something like
>      return g_strconcat((char *)",", (char *)g_strjoinv(",", reg_list), NULL);

Well, written like that you'd leak the result of g_strjoinv.

A better solution is to first element of reg_list be "", so that it's still just a single
memory allocation.

>     I think this interface should be
>
>           char **dis,
>
>     so that...
>
>     > @@ -746,25 +647,26 @@ static int Disassemble(const uint16 *data, std::string & dis,
>     >                                    * an ASE attribute and the requested version
>     >                                    * not having that attribute
>     >                                    */
>     > -                                dis = "ASE attribute mismatch";
>     > +                                strcpy(dis, "ASE attribute mismatch");
>
>     these become
>
>           *dis = g_strdup("string");
>
>     and the usage in nanomips_dis does not assume a fixed sized buffer.
>
>
>     r~
>
>
> I'm not sure why the fixed size buffer would be a problem here since the buffer size has
> already been limited by the caller.
> I.e. in the print_insn_nanomips function, the buf variable is defined as:
> char buf[200];

There would be no such declaration with the above change.


r~