From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Jacobowitz To: Andrew Cagney Cc: gdb-patches@sources.redhat.com Subject: Re: [rfa] mips heuristic_proc_start fix Date: Thu, 12 Jul 2001 14:19:00 -0000 Message-id: <20010712141905.A29671@nevyn.them.org> References: <20010706112010.A5578@nevyn.them.org> <3B46030B.2010007@cygnus.com> <20010706113232.A6209@nevyn.them.org> <20010706114028.A6366@nevyn.them.org> <3B4D4FC6.6090003@cygnus.com> <20010712121358.A25739@nevyn.them.org> <3B4E1277.8090003@cygnus.com> X-SW-Source: 2001-07/msg00312.html On Thu, Jul 12, 2001 at 05:11:19PM -0400, Andrew Cagney wrote: > > How's this instead? Instead of checking for pc == 0, check for pc < > > instlen. If fence overflows, that's fine, because start_pc will be > > less than fence; or I could explicitly check for that too. > > > I'd do both. I think it is better to spell out the intent of each thing > so that you can introduce a few invariants. Right. > > diff -u -r1.57 mips-tdep.c > > --- mips-tdep.c 2001/07/12 17:34:33 1.57 > > +++ mips-tdep.c 2001/07/12 19:12:20 > > @@ -1497,19 +1497,19 @@ > > int seen_adjsp = 0; > > > > pc = ADDR_BITS_REMOVE (pc); > > - start_pc = pc; > > - fence = start_pc - heuristic_fence_post; > > - if (start_pc == 0) > > + instlen = pc_is_mips16 (pc) ? MIPS16_INSTLEN : MIPS_INSTLEN; > > + > > + if (pc < instlen) > > return 0; > > > Suggest adding: > > gdb_assert ((pc % instlen) == 0); Nope! We just read pc off the stack. If the PC is legitimate in the first place, none of these nasty loops are a problem. My pc was 0x2; that's bad, but not cause for gdb to die. What'd you say to a warning and early return? > Should that also be ``pc <= instlen'' as otherwize: > > pc - instlen - instlen > > can underflow. I suspect it depends on the for loop. The for loop will check before instlen is subtracted the second time, so that's OK. > > + start_pc = pc - instlen; > > + fence = start_pc - heuristic_fence_post; > > > I think this should still have the underflow check as otherwize you're > not quite sure what fence is upto. Sure. > > if (heuristic_fence_post == UINT_MAX > > || fence < VM_MIN_ADDRESS) > > fence = VM_MIN_ADDRESS; > > > > - instlen = pc_is_mips16 (pc) ? MIPS16_INSTLEN : MIPS_INSTLEN; > > - > > > gdb_assert (fence >= VM_MIN_ADDRESS); > gdb_assert (start_pc >= instlen); > > Hmm, what happens if VM_MIN_ADDRESS < instlen. Well, VM_MIN_ADDRESS is just a constant 0x400000 right now (for whatever reason...) so I'm not too concerned about that. I suppose it could change, though. I don't really see the need for these asserts, especially given that if fence < VM_MIN_ADDRESS we set it back to VM_MIN_ADDRESS - or were you suggesting changing that? > > /* search back for previous return */ > > - for (start_pc -= instlen;; start_pc -= instlen) > > + for (;; start_pc -= instlen) > > > Er, if VM_MIN_ADDRESS == 0 (hence fence == 0) then this ain't going to work. Oh, yes, that's true. That's the problem with unsigned math :) Just doesn't behave intuitively. > what a rats nest. Would be better to change the for loop to: > > for (; start_pc > fence; start_pc -= instlen; > > reversing the exit condition? > > Alternativly, should fence be guarenteed to be >= instlen. Let me think about this and try to come up with something cleaner for this function. -- Daniel Jacobowitz Carnegie Mellon University MontaVista Software Debian GNU/Linux Developer