> On Nov. 10, 2015, 7:16 p.m., Andreas Sandberg wrote: > > src/arch/arm/remote_gdb.hh, lines 62-69 > > <http://reviews.gem5.org/r/3203/diff/1/?file=51490#file51490line62> > > > > These new offsets seem correct, but I don't like the fact that you > > renamed them and replaced the enum. > > > > Could you keep the old names and enum structure? That makes the code > > symmetrical with respect to the aarch64 and it would reduce the size of the > > changeset (and that makes reviewing much easier).
The intent of these offsets is different. The intent of the enum is to count registers, and in that sense those values are ordinal numbers. That design is fine as long as registers are uniformly sized, but in this place, they are not, causing problems across more than one architecture where this is the case. The fundamental issue is that we can't model a sequence of varying-size registers, some sizes not even powers of 2, as a reg32 array or reg64 array. In this case we are lucky, but what if there were a 32-bit register followed by a 64-bit register? How many registers are there before the 64-bit register? I do agree, however, with your "symmetry" argument. I have reverted the names and the enum to how they were, so that improvements to how the offsets are calculated can be made in a separate patch across the board (so that the code continues to be consistent). > On Nov. 10, 2015, 7:16 p.m., Andreas Sandberg wrote: > > src/arch/arm/remote_gdb.hh, lines 83-86 > > <http://reviews.gem5.org/r/3203/diff/1/?file=51490#file51490line83> > > > > Use a GDB32_NUMREGS constant similar to how aarch64 does it. See above. When some registers are strange sizes like 96 bits (and C compilers generally don't even give us something to put into that union of uint8/16/32/64), and others are 32 bits, to count registers and then multiply by the size, does not work. However, in this case we are lucky that we can (although the counting is somewhat perverse, but at least we don't end up with a fractional count -- which could easily happen), so I preserved the original structure. > On Nov. 10, 2015, 7:16 p.m., Andreas Sandberg wrote: > > src/arch/arm/remote_gdb.cc, line 206 > > <http://reviews.gem5.org/r/3203/diff/1/?file=51491#file51491line206> > > > > Why did you move this one into the aarch64 block? I didn't simply move it into the aarch64 block but to after the calculation of the size, so that bytes() will return the correct value. Granted, "size" is preset to a safe (large enough) value as well as the buffer is safely allocated for the largest case, but that is a spaghetti argument. > On Nov. 10, 2015, 7:16 p.m., Andreas Sandberg wrote: > > src/arch/arm/remote_gdb.cc, line 228 > > <http://reviews.gem5.org/r/3203/diff/1/?file=51491#file51491line228> > > > > Keep this in the common code path. See above. > On Nov. 10, 2015, 7:16 p.m., Andreas Sandberg wrote: > > src/arch/arm/remote_gdb.cc, lines 248-250 > > <http://reviews.gem5.org/r/3203/diff/1/?file=51491#file51491line248> > > > > Are FP register transfers currently broken? They are broken. By default, gdb on "arm-linux-gnueabihf" assumes extended precision, transferring 96 bits on the wire. Such a value does not fit in regs32[...] as written. Fixing this will be non-trivial. Even if the compiler were to provide 96-bit scalars to add to the union, counting registers breaks: how many 96-bit regs are sixteen 32-bit GPRs? - Boris ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3203/#review7535 ----------------------------------------------------------- On Nov. 12, 2015, 1:43 p.m., Boris Shingarov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3203/ > ----------------------------------------------------------- > > (Updated Nov. 12, 2015, 1:43 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 11209:83e890c83564 > \-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\- > arm: Remote GDB: register xfer for AArch32 > > This patch stops remote gdb from panicking when connecting to a 32-bit > target. > > We send the response to the g-packet from the buffer and the size in > GdbRegCache.size. Both the buffer is allocated, and the size is set, > at RemoteGDB object instantiation time, which is before we know if we > will be running an AArch32 or AArch64 workload; therefore the longest > possible size is assumed. The problem is that at actual packet > processing time, if we are in AArch32 mode with its smaller register > set, we still answer the whole buffer and gdb panics from seeing the > oversized packet. > To avoid this, I set the 'size' member to the correct value when > processing each g-packet, according to whether we are in 32- or 64-bit > mode. > > Another problem is the order in which register values are transferred. > Ideally, we should serve gdb with a feature descriptor XML document. > For now, I simply put in the default offsets assumed by gdb 7.10 in > the absence of feature descriptors. > Also in the absence of feature descriptors, gdb expects that the CPU > has extended-precision FP registers (96 bits wide). This situation > is beyond the simple design of the reg cache as a union of uint8/... > arrays. In my opinion, it is worth improving the general union-based > design (and possibly implementing feature descriptors) before fixing > FPs in this case; therefore this patch simply transfers zeros to gdb, > and ignores FP values coming from gdb. > > > Diffs > ----- > > src/sim/system.cc 5b9e9bac916d > > Diff: http://reviews.gem5.org/r/3203/diff/ > > > Testing > ------- > > Tested with gdb-7.10 configured with "--target=arm-linux-gnueabihf" > connecting to gem5 running an arm-linux-gnueabihf binary in SE mode. > Tested also with gdb-7.10 configured with "--target=aarch64-linux-gnu" > connecting to gem5 running an aarch64-linux-gnu binary in SE mode to ensure > against AArch64 regressions. > > Note that even with this patch, remote debugging of 32-bit ARM is still not > useful because of problems with inserting breakpoints which I address in a > separate patch. > > > Thanks, > > Boris Shingarov > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
