Re: [PATCH] disas/nanomips: Convert nanoMIPS disassembler to C

2022-08-01 Thread Peter Maydell 
On Mon, 1 Aug 2022 at 04:18, Vince Del Vecchio
 wrote:
>
> On Fri, 29 Jul 2022 at 10:18, Peter Maydell  wrote:
> > ...
> > Is it possible to break this down into smaller pieces so it isn't one 
> > single enormous 5000 line patch ?
> >
> > I guess partial conversion is likely to run into compilation difficulties 
> > mid-series; if so we could do "disable the disassembler; convert it; 
> > reenable it".
> >
> > The rationale here is code review -- reviewing a single huge patch is 
> > essentially impossible, so it needs to be broken down into coherent smaller 
> > pieces to be reviewable.

> Something like 90% of the patch is purely mechanical transformations of which 
> I've excerpted two examples below.  (There are 3-4 chunks of mechanical 
> transformations, of which I've shown the most complicated type that accounts 
> for ~60% of the changed lines.)  Did you intend to review this by having a 
> human inspect all of these?

If they're mechanical transformations then:
 * the commit message should say what the mechanical transformation is
   (ie quote the sed script or other mechanism used to create the commit)
 * where sensible, different transformations should be in different
   commits
 * the hand-implemented parts should be separate

And yes, review means human inspection. You need to make that
human inspection easy by separating out the stuff that is
"just change std::string to const char *" so that the human can
do a 30-second skim over that patch, and spend the time on
the patch containing the stuff that's more complicated.

-- PMM

RE: [PATCH] disas/nanomips: Convert nanoMIPS disassembler to C

2022-07-31 Thread Vince Del Vecchio via 
On Fri, 29 Jul 2022 at 10:18, Peter Maydell  wrote:
> ...
> Is it possible to break this down into smaller pieces so it isn't one single 
> enormous 5000 line patch ?
> 
> I guess partial conversion is likely to run into compilation difficulties 
> mid-series; if so we could do "disable the disassembler; convert it; reenable 
> it".
> 
> The rationale here is code review -- reviewing a single huge patch is 
> essentially impossible, so it needs to be broken down into coherent smaller 
> pieces to be reviewable.

Hi Peter,

Something like 90% of the patch is purely mechanical transformations of which 
I've excerpted two examples below.  (There are 3-4 chunks of mechanical 
transformations, of which I've shown the most complicated type that accounts 
for ~60% of the changed lines.)  Did you intend to review this by having a 
human inspect all of these?

Would it be feasible instead to provide a sed script to effect the mechanical 
parts of the patch (the reviewer could review the script, apply it themselves 
to verify equivalence if desired, and spot check the result) together with a 
(much smaller) non-mechanical diff?

-Vince


@@ -2004,17 +1740,17 @@ std::string NMD::ADD_S(uint64 instruction)
  * rt -
  *  rs -
  */
-std::string NMD::ADDIU_32_(uint64 instruction)
+static char *ADDIU_32_(uint64 instruction)
 {
 uint64 rt_value = extract_rt_25_24_23_22_21(instruction);
 uint64 rs_value = extract_rs_20_19_18_17_16(instruction);
 uint64 u_value = extract_u_15_to_0(instruction);
 
-std::string rt = GPR(copy(rt_value));
-std::string rs = GPR(copy(rs_value));
-std::string u = IMMEDIATE(copy(u_value));
+const char *rt = GPR(copy_ui(rt_value));
+const char *rs = GPR(copy_ui(rs_value));
+const char *u = IMMEDIATE_UI(copy_ui(u_value));
 
-return img::format("ADDIU %s, %s, %s", rt, rs, u);
+return img_format("ADDIU %s, %s, %s", rt, rs, u);
 }
 
 
@@ -2027,15 +1763,15 @@ std::string NMD::ADDIU_32_(uint64 instruction)
  * rt -
  *  rs -
  */
-std::string NMD::ADDIU_48_(uint64 instruction)
+static char *ADDIU_48_(uint64 instruction)
 {
 uint64 rt_value = extract_rt_41_40_39_38_37(instruction);
 int64 s_value = extract_s__se31_15_to_0_31_to_16(instruction);
 
-std::string rt = GPR(copy(rt_value));
-std::string s = IMMEDIATE(copy(s_value));
+const char *rt = GPR(copy_ui(rt_value));
+const char *s = IMMEDIATE_I(copy_i(s_value));
 
-return img::format("ADDIU %s, %s", rt, s);
+return img_format("ADDIU %s, %s", rt, s);
 }

Re: [PATCH] disas/nanomips: Convert nanoMIPS disassembler to C

2022-07-29 Thread Peter Maydell 
On Fri, 29 Jul 2022 at 15:13, Milica Lazarevic
 wrote:
>
> C++ features like class, exception handling and function overloading
> have been removed and replaced with equivalent C code.
>
> Signed-off-by: Milica Lazarevic 
> ---
> Please see the discussion about why converting it here:
> https://lists.nongnu.org/archive/html/qemu-devel/2022-06/msg01803.html
>
> The validity of the disassembler after refactoring has been tested with
> the QEMU emulator version 6.0.1. With the most recent version, there is a
> problem with the executing nanoMIPS programs in the semihosting mode. The
> issue is reported here: https://gitlab.com/qemu-project/qemu/-/issues/1126
> We're currently working on fixing this.
>
>  disas/meson.build  |2 +-
>  disas/{nanomips.cpp => nanomips.c} | 8407 ++--
>  disas/nanomips.h   | 1076 
>  3 files changed, 4154 insertions(+), 5331 deletions(-)
>  rename disas/{nanomips.cpp => nanomips.c} (73%)
>  delete mode 100644 disas/nanomips.h

Is it possible to break this down into smaller pieces so it isn't
one single enormous 5000 line patch ?

I guess partial conversion is likely to run into compilation
difficulties mid-series; if so we could do "disable the
disassembler; convert it; reenable it".

The rationale here is code review -- reviewing a single huge
patch is essentially impossible, so it needs to be broken
down into coherent smaller pieces to be reviewable.

thanks
-- PMM