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

[Milica Lazarevic]

 

On 9/5/22 10:55, Milica Lazarevic wrote:
> -static std::string save_restore_list(uint64 rt, uint64 count, uint64 gp)
> +static char *save_restore_list(uint64 rt, uint64 count, uint64 gp)
>   {
> -    std::string str;
> +    /*
> +     * Currently, this file compiles as a cpp file, so the explicit cast here
> +     * is necessary. Later, the cast will be removed.
> +     */
> +    char *str = (char *)g_malloc(200);
> +    str[0] = '\0';
>  
>       for (uint64 counter = 0; counter != count; counter++) {
>           bool use_gp = gp && (counter == count - 1);
>           uint64 this_rt = use_gp ? 28 : ((rt & 0x10) | (rt + counter)) & 0x1f;
> -        str += img_format(",%s", GPR(this_rt));
> +        strcat(str, img_format(",%s", GPR(this_rt)));
>       }
>  
>       return str;
>   }

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

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);

this?

Would it be acceptable to go with a similar implementation as in the patch, but without a call to img_format and with the g_strconcat instead of the strcat function?

> @@ -716,7 +617,7 @@ static uint64 extract_op_code_value(const uint16 *data, int size)
>    *      instruction size    - negative is error
>    *      disassembly string  - on error will constain error string
>    */
> -static int Disassemble(const uint16 *data, std::string & dis,
> +static int Disassemble(const uint16 *data, char *dis,


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];