From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 81006 invoked by alias); 16 Oct 2015 18:08:37 -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 80997 invoked by uid 89); 16 Oct 2015 18:08:37 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.7 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; Fri, 16 Oct 2015 18:08:35 +0000 Received: from EUSAAHC007.ericsson.se (Unknown_Domain [147.117.188.93]) by usevmg21.ericsson.net (Symantec Mail Security) with SMTP id 73.07.26730.8A0D0265; Fri, 16 Oct 2015 12:25:45 +0200 (CEST) Received: from [142.133.110.95] (147.117.188.8) by smtp-am.internal.ericsson.com (147.117.188.95) with Microsoft SMTP Server id 14.3.248.2; Fri, 16 Oct 2015 14:08:32 -0400 Subject: Re: [PATCH v2 4/7] Support breakpoint kinds for software breakpoints in GDBServer. To: Pedro Alves , , Yao Qi References: <1444063455-31558-1-git-send-email-antoine.tremblay@ericsson.com> <1444063455-31558-5-git-send-email-antoine.tremblay@ericsson.com> <561FCB85.4020500@redhat.com> <561FEA3A.5020801@ericsson.com> <56212081.5090703@redhat.com> From: Antoine Tremblay Message-ID: <56213D20.8090803@ericsson.com> Date: Fri, 16 Oct 2015 18:08:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: <56212081.5090703@redhat.com> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2015-10/txt/msg00295.txt.bz2 On 10/16/2015 12:06 PM, Pedro Alves wrote: > GDB calls them "placed address" and "requested address": > > struct bp_target_info > { > ... > /* Address at which the breakpoint was placed. This is normally > the same as REQUESTED_ADDRESS, except when adjustment happens in > gdbarch_breakpoint_from_pc. The most common form of adjustment > is stripping an alternate ISA marker from the PC which is used > to determine the type of breakpoint to insert. */ > CORE_ADDR placed_address; > > /* Address at which the breakpoint was requested. */ > CORE_ADDR reqstd_address; > > Nice I had not seen that ! I'll change pcfull for reqstd_address; But leave pc as pc for now, we can change it in a later patch. > I don't see why my comment conflicts with Yao's. But I think we > could simplify the interfaces and entry points, and get rid of the > duplication, like this: > > Replace the breakpoint_from_pc method with a breakpoint_kind_from_pc > method. This adjusts the PC (if necessary) and returns the > breakpoint _kind_ instead of the breakpoint opcode / data. > > enum arm_breakpoint_kinds > { > ARM_BP_KIND_THUMB = 2, > ARM_BP_KIND_THUMB2 = 3, > ARM_BP_KIND_ARM = 4, > }; > > static int > arm_breakpoint_kind_from_pc (CORE_ADDR *pcptr, int len) > { > if (IS_THUMB_ADDR (*pcptr)) > { > gdb_byte buf[2]; > > *pcptr = UNMAKE_THUMB_ADDR (*pcptr); > > /* Check whether we are replacing a thumb2 32-bit instruction. */ > if ((*the_target->read_memory) (*pcptr, buf, 2) == 0) > { > unsigned short inst1 = 0; > > (*the_target->read_memory) (*pcptr, (gdb_byte *) &inst1, 2); > if (thumb_insn_size (inst1) == 4) > return ARM_BP_KIND_THUMB2; > } > > return ARM_BP_KIND_THUMB; > } > else > return ARM_BP_KIND_ARM; > } > > Then the breakpoints functions and structures always work > with the already-adjusted PC, and with a breakpoint-kind. > > for internal breakpoints, we have: > > set_breakpoint_at (breakpoint_kind_from_pc, to find bp kind, > rest the same as today) > set_gdb_breakpoint_1 (same as today) > | > `--> set_breakpoint (address, kind) > | > `-->set_raw_breakpoint_at (address, kind) > | > `--> the_target->insert_point (address, kind) > > Everything thinks in terms of breakpoint kind. Then the only > places that need to know the real breakpoint instruction opcode > and opcode size can query the breakpoint_from_kind target method > you already added. > > About: > >> We could but then we would have to call breakpoint_from_kind in a lot of >> places basically everywhere bp->size is referenced like : >> >> check_mem_read >> check_mem_write >> insert_memory_breakpoint >> remove_memory_breakpoint >> set_raw_breakpoint_at >> validate_inserted_breakpoint >> delete_raw_breakpoint >> uninsert_raw_breakpoint >> reinsert_raw_breakpoint >> find_raw_breakpoint_at > > Minimizing the patch size is less important than making sure the > resulting code is clear > I was thinking about making the code clear. > Sounds like that's manageable with a trivial replace of bp->size > with a call to something like: > > static int > bp_size (struct raw_breakpoint *bp) > { > int size = bp->kind; > > breakpoint_from_kind (&size); > return size; > } > > Likewise for the opcode data: > > static const gdb_byte * > bp_opcode (struct raw_breakpoint *bp) > { > int size = bp->kind; > > return breakpoint_from_kind (&size); > } > > Doesn't seem to me like the end result would be any less clear. > I see what you mean more clearly now. I like the idea to treat only in kinds but I'm not sure about the advantage in terms of clarity. I would say it's debatable like you said in the end result is not any less clear but the current implementation doesn't seem less clear to me either. I do not like the detour we need to make when we do not have a real reason to have a kind, adding a "fake" unique kind and having to add breakpoint_from_kind implementations on all architectures, not just the ones that support software breakpoints. (Regardless of the patch size). Also even if we can call functions to get the size and opcode when needed, it still seems like since those values do not change for the life of the breakpoint and that they can be set only once from a meaningful context it's perfectly acceptable and more clear to set them once and avoid this level of abstraction to access these values. At this point I could do either but to avoid redoing the patch set multiple times, I would like to ask for Yao's opinion and I will work on the consensual option. Regards, Antoine