This is the mail archive of the gdb-patches@sources.redhat.com 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]

Re: The gdb x86 function prologue parser


Hi Jason,

I don't know much about x86.  I also didn't spend a lot of time looking
at this patch or its context - I hope that Mark will be able to do that
in more detail.  But a couple of things...

On Tue, Jun 07, 2005 at 10:51:36PM -0700, Jason Molenda wrote:
> 2. i386_frame_setup_skip_insns table expansion.  Because you can't  
> skip over an unknown instruction on x86 without knowing its length,  
> this was of paramount importance.  Initially I waited for users to  

I looked at your table.

(A) You've added jump instructions to it.  Assuming that I'm following
what you're doing with this table correctly, I'm not real comfortable
with that without special cases checking the targets of the jumps.

(B) Can't you do this using the opcodes library?  With only three
instructions and their alternate encodings in the table, the table made
sense.  With dozens, it doesn't seem to any more.  Disassemble the
blasted thing and see if it's a push that way.

> this list.  I also made a little testsuite generator where the input  
> looks like
> 
> # SOURCE: RedHat FedoraCore2 /lib/ld-2.3.3.so _dl_reloc_bad_type
> # PROLOGUE
>   push %ebp
>   shl $0x5, %ecx # [ 0xc1 0xe1 0x05 ]
>   mov %esp, %ebp
>   sub $0x8, %esp
> # EPILOGUE
>   add $0x8, %esp
>   pop %ebp
>   ret
> 
> and a script that transforms the patterns into a test program and a  
> Dejagnu expect script.  So you can ensure that you don't regress the  
> prologue parser.  This was the lesson we learned in writing our PPC  
> parser -- we have this wonderfully ornate parser with lots of  
> exceptions and known tricks, but no testsuite for it.  So whenever we  
> change it we're cringing because the gdb testsuite has nothing useful  
> in it.  (you need optimized, no debug info test cases to be sure it's  
> still working right).  The testsuite stuff isn't included in this  
> patch, but I'll put that together soon and send it along if anyone's  
> interested.

Interested?  Hell yes.  And the scripts, too.  This could be seriously
useful.

> 5. Huge i386_frame_cache() changes.  There's no way around it, this  
> function is just not right.  It doesn't handle frameless functions  
> correctly at all.  It's written without a clear understanding of the  
> different classes of functions it needs to handle and works primarily  
> by luck.  And for goodness sakes, if we can't figure out anything  
> about a function that's not at the top of the stack, don't you think  
> it'd be reasonable to assume that the function has set up a stack  
> frame and saved the caller's EBP?  Sure seems like a reasonable  
> assumption to me.  Why can't this function do something even that  
> basic?  This function really cheesed my mellow.

Because there is a GDB policy to determine information about the frame
based on the current frame, not based on where it lies on the stack.
I've experimented with this before; this change can have some weird
consequences... for instance, in any case where we can backtrace
through "foo" only because of the addition of this case, we won't be
able to backtrace through "foo" if it is on top of the stack.

You can find more information about this in the list archives, in
plenty of places; most recently Mark pulled together an implementation
of "set i386 trust-frame-pointer".

That said, it may still be better than nothing.  I am still undecided.

> +     If potentially_frameless == 0, there's no way the function we're
> +     examining is frameless; it has a stack frame set up with 
> +     the saved-EBP/saved-EIP at the standard locations.  
> +     (not entirely true -- if gcc's -fomit-frame-pointer is used you can
> +     have a function that doesn't ever set up the EBP, but calls other
> +     functions.  Handling that situation correctly is not easy with
> +     i386-tdep.c's frame code as it stands today.)  */
> +
> +  potentially_frameless = frame_relative_level (next_frame) == -1 
> +                       || get_frame_type (next_frame) == SIGTRAMP_FRAME;

You want != NORMAL_FRAME.  You've ignored the dummy frame case.

> +      /* We found a function-start address, 
> +         or $pc is at 0x0 (someone jmp'ed thru NULL ptr).  */
> +  if ((cache->pc != 0 || current_pc == 0)

No way that's right.  A jump through 0x0 is no different from a jump
through any other unmapped, non-code address.  Normally one uses a
different frame unwinder for that case.

-- 
Daniel Jacobowitz
CodeSourcery, LLC


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]