On Tuesday, July 7, 2020, Paolo Bonzini <pbonz...@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 <th...@redhat.com> ha scritto: > >> On 07/07/2020 18.40, Aleksandar Markovic wrote: >> > The following changes since commit 710fb08fd297d7a92163debce1959f >> ae8f3b6ed7: >> > >> > 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 fa6e7da119b6da4067e757924e165b >> c737bb1260: >> > >> > 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 >> >>
