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

[Richard Henderson]

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~