From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 26648 invoked by alias); 28 Sep 2015 09:55:00 -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 26634 invoked by uid 89); 28 Sep 2015 09:54:59 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.2 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-pa0-f51.google.com Received: from mail-pa0-f51.google.com (HELO mail-pa0-f51.google.com) (209.85.220.51) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Mon, 28 Sep 2015 09:54:58 +0000 Received: by pacex6 with SMTP id ex6so170348071pac.0 for ; Mon, 28 Sep 2015 02:54:56 -0700 (PDT) X-Received: by 10.68.193.163 with SMTP id hp3mr25423767pbc.136.1443434096348; Mon, 28 Sep 2015 02:54:56 -0700 (PDT) Received: from E107787-LIN (power-aix.osuosl.org. [140.211.15.154]) by smtp.gmail.com with ESMTPSA id x10sm18417789pas.12.2015.09.28.02.54.54 (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Mon, 28 Sep 2015 02:54:55 -0700 (PDT) From: Yao Qi To: Antoine Tremblay Cc: Subject: Re: [PATCH 2/5] Make breakpoint and breakpoint_len local variables in GDBServer. References: <1442577749-6650-1-git-send-email-antoine.tremblay@ericsson.com> <1442577749-6650-3-git-send-email-antoine.tremblay@ericsson.com> Date: Mon, 28 Sep 2015 09:55:00 -0000 In-Reply-To: <1442577749-6650-3-git-send-email-antoine.tremblay@ericsson.com> (Antoine Tremblay's message of "Fri, 18 Sep 2015 08:02:26 -0400") Message-ID: <867fna98d3.fsf@gmail.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2015-09/txt/msg00597.txt.bz2 Antoine Tremblay writes: > @@ -774,8 +800,9 @@ set_breakpoint_at (CORE_ADDR where, int (*handler) (C= ORE_ADDR)) > { > int err_ignored; >=20=20 > + /* default breakpoint_len will be initialized downstream. */ > return set_breakpoint (other_breakpoint, raw_bkpt_type_sw, > - where, breakpoint_len, handler, > + where, 0, handler, > &err_ignored); > } Why do you set breakpoint length to zero? I know you'll set the length downstream properly, but we should compute the breakpoint length here. 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? --=20 Yao (=E9=BD=90=E5=B0=A7)