From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 90653 invoked by alias); 28 Sep 2015 10:05:23 -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 88087 invoked by uid 89); 28 Sep 2015 10:05:23 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.0 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_NONE,SPF_SOFTFAIL autolearn=no version=3.3.2 X-HELO: mtaout20.012.net.il Received: from mtaout20.012.net.il (HELO mtaout20.012.net.il) (80.179.55.166) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 28 Sep 2015 10:05:19 +0000 Received: from conversion-daemon.a-mtaout20.012.net.il by a-mtaout20.012.net.il (HyperSendmail v2007.08) id <0NVD00J00TC3DO00@a-mtaout20.012.net.il> for gdb-patches@sourceware.org; Mon, 28 Sep 2015 13:05:16 +0300 (IDT) Received: from HOME-C4E4A596F7 ([84.94.185.246]) by a-mtaout20.012.net.il (HyperSendmail v2007.08) with ESMTPA id <0NVD00J79TCS3H60@a-mtaout20.012.net.il>; Mon, 28 Sep 2015 13:05:16 +0300 (IDT) Date: Mon, 28 Sep 2015 10:05:00 -0000 From: Eli Zaretskii Subject: Re: [PATCH 2/5] Make breakpoint and breakpoint_len local variables in GDBServer. In-reply-to: <867fna98d3.fsf@gmail.com> To: Yao Qi Cc: antoine.tremblay@ericsson.com, gdb-patches@sourceware.org Reply-to: Eli Zaretskii Message-id: <83a8s6eu55.fsf@gnu.org> References: <1442577749-6650-1-git-send-email-antoine.tremblay@ericsson.com> <1442577749-6650-3-git-send-email-antoine.tremblay@ericsson.com> <867fna98d3.fsf@gmail.com> X-IsSubscribed: yes X-SW-Source: 2015-09/txt/msg00598.txt.bz2 > From: Yao Qi > Cc: > Date: Mon, 28 Sep 2015 10:54:48 +0100 > > After thinking about it a bit more, I think this reveals some design > issues in GDBserver brekapoint, nowadays, GDBserver inserts its own > breakpoints and breakpoints requested by GDB. After this patch series, > GDBserver should be able to: > > 1) choose the right breakpoint instruction for its own breakpoints, > according to the breakpoint address, status register flag, etc, > through API set_breakpoint_at, > 2) choose the right breakpoint instruction for breakpoints requested > by GDB, according to the information in Z packets, through API > set_gdb_breakpoint > > there should be two paths for them, and each path needs different target > hook to choose breakpoint instructions. breakpoint_from_pc is needed for > #1, and breakpoint_from_length is needed for #2. In your current patch > set (in patch 4/5), two things are mixed together, which doesn't look > good to me. The current functions calls in GDBserver to create > breakpoint is like > > set_breakpoint_at > set_gdb_breakpoint_1 > | > `--> set_breakpoint > | > `-->set_raw_breakpoint_at > | > `--> the_target->insert_point > > we are handling the breakpoint length at the lowest level, in > insert_memory_breakpoint, and we use breakpoint_from_pc and > breakpoint_from_length together there, which looks unclean. Ideally, we > can move these code handling breakpoint length (breakpoint_from_pc and > breakpoint_from_length) to upper levels, like this, > > set_breakpoint_at (call breakpoint_from_pc) > set_gdb_breakpoint_1 (call breakpoint_from_length) > | > `--> set_breakpoint > | > `-->set_raw_breakpoint_at > | > `--> the_target->insert_point > > all needed information is saved in struct breakpoint or struct > raw_breakpoint, and function set_breakpoint and it callees can use > breakpoint or raw_breakpoint directly. It'll be cleaner in this way, > let me know what do you think? Sometimes only the target layer knows how to choose the length correctly. Are we sure this isn't one of those cases, and can never be?