[PULL 0/3] MIPS + TCG Continuous Benchmarking queue for July 7th, 2020

[Aleksandar Markovic]

On Tuesday, July 7, 2020, Paolo Bonzini <pbonzini@redhat.com> wrote:

I haven't looked at the disassembler code; assuming it comes from an upstream code base I don't think we should treat it differently from the ARM disassembler (or for that matter the binutils ones) and basically handle it as a black box for which we don't really care about the code quality or style. It's not security sensitive code, and forking the upstream is probably not justified. On the other hand the pull request should have explained the provenance of the code (even if only in the subject line of the relevant patch).

Regarding the recommendation of a "vacation" after last week's facts, QEMU's enforcing of behavior standards so far has been informal, therefore my suggestion cannot be anything more than a suggestion, though a strongly recommended one. I can also understand the soft freeze pressure. I defer to Peter as to whether to merge this pull request or not due to Thomas's objections, but I do wish to reinforce that taking some time off to cool down can often times be a good idea.

Sure, I respect the opinion of both Paolo and Thomas. I already reflected on everything, and will not be an obstacle in any way in qemu community.

There is truly no ulterior motive from my side regarding this pull request - I was just trying to integrate some code before soft-freeze.

Peter, please discard this pull request, I will send a new one, as soon as I can, most likely tomorrow, that will fully follow Paolo's and Thomas' recommendations, no need to worry.

Thanks,

Aleksamdar

 

Thanks,

Paolo

Il mar 7 lug 2020, 22:20 Thomas Huth <thuth@redhat.com> ha scritto:

On 07/07/2020 18.40, Aleksandar Markovic wrote:
> The following changes since commit 710fb08fd297d7a92163debce1959fae8f3b6ed7:
>
>   Merge remote-tracking branch 'remotes/huth-gitlab/tags/pull-request-2020-07-06' into staging (2020-07-07 12:41:15 +0100)
>
> are available in the git repository at:
>
>   https://github.com/AMarkovic/qemu tags/mips-queue-jul-07-2020
>
> for you to fetch changes up to fa6e7da119b6da4067e757924e165bc737bb1260:
>
>   scripts/performance: Add dissect.py script (2020-07-07 18:32:20 +0200)
>
> ----------------------------------------------------------------
>
> MIPS + TCG Continuous Benchmarking queue for July 7th, 2020
>
>   Highlights:
>
>      - Fix for a regression in FPU emulation add.s.
>      - Add Loongson 2F disassembler.
>      - Add a script for a GSoC project.
>
>   Note:
>
>      - A checkpatch error and a checkpatch warning are known and
>      should be ignored.
>
> ----------------------------------------------------------------
>
> Ahmed Karaman (1):
>   scripts/performance: Add dissect.py script
>
> Alex Richardson (1):
>   target/mips: fpu: Fix recent regression in add.s after 1ace099f2a
>
> Stefan Brankovic (1):
>   disas: mips: Add Loongson 2F disassembler
>
>  configure                      |    1 +
>  disas/loongson2f.h             | 2562 +++++++++++++
>  include/disas/dis-asm.h        |    1 +
>  include/exec/poison.h          |    1 +
>  target/mips/cpu.c              |    6 +
>  target/mips/fpu_helper.c       |    2 +-
>  MAINTAINERS                    |    1 +
>  disas/Makefile.objs            |    1 +
>  disas/loongson2f.cpp           | 8154 ++++++++++++++++++++++++++++++++++++++++

Honestly, no. Peter, please don't merge this pull request.

That disassembler source code is really huge, and I think someone should
give this a *proper* review first before we include this in our repo. I
just had a quick look at it, and I don't think that it is in the right
shape already. For example, there are hard-coded magic numbers there, like:

bool ADD::disas_output(disassemble_info *info)
+{
+    char alias1[5];
+    char alias2[5];
+    char alias3[5];
...

and in a completely different function, this hard-coded 5 is used again:

+void Instruction32::getAlias(char *buffer, int regNo)
+{
+    switch (regNo) {
+    case 0:
+        strncpy(buffer, "zero", 5);
+        break;
+    case 1:
+        strncpy(buffer, "at", 5);
...

That definitely needs to be turned into a proper #define or the length
needs to be passed as parameter to the function.

Also the coding style is weird in a couple of places, and there were
checkpatch warnings.

Apart from that, Paolo asked you to take a break from MIPS
maintainership for a while, Aleksandar. I strongly support that
suggestion. Your derogatory behavior during the last weeks, especially
in the last one, looked completely unacceptable to me. In my opinion you
really need some time to reflect yourself. You, and we all as a
community, now cannot continue just like nothing happened.

 Thanks,
  Thomas