----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3207/#review7617 -----------------------------------------------------------
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. src/arch/arm/remote_gdb.cc (lines 289 - 293) <http://reviews.gem5.org/r/3207/#comment6552> This could go into the base class now. src/arch/arm/remote_gdb.cc (line 308) <http://reviews.gem5.org/r/3207/#comment6551> Nit: New line after between parameter list and {. src/base/remote_gdb.hh (line 181) <http://reviews.gem5.org/r/3207/#comment6554> getRegs() src/base/remote_gdb.hh (line 187) <http://reviews.gem5.org/r/3207/#comment6555> setRegs() src/base/remote_gdb.hh (line 192) <http://reviews.gem5.org/r/3207/#comment6553> Consider making this a unique_ptr. src/base/remote_gdb.hh (line 212) <http://reviews.gem5.org/r/3207/#comment6550> 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. - Andreas Sandberg On Nov. 16, 2015, 12:19 p.m., Boris Shingarov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3207/ > ----------------------------------------------------------- > > (Updated Nov. 16, 2015, 12:19 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > 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. > > > Diffs > ----- > > src/arch/arm/remote_gdb.hh 7a9eeecf2b52 > src/arch/arm/remote_gdb.cc 7a9eeecf2b52 > src/base/remote_gdb.hh 7a9eeecf2b52 > src/base/remote_gdb.cc 7a9eeecf2b52 > > Diff: http://reviews.gem5.org/r/3207/diff/ > > > Testing > ------- > > 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. > > > Thanks, > > Boris Shingarov > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
