Re: [m5-dev] Review Request: imported patch ext/arm_gdb.patch
--- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/293/#review440 --- Ship it! - Nathan On 2010-11-08 15:35:16, Ali Saidi wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/293/ --- (Updated 2010-11-08 15:35:16) Review request for Default. Summary --- imported patch ext/arm_gdb.patch Diffs - src/arch/arm/SConscript f61e079ad05e src/arch/arm/remote_gdb.hh f61e079ad05e src/arch/arm/remote_gdb.cc PRE-CREATION src/arch/arm/utility.hh f61e079ad05e src/arch/arm/utility.cc f61e079ad05e Diff: http://reviews.m5sim.org/r/293/diff Testing --- Thanks, Ali ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
Re: [m5-dev] Review Request: imported patch ext/arm_gdb.patch
On 2010-11-09 11:50:33, Gabe Black wrote: Instead of packing/unpacking 32 bit values in 64 bit registers, why don't we template the base class on the type of register it's going to hold. I think the changes to the base class would be minimal, and it would simplify this ARM code nicely with almost no impact on the other ISAs. I sort of agree, but I don't think it's worth the time. The remote gdb code is already a disaster area and putting a bandaid on it doesn't really help much. It was quite a pain to make sure this was correct and test every register, so I'd prefer to leave it this way until some overall restructuring of the remote gdb code occurs. On 2010-11-09 11:50:33, Gabe Black wrote: src/arch/arm/remote_gdb.hh, line 51 http://reviews.m5sim.org/r/293/diff/1/?file=5061#file5061line51 If you're not going to indent the constants above (which seems to be the preferred way to do things) then you should unindent this. I think it's better to be consistent that inconsistently correct. Fixed. On 2010-11-09 11:50:33, Gabe Black wrote: src/arch/arm/remote_gdb.cc, line 165 http://reviews.m5sim.org/r/293/diff/1/?file=5062#file5062line165 This is already happening in the base class constructor. Fixed. - Ali --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/293/#review446 --- On 2010-11-08 15:35:16, Ali Saidi wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/293/ --- (Updated 2010-11-08 15:35:16) Review request for Default. Summary --- imported patch ext/arm_gdb.patch Diffs - src/arch/arm/SConscript f61e079ad05e src/arch/arm/remote_gdb.hh f61e079ad05e src/arch/arm/remote_gdb.cc PRE-CREATION src/arch/arm/utility.hh f61e079ad05e src/arch/arm/utility.cc f61e079ad05e Diff: http://reviews.m5sim.org/r/293/diff Testing --- Thanks, Ali ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
Re: [m5-dev] Review Request: imported patch ext/arm_gdb.patch
On 2010-11-09 11:50:33, Gabe Black wrote: Instead of packing/unpacking 32 bit values in 64 bit registers, why don't we template the base class on the type of register it's going to hold. I think the changes to the base class would be minimal, and it would simplify this ARM code nicely with almost no impact on the other ISAs. Ali Saidi wrote: I sort of agree, but I don't think it's worth the time. The remote gdb code is already a disaster area and putting a bandaid on it doesn't really help much. It was quite a pain to make sure this was correct and test every register, so I'd prefer to leave it this way until some overall restructuring of the remote gdb code occurs. Ok. Maybe we should add a bug to flyspray (and start actually using flyspray), and/or start talking about what needs to happen on m5-dev. I originally split up the remote gdb code from its monolithic Alpha implementation, and it was a big pain since the GDB protocol is a big pain. It's one of those things that just sort of goes off the rails rather than complaining about anything specific. I don't think the split is horrifically bad, but I also don't doubt it can be improved. I think there may be some functional issues too with breakpoints, although maybe that was specific to a particular ISA (Alpha, I think which was concerning) or maybe I wasn't using it right. It would be great to have some testing for this. This and checkpoints are just asking to bit rot since there's no testing and I don't even know how to use checkpointing. - Gabe --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/293/#review446 --- On 2010-11-08 15:35:16, Ali Saidi wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/293/ --- (Updated 2010-11-08 15:35:16) Review request for Default. Summary --- imported patch ext/arm_gdb.patch Diffs - src/arch/arm/SConscript f61e079ad05e src/arch/arm/remote_gdb.hh f61e079ad05e src/arch/arm/remote_gdb.cc PRE-CREATION src/arch/arm/utility.hh f61e079ad05e src/arch/arm/utility.cc f61e079ad05e Diff: http://reviews.m5sim.org/r/293/diff Testing --- Thanks, Ali ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
Re: [m5-dev] Review Request: imported patch ext/arm_gdb.patch
--- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/293/#review437 --- Was the remote_gdb.cc file copied with hg cp Be nice if it were. - Nathan On 2010-11-08 15:35:16, Ali Saidi wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/293/ --- (Updated 2010-11-08 15:35:16) Review request for Default. Summary --- imported patch ext/arm_gdb.patch Diffs - src/arch/arm/SConscript f61e079ad05e src/arch/arm/remote_gdb.hh f61e079ad05e src/arch/arm/remote_gdb.cc PRE-CREATION src/arch/arm/utility.hh f61e079ad05e src/arch/arm/utility.cc f61e079ad05e Diff: http://reviews.m5sim.org/r/293/diff Testing --- Thanks, Ali ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
Re: [m5-dev] Review Request: imported patch ext/arm_gdb.patch
On 2010-11-08 18:00:50, Nathan Binkert wrote: Was the remote_gdb.cc file copied with hg cp Be nice if it were. I'll make sure the committed code does that. Any other issues? - Ali --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/293/#review437 --- On 2010-11-08 15:35:16, Ali Saidi wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/293/ --- (Updated 2010-11-08 15:35:16) Review request for Default. Summary --- imported patch ext/arm_gdb.patch Diffs - src/arch/arm/SConscript f61e079ad05e src/arch/arm/remote_gdb.hh f61e079ad05e src/arch/arm/remote_gdb.cc PRE-CREATION src/arch/arm/utility.hh f61e079ad05e src/arch/arm/utility.cc f61e079ad05e Diff: http://reviews.m5sim.org/r/293/diff Testing --- Thanks, Ali ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
