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. Thanks, Paolo Il mar 7 lug 2020, 22:20 Thomas Huth <th...@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 > >
