From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 25029 invoked by alias); 8 Feb 2013 19:09:58 -0000 Received: (qmail 25009 invoked by uid 22791); 8 Feb 2013 19:09:57 -0000 X-SWARE-Spam-Status: No, hits=-1.9 required=5.0 tests=AWL,BAYES_00,RCVD_IN_HOSTKARMA_NO,TW_EG X-Spam-Check-By: sourceware.org Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 08 Feb 2013 19:09:50 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 36A722E845; Fri, 8 Feb 2013 14:09:50 -0500 (EST) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id 3GSJg5Y9WGfA; Fri, 8 Feb 2013 14:09:50 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id CBE1D2E0D4; Fri, 8 Feb 2013 14:09:49 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id 922AFC7030; Fri, 8 Feb 2013 11:09:46 -0800 (PST) Date: Fri, 08 Feb 2013 19:09:00 -0000 From: Joel Brobecker To: Jiong Wang Cc: gdb-patches@sourceware.org, Walter Lee , Tom Tromey Subject: Re: [RFC/TileGX 5/6]show registers in columns Message-ID: <20130208190946.GD17107@adacore.com> References: <50F91633.6000704@tilera.com> <20130118135039.GI3564@adacore.com> <50F97326.3040709@tilera.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <50F97326.3040709@tilera.com> User-Agent: Mutt/1.5.21 (2010-09-15) 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 X-SW-Source: 2013-02/txt/msg00224.txt.bz2 > yes, I think it's caused by my mailer. please see attachment for the > comparision One small request. For files as small as the ones you can, can you avoid packing them. It requires extra work to read them... > >To me the first 2 obvious remarks is that: > > > > - The number of columns should be dynamic, depending on the width > > of the terminal; and if the width is unset, then default to > > 80 characters. We may also have to make a decision if the width > > is unlimited. > > > > - This will need confirmation from the other Global Maintainers > > but I think it would be better if you used the ui-out routines > > to print the registers. > > > >One additional remark is that, with 64 registers, it might be good > >enough to print them in two columns always, and not deal with > >complexity of the terminal width. It even looks like you might be > >able to fit in 3 columns within 80 chars, if you prefer... > > thanks for your good suggestion. > yes, 3 columns still be within 80 chars. and by display in 3 > columns, it is with larger probability that the user do not need to > scroll down and up his mouse to view the results. by 2 columns, the > list is still a bit long, and the information is not that centered. > > So, is this patch OK? IMO, if would be better if you used the ui-out routines to display the data, rather than fprintf_filtered. But I am not a big specialist in this area. Maybe Tom knows better and could help review the printing part. > >>+/* Tilera private register printer. */ > >>+#define MAX_COLUMNS 3 The name of the macro needs to be changed, if the number of columns is going to be static. And The comment also needs to be more specific. > >>+static void > >>+tilegx_print_registers_info (struct gdbarch *gdbarch, struct ui_file *file, > >>+ struct frame_info *frame, > >>+ int regnum, int print_all) The usual "/* Implement the .... gdbarch method. */" function description is missing... It would also be nice to extend the function description to explain why you are providing your own print function, rather than relying on the default one. If someone stumbles on this at a later time, such a comment will help him understand what the motivation was, without needing to reverse-engineer the code. > >>+{ > >>+ int i; > >>+ int j; > >>+ int k; > >>+ gdb_byte buffer[MAX_REGISTER_SIZE]; > >>+ > >>+ if (regnum != -1) > >>+ { > >>+ default_print_registers_info (gdbarch, file, frame, regnum, print_all); > >>+ return; > >>+ } > >>+ > >>+ for (i = 0; i < TILEGX_NUM_EASY_REGS / MAX_COLUMNS + 1; ++i) > >>+ { > >>+ for (j = i; > >>+ j < TILEGX_NUM_EASY_REGS + 1; > >>+ j += TILEGX_NUM_EASY_REGS / MAX_COLUMNS + 1) > >>+ { > >>+ /* Some registers are never available. Skip them and print PC. */ > >>+ if (j > TILEGX_LR_REGNUM) > >>+ j = TILEGX_PC_REGNUM; This looks pretty bad to me. I'd much rather you explicitly excluded the registers you do not want to print. One nit: 2 spaces after a period (in your comment). > >>+ > >>+ if (j > i) > >>+ fprintf_filtered (file, " "); > >>+ > >>+ fputs_filtered (gdbarch_register_name (gdbarch, j), file); > >>+ print_spaces_filtered (5 - strlen (gdbarch_register_name > >>+ (gdbarch, j)), file); > >>+ > >>+ /* Get the data in raw format. */ > >>+ if (! deprecated_frame_register_read (frame, j, buffer)) Please do not add a new usage of a deprecated function. Use get_frame_register_value instead. > >>+ { > >>+ fprintf_filtered (file, " *** no value *** "); > >>+ } Small style issue: With only one statement inside a block, we omit the curly braces. > >>+ else > >>+ { > >>+ fprintf_filtered (file, "0x"); > >>+ for (k = 0; k < tilegx_reg_size; k++) > >>+ { > >>+ int idx; > >>+ if (gdbarch_byte_order (gdbarch) == BFD_ENDIAN_BIG) > >>+ idx = k; > >>+ else > >>+ idx = tilegx_reg_size - 1 - k; > >>+ fprintf_filtered (file, "%02x", (unsigned char) buffer[idx]); I do not think that this is the right way of printing the register value, by doing all the work yourself. I am not completely sure what the best solution would be, though. The other Global Maintainers may have a more definitive answer, but perhaps plongest (value_as_long (register_value)). My suggestion is a little iffy because it assumes for instance that all register values are signed / or unsigned. > >>+ } > >>+ } > >>+ } > >>+ > >>+ fprintf_filtered (file, "\n"); > >>+ } > >>+} > >>+ > >> static struct gdbarch * > >> tilegx_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches) > >> { > >>@@ -1000,6 +1060,9 @@ tilegx_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches) > >> /* Stack grows down. */ > >> set_gdbarch_inner_than (gdbarch, core_addr_lessthan); > >>+ /* Tilera private register printer */ > >>+ set_gdbarch_print_registers_info (gdbarch, tilegx_print_registers_info); > >>+ > >> /* Frame Info. */ > >> set_gdbarch_unwind_sp (gdbarch, tilegx_unwind_sp); > >> set_gdbarch_unwind_pc (gdbarch, tilegx_unwind_pc); -- Joel