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

[Thomas Huth <thuth@redhat.com>]

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