----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3203/#review7535 -----------------------------------------------------------
Hi Boris! Thanks for taking time to improve gem5's ARM backend. I have had a look at the patch and it seems like the the offset changes are correct. The issues I have highlighted above (with the exception of the change to FP register transfer) are mostly style related to make everyone's lives easier. I would apprecaite it if you could fix those issues. Also, before the change can be committed, the changeset needs to comply with gem5's commit log style. See http://www.m5sim.org/Submitting_Contributions. In particular, I'd like a more detailed description of what you change and why. src/arch/arm/remote_gdb.hh (lines 62 - 69) <http://reviews.gem5.org/r/3203/#comment6468> 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). src/arch/arm/remote_gdb.hh (lines 83 - 86) <http://reviews.gem5.org/r/3203/#comment6469> Use a GDB32_NUMREGS constant similar to how aarch64 does it. src/arch/arm/remote_gdb.cc (line 206) <http://reviews.gem5.org/r/3203/#comment6467> Why did you move this one into the aarch64 block? src/arch/arm/remote_gdb.cc (line 227) <http://reviews.gem5.org/r/3203/#comment6470> Keep this in the common code path. src/arch/arm/remote_gdb.cc (lines 229 - 231) <http://reviews.gem5.org/r/3203/#comment6471> Are FP register transfers currently broken? - Andreas Sandberg On Nov. 10, 2015, 10:25 a.m., Boris Shingarov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3203/ > ----------------------------------------------------------- > > (Updated Nov. 10, 2015, 10:25 a.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 11209:83e890c83564 > --------------------------- > arm: Remote GDB: register xfer for AArch32 > > > Diffs > ----- > > src/arch/arm/remote_gdb.hh 7a9eeecf2b52 > src/arch/arm/remote_gdb.cc 7a9eeecf2b52 > > 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
