> On Nov. 20, 2015, 9:32 a.m., Andreas Sandberg wrote: > > 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.
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. > On Nov. 20, 2015, 9:32 a.m., Andreas Sandberg wrote: > > src/base/remote_gdb.hh, line 220 > > <http://reviews.gem5.org/r/3207/diff/2/?file=51938#file51938line220> > > > > 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. 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. - Boris ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3207/#review7617 ----------------------------------------------------------- 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
