----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3207/#review7637 -----------------------------------------------------------
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 (lines 290 - 293) <http://reviews.gem5.org/r/3207/#comment6580> 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. src/base/remote_gdb.hh (line 171) <http://reviews.gem5.org/r/3207/#comment6579> 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. src/base/remote_gdb.hh (line 177) <http://reviews.gem5.org/r/3207/#comment6578> 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. src/base/remote_gdb.hh (line 188) <http://reviews.gem5.org/r/3207/#comment6582> Make this const wrt the object. - Andreas Sandberg On Nov. 23, 2015, 9:09 p.m., Boris Shingarov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3207/ > ----------------------------------------------------------- > > (Updated Nov. 23, 2015, 9:09 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.cc 021524c21cbc > src/base/remote_gdb.hh 021524c21cbc > src/base/remote_gdb.cc 021524c21cbc > src/arch/arm/remote_gdb.hh 021524c21cbc > > 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
