These two patches fix wrong format strings used in disas/nanomips.

The first patch replaces proprietary data types by POSIX data types,
because otherwise the PRI... macros cannot be used in the second patch.

Those patches are only relevant for 3.1 if full nanomips support is considered
important enought and if QEMU is used on big endian machines (I think
the old code will work on little endian hosts even with wrong format
strings).

I use those patches for QEMU for Windows because that is compiled with
more compiler warnings, so compilation would fail without the fix.

Regards
Stefan

On Tue, 27 Nov 2018 at 12:17, Stefan Weil <sw@weilnetz.de> wrote:
>
> These two patches fix wrong format strings used in disas/nanomips.
>
> The first patch replaces proprietary data types by POSIX data types,
> because otherwise the PRI... macros cannot be used in the second patch.

I think this is a good idea, but that first patch is a huge patch for
this point in the 3.1 release cycle. Can we fix the warnings
by just casting the arguments to the img::format() function ?

> Those patches are only relevant for 3.1 if full nanomips support is considered
> important enought and if QEMU is used on big endian machines (I think
> the old code will work on little endian hosts even with wrong format
> strings).

It only affects the disassembler, which is a debug tool.
A week ago this would have been a fairly definite "yes, fix it"
issue; at this point, with rc3 due to go out today, it's a bit
trickier to justify. (rc3 will be the last rc unless any showstopper
issues turn up.)

> I use those patches for QEMU for Windows because that is compiled with
> more compiler warnings, so compilation would fail without the fix.

My Windows cross-compiler builds work OK, FWIW, and -Werror is
only enabled by default for Linux builds, so warnings shouldn't
result in build failures.

thanks
-- PMM

Am 27.11.2018 um 13:36 schrieb Peter Maydell:
> On Tue, 27 Nov 2018 at 12:17, Stefan Weil <sw@weilnetz.de> wrote:
>> These two patches fix wrong format strings used in disas/nanomips.
>>
>> The first patch replaces proprietary data types by POSIX data types,
>> because otherwise the PRI... macros cannot be used in the second patch.
> I think this is a good idea, but that first patch is a huge patch for
> this point in the 3.1 release cycle. Can we fix the warnings
> by just casting the arguments to the img::format() function ?


Casting would be possible, but I try to avoid it if there are other
solutions.

The patch is huge, but the changes are purely mechanical. I used
commands like "perl -pi -e s/int64/int64_t/g nanomips.*" to make them.


>> Those patches are only relevant for 3.1 if full nanomips support is considered
>> important enought and if QEMU is used on big endian machines (I think
>> the old code will work on little endian hosts even with wrong format
>> strings).
> It only affects the disassembler, which is a debug tool.
> A week ago this would have been a fairly definite "yes, fix it"
> issue; at this point, with rc3 due to go out today, it's a bit
> trickier to justify. (rc3 will be the last rc unless any showstopper
> issues turn up.)


Yes.


>> I use those patches for QEMU for Windows because that is compiled with
>> more compiler warnings, so compilation would fail without the fix.
> My Windows cross-compiler builds work OK, FWIW, and -Werror is
> only enabled by default for Linux builds, so warnings shouldn't
> result in build failures.


I forgot to mention another potential portability issue: the proprietary
data types which I removed in the first patch work on all relevant hosts
/ compilers which I know, but there might also be platforms where the
definitions are wrong:

-typedef unsigned short uint16;
-typedef unsigned int uint32;
-typedef long long int64;
-typedef unsigned long long uint64;

Nevertheless I think that 3.1 can also be released without changing disas/nanomips.

Stefan
 

Stefan Weil wrote:

> These two patches fix wrong format strings used in disas/nanomips.

Stefan,

I truly appreciate your interest in nanoMIPS dissasembler.

In my opinion, this series comes too late in 3.1 development cycle to be
accepted. The described severity of undesired behavior is just way too low
for rc3 phase.

Also, as a general remark, in this series, there is no example of wrong
output for big endian host, only some belief or opinion - referent test
example would be helpful, and much better justification for the series.
Some build errors are mentioned, but they are not a part for official
builds. Related to these errors, certain additional build options are said
to cause build warnings, but such build options were not identified.

All in all, my judgement is that we should deal with these issues after 3.1
release.

However, I am still grateful to you for bringing these issues up, and you
are welcome to participate in any way in future development of this and
other segments of QEMU for MIPS.

Yours,
Aleksandar
On Nov 27, 2018 1:19 PM, "Stefan Weil" <sw@weilnetz.de> wrote:

> These two patches fix wrong format strings used in disas/nanomips.
>
> The first patch replaces proprietary data types by POSIX data types,
> because otherwise the PRI... macros cannot be used in the second patch.
>
> Those patches are only relevant for 3.1 if full nanomips support is
> considered
> important enought and if QEMU is used on big endian machines (I think
> the old code will work on little endian hosts even with wrong format
> strings).
>
> I use those patches for QEMU for Windows because that is compiled with
> more compiler warnings, so compilation would fail without the fix.
>
> Regards
> Stefan
>
>
>

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Subject: [Qemu-devel] [PATCH for-3.1 0/2] Fix disas/nanomips
Message-id: 20181127121724.19755-1-sw@weilnetz.de

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
cedb6ba disas/nanomips: Fix format strings
1c7e06b disas/nanomips: Replace proprietary by POSIX data types

=== OUTPUT BEGIN ===
Checking PATCH 1/2: disas/nanomips: Replace proprietary by POSIX data types...
ERROR: "foo * bar" should be "foo *bar"
#361: FILE: disas/nanomips.cpp:569:
+uint64_t NMD::extract_op_code_value(const uint16_t * data, int size)

ERROR: "foo * bar" should be "foo *bar"
#379: FILE: disas/nanomips.cpp:584:
+int NMD::Disassemble(const uint16_t * data, std::string & dis,

ERROR: "foo * bar" should be "foo *bar"
#388: FILE: disas/nanomips.cpp:602:
+int NMD::Disassemble(const uint16_t * data, std::string & dis,

WARNING: line over 80 characters
#559: FILE: disas/nanomips.cpp:793:
+int64_t NMD::extr_sil0il31bs1_il2il21bs10_il12il12bs9Tmsb31(uint64_t instruction)

WARNING: line over 80 characters
#2393: FILE: disas/nanomips.cpp:2681:
+    int64_t s_value = extr_sil0il31bs1_il2il21bs10_il12il12bs9Tmsb31(instruction);

WARNING: line over 80 characters
#6610: FILE: disas/nanomips.cpp:9126:
+    int64_t s_value = extr_sil0il31bs1_il2il21bs10_il12il12bs9Tmsb31(instruction);

WARNING: line over 80 characters
#11365: FILE: disas/nanomips.cpp:16407:
+    uint64_t code_value = extract_code_25_24_23_22_21_20_19_18_17_16(instruction);

WARNING: line over 80 characters
#11607: FILE: disas/nanomips.h:148:
+    int64_t extr_sil0il31bs1_il2il21bs10_il12il12bs9Tmsb31(uint64_t instruction);

total: 3 errors, 5 warnings, 12451 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 2/2: disas/nanomips: Fix format strings...
=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com