arm: remote GDB: rationalize structure of register offsets

Information
Submitter:	Boris Shingarov
Repository:	gem5
Branch:
Bugs:
Depends On:
Reviewers
Groups:	Default
People:

Description

arm: remote GDB: rationalize structure of register offsets

Currently, the wire format of register values in g- and G-packets is
modelled using a union of uint8/16/32/64 arrays.  The offset positions
of each register are expressed as a "register count" scaled according
to the width of the register in question.  This results in counter-
intuitive and error-prone "register count arithmetic", and some
formats would even be altogether unrepresentable in such model, e.g.
a 64-bit register following a 32-bit one would have a fractional index
in the regs64 array.
Another difficulty is that the array is allocated before the actual
architecture of the workload is known (and therefore before the correct 
size for the array can be calculated).

With this patch I propose a simpler mechanism for expressing the
register set structure.  In the new code, GdbRegCache is an abstract
class; its subclasses contain straightforward structs reflecting the
register representation.  The determination whether to use e.g. the
AArch32 vs. AArch64 register set (or SPARCv8 vs SPARCv9, etc.) is made 
by polymorphically dispatching getregs() to the concrete subclass.
The subclass is not instantiated until it is needed for actual
g-/G-packet processing, when the mode is already known.

This patch is not meant to be merged in on its own, because it changes
the contract between src/base/remote_gdb.* and src/arch/*/remote_gdb.*,
so as it stands right now, it would break the other architectures.
In this patch only the base and the ARM code are provided for review;
once we agree on the structure, I will provide src/arch/*/remote_gdb.*
for the other architectures; those patches could then be merged in
together.

Testing Done

Tested in SE mode with both AArch32 and AArch64, using gdb 7.10 configured with --target=arm-linux-gnueabihf and --target=aarch64-linux-gnu, as well as with our internal gdb RSP client.

Issue Summary

Description	From	Last Updated	Status
would like to see doxygen documentation for these methods	Ali Saidi	Nov. 16, 2015, 4:19 a.m.	Resolved
This could go into the base class now.	Andreas Sandberg	Nov. 23, 2015, 1:14 p.m.	Resolved
Nit: New line after between parameter list and {.	Andreas Sandberg	Nov. 23, 2015, 1:14 p.m.	Resolved
getRegs()	Andreas Sandberg	Nov. 23, 2015, 1:14 p.m.	Resolved
setRegs()	Andreas Sandberg	Nov. 23, 2015, 1:14 p.m.	Resolved
Consider making this a unique_ptr.	Andreas Sandberg	Nov. 23, 2015, 1:14 p.m.	Resolved
This should probably be gdbRegs() and you probably don't want to make it purely virtual unless you plan to fix ...	Andreas Sandberg	Nov. 23, 2015, 1:14 p.m.	Resolved
Won't this leak memory with the current use cases? (Actually, it seems like it won't work at all since a ...	Andreas Sandberg	Dec. 3, 2015, 1:53 a.m.	Resolved
It might make sense to make this a uint8_t instead. We probably want to manipulate it as a char array ...	Andreas Sandberg	Dec. 3, 2015, 2:03 a.m.	Resolved
I'd prefer if this was called size() instead. That's what I'd expect from most container types. Also, please make it ...	Andreas Sandberg	Dec. 3, 2015, 2:08 a.m.	Resolved
Make this const wrt the object.	Andreas Sandberg	Dec. 3, 2015, 2:08 a.m.	Resolved
I'd just loop over these, but don't worry about it.	Andreas Sandberg	Dec. 11, 2015, 2:11 a.m.	Dropped
Dito.	Andreas Sandberg	Dec. 11, 2015, 2:11 a.m.	Dropped

Boris Shingarov Nov. 16, 2015, 4:21 a.m. (Nov. 16, 2015, 4:21 a.m.)

I've added descriptions for the abstract class and the methods which each architecture should implement.
If the general shape of things look ok to you, I will submit the new implementations for the rest of the platforms.

src/base/remote_gdb.hh (Diff revision 1)

would like to see doxygen documentation for these methods

Show all issues

Overall this seems like a sensible approach to me

Diff:

Revision 2 (+186 -175)

Show changes

	src/arch/arm/remote_gdb.hh
	src/arch/arm/remote_gdb.cc
	src/base/remote_gdb.hh
	src/base/remote_gdb.cc

Looks like a good design to me. I'm currently testing it in aarch32 and it seems to work. There are a couple of nits below, but the code looks good overall.

There is one ARM-specific high-level issues that you will need to consider before committing this. What happens if the CPU switches between aarch32 and aarch64? As far as I can tell, the gdbregs() won't switch modes if this happens.

Feel free to add yourself (or whoever owns the copyright of your work) to the copyright header in the files where you have made significant changes.

Boris Shingarov Nov. 23, 2015, 1:20 p.m. (Nov. 23, 2015, 1:20 p.m.)

I have removed the lazy creation of the GdbRegCache in favor of allocating a new instance on each getRegs/setRegs.  This both allows the use of unique_ptr (because the compiler knows where the scope ends) and also behaves correctly if the CPU changes modes.

src/arch/arm/remote_gdb.cc (Diff revision 2)

This could go into the base class now.

Show all issues

src/arch/arm/remote_gdb.cc (Diff revision 2)

Nit: New line after between parameter list and {.

Show all issues

src/base/remote_gdb.hh (Diff revision 2)

getRegs()

Show all issues

src/base/remote_gdb.hh (Diff revision 2)

setRegs()

Show all issues

src/base/remote_gdb.hh (Diff revision 2)

Consider making this a unique_ptr.

Show all issues

src/base/remote_gdb.hh (Diff revision 2)

This should probably be gdbRegs() and you probably don't want to make it purely virtual unless you plan to fix up all other architectures in the same changeset.
Nit: The * should be next to the function name and not the type to be consistent with the rest of the file.

Show all issues

Boris Shingarov Nov. 23, 2015, 1:20 p.m. (Nov. 23, 2015, 1:20 p.m.)

As I indicated in the last paragraph of the description, the patches for all architectures will need to be pushed together.  (For example, RR3225 is the POWER counterpart).  I am dividing the patch by architecture so that they can be reviewed separately.  Two reasons for this desire, (1) getting feedback on the approach before doing potentially wasteful work on the remaining architectures, (2) reviewers who care about different architectures, may be different people.

Diff:

Revision 3 (+196 -189)

Show changes

	src/arch/arm/remote_gdb.hh
	src/arch/arm/remote_gdb.cc
	src/base/remote_gdb.hh
	src/base/remote_gdb.cc

Thanks for updating the review. I think the overall design is good, but the latest revision introduced a bug in the way the register cache is handled. Again, I'm happy with the overall design, so just nail the last couple of things and this is good to go.

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

Won't this leak memory with the current use cases? (Actually, it seems like it won't work at all since a lot of the calls will now work on different objects)
What I meant in the previous review round by using a unique_ptr here was that you'd store the cache in a unique_ptr in the RemoteGDB object. If the unique_ptr isn't set or the CPU mode has changed, you instantiate a (new) reg cache, then return a naked pointer from the unique_ptr. I'm sorry if I was unclear.
Another option would be to store one aarch64 and one aarch32 reg cache in the ARM implementation of the RemoteGDB class and just return the right pointer/reference here. I imagine you'd do that for ISAs that need just one reg cache implementation.

Show all issues

Boris Shingarov Dec. 3, 2015, 2:12 a.m. (Dec. 3, 2015, 2:12 a.m.)

The way how I normally like to use unique_ptr, is in a much more clear/simple way, best when the scope of the unique_ptr is obvious from it being local in a block.  It was very stupid on my part to not see that the getRegs()/setRegs() scope is too short for the cache object: the correct scope is the trap() function.  Within trap(), the lifespan of the cache object is completely obvious.

src/base/remote_gdb.hh (Diff revision 3)

It might make sense to make this a uint8_t instead. We probably want to manipulate it as a char array most of the time. Also, it might make sense to make a const version of this as well.

Show all issues

Boris Shingarov Dec. 3, 2015, 2:12 a.m. (Dec. 3, 2015, 2:12 a.m.)

We manipulate them in exactly two places -- hex2mem() and mem2hex().  Before this patch, these functions used to take a void*, and that was passed the void *regs.  But inside the tso functions, the pointer was cast to (char*).  Therefore I agree that a char pointer makes more sense.  And while changing the type, I noticed that the digit buffer was void* as well (also cast to (char*)).  There were also a number of casts on char *buffer.

src/base/remote_gdb.hh (Diff revision 3)

I'd prefer if this was called size() instead. That's what I'd expect from most container types.
Also, please make it const wrt the object.

Show all issues

src/base/remote_gdb.hh (Diff revision 3)

Make this const wrt the object.

Show all issues

Boris Shingarov Dec. 3, 2015, 2:12 a.m. (Dec. 3, 2015, 2:12 a.m.)

And also, symmetrically, the argument to getRegs() wants to be const, too.  For that, shouldn't ThreadContext::readIntReg() be const as well?  I don't want this patch (which is already big because of all the architectures now in it) to touch even more files, so I'd make this change a separate RR.

Andreas Sandberg Dec. 3, 2015, 2:52 a.m. (Dec. 3, 2015, 2:52 a.m.)

Good observation. The constness of ThreadContext::readIntReg() et al. is an itch I've wanted to scratch for a while, but it's a very messy change since it touches everything. Let's leave it for now.

Diff:

Revision 4 (+686 -806)

Show changes

	src/arch/alpha/kgdb.h
	src/arch/alpha/remote_gdb.hh
	src/arch/alpha/remote_gdb.cc
	src/arch/arm/remote_gdb.hh
	src/arch/arm/remote_gdb.cc
	src/arch/mips/remote_gdb.hh
	src/arch/mips/remote_gdb.cc
	src/arch/power/remote_gdb.hh
	7 more

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

I'd just loop over these, but don't worry about it.

Show all issues

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

Dito.

Show all issues

Thanks for sorting this out! Remind me to buy you a beer if we ever meet in person! :)

Minor nits above. Don't worry about it unless you need to update for some other reason. Also, try and get an ACK from someone who cares about x86 (e.g., Steve).

Boris Shingarov Dec. 11, 2015, 2:31 a.m. (Dec. 11, 2015, 2:31 a.m.)

Thanks Andreas!
Would you care to push this? I think this review has matured enough to be considered done.

I have been testing this a bit with the hello world program in SE mode for x86. It seems fine to me. Thanks for cleaning this up!

Diff:

Revision 5 (+686 -806)

Show changes

	src/arch/alpha/kgdb.h
	src/arch/alpha/remote_gdb.hh
	src/arch/alpha/remote_gdb.cc
	src/arch/arm/remote_gdb.hh
	src/arch/arm/remote_gdb.cc
	src/arch/mips/remote_gdb.hh
	src/arch/mips/remote_gdb.cc
	src/arch/power/remote_gdb.hh
	7 more

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Issue Summary

Diff:

Diff:

Diff:

Diff:

Status: Closed (submitted)