Re: RFA: ia64 portion of libunwind patch

Re: RFA: ia64 portion of libunwind patch

[J. Johnston]

David Mosberger has answered the questions below.  I'd like to include him in 
the discussion from now on.

-- Jeff J.

-------- Original Message --------

>>>>> On Thu, 30 Oct 2003 14:18:20 -0500, "J. Johnston" <jjohnstn@redhat.com> said:

   Andrew> Can you expand a little here on how this function interacts
   Andrew> with libunwind?

It is used by libunwind to access the unwind info.  This is read-only
data that part of the ELF image and (at least for ia64) is also mapped
into the target space.

   Andrew> I can see that its reading in data, but is that data found
   Andrew> in the target's space?

It could be found there.

   Andrew> If it is then the info should be pulled direct from the
   Andrew> target and the BFD/objfile should not be used.  The relevant
   Andrew> target stratum can then re-direct the request to a local
   Andrew> file.

I agree, it sounds like this would be a much cleaner way of doing it.

   Andrew> I'm also wondering if the unwind code (probably impossible I
   Andrew> know) could use a callback to request the memory rather than
   Andrew> require an entire buffer.

The way the libunwind interface works nowadays, the only buffering
that is strictly needed is for the unwind info of the procedure being
looked up (which usually has a size of the order of tens of bytes).
But this would require doing a binary search on the unwind-table in
the target space, which might be rather slow (there is one 24-byte
entry in this table per procedure).  Thus, it might be easier (and
certainly faster) to buffer the unwind table inside gdb.

	--david

Re: RFA: ia64 portion of libunwind patch

[Andrew Cagney]

> On Thu, 30 Oct 2003 14:18:20 -0500, "J. Johnston" <jjohnstn@redhat.com> said:
> 
>   Andrew> Can you expand a little here on how this function interacts
>   Andrew> with libunwind?
> 
> It is used by libunwind to access the unwind info.  This is read-only
> data that part of the ELF image and (at least for ia64) is also mapped
> into the target space.

Ok.  The target vector consists of a number of stratum.  When a memory 
request is sent to the target it is responsible (note, minus bugs) for 
servicing the request using the most applicable stratum.  For instance, 
given a core file, modified memory sections are supplied by the 
process/corefile stratum, while read-only sections are supplied by the 
file/executable stratum.

>   Andrew> I can see that its reading in data, but is that data found
>   Andrew> in the target's space?
> 
> It could be found there.
> 
>   Andrew> If it is then the info should be pulled direct from the
>   Andrew> target and the BFD/objfile should not be used.  The relevant
>   Andrew> target stratum can then re-direct the request to a local
>   Andrew> file.
> 
> I agree, it sounds like this would be a much cleaner way of doing it.

As they say, make it work correctly, then make it work fast :-)  If 
target i/o gets to be a problem, we [gdb] get to fix the target stack.

>   Andrew> I'm also wondering if the unwind code (probably impossible I
>   Andrew> know) could use a callback to request the memory rather than
>   Andrew> require an entire buffer.
> 
> The way the libunwind interface works nowadays, the only buffering
> that is strictly needed is for the unwind info of the procedure being
> looked up (which usually has a size of the order of tens of bytes).
> But this would require doing a binary search on the unwind-table in
> the target space, which might be rather slow (there is one 24-byte
> entry in this table per procedure).  Thus, it might be easier (and
> certainly faster) to buffer the unwind table inside gdb.

Given a PC, how is the table located?  I see the change does roughly:
   pc -> section -> objfile -> BFD -> unwind segment -> paddr/size?
(could it look up the BFD's .IA_64.unwind_* sections instead?)
I guess initially, it could just use the paddr/size to pull the memory 
from "current_target"?

I suspect though that long term a memory request callback will prove 
more effective - it would avoid locking GDB into a model that requires 
it to cache full and contigous unwind sections.  Using a back of 
envelope caculation (GDB appears to have a log2 (128k of unwind section 
/ 24) = ~14) I'm guessing that the binary search involves about 14 
fetches and provided they are serviced from a cache they will be very 
efficient.

Andrew

Re: RFA: ia64 portion of libunwind patch

[Marcel Moolenaar]

On Fri, Oct 31, 2003 at 02:25:14PM -0500, J. Johnston wrote:
> 
>   Andrew> I'm also wondering if the unwind code (probably impossible I
>   Andrew> know) could use a callback to request the memory rather than
>   Andrew> require an entire buffer.
> 
> The way the libunwind interface works nowadays, the only buffering
> that is strictly needed is for the unwind info of the procedure being
> looked up (which usually has a size of the order of tens of bytes).
> But this would require doing a binary search on the unwind-table in
> the target space, which might be rather slow (there is one 24-byte
> entry in this table per procedure).  Thus, it might be easier (and
> certainly faster) to buffer the unwind table inside gdb.

Getting the unwind tables for remote targets can take a while if the
load module is large and the communication is slow (say 9600 baud).
Since libunwind already does its own caching, we may be better off
not caching in gdb. This also works better with dynamic unwind
information, which cannot be cached without some way of validating
whether the cached information is still correct or not.

-- 
 Marcel Moolenaar	  USPA: A-39004		 marcel@xcllnt.net

Re: RFA: ia64 portion of libunwind patch

[David Mosberger]

>>>>> On Fri, 31 Oct 2003 15:46:19 -0500, Andrew Cagney <ac131313@redhat.com> said:

  >>  The way the libunwind interface works nowadays, the only
  >> buffering that is strictly needed is for the unwind info of the
  >> procedure being looked up (which usually has a size of the order
  >> of tens of bytes).  But this would require doing a binary search
  >> on the unwind-table in the target space, which might be rather
  >> slow (there is one 24-byte entry in this table per procedure).
  >> Thus, it might be easier (and certainly faster) to buffer the
  >> unwind table inside gdb.

  Andrew> Given a PC, how is the table located?  I see the change does
  Andrew> roughly: pc -> section -> objfile -> BFD -> unwind segment
  Andrew> -> paddr/size?

That's about right.

  Andrew> (GDB appears to have a log2 (128k of unwind section / 24) =
  Andrew> ~14) I'm guessing that the binary search involves about 14
  Andrew> fetches and provided they are serviced from a cache they
  Andrew> will be very efficient.

Yup.

	--david

Re: RFA: ia64 portion of libunwind patch

[David Mosberger]

>>>>> On Fri, 31 Oct 2003 13:35:51 -0800, Marcel Moolenaar <marcel@xcllnt.net> said:

  Marcel> Since libunwind already does its own caching, we may be
  Marcel> better off not caching in gdb.

That's true.  However, note that by default, caching is not enabled
for remote unwinding (which is what gdb does).  To enable caching, gdb
would have to make a call of the form:

	unw_set_caching_policy(ADDR_SPACE, UNW_CACHE_GLOBAL);

Also, whenever something changes in the target that could invalidate
cached unwind info, gdb would have to call:

	unw_flush_cache(ADDR_SPACE, LO, HI);

where LO and HI is the address range that was affected (or LO=0, HI=0
to flush the entire cache).

	--david

Re: RFA: ia64 portion of libunwind patch

[Andrew Cagney]

> On Fri, 31 Oct 2003 13:35:51 -0800, Marcel Moolenaar <marcel@xcllnt.net> said:
> 
> 
>   Marcel> Since libunwind already does its own caching, we may be
>   Marcel> better off not caching in gdb.
> 
> That's true.  However, note that by default, caching is not enabled
> for remote unwinding (which is what gdb does).  To enable caching, gdb
> would have to make a call of the form:
> 
> 	unw_set_caching_policy(ADDR_SPACE, UNW_CACHE_GLOBAL);
> 
> Also, whenever something changes in the target that could invalidate
> cached unwind info, gdb would have to call:
> 
> 	unw_flush_cache(ADDR_SPACE, LO, HI);

> where LO and HI is the address range that was affected (or LO=0, HI=0
> to flush the entire cache).

Just FYI, for GDB, it will be better to let the target stack to do the 
cache management.  That way the user can set/override the policy.  For 
instance, in the case in question, the user would be inane to specify 
anything other than:


	(gdb) set trust-readonly-sections 1

(which causes all read-only reads to be handled locally) this is also 
why the issue isn't really that much of a problem.

Andrew

Re: RFA: ia64 portion of libunwind patch

[David Mosberger]

>>>>> On Fri, 31 Oct 2003 18:42:54 -0500, Andrew Cagney <ac131313@redhat.com> said:

  >> On Fri, 31 Oct 2003 13:35:51 -0800, Marcel Moolenaar
  >> <marcel@xcllnt.net> said:
  >> 
  >> 
  Marcel> Since libunwind already does its own caching, we may be
  Marcel> better off not caching in gdb.
  >>  That's true.  However, note that by default, caching is not
  >> enabled for remote unwinding (which is what gdb does).  To enable
  >> caching, gdb would have to make a call of the form:
  >> 
  >> unw_set_caching_policy(ADDR_SPACE, UNW_CACHE_GLOBAL);
  >> 
  >> Also, whenever something changes in the target that could
  >> invalidate cached unwind info, gdb would have to call:
  >> 
  >> unw_flush_cache(ADDR_SPACE, LO, HI);

  >> where LO and HI is the address range that was affected (or LO=0,
  >> HI=0 to flush the entire cache).

  Andrew> Just FYI, for GDB, it will be better to let the target stack
  Andrew> to do the cache management.  That way the user can
  Andrew> set/override the policy.  For instance, in the case in
  Andrew> question, the user would be inane to specify anything other
  Andrew> than:

  Andrew> 	(gdb) set trust-readonly-sections 1

  Andrew> (which causes all read-only reads to be handled locally)
  Andrew> this is also why the issue isn't really that much of a
  Andrew> problem.

Just for clarification: note that libunwind doesn't cache the raw
bits.  Instead, it caches the decoded unwind information, so it's
really a higher-level cache.

	--david

Re: RFA: ia64 portion of libunwind patch

[Andrew Cagney]

> On Fri, 31 Oct 2003 15:46:19 -0500, Andrew Cagney <ac131313@redhat.com> said:
> 
> 
>   >>  The way the libunwind interface works nowadays, the only
>   >> buffering that is strictly needed is for the unwind info of the
>   >> procedure being looked up (which usually has a size of the order
>   >> of tens of bytes).  But this would require doing a binary search
>   >> on the unwind-table in the target space, which might be rather
>   >> slow (there is one 24-byte entry in this table per procedure).
>   >> Thus, it might be easier (and certainly faster) to buffer the
>   >> unwind table inside gdb.
> 
>   Andrew> Given a PC, how is the table located?  I see the change does
>   Andrew> roughly: pc -> section -> objfile -> BFD -> unwind segment
>   Andrew> -> paddr/size?
> 
> That's about right.
> 
>   Andrew> (GDB appears to have a log2 (128k of unwind section / 24) =
>   Andrew> ~14) I'm guessing that the binary search involves about 14
>   Andrew> fetches and provided they are serviced from a cache they
>   Andrew> will be very efficient.
> 
> Yup.

Would it be possible to define the interface so that, when 
dip->u.ti.table_data is NULL, the code just fetches values from memory 
using the memory callbacks?  i.e., don't require GDB to fetch the entire 
table but instead fetch the bits that are needed.

Andrew

Re: RFA: ia64 portion of libunwind patch

[David Mosberger]

>>>>> On Fri, 07 Nov 2003 16:46:59 -0500, Andrew Cagney <ac131313@redhat.com> said:

  Andrew> Would it be possible to define the interface so that, when
  Andrew> dip-> u.ti.table_data is NULL, the code just fetches values
  Andrew> from memory using the memory callbacks?  i.e., don't require
  Andrew> GDB to fetch the entire table but instead fetch the bits
  Andrew> that are needed.

Well, it's software, so anything is _possible_, but I'd rather not do
that, because it creates artificial differences between the
(speed-critical) local unwind case and the remote unwind case.
Furthermore, the unwind table is of a known size, relatively small,
and accessed fairly randomly (via binary search), so it is normally
preferable to read the table all at once and I'd rather design the API
for this common case.

	--david

Re: RFA: ia64 portion of libunwind patch

[Andrew Cagney]

> On Fri, 07 Nov 2003 16:46:59 -0500, Andrew Cagney <ac131313@redhat.com> said:
> 
> 
>   Andrew> Would it be possible to define the interface so that, when
>   Andrew> dip-> u.ti.table_data is NULL, the code just fetches values
>   Andrew> from memory using the memory callbacks?  i.e., don't require
>   Andrew> GDB to fetch the entire table but instead fetch the bits
>   Andrew> that are needed.
> 
> Well, it's software, so anything is _possible_, but I'd rather not do
> that, because it creates artificial differences between the
> (speed-critical) local unwind case and the remote unwind case.
> Furthermore, the unwind table is of a known size, relatively small,
> and accessed fairly randomly (via binary search), so it is normally
> preferable to read the table all at once and I'd rather design the API
> for this common case.

Your comparing the self unwind case (i.e., what happens when a program 
does a "throw") vs the external unwind case (i.e., like what GDB has to do)?

Is fetching the table elements via a function, rather than a direct 
access, really that significant an overhead?

Andrew

Re: RFA: ia64 portion of libunwind patch

[David Mosberger]

>>>>> On Fri, 07 Nov 2003 18:01:40 -0500, Andrew Cagney <ac131313@redhat.com> said:

  Andrew> Is fetching the table elements via a function, rather than a
  Andrew> direct access, really that significant an overhead?

Is allocating a scratch buffer in gdb really such a big issue?
It's very late for proposing libunwind API changes.

	--david

Re: RFA: ia64 portion of libunwind patch

[Andrew Cagney]

> On Fri, 07 Nov 2003 18:01:40 -0500, Andrew Cagney <ac131313@redhat.com> said:
> 
> 
>   Andrew> Is fetching the table elements via a function, rather than a
>   Andrew> direct access, really that significant an overhead?
> 
> Is allocating a scratch buffer in gdb really such a big issue?
> It's very late for proposing libunwind API changes.

Draging that entire buffer (128k in the case of a program like GDB) 
across the remote link is going to raise a few eyebrows so a careful 
examination is justified.  If that search is so critical can the 
algorithm be made more efficient?  If it isn't then, per my suggestion, 
have the code use the memory method as a fallback.

Andrew

Re: RFA: ia64 portion of libunwind patch

[David Mosberger]

>>>>> On Fri, 07 Nov 2003 18:38:43 -0500, Andrew Cagney <ac131313@redhat.com> said:

  Andrew> Draging that entire buffer (128k in the case of a program
  Andrew> like GDB) across the remote link is going to raise a few
  Andrew> eyebrows so a careful examination is justified.

Oh.  Can you remind me why gdb wouldn't be grabbing the unwind table
from the ELF binary instead?  You don't drag the debug info over the
remote link either, right?

	--david

Re: RFA: ia64 portion of libunwind patch

[Andrew Cagney]

>>>>>> On Fri, 07 Nov 2003 18:38:43 -0500, Andrew Cagney <ac131313@redhat.com> said:
> 
> 
>   Andrew> Draging that entire buffer (128k in the case of a program
>   Andrew> like GDB) across the remote link is going to raise a few
>   Andrew> eyebrows so a careful examination is justified.
> 
> Oh.  Can you remind me why gdb wouldn't be grabbing the unwind table
> from the ELF binary instead?  You don't drag the debug info over the
> remote link either, right?

Because it may not have the elf binary (at least not locally), and it 
may not have debug info.  People still expect GDB to do something 
reasonable in those cases.

Andrew

Re: RFA: ia64 portion of libunwind patch

[Kevin Buettner]

On Nov 7,  7:07pm, Andrew Cagney wrote:

> >>>>>> On Fri, 07 Nov 2003 18:38:43 -0500, Andrew Cagney <ac131313@redhat.com> said:
> > 
> > 
> >   Andrew> Draging that entire buffer (128k in the case of a program
> >   Andrew> like GDB) across the remote link is going to raise a few
> >   Andrew> eyebrows so a careful examination is justified.
> > 
> > Oh.  Can you remind me why gdb wouldn't be grabbing the unwind table
> > from the ELF binary instead?  You don't drag the debug info over the
> > remote link either, right?
> 
> Because it may not have the elf binary (at least not locally), and it 
> may not have debug info.  People still expect GDB to do something 
> reasonable in those cases.

I'm a little bit puzzled about why we wouldn't have the elf binary...

But, assuming for the moment that we do, we'll definitely have the
unwind info since it's mandated by the ABI.

Kevin

Re: RFA: ia64 portion of libunwind patch

[Andrew Cagney]

> Because it may not have the elf binary (at least not locally), and it 
>> may not have debug info.  People still expect GDB to do something 
>> reasonable in those cases.
> 
> 
> I'm a little bit puzzled about why we wouldn't have the elf binary...
> 
> But, assuming for the moment that we do, we'll definitely have the
> unwind info since it's mandated by the ABI.

"at least not locally".  The unwind info can always be found in the 
target's memory.

There are two choices here:
- make the interface more remote friendly
- just ignore the issue until someone complains

Hmm, a good question to ask is "how cross debug friendly" is libunwind.? 
If its anything like libthread-db then this discussion is mute.

Andrew

Re: RFA: ia64 portion of libunwind patch

[David Mosberger]

>>>>> On Fri, 07 Nov 2003 19:27:54 -0500, Andrew Cagney <ac131313@redhat.com> said:

  >> Because it may not have the elf binary (at least not locally),
  >> and it
  >>> may not have debug info.  People still expect GDB to do
  >>> something reasonable in those cases.

  >> I'm a little bit puzzled about why we wouldn't have the elf
  >> binary...

  >> But, assuming for the moment that we do, we'll definitely have
  >> the unwind info since it's mandated by the ABI.

  Andrew> "at least not locally".  The unwind info can always be found
  Andrew> in the target's memory.

  Andrew> There are two choices here: - make the interface more remote
  Andrew> friendly - just ignore the issue until someone complains

Can you make it so that the local ELF image is used _when_ it's
available?  I personally never used gdb without specifying an
executable file (I'm not even sure how that works), so I guess I'm
willing to wait until someone complains (note that it's not as easy as
simply setting table_data to NULL, because there would be no way to
calculate the address of the unwind-table entry, so I'm not sure what
you want could be done without breaking the existing API).

  Andrew> Hmm, a good question to ask is "how cross debug friendly" is
  Andrew> libunwind.?  If its anything like libthread-db then this
  Andrew> discussion is mute.

I don't know what the issue with libthread-db is, but libunwind is
designed to be cross-debug friendly.  It's a feature that gets
regularly tested, e.g., as part of libunwind built into HP's ia64
simulator (which runs fine on x86 linux, for example).  In theory,
libunwind also support multiple unwind targets in the same binary, but
this hasn't been tested much so far (but I also have no reason to
believe that it doesn't work).

	--david

Re: RFA: ia64 portion of libunwind patch

[Andrew Cagney]

>   Andrew> "at least not locally".  The unwind info can always be found
>   Andrew> in the target's memory.
> 
>   Andrew> There are two choices here: - make the interface more remote
>   Andrew> friendly - just ignore the issue until someone complains
> 
> Can you make it so that the local ELF image is used _when_ it's
> available?

"libunwind" should assume that GDB will fill each memory request using 
the most efficient [correct] source available.  One potential source 
being the local ELF image.  If this isn't happening then GDB has 
something to fix (significantly, "libunwind" should resist the 
temptation to work around any such problems).

What I don't think "libunwind" should be doing is assuming that most 
efficient way to obtain specific elements of that unwind table is to 
read that table en-mass.  Instead I think "libunwind" be careful to only 
request the specific memory locations that it needs - trust the client 
to supply the requests in the most efficient way possible.  I should 
note that this has come up before, GDB encountered performance problems 
with a library that was trying to out smart a memory read bottle neck by 
slurping ever increasing chunks of unneeded memory.  This made the 
performance problem worse not better - it was the volume of data and not 
the number of xfers that was the bottle neck.

If we look at GDB with its 128k of unwind data.  At 14*28 byte requests 
per unwind, it would take ~300 unwinds before GDB was required to xfer 
128k (yes I'm pushing the numbers a little here, but then I'm also 
ignoring the very significant locality of the searches).

> I personally never used gdb without specifying an
> executable file (I'm not even sure how that works), so I guess I'm
> willing to wait until someone complains (note that it's not as easy as
> simply setting table_data to NULL, because there would be no way to
> calculate the address of the unwind-table entry, so I'm not sure what
> you want could be done without breaking the existing API).

Scary as it is, GDB's already got a requrest to feltch a shared library 
image from the target's memory :-/.  Provided the remote target knows 
the address of the unwind table, GDB should be able to find a way of 
getting it to libunwind.

>   Andrew> Hmm, a good question to ask is "how cross debug friendly" is
>   Andrew> libunwind.?  If its anything like libthread-db then this
>   Andrew> discussion is mute.
> 
> I don't know what the issue with libthread-db is, but libunwind is
> designed to be cross-debug friendly.  It's a feature that gets
> regularly tested, e.g., as part of libunwind built into HP's ia64
> simulator (which runs fine on x86 linux, for example).  In theory,
> libunwind also support multiple unwind targets in the same binary, but
> this hasn't been tested much so far (but I also have no reason to
> believe that it doesn't work).

"all of the above", this is very good news!

Andrew

Re: RFA: ia64 portion of libunwind patch

[Marcel Moolenaar]

On Fri, Nov 07, 2003 at 03:12:29PM -0800, David Mosberger wrote:
> >>>>> On Fri, 07 Nov 2003 18:01:40 -0500, Andrew Cagney <ac131313@redhat.com> said:
> 
>   Andrew> Is fetching the table elements via a function, rather than a
>   Andrew> direct access, really that significant an overhead?
> 
> Is allocating a scratch buffer in gdb really such a big issue?
> It's very late for proposing libunwind API changes.

I think it's very early to have it nailed down. Despite ones efforts
to create a clean system, there are always assumptions underlying the
code, including the API. I think it's in the best interest of libunwind
if the developers (yes, you David :-) remain sensitive and open minded
about the how well libunwind interfaces with or integrates into code
that has a need for unwinding.

Just something to think about,

-- 
 Marcel Moolenaar	  USPA: A-39004		 marcel@xcllnt.net

Re: RFA: ia64 portion of libunwind patch

[David Mosberger]

>>>>> On Sat, 8 Nov 2003 17:34:21 -0800, Marcel Moolenaar <marcel@xcllnt.net> said:

  Marcel> On Fri, Nov 07, 2003 at 03:12:29PM -0800, David Mosberger wrote:
  >> >>>>> On Fri, 07 Nov 2003 18:01:40 -0500, Andrew Cagney <ac131313@redhat.com> said:

  Andrew> Is fetching the table elements via a function, rather than a
  Andrew> direct access, really that significant an overhead?

  >> Is allocating a scratch buffer in gdb really such a big issue?
  >> It's very late for proposing libunwind API changes.

  Marcel> I think it's very early to have it nailed down.

No, the static code part of libunwind is not really open to
non-backwards-compatible changes anymore.  Things are slightly
different for the dynamic code support, but the static part has too
many users already for making code-breaking changes.  Also,
backwards-compatible changes are of course much easier to add.

  Marcel> Despite ones efforts to create a clean system, there are
  Marcel> always assumptions underlying the code, including the API. I
  Marcel> think it's in the best interest of libunwind if the
  Marcel> developers (yes, you David :-) remain sensitive and open
  Marcel> minded about the how well libunwind interfaces with or
  Marcel> integrates into code that has a need for unwinding.

It's not a good idea to make broad conclusions based on one particular
issue.  In this case, the issue appears to be entirely theoretical
(nobody has produced any numbers showing that, in a realistic
scenario, the current scheme makes, say, gdb startup X% slower, with X
being some large number).  Unless somebody produces such numbers,
I'm certainly not going to change the API in a non-backwards-compatible
fashion.

Perhaps we can find a backwards-compatible way of doing this, in which
case it's _much_ easier to add the feature.

Also, please keep in mind that libunwind is an open-source project.
If you don't like something, or think a feature is missing, submit a
clean patch.

	--david

Re: RFA: ia64 portion of libunwind patch

[David Mosberger]

>>>>> On Sat, 08 Nov 2003 18:40:49 -0500, Andrew Cagney <cagney@gnu.org> said:

  Andrew> What I don't think "libunwind" should be doing is assuming
  Andrew> that most efficient way to obtain specific elements of that
  Andrew> unwind table is to read that table en-mass.  Instead I think
  Andrew> "libunwind" be careful to only request the specific memory
  Andrew> locations that it needs - trust the client to supply the
  Andrew> requests in the most efficient way possible.

That's good in some cases and bad in others.  Based on gdb's needs, I
certainly could believe that there are some cases where incremental
reading would be faster.  Perhaps a stronger argument for incremental
reading would be if it actually simplified the glue code that gdb
needs to talk to libunwind.  I care a lot about making the glue code
as simple as possible and anything we can do to help there is of
interest to me.

  Andrew> I should note that this has come up before, GDB encountered
  Andrew> performance problems with a library that was trying to out
  Andrew> smart a memory read bottle neck by slurping ever increasing
  Andrew> chunks of unneeded memory.  This made the performance
  Andrew> problem worse not better - it was the volume of data and not
  Andrew> the number of xfers that was the bottle neck.

  Andrew> If we look at GDB with its 128k of unwind data.  At 14*28
  Andrew> byte requests per unwind, it would take ~300 unwinds before
  Andrew> GDB was required to xfer 128k (yes I'm pushing the numbers a
  Andrew> little here, but then I'm also ignoring the very significant
  Andrew> locality of the searches).

Oh, but you're ignoring the latency effects.  N 1-byte transfers can
easily be much slower than a single N-byte transfer.

  Andrew> Scary as it is, GDB's already got a requrest to feltch a
  Andrew> shared library image from the target's memory :-/.

That kind of throws your speed argument out of the water, though,
doesn't it? ;-)

  Andrew> Provided the remote target knows the address of the unwind
  Andrew> table, GDB should be able to find a way of getting it to
  Andrew> libunwind.

OK, I still don't quite understand why this is a common and important
scenario.  It strikes me as a corner-case which _occasionally_ may be
useful, but if that's true, a bit of extra latency doesn't seem like a
huge deal.

In any case, perhaps it is possible to add incremental reading support
by stealing a bit from one of the members in the "unw_dyn_table_info".
All we really need is a single bit to indicate whether the table-data
should be fetched from remote-memory.  I'll think about it some more.

	--david

Re: RFA: ia64 portion of libunwind patch

[Andrew Cagney]

>   Andrew> If we look at GDB with its 128k of unwind data.  At 14*28
>   Andrew> byte requests per unwind, it would take ~300 unwinds before
>   Andrew> GDB was required to xfer 128k (yes I'm pushing the numbers a
>   Andrew> little here, but then I'm also ignoring the very significant
>   Andrew> locality of the searches).
> 
> Oh, but you're ignoring the latency effects.  N 1-byte transfers can
> easily be much slower than a single N-byte transfer.

It's easy to play with the numbers here. Ex:  The remote protocol 
typically caps each transfer at ~1k.  So that would be 128 (xfer all) vs 
14 (xfer needed) so it would still be faster.

More seriously.  If problems are identified in the remote protocol, GDB 
should fix them.  It is important though, that its clients don't program 
around perceived performance problems and in doing so create artifical 
loads.  As my previous e-mail mentioned, GDB's already seen one example 
of that - a library demanding ever increasing amounts of data in attempt 
to work around an io throughput bottle neck.

>   Andrew> Scary as it is, GDB's already got a requrest to feltch a
>   Andrew> shared library image from the target's memory :-/.
> 
> That kind of throws your speed argument out of the water, though,
> doesn't it? ;-)

The extraction will need to be done very carefully so only required data 
is read.

>   Andrew> Provided the remote target knows the address of the unwind
>   Andrew> table, GDB should be able to find a way of getting it to
>   Andrew> libunwind.
> 
> OK, I still don't quite understand why this is a common and important
> scenario.  It strikes me as a corner-case which _occasionally_ may be
> useful, but if that's true, a bit of extra latency doesn't seem like a
> huge deal.
> 
> In any case, perhaps it is possible to add incremental reading support
> by stealing a bit from one of the members in the "unw_dyn_table_info".
> All we really need is a single bit to indicate whether the table-data
> should be fetched from remote-memory.  I'll think about it some more.

It would be appreciated.  My suggestion was to use memory reads when the 
unwind table pointer was NULL.  However, anything would help.

Andrew

Re: RFA: ia64 portion of libunwind patch

[David Mosberger]

>>>>> On Mon, 10 Nov 2003 17:43:40 -0500, Andrew Cagney <ac131313@redhat.com> said:

  Andrew> It would be appreciated.  My suggestion was to use memory
  Andrew> reads when the unwind table pointer was NULL.

Unfortunately, that doesn't work, because we need to know which
(remote) address the table is stored at.  We can probably find a bit
_somewhere_, I'm just not sure yet what the right place is.  Perhaps
it should be part of the address-space structure, instead.

	--david

Re: RFA: ia64 portion of libunwind patch

[Marcel Moolenaar]

On Mon, Nov 10, 2003 at 01:53:33PM -0800, David Mosberger wrote:
> 
>   >> Is allocating a scratch buffer in gdb really such a big issue?
>   >> It's very late for proposing libunwind API changes.
> 
>   Marcel> I think it's very early to have it nailed down.
> 
> No, the static code part of libunwind is not really open to
> non-backwards-compatible changes anymore.

Changing the API and breaking backward compatibility are two different
things. I guess you specifically meant non-backward-compatible API
changes when you only said API changes. That wasn't obvious though...

	...
>   Marcel> developers (yes, you David :-) remain sensitive and open
>   Marcel> minded about the how well libunwind interfaces with or
>   Marcel> integrates into code that has a need for unwinding.

	...
> Perhaps we can find a backwards-compatible way of doing this, in which
> case it's _much_ easier to add the feature.

This is pretty much what I mean with open-minded. Discussing how a
different libunwind API fits gdb better does not mean that the
libunwind API should be changed.  It's part of exploring how well
(or not) libunwind fits the gdb paradigm and vice versa and what
it takes to make it fit better if there's such a need. This, for
all I care, can be purely academic.

Heck, maybe libunwind2 can benefit from it :-)

Anyway: point cleared up. Let's move on.

-- 
 Marcel Moolenaar	  USPA: A-39004		 marcel@xcllnt.net

Re: RFA: ia64 portion of libunwind patch

[David Mosberger]

Hi Andrew,

>>>>> On Mon, 10 Nov 2003 17:43:40 -0500, Andrew Cagney <ac131313@redhat.com> said:

  >> In any case, perhaps it is possible to add incremental reading
  >> support by stealing a bit from one of the members in the
  >> "unw_dyn_table_info".  All we really need is a single bit to
  >> indicate whether the table-data should be fetched from
  >> remote-memory.  I'll think about it some more.

  Andrew> It would be appreciated.  My suggestion was to use memory
  Andrew> reads when the unwind table pointer was NULL.  However,
  Andrew> anything would help.

OK, I looked into this a bit more: we can't just redefine the meaning
of the unwind-table pointer, because it may have the wrong size when
unwinding across platforms.  For example, when cross-unwinding an ia64
application on an x86 host (happens regularly with the Ski simulator),
the pointer will be 32 bits in size but the remote address will be 64
bits.  The reverse problem can happen when registering unwind info, so
there is no quick way out.

Fortunately, it's still possible to have a clean solution: we can
simply add a new unwind-table format, which is identical to the
standard table format except that the table-data is a remote-address
(i.e., "unw_word_t" instead of "unw_word_t *").  I checked in a patch
to do just that in libunwind:

	http://unwind.bkbits.net:8080/libunwind/cset@1.123

If you don't want to deal with bitkeeper, libunwind v0.95 should be
due out shortly and will include this change.  I'm rather happy with
this solution because it's clearly the clean way of doing it and it
retains backwards compatibility.

The one incompatibility introduced by this patch is that the interface
of the _Uia64_find_dyn_list() helper-routine changed slightly.  The
only known user of that interface is gdb at the moment, so I think
that's OK (and, in retrospect, the old interface was a mistake).

Below, I attached a patch which updates gdb to use the remote-table
facility.  It doesn't try to retain backwards compatibility.  If that
were an issue, you could autoconf-check whether the enumeration symbol
UNW_INFO_FORMAT_REMOTE_TABLE is defined by <libunwind.h> and use the
old code if not.

While I didn't try to run the gdb test-suite, I did try a test program
which does some on-the-fly code-generation and the backtrace command
seemed to work fine.  For example:

(gdb) bt
#0  0x6000000000004022 in ?? ()
#1  0x60000000000040a0 in ?? ()
#2  0x6000000000004140 in ?? ()
#3  0x40000000000017e0 in main (argc=-19616, argv=0x6000000000004180)
    at ../../libunwind/tests/ia64-test-dyn1.c:196

Now if only gdb were able to call unw_get_proc_name() when it can't
find a symbol-name.  If it did that, the backtrace would even show the
proper function names for the dynamically generatd code.  Is this
something that could be added?

Another concern: at the moment, libunwind-frame.c dlopen()'s
libunwind.so.  This doesn't really work, because libunwind.so is just
a convenience symlink which allows native apps to link against the
proper libunwind with "-lunwind".  The real library is called

	libunwind-$TARGET.so

where $TARGET would be one of ia64, x86, or hppa, depending on the
target architecture.  For details, see the "Files" section of
this URL:

 http://www.hpl.hp.com/research/linux/libunwind/man/libunwind(3).php

Also, if multi-target support is to work, gdb would have to do a
separate dlopen for each target architecture being debugged.

Thanks,

	--david

ChangeLog

2003-11-25 David Mosberger  <davidm@hpl.hp.com>

	* libunwind-frame.c (unw_find_dyn_list_p): Replace most arguments
	with a single unw_dyn_info_t pointer.
	(libunwind_find_dyn_list): Likewise.
	* libunwind-frame.h: Likewise.

	* ia64-tdep.c (ia64_find_unwind_table): Switch to using
	UNW_INFO_FORMAT_REMOTE_TABLE so we can avoid having to read in the
	entire unwind-table at once.  Support for this table format has
	been added to libunwind v0.95.
	(ia64_find_proc_info_x): Adjust for remote-unwind-table changes.
	(ia64_get_dyn_info_list): Adjust for interface change for
	libunwind_find_dyn_list().

Index: ia64-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/ia64-tdep.c,v
retrieving revision 1.104
diff -u -r1.104 ia64-tdep.c
--- ia64-tdep.c	17 Nov 2003 21:38:36 -0000	1.104
+++ ia64-tdep.c	25 Nov 2003 23:48:24 -0000
@@ -2452,23 +2452,11 @@
   dip->start_ip = segbase;
   dip->end_ip = dip->start_ip + p_text->p_memsz;
   dip->gp = FIND_GLOBAL_POINTER (ip);
-  dip->format = UNW_INFO_FORMAT_TABLE;
-  dip->u.ti.name_ptr = (unw_word_t) bfd_get_filename (bfd);
-  dip->u.ti.segbase = segbase;
-  dip->u.ti.table_len = p_unwind->p_memsz / sizeof (unw_word_t);
-
-  /* The following can happen in corner cases where dynamically
-     generated code falls into the same page that contains the
-     data-segment and the page-offset of the code is within the first
-     page of the executable.  */
-  if (ip < dip->start_ip || ip >= dip->end_ip)
-    return -UNW_ENOINFO;
-
-  /* Read in the libunwind table.  */
-  *buf = xmalloc (p_unwind->p_memsz);
-  target_read_memory (p_unwind->p_vaddr + load_base, (char *)(*buf), p_unwind->p_memsz);
-
-  dip->u.ti.table_data = (unw_word_t *)(*buf);
+  dip->format = UNW_INFO_FORMAT_REMOTE_TABLE;
+  dip->u.rti.name_ptr = (unw_word_t) bfd_get_filename (bfd);
+  dip->u.rti.segbase = segbase;
+  dip->u.rti.table_len = p_unwind->p_memsz / sizeof (unw_word_t);
+  dip->u.rti.table_data = p_unwind->p_vaddr + load_base;
 
   return 0;
 }
@@ -2490,22 +2478,32 @@
 	 version.  */
       if (get_kernel_table (ip, &di) < 0)
 	return -UNW_ENOINFO;
+
+      if (gdbarch_debug >= 1)
+	fprintf_unfiltered (gdb_stdlog, "%s: %lx -> "
+			    "(name=`%s',segbase=%lx,start=%lx,end=%lx,gp=%lx,"
+			    "length=%lu,data=%p)\n", __FUNCTION__,
+			    ip, (char *)di.u.ti.name_ptr,
+			    di.u.ti.segbase, di.start_ip, di.end_ip,
+			    di.gp, di.u.ti.table_len, di.u.ti.table_data);
     }
   else
     {
       ret = ia64_find_unwind_table (sec->objfile, ip, &di, &buf);
       if (ret < 0)
 	return ret;
-    }
 
-  if (gdbarch_debug >= 1)
-    fprintf_unfiltered (gdb_stdlog, "acquire_unwind_info: %lx -> "
-			"(name=`%s',segbase=%lx,start=%lx,end=%lx,gp=%lx,"
-			"length=%lu,data=%p)\n", ip, (char *)di.u.ti.name_ptr,
-			di.u.ti.segbase, di.start_ip, di.end_ip,
-			di.gp, di.u.ti.table_len, di.u.ti.table_data);
+      if (gdbarch_debug >= 1)
+	fprintf_unfiltered (gdb_stdlog, "%s: %lx -> "
+			    "(name=`%s',segbase=%lx,start=%lx,end=%lx,gp=%lx,"
+			    "length=%lu,data=%lx)\n", __FUNCTION__,
+			    ip, (char *)di.u.rti.name_ptr,
+			    di.u.rti.segbase, di.start_ip, di.end_ip,
+			    di.gp, di.u.rti.table_len, di.u.rti.table_data);
+    }
 
-  ret = libunwind_search_unwind_table (&as, ip, &di, pi, need_unwind_info, arg);
+  ret = libunwind_search_unwind_table (&as, ip, &di, pi, need_unwind_info,
+				       arg);
 
   /* We no longer need the dyn info storage so free it.  */
   xfree (buf);
@@ -2545,10 +2543,7 @@
       ret = ia64_find_unwind_table (objfile, ip, &di, &buf);
       if (ret >= 0)
 	{
-	  addr = libunwind_find_dyn_list (as, di.u.ti.table_data,
-					  (di.u.ti.table_len
-					   * sizeof (di.u.ti.table_data[0])),
-					  di.u.ti.segbase, di.gp, arg);
+	  addr = libunwind_find_dyn_list (as, &di, arg);
 	  /* We no longer need the dyn info storage so free it.  */
 	  xfree (buf);
 
Index: libunwind-frame.c
===================================================================
RCS file: /cvs/src/src/gdb/libunwind-frame.c,v
retrieving revision 1.1
diff -u -r1.1 libunwind-frame.c
--- libunwind-frame.c	14 Nov 2003 21:17:51 -0000	1.1
+++ libunwind-frame.c	25 Nov 2003 23:48:24 -0000
@@ -58,8 +58,8 @@
 static unw_addr_space_t (*unw_create_addr_space_p) (unw_accessors_t *, int);
 static int (*unw_search_unwind_table_p) (unw_addr_space_t, unw_word_t, unw_dyn_info_t *,
 					 unw_proc_info_t *, int, void *);
-static unw_word_t (*unw_find_dyn_list_p) (unw_addr_space_t, void *, size_t,
-					  unw_word_t, unw_word_t, void *);
+static unw_word_t (*unw_find_dyn_list_p) (unw_addr_space_t, unw_dyn_info_t *,
+					  void *);
 
 
 struct libunwind_frame_cache
@@ -170,11 +170,10 @@
   return cache;
 }
 
-unw_word_t 
-libunwind_find_dyn_list (unw_addr_space_t as, void *table, size_t table_size,
-			 unw_word_t segbase, unw_word_t gp, void *arg)
+unw_word_t
+libunwind_find_dyn_list (unw_addr_space_t as, unw_dyn_info_t *di, void *arg)
 {
-  return unw_find_dyn_list_p (as, table, table_size, segbase, gp, arg);
+  return unw_find_dyn_list_p (as, di, arg);
 }
 
 static const struct frame_unwind libunwind_frame_unwind =
Index: libunwind-frame.h
===================================================================
RCS file: /cvs/src/src/gdb/libunwind-frame.h,v
retrieving revision 1.1
diff -u -r1.1 libunwind-frame.h
--- libunwind-frame.h	14 Nov 2003 21:17:51 -0000	1.1
+++ libunwind-frame.h	25 Nov 2003 23:48:24 -0000
@@ -55,9 +55,9 @@
 int libunwind_search_unwind_table (void *as, long ip, void *di,
 				   void *pi, int need_unwind_info, void *args);
 
-unw_word_t libunwind_find_dyn_list (unw_addr_space_t, void *, size_t,
-				    unw_word_t, unw_word_t, void *);
-     
+unw_word_t libunwind_find_dyn_list (unw_addr_space_t, unw_dyn_info_t *,
+				    void *);
+
 #endif /* libunwind-frame.h */
 
 #endif /* HAVE_LIBUNWIND_H  */

Re: RFA: ia64 portion of libunwind patch

[David Mosberger]

I have not gotten any feedback regarding my mail & patch from Nov 10:

  http://sources.redhat.com/ml/gdb-patches/2003-11/msg00588.html

libunwind v0.95 has been released in the meantime so I'd appreciate if
this patch could be checked in if it looks OK.

Thanks,

	--david

Re: RFA: ia64 portion of libunwind patch

[Kevin Buettner]

On Dec 3,  6:15pm, David Mosberger wrote:

> I have not gotten any feedback regarding my mail & patch from Nov 10:
> 
>   http://sources.redhat.com/ml/gdb-patches/2003-11/msg00588.html
> 
> libunwind v0.95 has been released in the meantime so I'd appreciate if
> this patch could be checked in if it looks OK.

Jeff,

Could you give it a try?  If it works okay, I have no problem with it
being committed.

Kevin

Re: RFA: ia64 portion of libunwind patch

[J. Johnston]

David,

   A questions regarding the .so name issue you mentioned.  We are already 
grabbing the function names from UNW_OBJ macro from the generic libunwind.h 
header.  I think we could generate the libunwind.so name similarly using the 
UNW_TARGET.  Any problems with this strategy? (any scenarios where this value 
doesn't match the extension used by the libunwind library?)

-- Jeff J.

David Mosberger wrote:
> I have not gotten any feedback regarding my mail & patch from Nov 10:
> 
>   http://sources.redhat.com/ml/gdb-patches/2003-11/msg00588.html
> 
> libunwind v0.95 has been released in the meantime so I'd appreciate if
> this patch could be checked in if it looks OK.
> 
> Thanks,
> 
> 	--david
> 

Re: RFA: ia64 portion of libunwind patch

[David Mosberger]

>>>>> On Thu, 04 Dec 2003 18:57:49 -0500, "J. Johnston" <jjohnstn@redhat.com> said:

  Jeff> A questions regarding the .so name issue you mentioned.  We
  Jeff> are already grabbing the function names from UNW_OBJ macro
  Jeff> from the generic libunwind.h header.  I think we could
  Jeff> generate the libunwind.so name similarly using the UNW_TARGET.
  Jeff> Any problems with this strategy? (any scenarios where this
  Jeff> value doesn't match the extension used by the libunwind
  Jeff> library?)

No, that sounds fine to me.  The part that I don't understand is that
at the moment it seems that only one libunwind-$TARGET.so can be
loaded.  With a multi-target-capable gdb, that would obviously not be
sufficient, as you'd want to load, say, libunwind-ia64.so.1 for ia64
and libunwind-x86.so.1 for x86.  But it's mostly a theoretical issue
at this point.

Thanks,

	--david

Re: RFA: ia64 portion of libunwind patch

[J. Johnston]

[-- Attachment #1: Type: text/plain, Size: 1463 bytes --]

David Mosberger wrote:
>>>>>>On Thu, 04 Dec 2003 18:57:49 -0500, "J. Johnston" <jjohnstn@redhat.com> said:
> 
> 
>   Jeff> A questions regarding the .so name issue you mentioned.  We
>   Jeff> are already grabbing the function names from UNW_OBJ macro
>   Jeff> from the generic libunwind.h header.  I think we could
>   Jeff> generate the libunwind.so name similarly using the UNW_TARGET.
>   Jeff> Any problems with this strategy? (any scenarios where this
>   Jeff> value doesn't match the extension used by the libunwind
>   Jeff> library?)
> 
> No, that sounds fine to me.  The part that I don't understand is that
> at the moment it seems that only one libunwind-$TARGET.so can be
> loaded.  With a multi-target-capable gdb, that would obviously not be
> sufficient, as you'd want to load, say, libunwind-ia64.so.1 for ia64
> and libunwind-x86.so.1 for x86.  But it's mostly a theoretical issue
> at this point.
> 
> Thanks,
> 
> 	--david
> 

I have created the patch to use UNW_TARGET.

Yes, the current implementation doesn't support multiple gdb targets, but it is 
set up to make such a task relatively straightforward in the future (i.e. simply 
put the function handles off of the target vector and pass in the target name).

Kevin, ok to commit?

-- Jeff J.

2003-12-10  Jeff Johnston  <jjohnstn@redhat.com>

	* libunwind-frame.c: If LIBUNWIND_SO not defined, define it to be
	libunwind_target_lib.
	(libunwind_target_lib): New static char string.



[-- Attachment #2: libunwind.patch --]
[-- Type: text/plain, Size: 941 bytes --]

Index: libunwind-frame.c
===================================================================
RCS file: /cvs/src/src/gdb/libunwind-frame.c,v
retrieving revision 1.1
diff -u -r1.1 libunwind-frame.c
--- libunwind-frame.c	14 Nov 2003 21:17:51 -0000	1.1
+++ libunwind-frame.c	10 Dec 2003 20:52:10 -0000
@@ -46,7 +46,7 @@
 static struct gdbarch_data *libunwind_descr_handle;
 
 #ifndef LIBUNWIND_SO
-#define LIBUNWIND_SO "libunwind.so"
+#define LIBUNWIND_SO libunwind_target_lib
 #endif
 
 /* Required function pointers from libunwind.  */
@@ -83,6 +83,8 @@
 static char *create_addr_space_name = STRINGIFY(UNW_OBJ(create_addr_space));
 static char *search_unwind_table_name = STRINGIFY(UNW_OBJ(search_unwind_table));
 static char *find_dyn_list_name = STRINGIFY(UNW_OBJ(find_dyn_list));
+
+static char *libunwind_target_lib = "libunwind-" STRINGIFY(UNW_TARGET) ".so";
 
 static struct libunwind_descr *
 libunwind_descr (struct gdbarch *gdbarch)

Re: RFA: ia64 portion of libunwind patch

[David Mosberger]

>>>>> On Wed, 10 Dec 2003 15:58:37 -0500, "J. Johnston" <jjohnstn@redhat.com> said:

  Jeff> I have created the patch to use UNW_TARGET.

  Jeff> Yes, the current implementation doesn't support multiple gdb
  Jeff> targets, but it is set up to make such a task relatively
  Jeff> straightforward in the future (i.e. simply put the function
  Jeff> handles off of the target vector and pass in the target name).

Ah, I see.

Thanks for fixing this!

	--david

Re: RFA: ia64 portion of libunwind patch

[Kevin Buettner]

On Dec 10,  3:58pm, J. Johnston wrote:

> David Mosberger wrote:
> >>>>>>On Thu, 04 Dec 2003 18:57:49 -0500, "J. Johnston" <jjohnstn@redhat.com> said:
> > 
> > 
> >   Jeff> A questions regarding the .so name issue you mentioned.  We
> >   Jeff> are already grabbing the function names from UNW_OBJ macro
> >   Jeff> from the generic libunwind.h header.  I think we could
> >   Jeff> generate the libunwind.so name similarly using the UNW_TARGET.
> >   Jeff> Any problems with this strategy? (any scenarios where this
> >   Jeff> value doesn't match the extension used by the libunwind
> >   Jeff> library?)
> > 
> > No, that sounds fine to me.  The part that I don't understand is that
> > at the moment it seems that only one libunwind-$TARGET.so can be
> > loaded.  With a multi-target-capable gdb, that would obviously not be
> > sufficient, as you'd want to load, say, libunwind-ia64.so.1 for ia64
> > and libunwind-x86.so.1 for x86.  But it's mostly a theoretical issue
> > at this point.
> > 
> > Thanks,
> > 
> > 	--david
> > 
> 
> I have created the patch to use UNW_TARGET.
> 
> Yes, the current implementation doesn't support multiple gdb
> targets, but it is set up to make such a task relatively
> straightforward in the future (i.e.  simply put the function handles
> off of the target vector and pass in the target name).
> 
> Kevin, ok to commit?

Sure.

Kevin

Re: RFA: ia64 portion of libunwind patch

[Kevin Buettner]

On Nov 25,  4:11pm, David Mosberger wrote:

> 	* libunwind-frame.c (unw_find_dyn_list_p): Replace most arguments
> 	with a single unw_dyn_info_t pointer.
> 	(libunwind_find_dyn_list): Likewise.
> 	* libunwind-frame.h: Likewise.
> 
> 	* ia64-tdep.c (ia64_find_unwind_table): Switch to using
> 	UNW_INFO_FORMAT_REMOTE_TABLE so we can avoid having to read in the
> 	entire unwind-table at once.  Support for this table format has
> 	been added to libunwind v0.95.
> 	(ia64_find_proc_info_x): Adjust for remote-unwind-table changes.
> 	(ia64_get_dyn_info_list): Adjust for interface change for
> 	libunwind_find_dyn_list().

I've just committed this patch for David.

Kevin

make inferior calls work on ia64 even when syscall is pending

[David Mosberger]

Problem:

 $ cat t.c
 int main () {
   char ch;
   return read(0, &ch, 1);
 }
 $ gcc -g t.c
 $ gdb a.out
 GNU gdb 6.0-debian
 Copyright 2003 Free Software Foundation, Inc.
 GDB is free software, covered by the GNU General Public License, and you are
 welcome to change it and/or distribute copies of it under certain conditions.
 Type "show copying" to see the conditions.
 There is absolutely no warranty for GDB.  Type "show warranty" for details.
 This GDB was configured as "ia64-linux"...
 (gdb) r
 Starting program: /home/davidm/tmp/a.out

 [user hits Ctrl-C]

 Program received signal SIGINT, Interrupt.
 0x20000000001cc401 in read () from /lib/tls/libc.so.6.1
 (gdb) call write(1, "hello\n", 6)

 Program received signal SIGSEGV, Segmentation fault.
 0x2000000000020650 in __libc_memalign () from /lib/ld-linux-ia64.so.2
 The program being debugged was signaled while in a function called from GDB.
 GDB remains in the frame where the signal was received.
 To change this behavior use "set unwindonsignal on"
 Evaluation of the expression containing the function (malloc) will be abandoned.
 (gdb)

The attached patch below fixes this problem.  It took me a while to
understand what's going on, but in retrospect, the situation is
completely analogous to x86 linux: when interrupting execution in a
restartable syscall, gdb needs to cancel restarting of the syscall
before it can run an inferior call.  Otherwise, instead of executing
at the desired IP, we'll end up calling IP-1 (in my test-case, this
ended up calling into the tail of memalign, when the intended target
was malloc() to allocate memory for the string "hello\n"...).

The fix is for gdb to clear r10 when changing the IP.  This will
ensure that the kernel won't treat the value in r8 as an error-code
and effectively cancels the system-call restart.

It turns out that the kernel also needs a small patch, because it
currently checks r10 _before_ waking up the debugger, so that there
was no way for the debugger to cancel system-call restart.  See this
patch:

	http://lia64.bkbits.net:8080/to-linus-2.5/patch@1.1513

(Bjorn, this is also needed for Linux v2.4.)

With both the kernel and gdb fixed, I see four new passes in the
GDB test-suite:

+PASS: gdb.base/interrupt.exp: call function when asleep
+PASS: gdb.base/interrupt.exp: call function a second time
+PASS: gdb.base/interrupt.exp: continue
+PASS: gdb.base/interrupt.exp: send end of file
                === gdb Summary ===

-# of expected passes           9928
+# of expected passes           9932
 # of unexpected failures       119
 # of unexpected successes      4
 # of expected failures         58

If the gdb patch looks OK, please check it in.

Thanks!

	--david

2003-12-31  David Mosberger  <davidm@hpl.hp.com>

	* ia64-tdep.c (ia64_write_pc): Clear r10 after writing the
	instruction-pointer (PC) to prevent the kernel from attempting to
	restart an interrupt system call.

Index: ia64-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/ia64-tdep.c,v
retrieving revision 1.106
diff -u -r1.106 ia64-tdep.c
--- ia64-tdep.c	13 Dec 2003 03:51:56 -0000	1.106
+++ ia64-tdep.c	31 Dec 2003 19:49:49 -0000
@@ -683,6 +683,17 @@
 
   write_register_pid (IA64_PSR_REGNUM, psr_value, ptid);
   write_register_pid (IA64_IP_REGNUM, new_pc, ptid);
+
+  /* We must be careful with modifying the instruction-pointer: if we
+     just interrupt a system call, the kernel would ordinarily try to
+     restart it when we resume the inferior, which typically results
+     in SIGSEGV or SIGILL.  We prevent this by clearing r10, which
+     will tell the kernel that r8 does NOT contain a valid error code
+     and hence it will skip system-call restart.
+
+     The clearing of r10 is safe as long as ia64_write_pc() is only
+     called as part of setting up an inferior call.  */
+  write_register (IA64_GR10_REGNUM, 0);
 }
 
 #define IS_NaT_COLLECTION_ADDR(addr) ((((addr) >> 3) & 0x3f) == 0x3f)

Re: make inferior calls work on ia64 even when syscall is pending

[Mark Kettenis]

   From: David Mosberger <davidm@napali.hpl.hp.com>
   Date: Wed, 31 Dec 2003 12:19:20 -0800

   If the gdb patch looks OK, please check it in.

David, there are two issues with the patch:

* I suspect that this approach is Linux-specific.  If so, you really
  shouldn't be adding this bit of code to the generic ia64_write_pc().
  Instead you should create a Linux-specific ia64_linux_write_pc() and
  put it in ia64-linux-tdep.c; you'll probably want to call the
  generic ia64_write_pc() from there.  Then you must hook in the
  Linux-specific version if the OSABI is GDB_OSABI_LINUX.

* You should use write_register_pid() instead of write_register().

Mark

   Thanks!

	   --david

   2003-12-31  David Mosberger  <davidm@hpl.hp.com>

	   * ia64-tdep.c (ia64_write_pc): Clear r10 after writing the
	   instruction-pointer (PC) to prevent the kernel from attempting to
	   restart an interrupt system call.

   Index: ia64-tdep.c
   ===================================================================
   RCS file: /cvs/src/src/gdb/ia64-tdep.c,v
   retrieving revision 1.106
   diff -u -r1.106 ia64-tdep.c
   --- ia64-tdep.c	13 Dec 2003 03:51:56 -0000	1.106
   +++ ia64-tdep.c	31 Dec 2003 19:49:49 -0000
   @@ -683,6 +683,17 @@

      write_register_pid (IA64_PSR_REGNUM, psr_value, ptid);
      write_register_pid (IA64_IP_REGNUM, new_pc, ptid);
   +
   +  /* We must be careful with modifying the instruction-pointer: if we
   +     just interrupt a system call, the kernel would ordinarily try to
   +     restart it when we resume the inferior, which typically results
   +     in SIGSEGV or SIGILL.  We prevent this by clearing r10, which
   +     will tell the kernel that r8 does NOT contain a valid error code
   +     and hence it will skip system-call restart.
   +
   +     The clearing of r10 is safe as long as ia64_write_pc() is only
   +     called as part of setting up an inferior call.  */
   +  write_register (IA64_GR10_REGNUM, 0);
    }

    #define IS_NaT_COLLECTION_ADDR(addr) ((((addr) >> 3) & 0x3f) == 0x3f)

Re: make inferior calls work on ia64 even when syscall is pending

[David Mosberger]

>>>>> On Thu, 1 Jan 2004 00:33:23 +0100 (CET), Mark Kettenis <kettenis@chello.nl> said:

  Mark>    If the gdb patch looks OK, please check it in.

  Mark> David, there are two issues with the patch:

  Mark> * I suspect that this approach is Linux-specific.  If so, you
  Mark> really shouldn't be adding this bit of code to the generic
  Mark> ia64_write_pc().  Instead you should create a Linux-specific
  Mark> ia64_linux_write_pc() and put it in ia64-linux-tdep.c; you'll
  Mark> probably want to call the generic ia64_write_pc() from there.
  Mark> Then you must hook in the Linux-specific version if the OSABI
  Mark> is GDB_OSABI_LINUX.

I'm not sure how Linux-specific it really is.  I think most Unixish
ia64 OSes use r10 as the syscall error flag.  I'll leave this one to
the gdb experts that know the other ia64 OSes better than I do.

  Mark> * You should use write_register_pid() instead of
  Mark> write_register().

Ah, yes, I had the code in ia64_push_dummy_call() and failed to change
that when moving it to ia64_write_pc().

	--david

Re: make inferior calls work on ia64 even when syscall is pending

[David Mosberger]

>>>>> On Thu, 1 Jan 2004 00:33:23 +0100 (CET), Mark Kettenis <kettenis@chello.nl> said:

  Mark>    From: David Mosberger <davidm@napali.hpl.hp.com> Date: Wed,
  Mark> 31 Dec 2003 12:19:20 -0800

  Mark>    If the gdb patch looks OK, please check it in.

  Mark> David, there are two issues with the patch:

  Mark> * I suspect that this approach is Linux-specific.  If so, you
  Mark> really shouldn't be adding this bit of code to the generic
  Mark> ia64_write_pc().  Instead you should create a Linux-specific
  Mark> ia64_linux_write_pc() and put it in ia64-linux-tdep.c; you'll
  Mark> probably want to call the generic ia64_write_pc() from there.
  Mark> Then you must hook in the Linux-specific version if the OSABI
  Mark> is GDB_OSABI_LINUX.

  Mark> * You should use write_register_pid() instead of
  Mark> write_register().

How about the attached patch?

	--david

ChangeLog

2004-02-12  David Mosberger  <davidm@hpl.hp.com>

	* ia64-tdep.c (ia64_linux_write_pc): Declare as extern.
	(ia64_write_pc): Make it a global function.
	(ia64_gdbarch_init): For Linux targets, use ia64_linux_write_pc()
	instead of ia64_write_pc().

	* ia64-linux-tdep.c (ia64_write_pc): Declare as extern.
	(ia64_linux_write_pc): New function.  Works like ia64_write_pc(),
	except that it also clears r10 afterwards to prevent the kernel
	from attempting to restart an interrupt system call.

Index: ia64-linux-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/ia64-linux-tdep.c,v
retrieving revision 1.5
diff -u -r1.5 ia64-linux-tdep.c
--- ia64-linux-tdep.c	8 Jan 2004 16:46:27 -0000	1.5
+++ ia64-linux-tdep.c	13 Feb 2004 01:12:11 -0000
@@ -34,6 +34,8 @@
 /* Offset to sigcontext structure from frame of handler */
 #define IA64_LINUX_SIGCONTEXT_OFFSET 192
 
+extern void ia64_write_pc (CORE_ADDR, ptid_t);
+
 int
 ia64_linux_in_sigtramp (CORE_ADDR pc, char *func_name)
 {
@@ -92,4 +94,21 @@
       default :
 	return 0;
       }
+}
+
+void
+ia64_linux_write_pc (CORE_ADDR pc, ptid_t ptid)
+{
+  ia64_write_pc (pc, ptid);
+
+  /* We must be careful with modifying the instruction-pointer: if we
+     just interrupt a system call, the kernel would ordinarily try to
+     restart it when we resume the inferior, which typically results
+     in SIGSEGV or SIGILL.  We prevent this by clearing r10, which
+     will tell the kernel that r8 does NOT contain a valid error code
+     and hence it will skip system-call restart.
+
+     The clearing of r10 is safe as long as ia64_linux_write_pc() is only
+     called as part of setting up an inferior call.  */
+  write_register_pid (IA64_GR10_REGNUM, 0, ptid);
 }
Index: ia64-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/ia64-tdep.c,v
retrieving revision 1.112
diff -u -r1.112 ia64-tdep.c
--- ia64-tdep.c	10 Feb 2004 19:35:59 -0000	1.112
+++ ia64-tdep.c	13 Feb 2004 01:12:11 -0000
@@ -94,6 +94,7 @@
 extern CORE_ADDR ia64_linux_sigcontext_register_address (CORE_ADDR, int);
 extern CORE_ADDR ia64_aix_sigcontext_register_address (CORE_ADDR, int);
 extern unsigned long ia64_linux_getunwind_table (void *, size_t);
+extern void ia64_linux_write_pc (CORE_ADDR, ptid_t);
 
 static gdbarch_init_ftype ia64_gdbarch_init;
 
@@ -632,7 +633,7 @@
   return pc_value | (slot_num * SLOT_MULTIPLIER);
 }
 
-static void
+void
 ia64_write_pc (CORE_ADDR new_pc, ptid_t ptid)
 {
   int slot_num = (int) (new_pc & 0xf) / SLOT_MULTIPLIER;
@@ -3339,7 +3348,10 @@
   set_gdbarch_memory_remove_breakpoint (gdbarch, ia64_memory_remove_breakpoint);
   set_gdbarch_breakpoint_from_pc (gdbarch, ia64_breakpoint_from_pc);
   set_gdbarch_read_pc (gdbarch, ia64_read_pc);
-  set_gdbarch_write_pc (gdbarch, ia64_write_pc);
+  if (info.osabi == GDB_OSABI_LINUX)
+    set_gdbarch_write_pc (gdbarch, ia64_linux_write_pc);
+  else
+    set_gdbarch_write_pc (gdbarch, ia64_write_pc);
 
   /* Settings for calling functions in the inferior.  */
   set_gdbarch_push_dummy_call (gdbarch, ia64_push_dummy_call);

Re: make inferior calls work on ia64 even when syscall is pending

[Mark Kettenis]

   From: David Mosberger <davidm@napali.hpl.hp.com>
   Date: Thu, 12 Feb 2004 17:14:10 -0800

   >>>>> On Thu, 1 Jan 2004 00:33:23 +0100 (CET), Mark Kettenis <kettenis@chello.nl> said:

     Mark>    From: David Mosberger <davidm@napali.hpl.hp.com> Date: Wed,
     Mark> 31 Dec 2003 12:19:20 -0800

     Mark>    If the gdb patch looks OK, please check it in.

     Mark> David, there are two issues with the patch:

     Mark> * I suspect that this approach is Linux-specific.  If so, you
     Mark> really shouldn't be adding this bit of code to the generic
     Mark> ia64_write_pc().  Instead you should create a Linux-specific
     Mark> ia64_linux_write_pc() and put it in ia64-linux-tdep.c; you'll
     Mark> probably want to call the generic ia64_write_pc() from there.
     Mark> Then you must hook in the Linux-specific version if the OSABI
     Mark> is GDB_OSABI_LINUX.

     Mark> * You should use write_register_pid() instead of
     Mark> write_register().

   How about the attached patch?

Looks good, except for the "extern" function proptotypes.  But I guess
we can't blame you for following the bad example set by ia64-tdep.c.

Kevin, can you approve this patch and create an ia64-tdep.h file with
the function prototypes afterwards?

David, You're not mentioned in the MAINTAINERS file.  Do you have a
valid FSF copyright assignment for GDB?

Mark

Re: make inferior calls work on ia64 even when syscall is pending

[Andrew Cagney]

> David, You're not mentioned in the MAINTAINERS file.  Do you have a
> valid FSF copyright assignment for GDB?

Yes (I got an e-mail confirming this yesterday).

Re: make inferior calls work on ia64 even when syscall is pending

[Andrew Cagney]

> Looks good, except for the "extern" function proptotypes.  But I guess
> we can't blame you for following the bad example set by ia64-tdep.c.

Fixing, that by creating "ia64-tdep.h", is "obvious".

Andrew

Re: make inferior calls work on ia64 even when syscall is pending

[David Mosberger]

>>>>> On Fri, 13 Feb 2004 10:12:21 -0500, Andrew Cagney <cagney@gnu.org> said:

  >> Looks good, except for the "extern" function proptotypes.  But I
  >> guess we can't blame you for following the bad example set by
  >> ia64-tdep.c.

  Andrew> Fixing, that by creating "ia64-tdep.h", is "obvious".

Oh, man, you do know how to suck me into doing extra work...

How is this one?  Caveat: I know nothing about the gdb build
environment, so someone may want to double-check the Makefile changes.

Thanks,

	--david

ChangeLog

2004-02-13  David Mosberger  <davidm@hpl.hp.com>

	* Makefile.in (ia64_tdep_h): New macro.
	(ia64-linux-tdep.o): Mention $(ia64_tdep_h).
	(ia64-tdep.o): Likewise.

	* ia64-tdep.h (ia64_linux_write_pc): New file.
	* ia64-tdep.c: Update copyright notice.
	Include "ia64-tdep.h".
	(ia64_linux_sigcontext_register_address): Move decl to ia64-tdep.h.
	(ia64_aix_sigcontext_register_address): Likewise.
	(ia64_linux_getunwind_table): Likewise.
	* ia64-linux-tdep.c: Likewise.
	Include "regcache.h" to get write_register_pid() declared.

2004-02-12  David Mosberger  <davidm@hpl.hp.com>

	* ia64-tdep.c (ia64_linux_write_pc): Declare as extern.
	(ia64_write_pc): Make it a global function.
	(ia64_gdbarch_init): For Linux targets, use ia64_linux_write_pc()
	instead of ia64_write_pc().

	* ia64-linux-tdep.c (ia64_write_pc): Declare as extern.
	(ia64_linux_write_pc): New function.  Works like ia64_write_pc(),
	except that it also clears r10 afterwards to prevent the kernel
	from attempting to restart an interrupt system call.

Index: Makefile.in
--- Makefile.in
+++ Makefile.in
@@ -683,6 +683,7 @@
 i386_linux_tdep_h = i386-linux-tdep.h
 i386_tdep_h = i386-tdep.h
 i387_tdep_h = i387-tdep.h
+ia64_tdep_h = ia64-tdep.h
 infcall_h = infcall.h
 inferior_h = inferior.h $(breakpoint_h) $(target_h) $(frame_h)
 inf_loop_h = inf-loop.h
@@ -1916,13 +1917,14 @@
 ia64-aix-tdep.o: ia64-aix-tdep.c $(defs_h)
 ia64-linux-nat.o: ia64-linux-nat.c $(defs_h) $(gdb_string_h) $(inferior_h) \
 	$(target_h) $(gdbcore_h) $(regcache_h) $(gdb_wait_h) $(gregset_h)
-ia64-linux-tdep.o: ia64-linux-tdep.c $(defs_h) $(arch_utils_h) $(gdbcore_h)
+ia64-linux-tdep.o: ia64-linux-tdep.c $(defs_h) $(arch_utils_h) $(gdbcore_h) \
+	$(ia64_tdep_h)
 ia64-tdep.o: ia64-tdep.c $(defs_h) $(inferior_h) $(symfile_h) $(gdbcore_h) \
 	$(arch_utils_h) $(floatformat_h) $(regcache_h) $(reggroups_h) \
 	$(frame_h) $(frame_base_h) $(frame_unwind_h) $(doublest_h) \
 	$(value_h) $(gdb_assert_h) $(objfiles_h) $(elf_common_h) \
 	$(elf_bfd_h) $(elf_h) $(dis_asm_h) $(libunwind_frame_h) \
-	$(libunwind_ia64_h)
+	$(libunwind_ia64_h) $(ia64_tdep_h)
 infcall.o: infcall.c $(defs_h) $(breakpoint_h) $(target_h) $(regcache_h) \
 	$(inferior_h) $(gdb_assert_h) $(block_h) $(gdbcore_h) $(language_h) \
 	$(symfile_h) $(gdbcmd_h) $(command_h) $(gdb_string_h) $(infcall_h)
Index: ia64-linux-tdep.c
--- ia64-linux-tdep.c
+++ ia64-linux-tdep.c
@@ -20,8 +20,10 @@
    Boston, MA 02111-1307, USA.  */
 
 #include "defs.h"
+#include "ia64-tdep.h"
 #include "arch-utils.h"
 #include "gdbcore.h"
+#include "regcache.h"
 
 /* The sigtramp code is in a non-readable (executable-only) region
    of memory called the ``gate page''.  The addresses in question
@@ -93,3 +95,20 @@
 	return 0;
       }
 }
+
+void
+ia64_linux_write_pc (CORE_ADDR pc, ptid_t ptid)
+{
+  ia64_write_pc (pc, ptid);
+
+  /* We must be careful with modifying the instruction-pointer: if we
+     just interrupt a system call, the kernel would ordinarily try to
+     restart it when we resume the inferior, which typically results
+     in SIGSEGV or SIGILL.  We prevent this by clearing r10, which
+     will tell the kernel that r8 does NOT contain a valid error code
+     and hence it will skip system-call restart.
+
+     The clearing of r10 is safe as long as ia64_write_pc() is only
+     called as part of setting up an inferior call.  */
+  write_register_pid (IA64_GR10_REGNUM, 0, ptid);
+}
Index: ia64-tdep.c
--- ia64-tdep.c
+++ ia64-tdep.c
@@ -1,6 +1,6 @@
 /* Target-dependent code for the IA-64 for GDB, the GNU debugger.
 
-   Copyright 1999, 2000, 2001, 2002, 2003 Free Software Foundation, Inc.
+   Copyright 1999, 2000, 2001, 2002, 2003, 2004 Free Software Foundation, Inc.
 
    This file is part of GDB.
 
@@ -38,6 +38,7 @@
 #include "elf-bfd.h"
 #include "elf.h"                /* for PT_IA64_UNWIND value */
 #include "dis-asm.h"
+#include "ia64-tdep.h"
 
 #ifdef HAVE_LIBUNWIND_IA64_H
 #include "libunwind-frame.h"
@@ -90,11 +91,6 @@
 
 #define BUNDLE_LEN 16
 
-/* FIXME: These extern declarations should go in ia64-tdep.h.  */
-extern CORE_ADDR ia64_linux_sigcontext_register_address (CORE_ADDR, int);
-extern CORE_ADDR ia64_aix_sigcontext_register_address (CORE_ADDR, int);
-extern unsigned long ia64_linux_getunwind_table (void *, size_t);
-
 static gdbarch_init_ftype ia64_gdbarch_init;
 
 static gdbarch_register_name_ftype ia64_register_name;
@@ -632,7 +628,7 @@
   return pc_value | (slot_num * SLOT_MULTIPLIER);
 }
 
-static void
+void
 ia64_write_pc (CORE_ADDR new_pc, ptid_t ptid)
 {
   int slot_num = (int) (new_pc & 0xf) / SLOT_MULTIPLIER;
@@ -3339,7 +3343,10 @@
   set_gdbarch_memory_remove_breakpoint (gdbarch, ia64_memory_remove_breakpoint);
   set_gdbarch_breakpoint_from_pc (gdbarch, ia64_breakpoint_from_pc);
   set_gdbarch_read_pc (gdbarch, ia64_read_pc);
-  set_gdbarch_write_pc (gdbarch, ia64_write_pc);
+  if (info.osabi == GDB_OSABI_LINUX)
+    set_gdbarch_write_pc (gdbarch, ia64_linux_write_pc);
+  else
+    set_gdbarch_write_pc (gdbarch, ia64_write_pc);
 
   /* Settings for calling functions in the inferior.  */
   set_gdbarch_push_dummy_call (gdbarch, ia64_push_dummy_call);
Index: ia64-tdep.h
--- /dev/null
+++ ia64-tdep.h
@@ -0,0 +1,32 @@
+/* Target-dependent code for the ia64.
+
+   Copyright 2004
+   Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 2 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program; if not, write to the Free Software
+   Foundation, Inc., 59 Temple Place - Suite 330,
+   Boston, MA 02111-1307, USA.  */
+
+#ifndef IA64_TDEP_H
+#define IA64_TDEP_H
+
+extern CORE_ADDR ia64_linux_sigcontext_register_address (CORE_ADDR, int);
+extern CORE_ADDR ia64_aix_sigcontext_register_address (CORE_ADDR, int);
+extern unsigned long ia64_linux_getunwind_table (void *, size_t);
+extern void ia64_write_pc (CORE_ADDR, ptid_t);
+extern void ia64_linux_write_pc (CORE_ADDR, ptid_t);
+
+#endif /* IA64_TDEP_H */

Re: make inferior calls work on ia64 even when syscall is pending

[Andrew Cagney]

[-- Attachment #1: Type: text/plain, Size: 1172 bytes --]

> 2004-02-13  David Mosberger  <davidm@hpl.hp.com>
> 
> 	* Makefile.in (ia64_tdep_h): New macro.
> 	(ia64-linux-tdep.o): Mention $(ia64_tdep_h).
> 	(ia64-tdep.o): Likewise.
> 
> 	* ia64-tdep.h (ia64_linux_write_pc): New file.
> 	* ia64-tdep.c: Update copyright notice.
> 	Include "ia64-tdep.h".
> 	(ia64_linux_sigcontext_register_address): Move decl to ia64-tdep.h.
> 	(ia64_aix_sigcontext_register_address): Likewise.
> 	(ia64_linux_getunwind_table): Likewise.
> 	* ia64-linux-tdep.c: Likewise.
> 	Include "regcache.h" to get write_register_pid() declared.
> 
I've checked this part in as obvious (David doesn't want ssh access :-) 
which leaves:

> 2004-02-12  David Mosberger  <davidm@hpl.hp.com>
> 
> 	* ia64-tdep.c (ia64_linux_write_pc): Declare as extern.
> 	(ia64_write_pc): Make it a global function.
> 	(ia64_gdbarch_init): For Linux targets, use ia64_linux_write_pc()
> 	instead of ia64_write_pc().
> 
> 	* ia64-linux-tdep.c (ia64_write_pc): Declare as extern.
> 	(ia64_linux_write_pc): New function.  Works like ia64_write_pc(),
> 	except that it also clears r10 afterwards to prevent the kernel
> 	from attempting to restart an interrupt system call.

Andrew


[-- Attachment #2: diffs --]
[-- Type: text/plain, Size: 5296 bytes --]

2004-02-17  David Mosberger  <davidm@hpl.hp.com>

	Committed by Andrew Cagney.
	* Makefile.in (ia64_tdep_h): New macro.
	(ia64-linux-tdep.o): Mention $(ia64_tdep_h).
	(ia64-tdep.o): Likewise.
	* ia64-tdep.h: New file.
	* ia64-tdep.c: Update copyright notice.  Include "ia64-tdep.h".
	(ia64_linux_sigcontext_register_address): Move decl to ia64-tdep.h.
	(ia64_aix_sigcontext_register_address): Likewise.
	(ia64_linux_getunwind_table): Delete declaration.
	* ia64-linux-tdep.c: Likewise.

Index: Makefile.in
===================================================================
RCS file: /cvs/src/src/gdb/Makefile.in,v
retrieving revision 1.502
diff -u -r1.502 Makefile.in
--- Makefile.in	17 Feb 2004 15:21:21 -0000	1.502
+++ Makefile.in	17 Feb 2004 16:19:32 -0000
@@ -683,6 +683,7 @@
 i386_linux_tdep_h = i386-linux-tdep.h
 i386_tdep_h = i386-tdep.h
 i387_tdep_h = i387-tdep.h
+ia64_tdep_h = ia64-tdep.h
 infcall_h = infcall.h
 inferior_h = inferior.h $(breakpoint_h) $(target_h) $(frame_h)
 inf_loop_h = inf-loop.h
@@ -1823,13 +1824,14 @@
 ia64-aix-tdep.o: ia64-aix-tdep.c $(defs_h)
 ia64-linux-nat.o: ia64-linux-nat.c $(defs_h) $(gdb_string_h) $(inferior_h) \
 	$(target_h) $(gdbcore_h) $(regcache_h) $(gdb_wait_h) $(gregset_h)
-ia64-linux-tdep.o: ia64-linux-tdep.c $(defs_h) $(arch_utils_h) $(gdbcore_h)
+ia64-linux-tdep.o: ia64-linux-tdep.c $(defs_h) $(ia64_tdep_h) \
+	$(arch_utils_h) $(gdbcore_h)
 ia64-tdep.o: ia64-tdep.c $(defs_h) $(inferior_h) $(gdbcore_h) \
 	$(arch_utils_h) $(floatformat_h) $(regcache_h) $(reggroups_h) \
 	$(frame_h) $(frame_base_h) $(frame_unwind_h) $(doublest_h) \
 	$(value_h) $(gdb_assert_h) $(objfiles_h) $(elf_common_h) \
-	$(elf_bfd_h) $(elf_h) $(dis_asm_h) $(libunwind_frame_h) \
-	$(libunwind_ia64_h)
+	$(elf_bfd_h) $(elf_h) $(dis_asm_h) $(ia64_tdep_h) \
+	$(libunwind_frame_h) $(libunwind_ia64_h)
 infcall.o: infcall.c $(defs_h) $(breakpoint_h) $(target_h) $(regcache_h) \
 	$(inferior_h) $(gdb_assert_h) $(block_h) $(gdbcore_h) $(language_h) \
 	$(objfiles_h) $(gdbcmd_h) $(command_h) $(gdb_string_h) $(infcall_h)
Index: ia64-linux-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/ia64-linux-tdep.c,v
retrieving revision 1.5
diff -u -r1.5 ia64-linux-tdep.c
--- ia64-linux-tdep.c	8 Jan 2004 16:46:27 -0000	1.5
+++ ia64-linux-tdep.c	17 Feb 2004 16:19:32 -0000
@@ -1,6 +1,6 @@
 /* Target-dependent code for the IA-64 for GDB, the GNU debugger.
-   Copyright 2000, 2004
-   Free Software Foundation, Inc.
+
+   Copyright 2000, 2004 Free Software Foundation, Inc.
 
    This file is part of GDB.
 
@@ -20,6 +20,7 @@
    Boston, MA 02111-1307, USA.  */
 
 #include "defs.h"
+#include "ia64-tdep.h"
 #include "arch-utils.h"
 #include "gdbcore.h"
 
Index: ia64-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/ia64-tdep.c,v
retrieving revision 1.115
diff -u -r1.115 ia64-tdep.c
--- ia64-tdep.c	17 Feb 2004 15:21:21 -0000	1.115
+++ ia64-tdep.c	17 Feb 2004 16:19:32 -0000
@@ -1,6 +1,7 @@
 /* Target-dependent code for the IA-64 for GDB, the GNU debugger.
 
-   Copyright 1999, 2000, 2001, 2002, 2003 Free Software Foundation, Inc.
+   Copyright 1999, 2000, 2001, 2002, 2003, 2004 Free Software
+   Foundation, Inc.
 
    This file is part of GDB.
 
@@ -37,6 +38,7 @@
 #include "elf-bfd.h"
 #include "elf.h"                /* for PT_IA64_UNWIND value */
 #include "dis-asm.h"
+#include "ia64-tdep.h"
 
 #ifdef HAVE_LIBUNWIND_IA64_H
 #include "libunwind-frame.h"
@@ -88,11 +90,6 @@
 /* Length in bytes of an instruction bundle */
 
 #define BUNDLE_LEN 16
-
-/* FIXME: These extern declarations should go in ia64-tdep.h.  */
-extern CORE_ADDR ia64_linux_sigcontext_register_address (CORE_ADDR, int);
-extern CORE_ADDR ia64_aix_sigcontext_register_address (CORE_ADDR, int);
-extern unsigned long ia64_linux_getunwind_table (void *, size_t);
 
 static gdbarch_init_ftype ia64_gdbarch_init;
 
Index: ia64-tdep.h
===================================================================
RCS file: ia64-tdep.h
diff -N ia64-tdep.h
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ ia64-tdep.h	17 Feb 2004 16:19:32 -0000
@@ -0,0 +1,29 @@
+/* Target-dependent code for the ia64.
+
+   Copyright 2004 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 2 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program; if not, write to the Free Software
+   Foundation, Inc., 59 Temple Place - Suite 330,
+   Boston, MA 02111-1307, USA.  */
+
+#ifndef IA64_TDEP_H
+#define IA64_TDEP_H
+
+extern CORE_ADDR ia64_linux_sigcontext_register_address (CORE_ADDR, int);
+extern CORE_ADDR ia64_aix_sigcontext_register_address (CORE_ADDR, int);
+extern unsigned long ia64_linux_getunwind_table (void *, size_t);
+
+#endif /* IA64_TDEP_H */

Re: make inferior calls work on ia64 even when syscall is pending

[Kevin Buettner]

On Tue, 17 Feb 2004 11:21:54 -0500
Andrew Cagney <cagney@gnu.org> wrote:

> > 2004-02-13  David Mosberger  <davidm@hpl.hp.com>
> > 
> > 	* Makefile.in (ia64_tdep_h): New macro.
> > 	(ia64-linux-tdep.o): Mention $(ia64_tdep_h).
> > 	(ia64-tdep.o): Likewise.
> > 
> > 	* ia64-tdep.h (ia64_linux_write_pc): New file.
> > 	* ia64-tdep.c: Update copyright notice.
> > 	Include "ia64-tdep.h".
> > 	(ia64_linux_sigcontext_register_address): Move decl to ia64-tdep.h.
> > 	(ia64_aix_sigcontext_register_address): Likewise.
> > 	(ia64_linux_getunwind_table): Likewise.
> > 	* ia64-linux-tdep.c: Likewise.
> > 	Include "regcache.h" to get write_register_pid() declared.
> > 
> I've checked this part in as obvious (David doesn't want ssh access :-) 

Thanks for doing this.  (I was away last week and am slowing getting caught
up...)

> which leaves:
> 
> > 2004-02-12  David Mosberger  <davidm@hpl.hp.com>
> > 
> > 	* ia64-tdep.c (ia64_linux_write_pc): Declare as extern.
> > 	(ia64_write_pc): Make it a global function.
> > 	(ia64_gdbarch_init): For Linux targets, use ia64_linux_write_pc()
> > 	instead of ia64_write_pc().
> > 
> > 	* ia64-linux-tdep.c (ia64_write_pc): Declare as extern.
> > 	(ia64_linux_write_pc): New function.  Works like ia64_write_pc(),
> > 	except that it also clears r10 afterwards to prevent the kernel
> > 	from attempting to restart an interrupt system call.

I'll take care of committing this portion.

Kevin

Re: make inferior calls work on ia64 even when syscall is pending

[Kevin Buettner]

On Fri, 13 Feb 2004 14:06:54 -0800
David Mosberger <davidm@napali.hpl.hp.com> wrote:

> 2004-02-12  David Mosberger  <davidm@hpl.hp.com>
> 
> 	* ia64-tdep.c (ia64_linux_write_pc): Declare as extern.
> 	(ia64_write_pc): Make it a global function.
> 	(ia64_gdbarch_init): For Linux targets, use ia64_linux_write_pc()
> 	instead of ia64_write_pc().
> 
> 	* ia64-linux-tdep.c (ia64_write_pc): Declare as extern.
> 	(ia64_linux_write_pc): New function.  Works like ia64_write_pc(),
> 	except that it also clears r10 afterwards to prevent the kernel
> 	from attempting to restart an interrupt system call.

I've committed the above via the following patch.  Please let me know
if I've missed anything.

	* ia64-tdep.h (ia64_write_pc, ia64_linux_write_pc): Declare.
	* ia64-tdep.c (ia64_write_pc): Make it a global function.
	(ia64_gdbarch_init): For Linux targets, use ia64_linux_write_pc()
	instead of ia64_write_pc().
	* ia64-linux-tdep.c (regcache.h): Include.
	(ia64_linux_write_pc): New function.

Index: ia64-linux-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/ia64-linux-tdep.c,v
retrieving revision 1.6
diff -u -p -r1.6 ia64-linux-tdep.c
--- ia64-linux-tdep.c	17 Feb 2004 16:23:23 -0000	1.6
+++ ia64-linux-tdep.c	23 Feb 2004 21:06:43 -0000
@@ -23,6 +23,7 @@
 #include "ia64-tdep.h"
 #include "arch-utils.h"
 #include "gdbcore.h"
+#include "regcache.h"
 
 /* The sigtramp code is in a non-readable (executable-only) region
    of memory called the ``gate page''.  The addresses in question
@@ -93,4 +94,21 @@ ia64_linux_sigcontext_register_address (
       default :
 	return 0;
       }
+}
+
+void
+ia64_linux_write_pc (CORE_ADDR pc, ptid_t ptid)
+{
+  ia64_write_pc (pc, ptid);
+
+  /* We must be careful with modifying the instruction-pointer: if we
+     just interrupt a system call, the kernel would ordinarily try to
+     restart it when we resume the inferior, which typically results
+     in SIGSEGV or SIGILL.  We prevent this by clearing r10, which
+     will tell the kernel that r8 does NOT contain a valid error code
+     and hence it will skip system-call restart.
+
+     The clearing of r10 is safe as long as ia64_write_pc() is only
+     called as part of setting up an inferior call.  */
+  write_register_pid (IA64_GR10_REGNUM, 0, ptid);
 }
Index: ia64-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/ia64-tdep.c,v
retrieving revision 1.116
diff -u -p -r1.116 ia64-tdep.c
--- ia64-tdep.c	17 Feb 2004 16:23:23 -0000	1.116
+++ ia64-tdep.c	23 Feb 2004 21:06:44 -0000
@@ -628,7 +628,7 @@ ia64_read_pc (ptid_t ptid)
   return pc_value | (slot_num * SLOT_MULTIPLIER);
 }
 
-static void
+void
 ia64_write_pc (CORE_ADDR new_pc, ptid_t ptid)
 {
   int slot_num = (int) (new_pc & 0xf) / SLOT_MULTIPLIER;
@@ -3338,7 +3338,10 @@ ia64_gdbarch_init (struct gdbarch_info i
   set_gdbarch_memory_remove_breakpoint (gdbarch, ia64_memory_remove_breakpoint);
   set_gdbarch_breakpoint_from_pc (gdbarch, ia64_breakpoint_from_pc);
   set_gdbarch_read_pc (gdbarch, ia64_read_pc);
-  set_gdbarch_write_pc (gdbarch, ia64_write_pc);
+  if (info.osabi == GDB_OSABI_LINUX)
+    set_gdbarch_write_pc (gdbarch, ia64_linux_write_pc);
+  else
+    set_gdbarch_write_pc (gdbarch, ia64_write_pc);
 
   /* Settings for calling functions in the inferior.  */
   set_gdbarch_push_dummy_call (gdbarch, ia64_push_dummy_call);
Index: ia64-tdep.h
===================================================================
RCS file: /cvs/src/src/gdb/ia64-tdep.h,v
retrieving revision 1.1
diff -u -p -r1.1 ia64-tdep.h
--- ia64-tdep.h	17 Feb 2004 16:23:23 -0000	1.1
+++ ia64-tdep.h	23 Feb 2004 21:06:44 -0000
@@ -25,5 +25,7 @@
 extern CORE_ADDR ia64_linux_sigcontext_register_address (CORE_ADDR, int);
 extern CORE_ADDR ia64_aix_sigcontext_register_address (CORE_ADDR, int);
 extern unsigned long ia64_linux_getunwind_table (void *, size_t);
+extern void ia64_write_pc (CORE_ADDR, ptid_t);
+extern void ia64_linux_write_pc (CORE_ADDR, ptid_t);
 
 #endif /* IA64_TDEP_H */