| Message ID | 00c96d447252bc1333e14e626611f4f5a58f9bf5.1434432813.git.crosthwaite.peter@gmail.com |
|---|---|
| State | New |
| Headers | show |
Am 16.06.2015 um 07:46 schrieb Peter Crosthwaite: > Use the cpu_set_pc helper which will take care of CPUClass retrieval > for us. > > Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com> > --- > gdbstub.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/gdbstub.c b/gdbstub.c > index 75563db..ceb60ac 100644 > --- a/gdbstub.c > +++ b/gdbstub.c > @@ -754,12 +754,9 @@ static void gdb_breakpoint_remove_all(void) > static void gdb_set_cpu_pc(GDBState *s, target_ulong pc) > { > CPUState *cpu = s->c_cpu; > - CPUClass *cc = CPU_GET_CLASS(cpu); > > cpu_synchronize_state(cpu); > - if (cc->set_pc) { > - cc->set_pc(cpu, pc); > - } > + cpu_set_pc(cpu, pc, NULL); I believe this argument will probably go away; otherwise this should've been &error_abort or something instead of NULL. Regards, Andreas > } > > static CPUState *find_cpu(uint32_t thread_id) >
On Mon, Jun 22, 2015 at 10:31 AM, Andreas Färber <afaerber@suse.de> wrote: > Am 16.06.2015 um 07:46 schrieb Peter Crosthwaite: >> Use the cpu_set_pc helper which will take care of CPUClass retrieval >> for us. >> >> Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com> >> --- >> gdbstub.c | 5 +---- >> 1 file changed, 1 insertion(+), 4 deletions(-) >> >> diff --git a/gdbstub.c b/gdbstub.c >> index 75563db..ceb60ac 100644 >> --- a/gdbstub.c >> +++ b/gdbstub.c >> @@ -754,12 +754,9 @@ static void gdb_breakpoint_remove_all(void) >> static void gdb_set_cpu_pc(GDBState *s, target_ulong pc) >> { >> CPUState *cpu = s->c_cpu; >> - CPUClass *cc = CPU_GET_CLASS(cpu); >> >> cpu_synchronize_state(cpu); >> - if (cc->set_pc) { >> - cc->set_pc(cpu, pc); >> - } >> + cpu_set_pc(cpu, pc, NULL); > > I believe this argument will probably go away; otherwise this should've > been &error_abort or something instead of NULL. > I'm not sure. As I don't see what is catching the case of a gdb 'c' packet for a CPU that doesn't implement set_pc. I'd rather preserve the existing behaviour, and have the qom wrapper do nothing if it is not implemented. Regards, Peter > Regards, > Andreas > >> } >> >> static CPUState *find_cpu(uint32_t thread_id) >> > > > -- > SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB > 21284 (AG Nürnberg) >
On 24 June 2015 at 03:50, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > On Mon, Jun 22, 2015 at 10:31 AM, Andreas Färber <afaerber@suse.de> wrote: >> I believe this argument will probably go away; otherwise this should've >> been &error_abort or something instead of NULL. >> > > I'm not sure. As I don't see what is catching the case of a gdb 'c' > packet for a CPU that doesn't implement set_pc. I'd rather preserve > the existing behaviour, and have the qom wrapper do nothing if it is > not implemented. Well, this is one reason why every CPU needs to implement set_pc... -- PMM
On Wed, Jun 24, 2015 at 3:01 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 24 June 2015 at 03:50, Peter Crosthwaite > <peter.crosthwaite@xilinx.com> wrote: >> On Mon, Jun 22, 2015 at 10:31 AM, Andreas Färber <afaerber@suse.de> wrote: >>> I believe this argument will probably go away; otherwise this should've >>> been &error_abort or something instead of NULL. >>> >> >> I'm not sure. As I don't see what is catching the case of a gdb 'c' >> packet for a CPU that doesn't implement set_pc. I'd rather preserve >> the existing behaviour, and have the qom wrapper do nothing if it is >> not implemented. > > Well, this is one reason why every CPU needs to implement set_pc... > Well. I guess it works for a common case where a continue doesn't change the PC? If the debugger doesn't change the PC the "c" should work even without a set_pc call so we don't want to assert on this valid use case. Regards, Peter > -- PMM >
Am 24.06.2015 um 19:04 schrieb Peter Crosthwaite: > On Wed, Jun 24, 2015 at 3:01 AM, Peter Maydell <peter.maydell@linaro.org> wrote: >> On 24 June 2015 at 03:50, Peter Crosthwaite >> <peter.crosthwaite@xilinx.com> wrote: >>> On Mon, Jun 22, 2015 at 10:31 AM, Andreas Färber <afaerber@suse.de> wrote: >>>> I believe this argument will probably go away; otherwise this should've >>>> been &error_abort or something instead of NULL. >>>> >>> >>> I'm not sure. As I don't see what is catching the case of a gdb 'c' >>> packet for a CPU that doesn't implement set_pc. I'd rather preserve >>> the existing behaviour, and have the qom wrapper do nothing if it is >>> not implemented. >> >> Well, this is one reason why every CPU needs to implement set_pc... >> > > Well. I guess it works for a common case where a continue doesn't > change the PC? If the debugger doesn't change the PC the "c" should > work even without a set_pc call so we don't want to assert on this > valid use case. Guys, is there any target that does not implement set_pc today? If so, which? I'd rather implement it than carry around the iffery and resulting complications. I quickly counted 17 target-* in my tree and all 17 seemed to show up in git-grep. Can you confirm? Didn't check the latest tilegx series. Regards, Andreas
On Wed, Jun 24, 2015 at 10:16 AM, Andreas Färber <afaerber@suse.de> wrote: > Am 24.06.2015 um 19:04 schrieb Peter Crosthwaite: >> On Wed, Jun 24, 2015 at 3:01 AM, Peter Maydell <peter.maydell@linaro.org> wrote: >>> On 24 June 2015 at 03:50, Peter Crosthwaite >>> <peter.crosthwaite@xilinx.com> wrote: >>>> On Mon, Jun 22, 2015 at 10:31 AM, Andreas Färber <afaerber@suse.de> wrote: >>>>> I believe this argument will probably go away; otherwise this should've >>>>> been &error_abort or something instead of NULL. >>>>> >>>> >>>> I'm not sure. As I don't see what is catching the case of a gdb 'c' >>>> packet for a CPU that doesn't implement set_pc. I'd rather preserve >>>> the existing behaviour, and have the qom wrapper do nothing if it is >>>> not implemented. >>> >>> Well, this is one reason why every CPU needs to implement set_pc... >>> >> >> Well. I guess it works for a common case where a continue doesn't >> change the PC? If the debugger doesn't change the PC the "c" should >> work even without a set_pc call so we don't want to assert on this >> valid use case. > > Guys, is there any target that does not implement set_pc today? If so, > which? I'd rather implement it than carry around the iffery and > resulting complications. > > I quickly counted 17 target-* in my tree and all 17 seemed to show up in > git-grep. Can you confirm? Didn't check the latest tilegx series. > There are 17 targets and 18 sets by my count: $ git grep "set_pc.*=.*set_pc" target-* target-alpha/cpu.c: cc->set_pc = alpha_cpu_set_pc; target-arm/cpu.c: cc->set_pc = arm_cpu_set_pc; target-arm/cpu64.c: cc->set_pc = aarch64_cpu_set_pc; target-cris/cpu.c: cc->set_pc = cris_cpu_set_pc; target-i386/cpu.c: cc->set_pc = x86_cpu_set_pc; target-lm32/cpu.c: cc->set_pc = lm32_cpu_set_pc; target-m68k/cpu.c: cc->set_pc = m68k_cpu_set_pc; target-microblaze/cpu.c: cc->set_pc = mb_cpu_set_pc; target-mips/cpu.c: cc->set_pc = mips_cpu_set_pc; target-moxie/cpu.c: cc->set_pc = moxie_cpu_set_pc; target-openrisc/cpu.c: cc->set_pc = openrisc_cpu_set_pc; target-ppc/translate_init.c: cc->set_pc = ppc_cpu_set_pc; target-s390x/cpu.c: cc->set_pc = s390_cpu_set_pc; target-sh4/cpu.c: cc->set_pc = superh_cpu_set_pc; target-sparc/cpu.c: cc->set_pc = sparc_cpu_set_pc; target-tricore/cpu.c: cc->set_pc = tricore_cpu_set_pc; target-unicore32/cpu.c: cc->set_pc = uc32_cpu_set_pc; target-xtensa/cpu.c: cc->set_pc = xtensa_cpu_set_pc; ARM is doubled up. As long as no one else is missing a double up where there is not default set by a single base class we are OK. If there is a missing double up with a base class implementation, then there will be no change by your proposal anyway. Regards, Peter > Regards, > Andreas > > -- > SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB > 21284 (AG Nürnberg) >
On 24 June 2015 at 18:16, Andreas Färber <afaerber@suse.de> wrote: > Guys, is there any target that does not implement set_pc today? If so, > which? I'd rather implement it than carry around the iffery and > resulting complications. No, there are none, see my analysis in my review of patch 1 in this set. -- PMM
diff --git a/gdbstub.c b/gdbstub.c index 75563db..ceb60ac 100644 --- a/gdbstub.c +++ b/gdbstub.c @@ -754,12 +754,9 @@ static void gdb_breakpoint_remove_all(void) static void gdb_set_cpu_pc(GDBState *s, target_ulong pc) { CPUState *cpu = s->c_cpu; - CPUClass *cc = CPU_GET_CLASS(cpu); cpu_synchronize_state(cpu); - if (cc->set_pc) { - cc->set_pc(cpu, pc); - } + cpu_set_pc(cpu, pc, NULL); } static CPUState *find_cpu(uint32_t thread_id)
Use the cpu_set_pc helper which will take care of CPUClass retrieval for us. Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com> --- gdbstub.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)