From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 11565 invoked by alias); 2 Mar 2012 00:19:08 -0000 Received: (qmail 11554 invoked by uid 22791); 2 Mar 2012 00:19:06 -0000 X-SWARE-Spam-Status: No, hits=-6.8 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 02 Mar 2012 00:18:50 +0000 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q220InZS020964 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Thu, 1 Mar 2012 19:18:49 -0500 Received: from mesquite.lan (ovpn-113-100.phx2.redhat.com [10.3.113.100]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id q220ImMn022884 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO) for ; Thu, 1 Mar 2012 19:18:49 -0500 Date: Fri, 02 Mar 2012 00:19:00 -0000 From: Kevin Buettner To: gdb-patches@sourceware.org Subject: Re: [PATCH] [SH] Prologue skipping if there is none Message-ID: <20120301171847.306829ba@mesquite.lan> In-Reply-To: <87d38w4rxr.fsf@schwinge.name> References: <87pqdgciho.fsf@schwinge.name> <20120215075413.1313f7fa@mesquite.lan> <20120215165907.33f2e9a6@mesquite.lan> <8739aad9il.fsf@schwinge.name> <20120216182544.36b41a1b@mesquite.lan> <87mx8da3b9.fsf@schwinge.name> <20120220162029.2082b6a1@mesquite.lan> <87wr7c7aop.fsf@schwinge.name> <20120224144657.36bbd09f@mesquite.lan> <87r4xd528y.fsf@schwinge.name> <87d38w4rxr.fsf@schwinge.name> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-IsSubscribed: yes 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: 2012-03/txt/msg00061.txt.bz2 Hi Thomas, Please see my comments on your patch below... On Thu, 01 Mar 2012 10:00:00 +0100 Thomas Schwinge wrote: > @@ -594,6 +590,7 @@ sh_analyze_prologue (struct gdbarch *gdb > { > sav_reg = reg; > offset = (inst & 0xff) << 1; > + /* TODO: check that this is a valid address. */ > sav_offset = > read_memory_integer ((pc + 4) + offset, 2, byte_order); > } FIXME and TODO comments are generally frowned upon. All too often, they end up hanging about for many years. If you want to check the validity of the address, you should call something like target_read_memory() and examine the return status. You may want to just keep that TODO comment in your tree or in some other TODO list on the side. > @@ -608,13 +605,15 @@ sh_analyze_prologue (struct gdbarch *gdb > { > sav_reg = reg; > offset = (inst & 0xff) << 2; > + /* TODO: check that this is a valid address. */ Ditto. > sav_offset = > read_memory_integer (((pc & 0xfffffffc) + 4) + offset, > 4, byte_order); > } > } > } > - else if (IS_MOVI20 (inst)) > + else if (IS_MOVI20 (inst) > + && (pc + 2 < limit_pc)) For this, my preference would be for the limit check to be moved a bit later on... > { > if (sav_reg < 0) > { > @@ -623,14 +622,15 @@ sh_analyze_prologue (struct gdbarch *gdb > { > sav_reg = reg; > sav_offset = GET_SOURCE_REG (inst) << 16; > - /* MOVI20 is a 32 bit instruction! */ > - pc += 2; I.e. put the test here and break if the limit has been exceeded. Also, is there a good reason for moving the increment further on? I think it reads better to increment first and then fetch from the pc instead of pc+2. > sav_offset > - |= read_memory_unsigned_integer (pc, 2, byte_order); > + |= read_memory_unsigned_integer (pc + 2, 2, byte_order); > /* Now sav_offset contains an unsigned 20 bit value. > It must still get sign extended. */ > if (sav_offset & 0x00080000) > sav_offset |= 0xfff00000; > + > + /* MOVI20 is a 32-bit instruction. */ > + pc += 2; > } > } > } > @@ -656,14 +656,16 @@ sh_analyze_prologue (struct gdbarch *gdb > } > else if (IS_MOV_SP_FP (inst)) > { > + pc += 2; > + limit_pc = min (limit_pc, pc + (2 * 6)); /* NUMERO MYSTERIOSO */ Either get rid of that comment or put something meaningful in its place. (I'd sort of like to see an explanation of how 2*6 was decided upon. If it's historical and you don't know why, then just get rid of the comment.) > + > cache->uses_fp = 1; > /* At this point, only allow argument register moves to other > registers or argument register moves to @(X,fp) which are > moving the register arguments onto the stack area allocated > by a former add somenumber to SP call. Don't allow moving > to an fp indirect address above fp + cache->sp_offset. */ > - pc += 2; > - for (opc = pc + 12; pc < opc; pc += 2) > + for (; pc < limit_pc; pc += 2) > { > inst = read_memory_integer (pc, 2, byte_order); > if (IS_MOV_ARG_TO_IND_R14 (inst)) > @@ -686,7 +688,8 @@ sh_analyze_prologue (struct gdbarch *gdb > } > break; > } > - else if (IS_JSR (inst)) > + else if (IS_JSR (inst) > + && (pc + 2 < limit_pc)) Again, I'd like to see that limit check moved further on. The reason for this is that I'd like to have a case for the JSR instruction. I'd rather have the details regarding what might happen if some limit is exceeded to be contained in the code implementing that case. (This is just my preference. Some other maintainer may disagree with me.) > { > /* We have found a jsr that has been scheduled into the prologue. > If we continue the scan and return a pc someplace after this, > @@ -716,13 +719,13 @@ sh_analyze_prologue (struct gdbarch *gdb > static CORE_ADDR > sh_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc) > { > - CORE_ADDR post_prologue_pc, func_addr; > + CORE_ADDR post_prologue_pc, func_addr, func_end_addr, limit_pc; > struct sh_frame_cache cache; > > /* See if we can determine the end of the prologue via the symbol table. > If so, then return either PC, or the PC after the prologue, whichever > is greater. */ > - if (find_pc_partial_function (pc, NULL, &func_addr, NULL)) > + if (find_pc_partial_function (pc, NULL, &func_addr, &func_end_addr)) > { > post_prologue_pc = skip_prologue_using_sal (gdbarch, func_addr); > if (post_prologue_pc != 0) > @@ -732,8 +735,20 @@ sh_skip_prologue (struct gdbarch *gdbarc > /* Can't determine prologue from the symbol table, need to examine > instructions. */ > > + /* Find an upper limit on the function prologue using the debug > + information. If the debug information could not be used to provide > + that bound, then use an arbitrary large number as the upper bound. */ > + limit_pc = skip_prologue_using_sal (gdbarch, pc); > + if (limit_pc == 0) > + limit_pc = pc + (2 * 28); /* NUMERO MYSTERIOSO */ I assume this is the limit that had been used before? If so, just say so in a comment - if you like, you can also state that you don't know how this number was derived. FWIW, I'm not very fond of doing it this way. I think it'd be preferable to make the prologue analyzer bail when it hits a branch, call, or return instruction. It shouldn't be too hard to encode these cases in the prologue analyzer. We do need some limit though. I'm just concerned about debugging leaf functions where that limit will put us into the next function. (This was one of the problems with my earlier patch - it didn't handle that case.) > + > + /* Do not allow limit_pc to be past the function end, if we know > + where that end is... */ > + if (func_end_addr != 0) > + limit_pc = min (limit_pc, func_end_addr); > + > cache.sp_offset = -4; > - post_prologue_pc = sh_analyze_prologue (gdbarch, pc, (CORE_ADDR) -1, &cache, 0); > + post_prologue_pc = sh_analyze_prologue (gdbarch, pc, limit_pc, &cache, 0); > if (cache.uses_fp) > pc = post_prologue_pc; Kevin