Re: [PATCH] Improve i386 prologue analyzer

Re: [PATCH] Improve i386 prologue analyzer

[Michael Elizabeth Chastain]

It works for me.  Fixes the bug, no regressions anywhere in the test
suite, eight configurations tested (all my gcc's with -gdwarf-2 and
-gstabs+). 

I also took a little test program and stepi'd it through each
instruction in select() and the stack trace was good after each
instruction.

I think this is ready for the branch.

Michael C

===

Index: ChangeLog
from  Mark Kettenis  <kettenis@gnu.org>

	* i386-tdep.c (i386_analyze_register_saves): Handle register saves
	at the start of a frameless function.  This probably fixes PR
	backtrace/1338.

Re: [PATCH] Improve i386 prologue analyzer

[Mark Kettenis]

   Date: Mon, 18 Aug 2003 05:42:39 -0400
   From: Michael Elizabeth Chastain <mec@shout.net>

   It works for me.  Fixes the bug, no regressions anywhere in the test
   suite, eight configurations tested (all my gcc's with -gdwarf-2 and
   -gstabs+). 

   I also took a little test program and stepi'd it through each
   instruction in select() and the stack trace was good after each
   instruction.

   I think this is ready for the branch.

Checked in there too.  Thanks for testing.  But as soon as GCC starts
intermingling the moves with the pushes in the prologue we'll lose
again.  Ah, such is life :-(.

Mark

Re: [PATCH] Improve i386 prologue analyzer

[Andrew Cagney]

>>>> > And I still don't understand what is the rush to release the MIPS
>>>> > patch without waiting for another week or two and then releasing the
>>>> > i386 patch as well.
>>
>>> 
>>> If I were a MIPS user (hmm, I'm even the maintainer), I'd be pretty 
>>> cheesed off that a fix to get `break main; run' working was being held 
>>> back due the inistance that it be bundled with an unrelated i386 fix.
> 
> 
> Sorry, Andrew, this is just reiterating what I already said I didn't
> understand: if we care so much for the MIPS, why did you refuse to
> wait for it to be fixed in 6.2?

I think I answered this:
> The MIPS breakage wasn't important enough to deny our mainline users a new release of GDB.  Especially when it fixed so many bugs, and especially when the delay could easily be a month..

I've since spoken to some i386 users that benefited from the fixes, they 
are most pleased - threading debugging is way better - dwarf2 back 
traces are much better.  They clearly benefited from pushing forward 
with the 6.2 release.  Only thing is, they are now asking if we can fix 
.... :-)

> And if 6.2 could hit the street with
> MIPS broken, why cannot it stay broken for a week or two more, instead
> of letting the i386 remain broken longer?

And I also think I've covered this:
> However, now that we've got the MIPS patched up, I see no good reason for holding it back.

It's no significant work on my part, and provides us with the 
satisfaction of delivering a better GDB (for MIPS users) sooner.  I'm 
sure the MIPS users would have appreciated the kind gesture.

Andrew

Re: [PATCH] Improve i386 prologue analyzer

[Eli Zaretskii]

> Date: Thu, 12 Aug 2004 08:42:56 -0400
> From: Andrew Cagney <cagney@gnu.org>
> 
> Please, please, articulate the `pain on our part' that we're apparently 
> all suffering.  I'm the release manager, if anyone is is going to 
> experience pain, it's going to be me.

Yes, and I don't think we should dismiss your pain (==loss of time) so
easily.

> > It's also the pain of our users
> > who will need to install two versions within 4 weeks.
> 
> This update is for _MIPS_ users only.  The next update is for _i386_ 
> users only.

What about people (or sysadmins) who need both, e.g. because they
actually make a good use of the multi-arch features and debug several
different CPUs from the same system?

Anyway, those are not the important issues here.

> > And I still don't understand what is the rush to release the MIPS
> > patch without waiting for another week or two and then releasing the
> > i386 patch as well.
> 
> If I were a MIPS user (hmm, I'm even the maintainer), I'd be pretty 
> cheesed off that a fix to get `break main; run' working was being held 
> back due the inistance that it be bundled with an unrelated i386 fix.

Sorry, Andrew, this is just reiterating what I already said I didn't
understand: if we care so much for the MIPS, why did you refuse to
wait for it to be fixed in 6.2?  And if 6.2 could hit the street with
MIPS broken, why cannot it stay broken for a week or two more, instead
of letting the i386 remain broken longer?

I keep asking this same question for about eternity, and you keep not
answering it, for some reason that is beyond me.  It's embarrassing.
If there is some logic that would show why these two decisions do not
contradict each other, please explain that logic.  You don't need my
agreement to go ahead, but I think I deserve to at least know your
reasoning, even if I disagree.

Re: [PATCH] Improve i386 prologue analyzer

[Andrew Cagney]

>>> Date: Wed, 11 Aug 2004 13:13:42 -0400
>>> From: Andrew Cagney <cagney@gnu.org>
>>
>>>> > 
>>>> > We are talking about two weeks, not about months or something.  Why is
>>>> > it worthwhile to go through the pains of another version just so the
>>>> > MIPS patch could be released a week or two earlier than if it were to
>>>> > be part of the same version as the i386 prologue patch?
>>
>>> 
>>> Sorry, I don't follow.  I would have thought that a little bit of pain 
>>> (on our part) would be worth the satisfaction of seeing us deliver a 
>>> better working GDB sooner.
> 
> 
> It's not only pain on our part (which I don't think we should dismiss
> so lightly, btw, but that's just me).

Please, please, articulate the `pain on our part' that we're apparently 
all suffering.  I'm the release manager, if anyone is is going to 
experience pain, it's going to be me.

> It's also the pain of our users
> who will need to install two versions within 4 weeks.

This update is for _MIPS_ users only.  The next update is for _i386_ 
users only.

> And I still don't understand what is the rush to release the MIPS
> patch without waiting for another week or two and then releasing the
> i386 patch as well.

If I were a MIPS user (hmm, I'm even the maintainer), I'd be pretty 
cheesed off that a fix to get `break main; run' working was being held 
back due the inistance that it be bundled with an unrelated i386 fix.

Andrew

Re: [PATCH] Improve i386 prologue analyzer

[Eli Zaretskii]

> Date: Wed, 11 Aug 2004 13:13:42 -0400
> From: Andrew Cagney <cagney@gnu.org>
> > 
> > We are talking about two weeks, not about months or something.  Why is
> > it worthwhile to go through the pains of another version just so the
> > MIPS patch could be released a week or two earlier than if it were to
> > be part of the same version as the i386 prologue patch?
> 
> Sorry, I don't follow.  I would have thought that a little bit of pain 
> (on our part) would be worth the satisfaction of seeing us deliver a 
> better working GDB sooner.

It's not only pain on our part (which I don't think we should dismiss
so lightly, btw, but that's just me).  It's also the pain of our users
who will need to install two versions within 4 weeks.

And I still don't understand what is the rush to release the MIPS
patch without waiting for another week or two and then releasing the
i386 patch as well.

Re: [PATCH] Improve i386 prologue analyzer

[Andrew Cagney]

>>> Date: Tue, 10 Aug 2004 20:02:04 -0400
>>> From: Andrew Cagney <cagney@gnu.org>
>>> 
>>> However, now that we've got the MIPS patched up, I see no good reason 
>>> for holding it back.
> 
> 
> We are talking about two weeks, not about months or something.  Why is
> it worthwhile to go through the pains of another version just so the
> MIPS patch could be released a week or two earlier than if it were to
> be part of the same version as the i386 prologue patch?

Sorry, I don't follow.  I would have thought that a little bit of pain 
(on our part) would be worth the satisfaction of seeing us deliver a 
better working GDB sooner.

Andrew

Re: [PATCH] Improve i386 prologue analyzer

[Eli Zaretskii]

> Date: Tue, 10 Aug 2004 20:02:04 -0400
> From: Andrew Cagney <cagney@gnu.org>
> 
> However, now that we've got the MIPS patched up, I see no good reason 
> for holding it back.

We are talking about two weeks, not about months or something.  Why is
it worthwhile to go through the pains of another version just so the
MIPS patch could be released a week or two earlier than if it were to
be part of the same version as the i386 prologue patch?

Re: [PATCH] Improve i386 prologue analyzer

[Andrew Cagney]

>>> Date: Sun, 08 Aug 2004 18:52:17 -0400
>>> From: Andrew Cagney <cagney@gnu.org>
>>> 
>>> week 1.5: 6.2.1:
>>> Critical MIPS problem fixed.
>>> Fix for long-standing i386 bug known (needs 2 weeks testing)
>>> 
>>> week 4.0: 6.2.2:
>>> Long standing bug on old i386 systems fixed.
>>> 
>>> Does this seem reasonable.
> 
> 
> No.
> 
> Sorry, you didn't answer my question, so I'll try to explain what bugs
> me: it's the logic behind these decisions that I cannot grasp.
> 
> That is, if fixing MIPS is so important, then why did we release GDB
> 6.2 without waiting for the fix?  And if it wasn't important enough
> for 6.2 to wait for it, why can't it wait for a couple of weeks and be
> released in 6.2.1 together with the i386 prologue analyzer fix?

The MIPS breakage wasn't important enough to deny our mainline users a 
new release of GDB.  Especially when it fixed so many bugs, and 
especially when the delay could easily be a month..

However, now that we've got the MIPS patched up, I see no good reason 
for holding it back.

> I need to understand how to reconcile these two contradicting moves in
> order to judge whether I can live with the decision, whatever it is.

Andrew

Re: [PATCH] Improve i386 prologue analyzer

[Eli Zaretskii]

> Date: Sun, 08 Aug 2004 18:52:17 -0400
> From: Andrew Cagney <cagney@gnu.org>
> 
> week 1.5: 6.2.1:
> Critical MIPS problem fixed.
> Fix for long-standing i386 bug known (needs 2 weeks testing)
> 
> week 4.0: 6.2.2:
> Long standing bug on old i386 systems fixed.
> 
> Does this seem reasonable.

No.

Sorry, you didn't answer my question, so I'll try to explain what bugs
me: it's the logic behind these decisions that I cannot grasp.

That is, if fixing MIPS is so important, then why did we release GDB
6.2 without waiting for the fix?  And if it wasn't important enough
for 6.2 to wait for it, why can't it wait for a couple of weeks and be
released in 6.2.1 together with the i386 prologue analyzer fix?

I need to understand how to reconcile these two contradicting moves in
order to judge whether I can live with the decision, whatever it is.

Re: [PATCH] Improve i386 prologue analyzer

[Michael Chastain]

Andrew Cagney <cagney@gnu.org> wrote:
> week 0.0: @6.2:
> week 1.5: 6.2.1:
> week 4.0: 6.2.2:
>
> Does this seem reasonable.

It works for me.

Michael C

Re: [PATCH] Improve i386 prologue analyzer

[Mark Kettenis]

   Date: Sun, 08 Aug 2004 18:52:17 -0400
   From: Andrew Cagney <cagney@gnu.org>

   Here's how things are panning out:

   week 0.0: @6.2:
   For current mainstream systems, our best releaes ever!
   Late breaking discovery that MIPS is broken, time to resolution unknown 
   (guess 2 weeks) but workaround in hand.

   week 1.5: 6.2.1:
   Critical MIPS problem fixed.
   Fix for long-standing i386 bug known (needs 2 weeks testing)

   week 4.0: 6.2.2:
   Long standing bug on old i386 systems fixed.

   Does this seem reasonable.

Seems reasonable to me.  I trust you to drop a note when 6.2.1 has
been tagged, such that I can get in the i386 prologue analyzer
improvent as soon as possible.

Mark

Re: [PATCH] Improve i386 prologue analyzer

[Andrew Cagney]

>>> Date: Sun, 08 Aug 2004 10:07:58 -0400
>>> From: Andrew Cagney <cagney@gnu.org>
>>> 
>>> So by releasing 6.2._2_ in 2-3 weeks with this patch, and 6.2.1 today we 
>>> get a win-win.  The MIPS fix isn't held up, and the i386 fix gets 2-3 
>>> more weeks to mature :-)
> 
> 
> You know, I really don't understand what is going on here.  First, I'm
> told that GDB 6.2 cannot wait and must be released with the MIPS
> broken, and now GDB 6.2.1 cannot wait for a week or two because MIPS
> is broken?  How does that make any sense?

Here's how things are panning out:

week 0.0: @6.2:
For current mainstream systems, our best releaes ever!
Late breaking discovery that MIPS is broken, time to resolution unknown 
(guess 2 weeks) but workaround in hand.

week 1.5: 6.2.1:
Critical MIPS problem fixed.
Fix for long-standing i386 bug known (needs 2 weeks testing)

week 4.0: 6.2.2:
Long standing bug on old i386 systems fixed.

Does this seem reasonable.

Andrew

Re: [PATCH] Improve i386 prologue analyzer

[Eli Zaretskii]

> Date: Sun, 08 Aug 2004 10:07:58 -0400
> From: Andrew Cagney <cagney@gnu.org>
> 
> So by releasing 6.2._2_ in 2-3 weeks with this patch, and 6.2.1 today we 
> get a win-win.  The MIPS fix isn't held up, and the i386 fix gets 2-3 
> more weeks to mature :-)

You know, I really don't understand what is going on here.  First, I'm
told that GDB 6.2 cannot wait and must be released with the MIPS
broken, and now GDB 6.2.1 cannot wait for a week or two because MIPS
is broken?  How does that make any sense?

Re: [PATCH] Improve i386 prologue analyzer

[Eli Zaretskii]

> Date: Sun, 8 Aug 2004 13:08:12 +0200 (CEST)
> From: Mark Kettenis <kettenis@chello.nl>
> 
> Personally I'd like to wait for another two weeks, before deciding
> it's ok.

Two weeks is what I suggested.

> I'd be willing to check it in on the branch *now* if we'd
> agree we'd release 6.2.1 after that period.  That gives me the
> oportunity to pull it out if any problems arise.

I'd say let's do it now.

Re: [PATCH] Improve i386 prologue analyzer

[Eli Zaretskii]

> Date: Sun, 08 Aug 2004 06:24:04 -0400
> From: Michael Chastain <mec.gnu@mindspring.com>
> 
> eliz> If so, could you please explain why you think it needs any more
> eliz> ``marinating'' in HEAD?
> 
> It's a new design.

If that's the problem, we could use the less invasive change that I
posted.  It just adds a few more cases to a switch, but leaves the
overall structure of the code intact.

> And this feature interacts with many different external variations.
> For example, I haven't seen anyone test it with gcc 2.
> Or with a program that uses a lot of floating point.

As Mark points out, these issues are either already broken or
unaffected.

Re: [PATCH] Improve i386 prologue analyzer

[Mark Kettenis]

   Date: Sun, 08 Aug 2004 10:07:58 -0400
   From: Andrew Cagney <cagney@gnu.org>

   > That shouldn't be too relevant.  I mean, if they were broken before
   > the patch, they're probably still broken.  But that doesn't really
   > matter for the branch, does it?
   > 
   > Personally I'd like to wait for another two weeks, before deciding
   > it's ok.  I'd be willing to check it in on the branch *now* if we'd
   > agree we'd release 6.2.1 after that period.  That gives me the
   > oportunity to pull it out if any problems arise.

   Right,

   So by releasing 6.2._2_ in 2-3 weeks with this patch, and 6.2.1 today we 
   get a win-win.  The MIPS fix isn't held up, and the i386 fix gets 2-3 
   more weeks to mature :-)

Fine by me.  The impact of the problem isn't huge.  Most people won't
see it since they're using DWARF2.  But it'd be nice to fix it for
people who're still stuck with stabs.

Mark

Re: [PATCH] Improve i386 prologue analyzer

[Andrew Cagney]

> That shouldn't be too relevant.  I mean, if they were broken before
> the patch, they're probably still broken.  But that doesn't really
> matter for the branch, does it?
> 
> Personally I'd like to wait for another two weeks, before deciding
> it's ok.  I'd be willing to check it in on the branch *now* if we'd
> agree we'd release 6.2.1 after that period.  That gives me the
> oportunity to pull it out if any problems arise.

Right,

So by releasing 6.2._2_ in 2-3 weeks with this patch, and 6.2.1 today we 
get a win-win.  The MIPS fix isn't held up, and the i386 fix gets 2-3 
more weeks to mature :-)

Andrew

Re: [PATCH] Improve i386 prologue analyzer

[Mark Kettenis]

   Date: Sun, 08 Aug 2004 06:24:04 -0400
   Sender: mec.gnu@mindspring.com

   eliz> If so, could you please explain why you think it needs any more
   eliz> ``marinating'' in HEAD?

   It's a new design.

Yeah, it's a pretty invasive change.  It not only affects the
unwinder, but also prologue skipping in order to determine the first
line of real code for a function.

   And this feature interacts with many different external variations.
   For example, I haven't seen anyone test it with gcc 2.

I see no changes in the testsuite on i386-unknown-openbsd3.5 and
i386-unknown-freebsd4.7 which both use GCC 2.95.3/4 as their system
compiler.  I think these are fine since the code is never used.  I'm
more worried about GCC 3.1/3.2.  Those compilers are in pretty wide use.

   Or with a program that uses a lot of floating point.

That shouldn't be too relevant.  I mean, if they were broken before
the patch, they're probably still broken.  But that doesn't really
matter for the branch, does it?

Personally I'd like to wait for another two weeks, before deciding
it's ok.  I'd be willing to check it in on the branch *now* if we'd
agree we'd release 6.2.1 after that period.  That gives me the
oportunity to pull it out if any problems arise.

Mark

Re: [PATCH] Improve i386 prologue analyzer

[Michael Chastain]

"Eli Zaretskii" <eliz@gnu.org> wrote:
eliz> Are you talking about the prologue analyzer change?

Yes.

eliz> If so, could you please explain why you think it needs any more
eliz> ``marinating'' in HEAD?

It's a new design.

And this feature interacts with many different external variations.
For example, I haven't seen anyone test it with gcc 2.
Or with a program that uses a lot of floating point.

Michael C

Re: [PATCH] Improve i386 prologue analyzer

[Eli Zaretskii]

> Date: Sat, 07 Aug 2004 18:41:07 -0400
> From: Michael Chastain <mec.gnu@mindspring.com>
> 
> I'm gonna side with Andrew on this one.  Just looking superficially at
> the code, it's the kind of code that benefits from marinating in HEAD
> for a while, getting exposed to different use conditions.

Are you talking about the prologue analyzer change?  If so, could you
please explain why you think it needs any more ``marinating'' in HEAD?

Re: [PATCH] Improve i386 prologue analyzer

[Michael Chastain]

I'm gonna side with Andrew on this one.  Just looking superficially at
the code, it's the kind of code that benefits from marinating in HEAD
for a while, getting exposed to different use conditions.

Michael C

Re: [PATCH] Improve i386 prologue analyzer

[Andrew Cagney]

>>Remember, at present MIPS is dead in the water.  For how long should we 
>>> delay pushing out a fix for that?
> 
> 
> mips-irix has been broken for a long time now, and I don't remember
> seeing any complaint. So I don't think we have that much pressure
> to produce a good release within a short time frame. If anything,
> I think it can wait until 6.3. The alternative for the users is to
> either use 5.3, which was pretty good - or use a patched-up 6.2.1
> with the couple of fixes I sent (this may be necessary if they use
> a very recent version of GCC, I don't know).

For MIPS-IRIX, yes.

Here the question is generic mips and the bfd/ fix you back ported. 
That is a good reason for a prompt re-spin of 6.2.

Andrew

Re: [PATCH] Improve i386 prologue analyzer

[Joel Brobecker]

> Remember, at present MIPS is dead in the water.  For how long should we 
> delay pushing out a fix for that?

mips-irix has been broken for a long time now, and I don't remember
seeing any complaint. So I don't think we have that much pressure
to produce a good release within a short time frame. If anything,
I think it can wait until 6.3. The alternative for the users is to
either use 5.3, which was pretty good - or use a patched-up 6.2.1
with the couple of fixes I sent (this may be necessary if they use
a very recent version of GCC, I don't know).

My two cents,
-- 
Joel

Re: [PATCH] Improve i386 prologue analyzer

[Andrew Cagney]

Mark, Eli is pressing hard for, I guess you, to do an accelerated 
backport.  What do you think?

Andrew


>>Date: Sat, 07 Aug 2004 12:20:49 -0400
>>> From: Andrew Cagney <cagney@gnu.org>
>>> 
>>> Mark wrote:
>>> 
>>
>>>> > I'd really like this to get some exposure in HEAD before backporting
>>>> > it, but otherwise I do agree.
> 
> 
> Sure, but IMHO the change doesn't need too much testing more than it
> already got.  Don't the developers use CVS HEAD most of the time?
> Don't they use it mostly on IA32 (GNU/Linux or otherwise) systems?  If
> so, the patch is mostly tested.
> 
> 
>>> Remember, at present MIPS is dead in the water.
> 
> 
> With backtraces misbehaving as they do now, I consider the x86 broken
> almost as badly as the MIPS.  As I told before, because of that bug, I
> mistrusted GDB 6.1 to such a degree that I started to use GDB 5.x.
> 
> 
>>> For how long should we delay pushing out a fix for that?
> 
> 
> A week or two won't do much harm, IMHO.  But I doubt that we need even
> that much.
> 

Re: [PATCH] Improve i386 prologue analyzer

[Eli Zaretskii]

> Date: Sat, 07 Aug 2004 12:20:49 -0400
> From: Andrew Cagney <cagney@gnu.org>
> 
> Mark wrote:
> 
> > I'd really like this to get some exposure in HEAD before backporting
> > it, but otherwise I do agree.

Sure, but IMHO the change doesn't need too much testing more than it
already got.  Don't the developers use CVS HEAD most of the time?
Don't they use it mostly on IA32 (GNU/Linux or otherwise) systems?  If
so, the patch is mostly tested.

> Remember, at present MIPS is dead in the water.

With backtraces misbehaving as they do now, I consider the x86 broken
almost as badly as the MIPS.  As I told before, because of that bug, I
mistrusted GDB 6.1 to such a degree that I started to use GDB 5.x.

> For how long should we delay pushing out a fix for that?

A week or two won't do much harm, IMHO.  But I doubt that we need even
that much.

Re: [PATCH] Improve i386 prologue analyzer

[Andrew Cagney]

>>Date: Fri, 06 Aug 2004 16:29:47 -0400
>>> From: Andrew Cagney <cagney@gnu.org>
>>> 
>>> Ah.  We need to push the MIPS fixes out fairly quickly.  Sounds like 
>>> this should be held back, perhaphs for 6.3 or a 6.2.2.
> 
> 
> Please don't hold this back!  Producing a decent backtrace is one of
> the most important features of a debugger (think about debugging a
> crash from a core dump); a debugger that cannot reliably do that
> cannot be trusted.
> 
> I really don't understand what's to ``hold back'' here: Mark already
> made a patch, that patch was reviewed by myself and by Michael, and I
> already reported that it fixes the original problems.  So it is beyond
> me why the MIPS fixes can be in GDB 6.2.1, while the i386 prolog
> analysis patch cannot.

Mark wrote:

> I'd really like this to get some exposure in HEAD before backporting
> it, but otherwise I do agree.

Remember, at present MIPS is dead in the water.  For how long should we 
delay pushing out a fix for that?

Andrew

Re: [PATCH] Improve i386 prologue analyzer

[Eli Zaretskii]

> Date: Fri, 06 Aug 2004 16:29:47 -0400
> From: Andrew Cagney <cagney@gnu.org>
> 
> Ah.  We need to push the MIPS fixes out fairly quickly.  Sounds like 
> this should be held back, perhaphs for 6.3 or a 6.2.2.

Please don't hold this back!  Producing a decent backtrace is one of
the most important features of a debugger (think about debugging a
crash from a core dump); a debugger that cannot reliably do that
cannot be trusted.

I really don't understand what's to ``hold back'' here: Mark already
made a patch, that patch was reviewed by myself and by Michael, and I
already reported that it fixes the original problems.  So it is beyond
me why the MIPS fixes can be in GDB 6.2.1, while the i386 prolog
analysis patch cannot.

Re: [PATCH] Improve i386 prologue analyzer

[Eli Zaretskii]

> Date: Fri, 6 Aug 2004 21:33:03 +0200 (CEST)
> From: Mark Kettenis <kettenis@chello.nl>
> 
> I was under the impression that it only affected GCC 3.4.  Is GCC 3.3
> affected too Eli?

I have GCC 3.3.3 installed on the machine where I found this bug.  So
yes, 3.3.x is also affected.

> I'd really like this to get some exposure in HEAD before backporting
> it, but otherwise I do agree.

Perhaps people who use GCC 3.3.3 or later could give this some ride.

Re: [PATCH] Improve i386 prologue analyzer

[Andrew Cagney]

>    Date: Tue, 03 Aug 2004 06:53:13 +0300
>    From: "Eli Zaretskii" <eliz@gnu.org>
> 
>    > Date: Mon, 02 Aug 2004 17:18:51 -0400
>    > From: Andrew Cagney <cagney@gnu.org>
>    > 
>    > Should this be back-ported?
> 
>    Probably, especially if we are going to have GDB 6.2.1.  This bug
>    means that anyone who uses GCC 3.3 and later will see bogus backtraces
>    in optimized programs.
> 
> I was under the impression that it only affected GCC 3.4.  Is GCC 3.3
> affected too Eli?
> 
> I'd really like this to get some exposure in HEAD before backporting
> it, but otherwise I do agree.

Ah.  We need to push the MIPS fixes out fairly quickly.  Sounds like 
this should be held back, perhaphs for 6.3 or a 6.2.2.

Andrew

Re: [PATCH] Improve i386 prologue analyzer

[Mark Kettenis]

   Date: Tue, 03 Aug 2004 06:53:13 +0300
   From: "Eli Zaretskii" <eliz@gnu.org>

   > Date: Mon, 02 Aug 2004 17:18:51 -0400
   > From: Andrew Cagney <cagney@gnu.org>
   > 
   > Should this be back-ported?

   Probably, especially if we are going to have GDB 6.2.1.  This bug
   means that anyone who uses GCC 3.3 and later will see bogus backtraces
   in optimized programs.

I was under the impression that it only affected GCC 3.4.  Is GCC 3.3
affected too Eli?

I'd really like this to get some exposure in HEAD before backporting
it, but otherwise I do agree.

Re: [PATCH] Improve i386 prologue analyzer

[Eli Zaretskii]

> Date: Mon, 02 Aug 2004 17:18:51 -0400
> From: Andrew Cagney <cagney@gnu.org>
> 
> Should this be back-ported?

Probably, especially if we are going to have GDB 6.2.1.  This bug
means that anyone who uses GCC 3.3 and later will see bogus backtraces
in optimized programs.

Re: [PATCH] Improve i386 prologue analyzer

[Andrew Cagney]

>>Date: Sun, 1 Aug 2004 23:58:51 +0200 (CEST)
>>> From: Mark Kettenis <kettenis@chello.nl>
>>> 
>>> This revamps the way i386_analyze_frame_setup() tries to skip over the
>>> code instructions that GCC might migrate into the prologue.  The
>>> instructions are now in a list that can easily be extended if the need
>>> arises.
> 
> 
> Thanks, I verified that the original problems I reported no longer
> exist with this version.

Should this be back-ported?

Andrew

Re: [PATCH] Improve i386 prologue analyzer

[Eli Zaretskii]

> Date: Sun, 1 Aug 2004 23:58:51 +0200 (CEST)
> From: Mark Kettenis <kettenis@chello.nl>
> 
> This revamps the way i386_analyze_frame_setup() tries to skip over the
> code instructions that GCC might migrate into the prologue.  The
> instructions are now in a list that can easily be extended if the need
> arises.

Thanks, I verified that the original problems I reported no longer
exist with this version.

[PATCH] Improve i386 prologue analyzer

[Mark Kettenis]

This revamps the way i386_analyze_frame_setup() tries to skip over the
code instructions that GCC might migrate into the prologue.  The
instructions are now in a list that can easily be extended if the need
arises.

Committed,

Mark


Index: ChangeLog
from  Mark Kettenis  <kettenis@gnu.org>

	* i386-tdep.c (I386_MAX_INSN_LEN): New define.
	(struct i386_insn): New structure.
	(i386_match_insn): New function.
	(i386_frame_setup_skip_insns): New variable.
	(i386_analyze_frame_setup): Change to use i386_match_insn and the
	array i386_frame_setup_insns of instructions that should be
	skipped inside the frame setup sequence.

Index: i386-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/i386-tdep.c,v
retrieving revision 1.199
diff -u -p -r1.199 i386-tdep.c
--- i386-tdep.c 26 Jul 2004 14:53:01 -0000 1.199
+++ i386-tdep.c 1 Aug 2004 21:51:11 -0000
@@ -472,21 +472,123 @@ i386_skip_probe (CORE_ADDR pc)
   return pc;
 }
 
+/* Maximum instruction length we need to handle.  */
+#define I386_MAX_INSN_LEN	6
+
+/* Instruction description.  */
+struct i386_insn
+{
+  size_t len;
+  unsigned char insn[I386_MAX_INSN_LEN];
+  unsigned char mask[I386_MAX_INSN_LEN];
+};
+
+/* Search for the instruction at PC in the list SKIP_INSNS.  Return
+   the first instruction description that matches.  Otherwise, return
+   NULL.  */
+
+static struct i386_insn *
+i386_match_insn (CORE_ADDR pc, struct i386_insn *skip_insns)
+{
+  struct i386_insn *insn;
+  unsigned char op;
+
+  op = read_memory_unsigned_integer (pc, 1);
+
+  for (insn = skip_insns; insn->len > 0; insn++)
+    {
+      if ((op & insn->mask[0]) == insn->insn[0])
+	{
+	  unsigned char buf[I386_MAX_INSN_LEN - 1];
+	  size_t i;
+
+	  gdb_assert (insn->len > 1);
+	  gdb_assert (insn->len <= I386_MAX_INSN_LEN);
+
+	  read_memory (pc + 1, buf, insn->len - 1);
+	  for (i = 1; i < insn->len; i++)
+	    {
+	      if ((buf[i - 1] & insn->mask[i]) != insn->insn[i])
+		break;
+
+	      return insn;
+	    }
+	}
+    }
+
+  return NULL;
+}
+
+/* Some special instructions that might be migrated by GCC into the
+   part of the prologue that sets up the new stack frame.  Because the
+   stack frame hasn't been setup yet, no registers have been saved
+   yet, and only the scratch registers %eax, %ecx and %edx can be
+   touched.  */
+
+struct i386_insn i386_frame_setup_skip_insns[] =
+{
+  /* Check for `movb imm8, r' and `movl imm32, r'. 
+    
+     ??? Should we handle 16-bit operand-sizes here?  */
+
+  /* `movb imm8, %al' and `movb imm8, %ah' */
+  /* `movb imm8, %cl' and `movb imm8, %ch' */
+  { 2, { 0xb0, 0x00 }, { 0xfa, 0x00 } },
+  /* `movb imm8, %dl' and `movb imm8, %dh' */
+  { 2, { 0xb2, 0x00 }, { 0xfb, 0x00 } },
+  /* `movl imm32, %eax' and `movl imm32, %ecx' */
+  { 5, { 0xb8 }, { 0xfe } },
+  /* `movl imm32, %edx' */
+  { 5, { 0xba }, { 0xff } },
+
+  /* Check for `mov imm32, r32'.  Note that there is an alternative
+     encoding for `mov m32, %eax'.
+
+     ??? Should we handle SIB adressing here?
+     ??? Should we handle 16-bit operand-sizes here?  */
+
+  /* `movl m32, %eax' */
+  { 5, { 0xa1 }, { 0xff } },
+  /* `movl m32, %eax' and `mov; m32, %ecx' */
+  { 6, { 0x89, 0x05 }, {0xff, 0xf7 } },
+  /* `movl m32, %edx' */
+  { 6, { 0x89, 0x15 }, {0xff, 0xff } },
+
+  /* Check for `xorl r32, r32' and the equivalent `subl r32, r32'.
+     Because of the symmetry, there are actually two ways to encode
+     these instructions; opcode bytes 0x29 and 0x2b for `subl' and
+     opcode bytes 0x31 and 0x33 for `xorl'.  */
+
+  /* `subl %eax, %eax' */
+  { 2, { 0x29, 0xc0 }, { 0xfd, 0xff } },
+  /* `subl %ecx, %ecx' */
+  { 2, { 0x29, 0xc9 }, { 0xfd, 0xff } },
+  /* `subl %edx, %edx' */
+  { 2, { 0x29, 0xd2 }, { 0xfd, 0xff } },
+  /* `xorl %eax, %eax' */
+  { 2, { 0x31, 0xc0 }, { 0xfd, 0xff } },
+  /* `xorl %ecx, %ecx' */
+  { 2, { 0x31, 0xc9 }, { 0xfd, 0xff } },
+  /* `xorl %edx, %edx' */
+  { 2, { 0x31, 0xd2 }, { 0xfd, 0xff } },
+  { 0 }
+};
+
 /* Check whether PC points at a code that sets up a new stack frame.
    If so, it updates CACHE and returns the address of the first
-   instruction after the sequence that sets removes the "hidden"
-   argument from the stack or CURRENT_PC, whichever is smaller.
-   Otherwise, return PC.  */
+   instruction after the sequence that sets up the frame or LIMIT,
+   whichever is smaller.  If we don't recognize the code, return PC.  */
 
 static CORE_ADDR
-i386_analyze_frame_setup (CORE_ADDR pc, CORE_ADDR current_pc,
+i386_analyze_frame_setup (CORE_ADDR pc, CORE_ADDR limit,
 			  struct i386_frame_cache *cache)
 {
+  struct i386_insn *insn;
   unsigned char op;
   int skip = 0;
 
-  if (current_pc <= pc)
-    return current_pc;
+  if (limit <= pc)
+    return limit;
 
   op = read_memory_unsigned_integer (pc, 1);
 
@@ -496,87 +598,48 @@ i386_analyze_frame_setup (CORE_ADDR pc, 
 	 starts this instruction sequence.  */
       cache->saved_regs[I386_EBP_REGNUM] = 0;
       cache->sp_offset += 4;
+      pc++;
 
       /* If that's all, return now.  */
-      if (current_pc <= pc + 1)
-	return current_pc;
-
-      op = read_memory_unsigned_integer (pc + 1, 1);
+      if (limit <= pc)
+	return limit;
 
       /* Check for some special instructions that might be migrated by
-	 GCC into the prologue.  At this point in the prologue, code
-	 should only touch the scratch registers %eax, %ecx and %edx,
-	 so we check for
-
-	    movl $XXX, %eax
-	    movl $XXX, %ecx
-	    movl $XXX, %edx
-
-	 These instructions have opcodes 0xb8, 0xb9 and 0xba.
-
-	 We also check for
-
-	    xorl %eax, %eax
-	    xorl %ecx, %ecx
-	    xorl %edx, %edx
-
-	 and the equivalent
-
-	    subl %eax, %eax
-	    subl %ecx, %ecx
-	    subl %edx, %edx
-
-	 Because of the symmetry, there are actually two ways to
-	 encode these instructions; with opcode bytes 0x29 and 0x2b
-	 for `subl' and opcode bytes 0x31 and 0x33 for `xorl'.
+	 GCC into the prologue and skip them.  At this point in the
+	 prologue, code should only touch the scratch registers %eax,
+	 %ecx and %edx, so while the number of posibilities is sheer,
+	 it is limited.
 
 	 Make sure we only skip these instructions if we later see the
 	 `movl %esp, %ebp' that actually sets up the frame.  */
-      while ((op >= 0xb8 && op <= 0xba)
-	     || op == 0x29 || op == 0x2b
-	     || op == 0x31 || op == 0x33)
+      while (pc + skip < limit)
 	{
-	  if (op >= 0xb8 && op <= 0xba)
-	    {
-	      /* Skip the `movl' instructions cited above.  */
-	      skip += 5;
-	    }
-	  else
-	    {
-	      /* Skip the `subl' and `xorl' instructions cited above.  */
-	      op = read_memory_unsigned_integer (pc + skip + 2, 1);
-	      switch (op)
-		{
-		case 0xc0:	/* %eax */
-		case 0xc9:	/* %ecx */
-		case 0xd2:	/* %edx */
-		  skip += 2;
-		  break;
-		default:
-		  return pc + 1;
-		}
-	    }
-
-	  /* If that's all, return now.  */
-	  if (current_pc <= pc + skip + 1)
-	    return current_pc;
+	  insn = i386_match_insn (pc + skip, i386_frame_setup_skip_insns);
+	  if (insn == NULL)
+	    break;
 
-	  op = read_memory_unsigned_integer (pc + skip + 1, 1);
+	  skip += insn->len;
 	}
 
+      /* If that's all, return now.  */
+      if (limit <= pc + skip)
+	return limit;
+
+      op = read_memory_unsigned_integer (pc + skip, 1);
+
       /* Check for `movl %esp, %ebp' -- can be written in two ways.  */
       switch (op)
 	{
 	case 0x8b:
-	  if (read_memory_unsigned_integer (pc + skip + 2, 1) != 0xec)
-	    return pc + 1;
+	  if (read_memory_unsigned_integer (pc + skip + 1, 1) != 0xec)
+	    return pc;
 	  break;
 	case 0x89:
-	  if (read_memory_unsigned_integer (pc + skip + 2, 1) != 0xe5)
-	    return pc + 1;
+	  if (read_memory_unsigned_integer (pc + skip + 1, 1) != 0xe5)
+	    return pc;
 	  break;
 	default:
-	  return pc + 1;
+	  return pc;
 	}
 
       /* OK, we actually have a frame.  We just don't know how large
@@ -584,11 +647,11 @@ i386_analyze_frame_setup (CORE_ADDR pc, 
 	 necessary.  We also now commit to skipping the special
 	 instructions mentioned before.  */
       cache->locals = 0;
-      pc += skip;
+      pc += (skip + 2);
 
       /* If that's all, return now.  */
-      if (current_pc <= pc + 3)
-	return current_pc;
+      if (limit <= pc)
+	return limit;
 
       /* Check for stack adjustment 
 
@@ -596,37 +659,37 @@ i386_analyze_frame_setup (CORE_ADDR pc, 
 
 	 NOTE: You can't subtract a 16-bit immediate from a 32-bit
 	 reg, so we don't have to worry about a data16 prefix.  */
-      op = read_memory_unsigned_integer (pc + 3, 1);
+      op = read_memory_unsigned_integer (pc, 1);
       if (op == 0x83)
 	{
 	  /* `subl' with 8-bit immediate.  */
-	  if (read_memory_unsigned_integer (pc + 4, 1) != 0xec)
+	  if (read_memory_unsigned_integer (pc + 1, 1) != 0xec)
 	    /* Some instruction starting with 0x83 other than `subl'.  */
-	    return pc + 3;
+	    return pc;
 
-	  /* `subl' with signed byte immediate (though it wouldn't make
-	     sense to be negative).  */
-	  cache->locals = read_memory_integer (pc + 5, 1);
-	  return pc + 6;
+	  /* `subl' with signed 8-bit immediate (though it wouldn't
+	     make sense to be negative).  */
+	  cache->locals = read_memory_integer (pc + 2, 1);
+	  return pc + 3;
 	}
       else if (op == 0x81)
 	{
 	  /* Maybe it is `subl' with a 32-bit immediate.  */
-	  if (read_memory_unsigned_integer (pc + 4, 1) != 0xec)
+	  if (read_memory_unsigned_integer (pc + 1, 1) != 0xec)
 	    /* Some instruction starting with 0x81 other than `subl'.  */
-	    return pc + 3;
+	    return pc;
 
 	  /* It is `subl' with a 32-bit immediate.  */
-	  cache->locals = read_memory_integer (pc + 5, 4);
-	  return pc + 9;
+	  cache->locals = read_memory_integer (pc + 2, 4);
+	  return pc + 6;
 	}
       else
 	{
 	  /* Some instruction other than `subl'.  */
-	  return pc + 3;
+	  return pc;
 	}
     }
-  else if (op == 0xc8)		/* enter $XXX */
+  else if (op == 0xc8)		/* enter */
     {
       cache->locals = read_memory_unsigned_integer (pc + 1, 2);
       return pc + 4;

Re: [PATCH] Improve i386 prologue analyzer

[Michael Elizabeth Chastain]

> Picking some low-hanging fruit :-).  This probably fixes PR
> backtrace/1338.  I'll be checking this in on the branch if Michael
> confirms that this fixes the bug.

All right!

I will test this several hours later this evening.

My test bed is busy right now, I am fixing pr gdb/296 and pr gdb/1317
(the ENUM_BITFIELD maintenance stuff).

Michael C

===

Index: ChangeLog
from  Mark Kettenis  <kettenis@gnu.org>

	* i386-tdep.c (i386_analyze_register_saves): Handle register saves
	at the start of a frameless function.  This probably fixes PR
	backtrace/1338.

[PATCH] Improve i386 prologue analyzer

[Mark Kettenis]

Picking some low-hanging fruit :-).  This probably fixes PR
backtrace/1338.  I'll be checking this in on the branch if Michael
confirms that this fixes the bug.  I'll add a testcase shortly.

Mark

Index: ChangeLog
from  Mark Kettenis  <kettenis@gnu.org>

	* i386-tdep.c (i386_analyze_register_saves): Handle register saves
	at the start of a frameless function.  This probably fixes PR
	backtrace/1338.

Index: i386-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/i386-tdep.c,v
retrieving revision 1.165
diff -u -p -r1.165 i386-tdep.c
--- i386-tdep.c 12 Aug 2003 16:12:33 -0000 1.165
+++ i386-tdep.c 17 Aug 2003 23:11:19 -0000
@@ -571,23 +571,22 @@ static CORE_ADDR
 i386_analyze_register_saves (CORE_ADDR pc, CORE_ADDR current_pc,
 			     struct i386_frame_cache *cache)
 {
-  if (cache->locals >= 0)
-    {
-      CORE_ADDR offset;
-      unsigned char op;
-      int i;
+  CORE_ADDR offset = 0;
+  unsigned char op;
+  int i;
 
-      offset = - 4 - cache->locals;
-      for (i = 0; i < 8 && pc < current_pc; i++)
-	{
-	  op = read_memory_unsigned_integer (pc, 1);
-	  if (op < 0x50 || op > 0x57)
-	    break;
+  if (cache->locals > 0)
+    offset -= cache->locals;
+  for (i = 0; i < 8 && pc < current_pc; i++)
+    {
+      op = read_memory_unsigned_integer (pc, 1);
+      if (op < 0x50 || op > 0x57)
+	break;
 
-	  cache->saved_regs[op - 0x50] = offset;
-	  offset -= 4;
-	  pc++;
-	}
+      offset -= 4;
+      cache->saved_regs[op - 0x50] = offset;
+      cache->sp_offset += 4;
+      pc++;
     }
 
   return pc;