Re: [Qemu-devel] [PULL 3/3] target/mips: Add disassembler support for nanoMIPS

[Aleksandar Markovic]

Hello, Stefan.

I appreciate your note and guidance.

> 
> On 25.10.18 22:19, Aleksandar Markovic wrote:
> > From: Aleksandar Markovic <address@hidden>
> > 
> > Add disassembler support for nanoMIPS.
> > 
> > Reviewed-by: Stefan Markovic <address@hidden>
> > Signed-off-by: Matthew Fortune <address@hidden>
> > Signed-off-by: Aleksandar Markovic <address@hidden>
> > ---
> >  MAINTAINERS           |     2 +
> >  configure             |     3 +
> >  disas/Makefile.objs   |     1 +
> >  disas/nanomips.cpp    | 22242 
> > ++++++++++++++++++++++++++++++++++++++++++++++++
> >  disas/nanomips.h      |  1099 +++
> >  include/disas/bfd.h   |     1 +
> >  include/exec/poison.h |     1 +
> >  target/mips/cpu.c     |    13 +-
> >  8 files changed, 23360 insertions(+), 2 deletions(-)
> >  create mode 100644 disas/nanomips.cpp
> >  create mode 100644 disas/nanomips.h
> 
> 
> Hi,
> 
> the disassembler needs more work for the next QEMU release: it uses lots
> of wrong format strings which will result in wrong output at least on
> big endian hosts.
> 
> This patch enables the compiler errors to see those bugs:
> 
> diff --git a/disas/nanomips.cpp b/disas/nanomips.cpp
> index 1238c2ff33..62be5b6d00 100644
> --- a/disas/nanomips.cpp
> +++ b/disas/nanomips.cpp
> @@ -138,7 +138,7 @@ namespace img
>          return a;
>      }
> 
> -    std::string format(const char *format, ...)
> +    std::string GCC_FMT_ATTR(1, 2) format(const char *format, ...)
>      {
>          char buffer[256];
>          va_list args;
> 

After applying this oatch, I see these warnings: 

address@hidden:~/Build/qemu-tot$ make
        CHK version_gen.h
  CXX     disas/nanomips.o
disas/nanomips.cpp: In member function ‘uint64 NMD::renumber_registers(uint64, 
uint64*, size_t)’:
disas/nanomips.cpp:288:45: error: format ‘%d’ expects argument of type ‘int’, 
but argument 2 has type ‘uint64 {aka long long unsigned int}’ [-Werror=format=]
                    index, register_list_size));
                                             ^
disas/nanomips.cpp:288:45: error: format ‘%d’ expects argument of type ‘int’, 
but argument 3 has type ‘size_t {aka long unsigned int}’ [-Werror=format=]
disas/nanomips.cpp: In member function ‘std::__cxx11::string NMD::GPR(uint64)’:
disas/nanomips.cpp:504:78: error: format ‘%d’ expects argument of type ‘int’, 
but argument 2 has type ‘uint64 {aka long long unsigned int}’ [-Werror=format=]
     throw std::runtime_error(img::format("Invalid GPR register index %d", 
reg));
                                                                              ^
disas/nanomips.cpp: In member function ‘std::__cxx11::string NMD::FPR(uint64)’:
disas/nanomips.cpp:521:78: error: format ‘%d’ expects argument of type ‘int’, 
but argument 2 has type ‘uint64 {aka long long unsigned int}’ [-Werror=format=]
     throw std::runtime_error(img::format("Invalid FPR register index %d", 
reg));
                                                                              ^
disas/nanomips.cpp: In member function ‘std::__cxx11::string NMD::AC(uint64)’:
disas/nanomips.cpp:535:77: error: format ‘%d’ expects argument of type ‘int’, 
but argument 2 has type ‘uint64 {aka long long unsigned int}’ [-Werror=format=]
     throw std::runtime_error(img::format("Invalid AC register index %d", reg));
                                                                             ^
disas/nanomips.cpp: In member function ‘std::__cxx11::string 
NMD::IMMEDIATE(uint64)’:
disas/nanomips.cpp:541:37: error: format ‘%x’ expects argument of type 
‘unsigned int’, but argument 2 has type ‘uint64 {aka long long unsigned int}’ 
[-Werror=format=]
     return img::format("0x%x", value);
                                     ^
disas/nanomips.cpp: In member function ‘std::__cxx11::string 
NMD::IMMEDIATE(int64)’:
disas/nanomips.cpp:547:35: error: format ‘%d’ expects argument of type ‘int’, 
but argument 2 has type ‘int64 {aka long long int}’ [-Werror=format=]
     return img::format("%d", value);
                                   ^
disas/nanomips.cpp: In member function ‘std::__cxx11::string NMD::CPR(uint64)’:
disas/nanomips.cpp:554:35: error: format ‘%d’ expects argument of type ‘int’, 
but argument 2 has type ‘uint64 {aka long long unsigned int}’ [-Werror=format=]
     return img::format("CP%d", reg);
                                   ^
cc1plus: all warnings being treated as errors
/home/rtrk/Build/qemu-tot/rules.mak:94: recipe for target 'disas/nanomips.o' 
failed
make: *** [disas/nanomips.o] Error 1

Did you have the same warnings? Is there any way to get more similar warnings?

Can I modify:

    std::string format(const char *format,
                       std::string s)
    {
        char buffer[256];

        sprintf(buffer, format, s.c_str());

        return buffer;
    }

to get some more warnings of that kind?

In all cases of warnings above, int64 is avoidable - it is used for things like 
ordinal number of a register, which is obviously a small integer.

The whole disassembler, it looks to me, does not need 64-bit integers, except 
in some limited cases of decoding 48-bit instructions.

In any case, a cleanup is planned for the 3.2., for this and other things. I'll 
think about that.

Thanks,
Aleksandar


> I also noticed that the code does not use POSIX data types but
> introduces its own integer types in disas/nanomips.h. This should be
> fixed, too, because it prevents the use of PRIu64 etc. in the format
> strings.
> 
> Regards
> Stefan Weil
>