> On Nov. 23, 2015, 11:26 p.m., Andreas Sandberg wrote: > > src/arch/arm/remote_gdb.cc, lines 348-351 > > <http://reviews.gem5.org/r/3207/diff/3/?file=52044#file52044line348> > > > > 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.
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. > On Nov. 23, 2015, 11:26 p.m., Andreas Sandberg wrote: > > src/base/remote_gdb.hh, line 179 > > <http://reviews.gem5.org/r/3207/diff/3/?file=52045#file52045line179> > > > > 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. 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. > On Nov. 23, 2015, 11:26 p.m., Andreas Sandberg wrote: > > src/base/remote_gdb.hh, line 201 > > <http://reviews.gem5.org/r/3207/diff/3/?file=52045#file52045line201> > > > > Make this const wrt the object. 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. - Boris ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3207/#review7637 ----------------------------------------------------------- On Dec. 3, 2015, 9:53 a.m., Boris Shingarov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3207/ > ----------------------------------------------------------- > > (Updated Dec. 3, 2015, 9:53 a.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 135c16fa409d > src/arch/mips/remote_gdb.hh 135c16fa409d > src/arch/mips/remote_gdb.cc 135c16fa409d > src/arch/power/remote_gdb.hh 135c16fa409d > src/arch/power/remote_gdb.cc 135c16fa409d > src/arch/sparc/remote_gdb.hh 135c16fa409d > src/arch/sparc/remote_gdb.cc 135c16fa409d > src/arch/x86/remote_gdb.hh 135c16fa409d > src/arch/x86/remote_gdb.cc 135c16fa409d > src/base/remote_gdb.hh 135c16fa409d > src/base/remote_gdb.cc 135c16fa409d > src/arch/alpha/kgdb.h 135c16fa409d > src/arch/alpha/remote_gdb.hh 135c16fa409d > src/arch/alpha/remote_gdb.cc 135c16fa409d > src/arch/arm/remote_gdb.hh 135c16fa409d > > 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
