From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 65233 invoked by alias); 7 Sep 2018 22:03:56 -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 65216 invoked by uid 89); 7 Sep 2018 22:03:55 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.4 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=Sometimes, sk:rs6000-, sk:rs6000 X-HELO: gateway36.websitewelcome.com Received: from gateway36.websitewelcome.com (HELO gateway36.websitewelcome.com) (192.185.186.5) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 07 Sep 2018 22:03:54 +0000 Received: from cm15.websitewelcome.com (cm15.websitewelcome.com [100.42.49.9]) by gateway36.websitewelcome.com (Postfix) with ESMTP id 7800A4023808C for ; Fri, 7 Sep 2018 16:08:34 -0500 (CDT) Received: from box5379.bluehost.com ([162.241.216.53]) by cmsmtp with SMTP id yOqUf1AtzbXuJyOqofKx4V; Fri, 07 Sep 2018 17:03:52 -0500 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=tromey.com; s=default; h=Content-Type:MIME-Version:Message-ID:In-Reply-To:Date: References:Subject:Cc:To:From:Sender:Reply-To:Content-Transfer-Encoding: Content-ID:Content-Description:Resent-Date:Resent-From:Resent-Sender: Resent-To:Resent-Cc:Resent-Message-ID:List-Id:List-Help:List-Unsubscribe: List-Subscribe:List-Post:List-Owner:List-Archive; bh=dNu87NJqootCX1q8n4a1p6ePvu5jTu6+JQrrxGTL9+o=; b=Yv1xrwiD1Al1JQl44GyTxG6O1a /OtURpM+CIZvBeWosUqU0K+6our7Xa6irIFW/Yt5/ikS4r0yzvx03t4dJqigdHCgeqenofAHiGqIk pNUHxmRDKNXCGztZ0uaddwAjX; Received: from 75-166-85-72.hlrn.qwest.net ([75.166.85.72]:51380 helo=pokyo) by box5379.bluehost.com with esmtpsa (TLSv1.2:ECDHE-RSA-AES256-GCM-SHA384:256) (Exim 4.91) (envelope-from ) id 1fyOqT-001t9u-UV; Fri, 07 Sep 2018 17:03:18 -0500 From: Tom Tromey To: John Darrington Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 4/4] GDB: New target s12z References: <20180829141845.26378-1-john@darrington.wattle.id.au> <20180829141845.26378-5-john@darrington.wattle.id.au> Date: Fri, 07 Sep 2018 22:03:00 -0000 In-Reply-To: <20180829141845.26378-5-john@darrington.wattle.id.au> (John Darrington's message of "Wed, 29 Aug 2018 16:18:45 +0200") Message-ID: <874lf1vuca.fsf@tromey.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1.50 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-SW-Source: 2018-09/txt/msg00131.txt.bz2 >>>>> "John" == John Darrington writes: Thank you for the patch. It seems generally good. I have a few nits, but nothing serious. John> diff --git a/gdb/Makefile.in b/gdb/Makefile.in John> index 118c3c8062..f448d1ee19 100644 John> --- a/gdb/Makefile.in John> +++ b/gdb/Makefile.in John> @@ -758,6 +758,7 @@ ALL_TARGET_OBS = \ John> rs6000-lynx178-tdep.o \ John> rs6000-tdep.o \ John> rx-tdep.o \ John> + s12z-tdep.o \ This change should be mentioned in the ChangeLog. John> + John> +#define N_PHYSICAL_REGISTERS (S12Z_N_REGISTERS - 2) John> + John> +static const int reg_perm[N_PHYSICAL_REGISTERS] = John> + { New functions, comments, and macros should have an explanatory comment. Sometimes this can be just a reference to some generic thing, like implementations of gdbarch methods can refer to gdbarch.h. There are a number of cases of this in the file. John> + /* registers is declared in opcodes/s12z.h */ GNU comment style is to start with a capital letter and end with a period and two spaces. John> + if (0 != prologue_end) John> + { John> + struct symtab_and_line prologue_sal = find_pc_line (start_pc, 0); John> + struct compunit_symtab *compunit John> + = SYMTAB_COMPUNIT (prologue_sal.symtab); John> + const char *debug_format = COMPUNIT_DEBUGFORMAT (compunit); John> + John> + if ((NULL != debug_format) John> + && (strlen ("dwarf") <= strlen (debug_format)) John> + && (0 == strncasecmp ("dwarf", debug_format, strlen ("dwarf")))) John> + return (prologue_end > pc) ? prologue_end : pc; John> + } Is this stuff useful? I would think that because s12z_frame_unwind is added after the DWARF unwinders that this would perhaps just be dead code. I do not know for sure. I see this in or1k-tdep.c. gdb is kind of lenient about some targets, but it seems like there should be a better way. John> + /* JPB: 28-Apr-11. This is a temporary patch, to get round GDB John> + crashing right at the beginning. Build the frame ID as best we John> + can. */ John> + trad_frame_set_id (info, frame_id_build (this_sp, this_pc)); Could you explain this more? I wonder if there is something we could change. John> +struct gdbarch_tdep John> +{ John> +}; I wonder if it could simply be eliminated. Not super important to me. John> + int i; John> + for (i = 0; i < stop_1 - len; ++i) We've been preferring "for (int i = ...". John> + const int numregs = gdbarch_num_regs (gdbarch) John> + + gdbarch_num_pseudo_regs (gdbarch); Parens around the right hand side when line-breaking is the GNU style. John> + int reg; John> + for (reg = 0; reg < numregs; reg++) for (int reg ... Tom