This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |
Other format: | [Raw text] |
Hi Kevin! Thanks for reviewing; here are my comments. On Thu, 1 Mar 2012 17:18:47 -0700, Kevin Buettner <kevinb@redhat.com> wrote: > On Thu, 01 Mar 2012 10:00:00 +0100 > Thomas Schwinge <thomas@codesourcery.com> 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. > You may want to just keep that TODO comment in your tree or in > some other TODO list on the side. Hmm, I don't agree. I think it's better to have such comments in a central place, instead of each developer having their own set of them in their own files. I do agree that source code comments are not useful for more *general* ``work to be done'', but this is a very local issue, where the comment applies directly to the next line. Anyway, I'm not the one to set the rules here; I've taken these out. > > - 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. Here is my reasoning for moving the check early: for having a valid movi20 instruction at pc (and thus this if-case to match), it is required that four bytes can be read, that is, the words at pc as well as pc + 2 are within the limit. If that is not the case (because the limit is hit), we can't analyze this as a movi20 instruction. Also, no other if-case can then match anyway (if the movi20 one had matched here), and we will drop out of the loop. Does that make sense now? Would a comment clarify this? > 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; > > } This (non-functional) change was my attempt to make the code easier to grasp: first handle the full 32-bit instruction (the part at pc, and the one at pc + 2), then advance over it. I don't feel strongly about this; I backed it out. > > @@ -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.) > > - pc += 2; > > - for (opc = pc + 12; pc < opc; pc += 2) > > + for (; pc < limit_pc; pc += 2) I don't like silently hiding constant values (that nobody knows and/or researched where they're coming from). What I calculate as 2 * 6 was hidden in the value 12 quoted above. Anyway, I added a bit of a comment. > > - 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.) In this case, I can be convinced; jsr is not a 32-bit instruction, so this if-case may well trigger also if only two bytes are within the limit. But, if won't ever matter in practice, as no complying function is going to end with an jsr and nothing after it. It may only be that we have a jsr as the last instruction before we hit limit_pc, and in that case things are a bit dubious anyway. I changed this. > > @@ -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. Same as with the 2 * 6 case above. Also, a lot of other architectures' tdep files that I looked at have similar comments, with ``Magic'' being a very prominent one. We're now clearly improving upon that! > FWIW, I'm not very fond of doing it this way. (I'm assuming ``this'' is the 28 instructions limit.) This is what many other architectures' tdep files are doing, too, and it triggers only as a safeguard if we fail to find any other limit. Also, a similar thing is used in sh_in_function_epilogue_p. > 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. I consider your suggestion to be an additional improvement, but still think that my patch (more specifically here, the 28 instructions limit) remains valid, too. > 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.) As I said, this limit is only a safeguard if everything else fails. Before that, the end of the function will have tried to be determined with the symbol table (find_pc_partial_function), or debug information (skip_prologue_using_sal), which will typically trigger (but not in PLT slots). How's this version? * sh-tdep.c (sh_skip_prologue): Provide an upper limit on the function prologue to sh_analyze_prologue. (sh_analyze_prologue): Make better use of such an upper limit, and generally be more cautious about accessing memory. Index: gdb/sh-tdep.c =================================================================== RCS file: /cvs/src/src/gdb/sh-tdep.c,v retrieving revision 1.239 diff -u -p -r1.239 sh-tdep.c --- gdb/sh-tdep.c 27 Feb 2012 16:40:48 -0000 1.239 +++ gdb/sh-tdep.c 2 Mar 2012 11:12:15 -0000 @@ -534,22 +534,18 @@ sh_breakpoint_from_pc (struct gdbarch *g static CORE_ADDR sh_analyze_prologue (struct gdbarch *gdbarch, - CORE_ADDR pc, CORE_ADDR current_pc, + CORE_ADDR pc, CORE_ADDR limit_pc, struct sh_frame_cache *cache, ULONGEST fpscr) { enum bfd_endian byte_order = gdbarch_byte_order (gdbarch); ULONGEST inst; - CORE_ADDR opc; int offset; int sav_offset = 0; int r3_val = 0; int reg, sav_reg = -1; - if (pc >= current_pc) - return current_pc; - cache->uses_fp = 0; - for (opc = pc + (2 * 28); pc < opc; pc += 2) + for (; pc < limit_pc; pc += 2) { inst = read_memory_unsigned_integer (pc, 2, byte_order); /* See where the registers will be saved to. */ @@ -614,7 +610,8 @@ sh_analyze_prologue (struct gdbarch *gdb } } } - else if (IS_MOVI20 (inst)) + else if (IS_MOVI20 (inst) + && (pc + 2 < limit_pc)) { if (sav_reg < 0) { @@ -656,14 +653,17 @@ sh_analyze_prologue (struct gdbarch *gdb } else if (IS_MOV_SP_FP (inst)) { + pc += 2; + /* Don't go any further than six more instructions. */ + limit_pc = min (limit_pc, pc + (2 * 6)); + 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)) @@ -695,10 +695,13 @@ sh_analyze_prologue (struct gdbarch *gdb jsr, which will be very confusing. Most likely the next instruction is going to be IS_MOV_SP_FP in the delay slot. If so, note that before returning the current pc. */ - inst = read_memory_integer (pc + 2, 2, byte_order); - if (IS_MOV_SP_FP (inst)) - cache->uses_fp = 1; - break; + if (pc + 2 < limit_pc) + { + inst = read_memory_integer (pc + 2, 2, byte_order); + if (IS_MOV_SP_FP (inst)) + cache->uses_fp = 1; + } + break; } #if 0 /* This used to just stop when it found an instruction that was not considered part of the prologue. Now, @@ -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,21 @@ 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) + /* Don't go any further than 28 instructions. */ + limit_pc = pc + (2 * 28); + + /* 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; GrÃÃe, Thomas
Attachment:
pgp00000.pgp
Description: PGP signature
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |