Hello, Stefan.
I appreciate your note and guidance.
>
> On 25.10.18 22:19, Aleksandar Markovic wrote:
> > From: Aleksandar Markovic <amarko...@wavecomp.com>
> >
> > Add disassembler support for nanoMIPS.
> >
> > Reviewed-by: Stefan Markovic <smarko...@wavecomp.com>
> > Signed-off-by: Matthew Fortune <matthew.fort...@mips.com>
> > Signed-off-by: Aleksandar Markovic <amarko...@wavecomp.com>
> > ---
> > 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:
rtrk@rtrkw774-lin:~/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
>
