From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 38485 invoked by alias); 23 Sep 2015 12:37:15 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 38467 invoked by uid 89); 23 Sep 2015 12:37:14 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.6 required=5.0 tests=AWL,BAYES_00,SPF_PASS autolearn=ham version=3.3.2 X-HELO: usevmg21.ericsson.net Received: from usevmg21.ericsson.net (HELO usevmg21.ericsson.net) (198.24.6.65) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Wed, 23 Sep 2015 12:37:12 +0000 Received: from EUSAAHC002.ericsson.se (Unknown_Domain [147.117.188.78]) by usevmg21.ericsson.net (Symantec Mail Security) with SMTP id 62.E7.26730.FA132065; Wed, 23 Sep 2015 06:59:27 +0200 (CEST) Received: from [142.133.110.95] (147.117.188.8) by smtp-am.internal.ericsson.com (147.117.188.80) with Microsoft SMTP Server id 14.3.248.2; Wed, 23 Sep 2015 08:37:08 -0400 Subject: Re: [PATCH 1/5] Support multiple breakpoint types per target in GDBServer. To: Yao Qi References: <1442577749-6650-1-git-send-email-antoine.tremblay@ericsson.com> <1442577749-6650-2-git-send-email-antoine.tremblay@ericsson.com> <86oagt9znr.fsf@gmail.com> CC: From: Antoine Tremblay Message-ID: <56029CF3.8000001@ericsson.com> Date: Wed, 23 Sep 2015 12:37:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.2.0 MIME-Version: 1.0 In-Reply-To: <86oagt9znr.fsf@gmail.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2015-09/txt/msg00546.txt.bz2 On 09/23/2015 06:51 AM, Yao Qi wrote: > Antoine Tremblay writes: > >> +static const unsigned char * >> +aarch64_breakpoint_from_pc (CORE_ADDR *pcptr, int *len) > > Please add comment like this to all these $ARCH_breakpoint_from_pc functions. > > /* Implementation of linux_target_ops method "breakpoint_from_pc". */ > Done >> diff --git a/gdb/gdbserver/linux-cris-low.c b/gdb/gdbserver/linux-cris-low.c >> index e0bfa1a..da5876d 100644 >> --- a/gdb/gdbserver/linux-cris-low.c >> +++ b/gdb/gdbserver/linux-cris-low.c >> @@ -62,7 +62,7 @@ cris_cannot_fetch_register (int regno) >> extern int debug_threads; >> >> static CORE_ADDR >> -cris_get_pc (struct regcache *regcache, void) >> +cris_get_pc (struct regcache *regcache) >> { >> unsigned long pc; >> collect_register_by_name (regcache, "pc", &pc); > > This is a unrelated change. Please move it out this patch, and submit > it separately. > Done >> diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c >> index f5b64ab..bb08761 100644 >> --- a/gdb/gdbserver/linux-low.c >> +++ b/gdb/gdbserver/linux-low.c >> @@ -3012,7 +3012,11 @@ linux_wait_1 (ptid_t ptid, >> if (!ptid_equal (step_over_bkpt, null_ptid) >> && event_child->stop_reason == TARGET_STOPPED_BY_SW_BREAKPOINT) >> { >> - unsigned int increment_pc = the_low_target.breakpoint_len; >> + int increment_pc = 0; >> + CORE_ADDR stop_pc = event_child->stop_pc; >> + >> + (*the_low_target.breakpoint_from_pc) >> + (&stop_pc, &increment_pc); >> > > There was a problem here and your patch doesn't fix it. I want to raise > it here first. It is incorrect to get increment_pc by > the_low_target.breakpoint_len or (*the_low_target.breakpoint_from_pc) > for arm/thumb, because given the stop_pc, we can't tell the breakpoint > size (2-byte or 4-byte).We need a new target hook, say > breakpoint_from_current_state, and its default implementation is > breakpoint_from_pc. However, its arm implementation checks whether > the program is in thumb mode by CPSR and return the right breakpoint size. > > IIUC, the code here is used for step-over GDBserver breakpoint, so it is > not used for arm target until we support conditional breakpoint or > tracepoint, but we should fix it before supporting conditional > breakpoint and tracepoint for arm target. > Indeed good point. It's too bad another target hook needs to be there for this but there's no choice I think. I will fix it in my next patchset introducing conditional breakpoints. >> if (debug_threads) >> { >> @@ -6932,6 +6936,17 @@ current_lwp_ptid (void) >> return ptid_of (current_thread); >> } >> >> +const unsigned char * >> +linux_breakpoint_from_pc (CORE_ADDR *pcptr, int *lenptr) >> +{ >> + if (the_low_target.breakpoint_from_pc != NULL) >> + { >> + return (*the_low_target.breakpoint_from_pc) (pcptr, lenptr); >> + } > > "{" and "}" is not needed. Done > >> diff --git a/gdb/gdbserver/linux-low.h b/gdb/gdbserver/linux-low.h >> index f8f6e78..c623150 100644 >> --- a/gdb/gdbserver/linux-low.h >> +++ b/gdb/gdbserver/linux-low.h >> @@ -141,8 +141,13 @@ struct linux_target_ops >> >> CORE_ADDR (*get_pc) (struct regcache *regcache); >> void (*set_pc) (struct regcache *regcache, CORE_ADDR newpc); >> - const unsigned char *breakpoint; >> - int breakpoint_len; >> + >> + /* Return the raw breakpoint for this target based on PC. Note that the PC > > s/PC/*PCPTR/ > >> + can be NULL, the default breakpoint for the target should be returned in > > PC can't be NULL after your patch #2. You can remove the second > sentence in this patch or patch #2. > I think you mean after patch #3 ? But it can still be NULL see in #3 : /* Default if no pc is set to arm breakpoint. */ + if (pcptr == NULL) + { + *lenptr = arm_breakpoint_len; + return (unsigned char *) &arm_breakpoint; + } >> + this case. The PC is ajusted to the real memory location in case a flag was >> + present in the PC. */ >> + const unsigned char *(*breakpoint_from_pc) (CORE_ADDR *pcptr, int *lenptr); >> + > Note also that I removed the win32 changes, they were not needed and broken, I can't test win32 properly anyway.