Hi Deyuan,

Thanks for the submission. There are just a couple of minor issues I'd like you to address if you could. Also you posted some ways you tested gdb but you didn't provide any of the output. Would you mind doing that too?

Thanks,
Ali

src/arch/mips/remote_gdb.cc (Diff revision 1)

You don't really need the else, you can just end this here. If the panic is called the rest of the code will never be reached. 

src/arch/mips/remote_gdb.cc (Diff revision 1)

Can you use a constant for 15 here?

src/arch/mips/remote_gdb.cc (Diff revision 1)

Same here.. also the random +19 isn't great

src/arch/mips/remote_gdb.cc (Diff revision 1)

Also in these functions

src/arch/mips/remote_gdb.cc (Diff revision 1)

Do you mean for this to be a single =? If you're just doing please pull out the notTakenBkpt and set it above, then pass notTakenBkpt to the function.

src/arch/mips/remote_gdb.cc (Diff revision 1)

Same.

First, thank you for taking the time to share your work with everybody. It's easy to skip that step, and I'm glad you didn't. It'll be great to have this checked in and to fill in that gap in our MIPS support.

Before then, though, there are some style problems that we'll need to fix, but they should be fairly easy to clean up. I haven't mentioned every single instance where something needs to be fixed since that would be annoying for both of us. Please look to see if a comment I made in one place applies elsewhere and fix those places as well.

src/arch/mips/remote_gdb.hh (Diff revision 3)

Where do these constants come from? If there's a header file we can grab them from that would be nice, or if not then these constants are fine. Since they're not macros they shouldn't be in all caps though. I just checked, and in 2009 somebody put into the style guide that constants should have the same format as regular variables. There are many examples of capitalizing the first letter of each word in the name, though, so I think you should be consistent with the existing code.

src/arch/mips/remote_gdb.hh (Diff revision 3)

Your indentation is off here. Make sure you're using spaces and not tabs. I'm not sure if that's the problem, but just in case.

src/arch/mips/remote_gdb.cc (Diff revision 3)

There are a lot of header files here, and I'm guessing a few aren't necessary. Please prune this down to what you're actually using.

src/arch/mips/remote_gdb.cc (Diff revision 3)

Spaces around the operators. Since you repeat this pattern over and over here, it would be good to use a simple helper function to glue the halves together.

src/arch/mips/remote_gdb.cc (Diff revision 3)

This line is too long.

src/arch/mips/remote_gdb.cc (Diff revision 3)

So is this one.

src/arch/mips/remote_gdb.cc (Diff revision 3)

And this one.

src/arch/mips/remote_gdb.cc (Diff revision 3)

Spaces around operators, and a helper function would be good here too.

src/arch/mips/remote_gdb.cc (Diff revision 3)

Since MIPS has a delay slot as you mention on the next line, lets just say next NPC (or whatever the correct term is) and not need the next line to clarify.

src/arch/mips/remote_gdb.cc (Diff revision 3)

It shouldn't be necessary to override this if you're just using the base class implementation. Is there a reason you need this function?

This is looking pretty good. There are still some minor adjustments to be made, but not too much.

src/arch/mips/remote_gdb.hh (Diff revision 4)

This would be better as / 2. Any compiler worth anything will figure out that a shift will work better, and it's clearer what your intention is.

src/arch/mips/remote_gdb.hh (Diff revision 4)

I think lo and hi should be uint32_t. If int was somehow less than 32 bits wide (weird bug possible) you'd chop off some bits.

Also, the return types should go on the line before the function name, and functions defined inside a class definition are implicitly inline so you don't need to have that on there. It would look more like this:

uint64_t
pack(uint32_t lo, uint32_t hi)
{
    ...
}

src/arch/mips/remote_gdb.cc (Diff revision 4)

You could just return the result of the condition of the if and simplify this a little.

src/arch/mips/remote_gdb.cc (Diff revision 4)

Yeah, that looks better :-)