From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 11189 invoked by alias); 24 Jun 2014 09:13: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 11097 invoked by uid 89); 24 Jun 2014 09:13:55 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,SPF_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Tue, 24 Jun 2014 09:13:52 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s5O9Dl42013388 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Tue, 24 Jun 2014 05:13:47 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s5O9Dil9024826; Tue, 24 Jun 2014 05:13:44 -0400 Message-ID: <53A94147.4050700@redhat.com> Date: Tue, 24 Jun 2014 09:13:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.6.0 MIME-Version: 1.0 To: Ajit Kumar Agarwal CC: "gdb-patches@sourceware.org" , Michael Eager , Vinod Kathail , Vidhumouli Hunsigida , Nagaraju Mekala Subject: Re: [Patch, microblaze]: Fix for remote G Packet message too long error for baremetal. References: <53A023B1.5000105@redhat.com> <859f27cb-8c46-46c1-9625-7287c60f3ae9@BY2FFO11FD007.protection.gbl> <53A1ABF0.9080004@redhat.com> <74281fd5-518a-4d7f-977a-6fa1320f6db9@BY2FFO11FD016.protection.gbl> <53A1B61F.9080803@redhat.com> <736c2e0d-6ff1-40c3-8120-dc6f5d91e6b1@BL2FFO11FD052.protection.gbl> <53A8290A.1050701@redhat.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-SW-Source: 2014-06/txt/msg00838.txt.bz2 Hi, I read this again and found things I should have mentioned before or things I mentioned before but weren't addressed. See below. On 06/23/2014 08:36 PM, Ajit Kumar Agarwal wrote: > diff --git a/gdb/microblaze-tdep.c b/gdb/microblaze-tdep.c > index 14c1b52..c57437c 100644 > --- a/gdb/microblaze-tdep.c > +++ b/gdb/microblaze-tdep.c > @@ -33,13 +33,15 @@ > #include "frame-unwind.h" > #include "dwarf2-frame.h" > #include "osabi.h" > - > +#include "features/microblaze-with-stack-protect.c" > +#include "features/microblaze.c" > #include "gdb_assert.h" A little odd to see the .c files included in the middle of the .h files. All other files seem to include the ".c" files after all the .h files. I wonder which existing file you modelled from? > #include > #include "target-descriptions.h" > #include "opcodes/microblaze-opcm.h" > #include "opcodes/microblaze-dis.h" > #include "microblaze-tdep.h" > +#include "remote.h" > > > /* Instruction macros used for analyzing the prologue. */ > /* This set of instruction macros need to be changed whenever the > @@ -73,7 +75,8 @@ static const char *microblaze_register_names[] = > "rpc", "rmsr", "rear", "resr", "rfsr", "rbtr", > "rpvr0", "rpvr1", "rpvr2", "rpvr3", "rpvr4", "rpvr5", "rpvr6", > "rpvr7", "rpvr8", "rpvr9", "rpvr10", "rpvr11", > - "redr", "rpid", "rzpr", "rtlbx", "rtlbsx", "rtlblo", "rtlbhi" > + "redr", "rpid", "rzpr", "rtlbx", "rtlbsx", "rtlblo", "rtlbhi", > + "rslr", "rshr" > }; > > #define MICROBLAZE_NUM_REGS ARRAY_SIZE (microblaze_register_names) > @@ -663,17 +666,66 @@ microblaze_dwarf2_reg_to_regnum (struct gdbarch *gdbarch, int reg) > gdb_assert (reg < sizeof (dwarf2_to_reg_map)); > return dwarf2_to_reg_map[reg]; > } > +static void Please make you sure there's an empty line between functions. I believe I commented on this before. > +microblaze_register_g_packet_guesses (struct gdbarch *gdbarch) > +{ > + register_remote_g_packet_guess (gdbarch, > + MICROBLAZE_NUM_CORE_REGS, > + tdesc_microblaze); > > + register_remote_g_packet_guess (gdbarch, > + MICROBLAZE_NUM_REGS, > + tdesc_microblaze_with_stack_protect); > +} > static struct gdbarch * Here too. > microblaze_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches) > { > struct gdbarch_tdep *tdep; > struct gdbarch *gdbarch; > + struct tdesc_arch_data *tdesc_data = NULL; > + const struct target_desc *tdesc = info.target_desc; > > /* If there is already a candidate, use it. */ > arches = gdbarch_list_lookup_by_info (arches, &info); > if (arches != NULL) > return arches->gdbarch; > + if (tdesc == NULL) > + tdesc = tdesc_microblaze_with_stack_protect; Shouldn't the default be to _not_ assume stack protect ? > + /* Check any target description for validity. */ > + if (tdesc_has_registers (tdesc)) > + { > + const struct tdesc_feature *feature; > + int valid_p; > + int i; > + > + feature = tdesc_find_feature (tdesc, > + "org.gnu.gdb.microblaze.core"); > + if (feature == NULL) > + return NULL; > + tdesc_data = tdesc_data_alloc (); > + > + valid_p = 1; > + for (i = 0; i < MICROBLAZE_NUM_CORE_REGS; i++) > + valid_p &= tdesc_numbered_register (feature, tdesc_data, i, > + microblaze_register_names[i]); > + feature = tdesc_find_feature (tdesc, > + "org.gnu.gdb.microblaze.stack-protect"); > + > + if (feature != NULL) > + { > + valid_p = 1; > + valid_p &= tdesc_numbered_register (feature, tdesc_data, > + MICROBLAZE_SLR_REGNUM, > + "rslr"); > + valid_p &= tdesc_numbered_register (feature, tdesc_data, > + MICROBLAZE_SHR_REGNUM, > + "rshr"); > + } > + } > + tdep = xcalloc (1, sizeof (struct gdbarch_tdep)); > + gdbarch = gdbarch_alloc (&info, tdep); > + > + microblaze_register_g_packet_guesses (gdbarch); > > /* Allocate space for the new architecture. */ > tdep = XNEW (struct gdbarch_tdep); > @@ -725,7 +777,11 @@ microblaze_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches) > dwarf2_append_unwinders (gdbarch); > frame_unwind_append_unwinder (gdbarch, µblaze_frame_unwind); > frame_base_append_sniffer (gdbarch, dwarf2_frame_base_sniffer); > - > + if (tdesc_data != NULL) > + { > + tdesc_use_registers (gdbarch, tdesc, tdesc_data); > + set_gdbarch_register_type (gdbarch, microblaze_register_type); Hmm, why is this set_gdbarch_register_type call necessary? > + } > return gdbarch; > } > > + /* Offsets to saved registers. */ > + int register_offsets[MICROBLAZE_NUM_REGS]; /* Must match MICROBLAZE_NUM_REGS. */ The "Must match MICROBLAZE_NUM_REGS" comment now looks unnecessary to me. As I mentioned before, please don't forget to document the new target features in the manual. -- Pedro Alves