From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 12663 invoked by alias); 12 Aug 2014 23:52:51 -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 12651 invoked by uid 89); 12 Aug 2014 23:52:50 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.2 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_LOW,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-ig0-f201.google.com Received: from mail-ig0-f201.google.com (HELO mail-ig0-f201.google.com) (209.85.213.201) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Tue, 12 Aug 2014 23:52:48 +0000 Received: by mail-ig0-f201.google.com with SMTP id h3so11272igd.0 for ; Tue, 12 Aug 2014 16:52:46 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:mime-version:content-type :content-transfer-encoding:message-id:date:to:cc:subject:in-reply-to :references; bh=UX77CgIRINFZEMRyaAYt7hF7vG7wfbfJ3fyxGoH3bWU=; b=bWLvEHkyBs4rfymg2r6DyHto+yD7YlORSB/8ECe6pKfrPMHBpcIp5r6Kejwru09r+m UJVhaQOoad5w5zAvV4yHEM+ncO3GTUXd+pDPSkRLRybayNSN+wpNXxnrDtloA9DkDR2B LXK4VyBKwMy7xWmobM0yMNpPg8GBL1R53KfO5Z5rlt6YQ6VVgpcA77SqPgPAA4xBWMHx lfpvNAq4vZNOm1AiW1Rvt7+UW0fFgqrNPYdx3LOSZB/Pe3gSkvhFc0ak2U/qES65PQZ+ iBR/tGH+nIYlAIp3/H1WFLYAw4+tQqBvTczlPt98rWDYxIg5JWs+PLcU+19WPZWSewiV iTcQ== X-Gm-Message-State: ALoCoQkRRSeyxLYDi5hf5hbGMT0RqcGiMl0EIKaVKMIXjxDZZefpUDIsCXAkp5QGdBVxRP/V/LTD X-Received: by 10.42.207.5 with SMTP id fw5mr1120576icb.20.1407887566802; Tue, 12 Aug 2014 16:52:46 -0700 (PDT) Received: from corp2gmr1-1.hot.corp.google.com (corp2gmr1-1.hot.corp.google.com [172.24.189.92]) by gmr-mx.google.com with ESMTPS id h42si15812yhj.3.2014.08.12.16.52.46 for (version=TLSv1.1 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Tue, 12 Aug 2014 16:52:46 -0700 (PDT) Received: from ruffy.mtv.corp.google.com (ruffy.mtv.corp.google.com [172.17.128.44]) by corp2gmr1-1.hot.corp.google.com (Postfix) with ESMTP id E1EC831C5C8; Tue, 12 Aug 2014 16:52:45 -0700 (PDT) From: Doug Evans MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit Message-ID: <21482.43213.338533.34714@ruffy.mtv.corp.google.com> Date: Tue, 12 Aug 2014 23:52:00 -0000 To: Gary Benson Cc: gdb-patches@sourceware.org, Pedro Alves Subject: Re: [PATCH 4/4 v6] Introduce common-debug.h In-Reply-To: <1407770255-2589-5-git-send-email-gbenson@redhat.com> References: <1407770255-2589-1-git-send-email-gbenson@redhat.com> <1407770255-2589-5-git-send-email-gbenson@redhat.com> X-IsSubscribed: yes X-SW-Source: 2014-08/txt/msg00203.txt.bz2 Gary Benson writes: > This introduces common-debug.h. This holds the flag debug_hw_points > (which can be set to enable debugging of the hardware breakpoint/ > watchpoint support code) and debug_printf and debug_vprintf, two > functions that the common code can use to print debugging messages. > Clients of the common code are expected to implement debug_vprintf; > a debug_vprintf function is written from scratch for GDB, and > gdbserver's existing debug_printf is repurposed as debug_vprintf. > > common/agent.c is changed to use debug_vprintf rather than > defining the macro DEBUG_AGENT depending on GDBSERVER. > > nat/i386-dregs.c is changed to use the externally-implemented > debug_printf, rather than defining it itself, and a now-unnecessary > declaration of debug_hw_points is removed. > > Various other files are changed to remove declarations of > debug_hw_points. > > gdb/ > 2014-08-11 Tom Tromey > Gary Benson > > * common/common-debug.h: New file. > * common/common-debug.c: Likewise. > * debug.c: Likewise. > * Makefile.in (SFILES): Add common/common-debug.c. > (HFILES_NO_SRCDIR): Add common/common-debug.h. > (COMMON_OBS): Add common-debug.o and debug.o. > (common-debug.o): New rule. > * common/common-defs.h: Include common-debug.h. > * common/agent.c (debug_agent_printf): New function. > (DEBUG_AGENT): Redefine. > * nat/i386-dregs.c (debug_printf): Undefine. > (debug_hw_points): Remove. > * aarch64-linux-nat.c (debug_hw_points): Likewise. > * i386-nat.c (debug_hw_points): Likewise. > > gdb/gdbserver/ > 2014-08-11 Tom Tromey > Gary Benson > > * Makefile.in (SFILES): Add common/common-debug.c. > (OBS): Add common-debug.o. > (common-debug.o): New rule. > * debug.h (debug_printf): Don't declare. > * debug.c (debug_printf): Renamed and rewritten as... > (debug_vprintf): New function. > * server.h (debug_hw_points): Remove. > * server.c (debug_hw_points): Likewise. > * linux-aarch64-low.c (debug_hw_points): Likewise. Hi. I think there's at least still one TODO here. I don't have a strong preference on how it's solved, even keeping the current situation (though it feels less preferable), but I haven't seen it discussed so I want to make sure that the issue has at least been considered. i386-nat.c has this: static void add_show_debug_regs_command (void) { /* A maintenance command to enable printing the internal DRi mirror variables. */ add_setshow_boolean_cmd ("show-debug-regs", class_maintenance, &debug_hw_points, _("\ Set whether to show variables that mirror the x86 debug registers."), _("\ Show whether to show variables that mirror the x86 debug registers."), _("\ Use \"on\" to enable, \"off\" to disable.\n\ If enabled, the debug registers values are shown when GDB inserts\n\ or removes a hardware breakpoint or watchpoint, and when the inferior\n\ triggers a breakpoint or watchpoint."), NULL, NULL, &maintenance_set_cmdlist, &maintenance_show_cmdlist); } and similarly aarch64-linux-nat.c has this: static void add_show_debug_regs_command (void) { /* A maintenance command to enable printing the internal DRi mirror variables. */ add_setshow_boolean_cmd ("show-debug-regs", class_maintenance, &debug_hw_points, _("\ Set whether to show variables that mirror the AArch64 debug registers."), _("\ Show whether to show variables that mirror the AArch64 debug registers."), _("\ Use \"on\" to enable, \"off\" to disable.\n\ If enabled, the debug registers values are shown when GDB inserts\n\ or removes a hardware breakpoint or watchpoint, and when the inferior\n\ triggers a breakpoint or watchpoint."), NULL, NULL, &maintenance_set_cmdlist, &maintenance_show_cmdlist); } If debug_hw_points is now a "common" variable, it seems like non-architecture-specific code should define the parameter to set/show it. OTOH, it is nice that the parameter only get defined if/when it's useful. [I realize being "nat" files these files would never get compiled together in the same binary.] OTOOH, it's a debug parameter, I'm less inclined to rigid cleanliness here. Plus being a "common" parameter means other arches can use it and would be less inclined to unnecessarily invent a different parameter. --- Hmmm, I just noticed something: The aarch64 version also uses add_setshow_boolean_cmd, but there is code like this in aarch64-linux-nat.c: if (debug_hw_points > 1) If we make this a non-arch-specific parameter, we should probably make it an integer parameter. --- btw, it's confusing that the variable is named "debug_hw_points" but the command to set it is "maint set show-debug-regs". Bleah. The intuitive naming is to base the variable name off of the parameter name, but I'm also ok with changing the parameter name. "set debug hw-points " ? I don't have a strong opinion, other than if we're making changes in this area IWBN to clean up the naming while we're at it. Plus "set debug ..." is more consistent with other such parameters than "maint set ...".