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

[Thomas Huth]

On 15/08/2022 09.26, Milica Lazarevic wrote:
> The return type of typedef disassembly_function is changed to
> const char * instead of std::string. Therefore, for every particular
> disassembly_function function signature is changed.
> For example:
> - static std::string ABS_D(uint64 instruction) {...} is replaced with
> - static const char *ABS_D(uint64 instruction) {...}
>
> Every helper function used to return std::string is changed to return
> const char *. This applies to following functions: img_format,
> to_string, GPR, save_restore_list, FPR, etc.
>
> Now that we replaced every std::string for const char * or char *, it is
> possible to delete multiple versions of the img_format function. The
> general version:
> - static const char *img_format(const char *format, ...) {...}
> can handle all string formatting, so others have been deleted.
>
> Where necessary, strdup() is used to malloc string. Memory leaking needs
> to be prevented, so matching free() calls will be added later.
>
> Simple assignments like:
> - x = "string"
> are handled using the strcpy() function where needed.
>
> String concatenation in the save_restore_list() function is handled
> using strcat() function instead of += operator.
>
> Without applying all of these changes, the nanomips disassembler may be
> buildable but can't produce the appropriate output, so all of them are
> made together.
>
> Signed-off-by: Milica Lazarevic <milica.lazarevic@syrmia.com>
> ---
>   disas/nanomips.cpp | 4721 ++++++++++++++++++++++----------------------
>   1 file changed, 2312 insertions(+), 2409 deletions(-)
>
> diff --git a/disas/nanomips.cpp b/disas/nanomips.cpp
> index 23db8177ef..561e4ff095 100644
> --- a/disas/nanomips.cpp
> +++ b/disas/nanomips.cpp
> @@ -30,13 +30,11 @@
>   #include "qemu/osdep.h"
>   #include "disas/dis-asm.h"
>   
> -#include <cstring>
> +#include <string.h>
>   #include <stdexcept>
>   #include <stdio.h>
>   #include <stdarg.h>
>   
> -#include <string>
> -
>   typedef int64_t int64;
>   typedef uint64_t uint64;
>   typedef uint32_t uint32;
> @@ -44,7 +42,7 @@ typedef uint16_t uint16;
>   typedef uint64_t img_address;
>   
>   typedef bool(*conditional_function)(uint64 instruction);
> -typedef std::string(*disassembly_function)(uint64 instruction);
> +typedef const char *(*disassembly_function)(uint64 instruction);
>   
>   enum TABLE_ENTRY_TYPE {
>       instruction,
> @@ -93,7 +91,7 @@ struct Pool {
>   static img_address           m_pc;
>   static TABLE_ATTRIBUTE_TYPE   m_requested_instruction_categories;
>   
> -std::string img_format(const char *format, ...)
> +static const char *img_format(const char *format, ...)
>   {
>       char buffer[256];
>       va_list args;
> @@ -103,112 +101,15 @@ std::string img_format(const char *format, ...)
>           perror(buffer);
>       }
>       va_end(args);
> -    return buffer;
> +    return strdup(buffer);
>   }

If you're returning allocated memory, the return type could also be "char *" 
instead of "const char *" - that way you could get rid of a lot of casting 
in the next patch ("free((char *)....)").

> -std::string to_string(img_address a)
> +static const char *to_string(img_address a)
>   {
>       char buffer[256];
>       sprintf(buffer, "0x%" PRIx64, a);
> -    return buffer;
> +    return strdup(buffer);
>   }

Maybe it would also be better to switch to the functions from glib instead, 
you could avoid hard-coded array sizes that way. E.g. for this function:

static const char *to_string(img_address a)
{
    return g_strdup_printf("0x%" PRIx64, a);
}

See: https://docs.gtk.org/glib/func.strdup_printf.html

> @@ -617,21 +518,22 @@ static std::string GPR(uint64 reg)
>   }
>   
>   
> -static std::string save_restore_list(uint64 rt, uint64 count, uint64 gp)
> +static const char *save_restore_list(uint64 rt, uint64 count, uint64 gp)
>   {
> -    std::string str;
> +    char str[256];
> +    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;
> +    return strdup(str);
>   }

Using a hard-coded array first and doing a strdup() at the end looks weird 
... why don't you malloc() the str buffer right at the beginning of the 
function? (or use g_malloc instead to avoid
checking for NULL).

 Thomas