From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 22682 invoked by alias); 11 Jul 2014 13:51:19 -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 22668 invoked by uid 89); 11 Jul 2014 13:51:18 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.7 required=5.0 tests=AWL,BAYES_00 autolearn=ham version=3.3.2 X-HELO: rock.gnat.com Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Fri, 11 Jul 2014 13:51:16 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 10693116208; Fri, 11 Jul 2014 09:51:15 -0400 (EDT) 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 P8tYdLAEEKXh; Fri, 11 Jul 2014 09:51:15 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id CE42711616B; Fri, 11 Jul 2014 09:51:14 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id 280D940EBF; Fri, 11 Jul 2014 06:51:14 -0700 (PDT) Date: Fri, 11 Jul 2014 14:05:00 -0000 From: Joel Brobecker To: Ajit Kumar Agarwal Cc: "gdb-patches@sourceware.org" , Michael Eager , Pedro Alves , Vinod Kathail , Vidhumouli Hunsigida , Nagaraju Mekala Subject: Re: [Patch, microblaze]: Add support of microblaze software single stepping Message-ID: <20140711135114.GA4888@adacore.com> References: <20140707145634.GF6038@adacore.com> <35bccb81-cae6-4581-b5fd-16dea7171d28@BN1AFFO11FD012.protection.gbl> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <35bccb81-cae6-4581-b5fd-16dea7171d28@BN1AFFO11FD012.protection.gbl> User-Agent: Mutt/1.5.21 (2010-09-15) X-SW-Source: 2014-07/txt/msg00270.txt.bz2 > Based on the feedback review comments are incorporated. > ChangeLog: > 2014-07-11 Ajit Agarwal > > * microblaze-tdep.c (microblaze_software_single_step): New. > (microblaze_gdbarch_init): Use of set_gdbarch_software_single_step. Thank you. Some additional trivial coments below. Please also explain how this patch was tested. > +/* This function handles software single step, branches with delay slot > + imm instruction in microblaze. */ > +static int > +microblaze_software_single_step (struct frame_info *frame) Two comments, in this case: My first comment is that the GDB coding style requires a space after the comment describing the function, before the function itself starts. The second is that functions meant to be installed as gdbarch hooks should be documented as such. The theory is that the purpose of these functions, as well as their API, is documented in the gdbarch code, and thus does not need to be repeated here. So, the typical function description for these functions is: /* Implement the software_single_step gdbarch method. */ If you feel that the extra information you had above is useful, just add it after the introductory comment, like so, for instance: /* Implement the software_single_step gdbarch method. This function handles software single step, branches with delay slot imm instruction in microblaze. */ > + /* Set a breakpoint at the next instruction. If the current > + instruction is an imm, set it at the inst after. If the > + instruction has a delay slot, skip the delay slot. */ This comment could also be merged with the comment above. Or, if you prefer, the extra info from the function documentation could also be merged with this comment. Formatting note: 2 spaces after periods. > + pc = get_frame_pc (frame); > + > + insn = microblaze_fetch_instruction (pc); > + > + minstr = get_insn_microblaze (insn, > + &isunsignednum, > + &insn_type, > + &delay_slots); I will leave it to you to decide, but we typically do not write function calls with one argument per line, unless perhaps when the arguments are function calls, where having one per line would help the reader find which arguments belong to which function call. Particularly in the case below.... > + minstr = microblaze_decode_insn (insn, > + &rd, > + &ra, > + &rb, > + &imm); ... joining all arguments on the same line would save you a lot of screen real estate. > + if (insn_type != return_inst) > + breaks[0] = pc + delay_slots * INST_WORD_SIZE + INST_WORD_SIZE; > + Trailing spaces here. > + bfd_boolean targetvalid; > + bfd_boolean unconditionalbranch; > + And here. > + if (lrb >= 0 && lrb < MICROBLAZE_NUM_REGS) > + rb = get_frame_register_unsigned (frame, lrb); > + else > + rb = 0; > + And here. > + address = microblaze_get_target_address (insn, > + immfound, > + imm, > + pc, > + ra, > + rb, > + &targetvalid, > + &unconditionalbranch); > + > + if (!unconditionalbranch) > + breaks[1] = address; > + } > + And there (3 lines have trailing spaces) Thank you, -- Joel