thr3ads.net - Xen devel - [Xen-devel] [PATCH] gdbserver-xen: fix corefile access [Mar 2006]

If this information is useful, please help other people find it:
Share via:

Horms

2006-Mar-02 08:28 UTC

[Xen-devel] [PATCH] gdbserver-xen: fix corefile access

# HG changeset patch
# User Horms <horms@verge.net.au>
# Node ID d15c3e045aa21d0753761121ef3c9a0f73c0e82a
# Parent  48cbeecfa8a4b438f72fbe440f0642344f2f24a4
gdbserver-xen: fix corefile access

This patch fixes corefile access by ensuring that the open fd
to the corefile (current_domid) is passed to the underlying
xc_routines as neccessary. Currently the prevailing pid is passed,
which works fine when attaching to a running domain, but fails
for the corefile case.

This is done by creaating a wrapper for curvcpuid(), called
curvcpuid_or_domfd() which returns current_domid if the
global isfile is set. This assumes that isfile is stable,
if this is not the caes, it could easily be saved at
the time that myptrace is set using this variable.

I did not replace curvcpuid(), as it seems to be needed
elsewhere, though it may be possible to eliminate it.

This patch also relocates curvcpuid() (without modifying it).
This is because curvcpuid_or_domfd() needs to below the
declaration of isfile, and it seems to make sense to put
curvcpuid() and curvcpuid_or_domfd() next to each other.

Signed-Off-By: Horms <horms@verge.net.au>

diff -r 48cbeecfa8a4 -r d15c3e045aa2
tools/debugger/gdb/gdb-6.2.1-xen-sparse/gdb/gdbserver/linux-xen-low.c
--- a/tools/debugger/gdb/gdb-6.2.1-xen-sparse/gdb/gdbserver/linux-xen-low.c	Thu
Mar  2 09:30:54 2006 +0900
+++ b/tools/debugger/gdb/gdb-6.2.1-xen-sparse/gdb/gdbserver/linux-xen-low.c	Thu
Mar  2 17:18:42 2006 +0900
@@ -45,17 +45,6 @@ int (*myxcwait)(int xc_handle, int domai
 int (*myxcwait)(int xc_handle, int domain, int *status, int options) ;
 static int xc_handle;
 
-static inline int
-curvcpuid()
-{
-  struct process_info *process;
-  if (current_inferior == NULL)
-      return 0;
-  process = get_thread_process(current_inferior);
-  return (process->thread_known ? process->tid : 0);
-
-}
-
 
 #define DOMFLAGS_DYING     (1<<0) /* Domain is scheduled to die.         
*/
 #define DOMFLAGS_SHUTDOWN  (1<<2) /* The guest OS has shut down.         
*/
@@ -87,6 +76,23 @@ struct pending_signals
 
 static int use_regsets_p = 1;
 
+static inline int
+curvcpuid()
+{
+  struct process_info *process;
+  if (current_inferior == NULL)
+      return 0;
+  process = get_thread_process(current_inferior);
+  return (process->thread_known ? process->tid : 0);
+}
+
+static inline int
+curvcpuid_or_domfd()
+{
+  if(isfile)
+	  return current_domid;
+  return curvcpuid();
+}
 
 #define pid_of(proc) ((proc)->head.id)
 
@@ -276,7 +282,7 @@ regsets_fetch_inferior_registers ()
 
       buf = malloc (regset->size);
       res = myptrace (xc_handle, regset->get_request, 
-		      curvcpuid(),
+		      curvcpuid_or_domfd(),
 		      0, (PTRACE_XFER_TYPE)buf);
       if (res < 0)
 	{
@@ -329,7 +335,7 @@ regsets_store_inferior_registers ()
 
       buf = malloc (regset->size);
       regset->fill_function (buf);
-      res = myptrace (xc_handle, regset->set_request, curvcpuid(), 0,
(PTRACE_XFER_TYPE)buf);
+      res = myptrace (xc_handle, regset->set_request, curvcpuid_or_domfd(),
0, (PTRACE_XFER_TYPE)buf);
       if (res < 0)
 	{
 	  if (errno == EIO)
@@ -407,7 +413,7 @@ linux_read_memory (CORE_ADDR memaddr, ch
   for (i = 0; i < count; i++, addr += sizeof (PTRACE_XFER_TYPE))
     {
       errno = 0;
-      buffer[i] = myptrace (xc_handle, PTRACE_PEEKTEXT, curvcpuid(),
(PTRACE_ARG3_TYPE) addr, 0);
+      buffer[i] = myptrace (xc_handle, PTRACE_PEEKTEXT, curvcpuid_or_domfd(),
(PTRACE_ARG3_TYPE) addr, 0);
       if (errno)
 	return errno;
     }
@@ -440,13 +446,13 @@ linux_write_memory (CORE_ADDR memaddr, c
 
   /* Fill start and end extra bytes of buffer with existing memory data.  */
 
-  buffer[0] = myptrace (xc_handle, PTRACE_PEEKTEXT, curvcpuid(),
+  buffer[0] = myptrace (xc_handle, PTRACE_PEEKTEXT, curvcpuid_or_domfd(),
 		      (PTRACE_ARG3_TYPE) addr, 0);
 
   if (count > 1)
     {
       buffer[count - 1]
-	= myptrace (xc_handle, PTRACE_PEEKTEXT, curvcpuid(),
+	= myptrace (xc_handle, PTRACE_PEEKTEXT, curvcpuid_or_domfd(),
 		  (PTRACE_ARG3_TYPE) (addr + (count - 1)
 				      * sizeof (PTRACE_XFER_TYPE)),
 		  0);
@@ -460,7 +466,7 @@ linux_write_memory (CORE_ADDR memaddr, c
   for (i = 0; i < count; i++, addr += sizeof (PTRACE_XFER_TYPE))
     {
       errno = 0;
-      myptrace (xc_handle, PTRACE_POKETEXT, curvcpuid(), 
+      myptrace (xc_handle, PTRACE_POKETEXT, current_domid, 
 		(PTRACE_ARG3_TYPE) addr, buffer[i]);
       if (errno)
 	return errno;


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Keir Fraser

2006-Mar-02 10:45 UTC

head link

Re: [Xen-devel] [PATCH] gdbserver-xen: fix corefile access

On 2 Mar 2006, at 08:28, Horms wrote:
> This patch fixes corefile access by ensuring that the open fd
> to the corefile (current_domid) is passed to the underlying
> xc_routines as neccessary. Currently the prevailing pid is passed,
> which works fine when attaching to a running domain, but fails
> for the corefile case.
This is too gross.

The correct fix is to update the xc_ptrace_core() interface to match 
the xc_ptrace() interface. Kip Macy made the latter SMP aware, but 
didn''t fix up the former.

It should be easy to do -- note how xc_ptrace() takes a domid on 
PTRACE_ATTACH, and vcpuid at all other times. xc_ptrace_core() should 
take a fd on PTRACE_ATTACH, and vcpuid at all other times. Since we 
don''t dump SMP core files right now, vcpuid should either be ignored 
for the time being, or fail the call if vcpuid!=0.

  -- Keir

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Horms

2006-Mar-02 12:19 UTC

head link

Re: [Xen-devel] [PATCH] gdbserver-xen: fix corefile access

On Thu, Mar 02, 2006 at 10:45:02AM +0000, Keir Fraser
wrote:> 
> On 2 Mar 2006, at 08:28, Horms wrote:
> 
> >This patch fixes corefile access by ensuring that the open fd
> >to the corefile (current_domid) is passed to the underlying
> >xc_routines as neccessary. Currently the prevailing pid is passed,
> >which works fine when attaching to a running domain, but fails
> >for the corefile case.
> 
> This is too gross.
Sorry about that.
> The correct fix is to update the xc_ptrace_core() interface to match 
> the xc_ptrace() interface. Kip Macy made the latter SMP aware, but 
> didn''t fix up the former.
> 
> It should be easy to do -- note how xc_ptrace() takes a domid on 
> PTRACE_ATTACH, and vcpuid at all other times. xc_ptrace_core() should 
> take a fd on PTRACE_ATTACH, and vcpuid at all other times. Since we 
> don''t dump SMP core files right now, vcpuid should either be
ignored
> for the time being, or fail the call if vcpuid!=0.
I didn''t notice that, but I should have.

Are you suggesting that xc_ptrace_core() should record the fd passed
to it on PTRACE_ATTACH and use that later, presumably in current_domid?
If so, yes that does look very easy. If not, can you explain a little
further? In any case, I''ll look into it tomorrow.

-- 
Horms

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Keir Fraser

2006-Mar-02 13:24 UTC

head link

Re: [Xen-devel] [PATCH] gdbserver-xen: fix corefile access

On 2 Mar 2006, at 12:19, Horms wrote:
>> The correct fix is to update the xc_ptrace_core() interface to match
>> the xc_ptrace() interface. Kip Macy made the latter SMP aware, but
>> didn''t fix up the former.
>>
>> It should be easy to do -- note how xc_ptrace() takes a domid on
>> PTRACE_ATTACH, and vcpuid at all other times. xc_ptrace_core() should
>> take a fd on PTRACE_ATTACH, and vcpuid at all other times. Since we
>> don''t dump SMP core files right now, vcpuid should either be
ignored
>> for the time being, or fail the call if vcpuid!=0.
>
> I didn''t notice that, but I should have.
>
> Are you suggesting that xc_ptrace_core() should record the fd passed
> to it on PTRACE_ATTACH and use that later, presumably in current_domid?
> If so, yes that does look very easy. If not, can you explain a little
> further? In any case, I''ll look into it tomorrow.
Yeah, you should record it the same way that xc_ptrace() records the 
domid. Really the two calls (xc_ptrace and xc_ptrace_core) should 
probably be merged -- we could pass an extra flag to PTRACE_ATTACH to 
indicate whether we are attaching to a coredump or to a live domain. 
Then we could get rid of xc_ptrace_core altogether.

  -- Keir


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Horms

2006-Mar-03 01:20 UTC

head link

Re: [Xen-devel] [PATCH] gdbserver-xen: fix corefile access

On Thu, Mar 02, 2006 at 01:24:41PM +0000, Keir Fraser
wrote:> 
> On 2 Mar 2006, at 12:19, Horms wrote:
> 
> >>The correct fix is to update the xc_ptrace_core() interface to
match
> >>the xc_ptrace() interface. Kip Macy made the latter SMP aware, but
> >>didn''t fix up the former.
> >>
> >>It should be easy to do -- note how xc_ptrace() takes a domid on
> >>PTRACE_ATTACH, and vcpuid at all other times. xc_ptrace_core()
should
> >>take a fd on PTRACE_ATTACH, and vcpuid at all other times. Since we
> >>don''t dump SMP core files right now, vcpuid should either
be ignored
> >>for the time being, or fail the call if vcpuid!=0.
> >
> >I didn''t notice that, but I should have.
> >
> >Are you suggesting that xc_ptrace_core() should record the fd passed
> >to it on PTRACE_ATTACH and use that later, presumably in current_domid?
> >If so, yes that does look very easy. If not, can you explain a little
> >further? In any case, I''ll look into it tomorrow.
> 
> Yeah, you should record it the same way that xc_ptrace() records the 
> domid. Really the two calls (xc_ptrace and xc_ptrace_core) should 
> probably be merged -- we could pass an extra flag to PTRACE_ATTACH to 
> indicate whether we are attaching to a coredump or to a live domain. 
> Then we could get rid of xc_ptrace_core altogether.
That sounds reasonable to me. Though internally the do different things,
so would the idea be to something like:

  rename xc_ptrace xc_ptrace_thread
  make xc_ptrace a wapper for xc_ptrace_thread and xc_ptrace_core

Also, I haven''t poked into this, but it seems that
xc_waitdomain_core/xc_waitdomain could have the same treatment, and thus
both myptrace and myxcwait could be replaced with direct calls to
xc_ptrace and xc_waitdomain respectively.

-- 
Horms

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Horms

2006-Mar-03 01:28 UTC

head link

Re: [Xen-devel] [PATCH] gdbserver-xen: fix corefile access

On Fri, Mar 03, 2006 at 10:20:21AM +0900, Horms wrote:> On Thu, Mar 02, 2006 at 01:24:41PM +0000, Keir Fraser wrote:
> > 
> > On 2 Mar 2006, at 12:19, Horms wrote:
> > 
> > >>The correct fix is to update the xc_ptrace_core() interface to
match
> > >>the xc_ptrace() interface. Kip Macy made the latter SMP aware,
but
> > >>didn''t fix up the former.
> > >>
> > >>It should be easy to do -- note how xc_ptrace() takes a domid
on
> > >>PTRACE_ATTACH, and vcpuid at all other times. xc_ptrace_core()
should
> > >>take a fd on PTRACE_ATTACH, and vcpuid at all other times.
Since we
> > >>don''t dump SMP core files right now, vcpuid should
either be ignored
> > >>for the time being, or fail the call if vcpuid!=0.
> > >
> > >I didn''t notice that, but I should have.
> > >
> > >Are you suggesting that xc_ptrace_core() should record the fd
passed
> > >to it on PTRACE_ATTACH and use that later, presumably in
current_domid?
> > >If so, yes that does look very easy. If not, can you explain a
little
> > >further? In any case, I''ll look into it tomorrow.
> > 
> > Yeah, you should record it the same way that xc_ptrace() records the 
> > domid. Really the two calls (xc_ptrace and xc_ptrace_core) should 
> > probably be merged -- we could pass an extra flag to PTRACE_ATTACH to 
> > indicate whether we are attaching to a coredump or to a live domain. 
> > Then we could get rid of xc_ptrace_core altogether.
> 
> That sounds reasonable to me. Though internally the do different things,
> so would the idea be to something like:
> 
>   rename xc_ptrace xc_ptrace_thread
>   make xc_ptrace a wapper for xc_ptrace_thread and xc_ptrace_core
Sorry, scratch that. I think they can just be merged into one
function, I''ll try and get a patch together.

Do you think it would be better to pass the isfile flag in the form
of PTRACE_ATTACH|XC_PTRACE_FILE as the third argument to xc_ptrace,
or parhaps use the last argument to xc_ptrace, edata (or perhaps the
second last one, eaddr), which are unused for ATTACH.

-- 
Horms

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Keir Fraser

2006-Mar-03 08:11 UTC

head link

Re: [Xen-devel] [PATCH] gdbserver-xen: fix corefile access

On 3 Mar 2006, at 01:28, Horms wrote:
> Sorry, scratch that. I think they can just be merged into one
> function, I''ll try and get a patch together.
>
> Do you think it would be better to pass the isfile flag in the form
> of PTRACE_ATTACH|XC_PTRACE_FILE as the third argument to xc_ptrace,
> or parhaps use the last argument to xc_ptrace, edata (or perhaps the
> second last one, eaddr), which are unused for ATTACH.
Having xc_ptrace_thread/xc_ptrace_core *internally* in libxenctrl would 
be fine, but there should be only one public function (xc_ptrace). You 
can update PTRACE_ATTACH as you wish -- I guess adding a new ptrac 
ecommand would be cleaned: PTRACE_ATTACH_COREFILE would be okay.

And yes, I expect you can give the same treatment to xc_waitdomain. It 
probably should be a no-op when debugging a core file (although 
currently it loads the core file state into memory, which is weird but 
I guess works okay).

  -- Keir


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Horms

2006-Mar-03 11:15 UTC

head link

Re: [Xen-devel] [PATCH] gdbserver-xen: fix corefile access

On Fri, Mar 03, 2006 at 10:28:05AM +0900, Horms wrote:> On Fri, Mar 03, 2006 at 10:20:21AM +0900, Horms wrote:
> > On Thu, Mar 02, 2006 at 01:24:41PM +0000, Keir Fraser wrote:
> > > 
> > > On 2 Mar 2006, at 12:19, Horms wrote:
> > > 
> > > >>The correct fix is to update the xc_ptrace_core()
interface to match
> > > >>the xc_ptrace() interface. Kip Macy made the latter SMP
aware, but
> > > >>didn''t fix up the former.
> > > >>
> > > >>It should be easy to do -- note how xc_ptrace() takes a
domid on
> > > >>PTRACE_ATTACH, and vcpuid at all other times.
xc_ptrace_core() should
> > > >>take a fd on PTRACE_ATTACH, and vcpuid at all other
times. Since we
> > > >>don''t dump SMP core files right now, vcpuid
should either be ignored
> > > >>for the time being, or fail the call if vcpuid!=0.
> > > >
> > > >I didn''t notice that, but I should have.
> > > >
> > > >Are you suggesting that xc_ptrace_core() should record the fd
passed
> > > >to it on PTRACE_ATTACH and use that later, presumably in
current_domid?
> > > >If so, yes that does look very easy. If not, can you explain
a little
> > > >further? In any case, I''ll look into it tomorrow.
> > > 
> > > Yeah, you should record it the same way that xc_ptrace() records
the
> > > domid. Really the two calls (xc_ptrace and xc_ptrace_core) should
> > > probably be merged -- we could pass an extra flag to
PTRACE_ATTACH to
> > > indicate whether we are attaching to a coredump or to a live
domain.
> > > Then we could get rid of xc_ptrace_core altogether.
> > 
> > That sounds reasonable to me. Though internally the do different
things,
> > so would the idea be to something like:
> > 
> >   rename xc_ptrace xc_ptrace_thread
> >   make xc_ptrace a wapper for xc_ptrace_thread and xc_ptrace_core
> 
> Sorry, scratch that. I think they can just be merged into one
> function, I''ll try and get a patch together.
> 
> Do you think it would be better to pass the isfile flag in the form
> of PTRACE_ATTACH|XC_PTRACE_FILE as the third argument to xc_ptrace,
> or parhaps use the last argument to xc_ptrace, edata (or perhaps the
> second last one, eaddr), which are unused for ATTACH.
I have made a first pass at this, passing isfile as the data argument
to xc_ptrace() ATTACH. Its a little rough, but I wanted to get it out
for feedback before I head off for the day.

There are 4 patches attached, which need to be applied in order.
Please let me know if posting patches in this way is a problem.
And of course, please let me know of any and all objections
so that I can refactor (or explain) accordingly.

-- 
Horms







_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Keir Fraser

2006-Mar-03 13:02 UTC

head link

Re: [Xen-devel] [PATCH] gdbserver-xen: fix corefile access

On 3 Mar 2006, at 11:15, Horms wrote:
> I have made a first pass at this, passing isfile as the data argument
> to xc_ptrace() ATTACH. Its a little rough, but I wanted to get it out
> for feedback before I head off for the day.
>
> There are 4 patches attached, which need to be applied in order.
> Please let me know if posting patches in this way is a problem.
> And of course, please let me know of any and all objections
> so that I can refactor (or explain) accordingly.
Yeah, diffstat tells me that you probably have the right approach (more 
lines deleted than added). I''d prefer fewer but bigger patches. e.g., 
merge the first three ''cleanup'' patches into one.

  -- Keir


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Horms

2006-Mar-06 10:08 UTC

head link

Re: [Xen-devel] [PATCH] gdbserver-xen: fix corefile access

On Fri, Mar 03, 2006 at 01:02:59PM +0000, Keir Fraser
wrote:> 
> On 3 Mar 2006, at 11:15, Horms wrote:
> 
> >I have made a first pass at this, passing isfile as the data argument
> >to xc_ptrace() ATTACH. Its a little rough, but I wanted to get it out
> >for feedback before I head off for the day.
> >
> >There are 4 patches attached, which need to be applied in order.
> >Please let me know if posting patches in this way is a problem.
> >And of course, please let me know of any and all objections
> >so that I can refactor (or explain) accordingly.
> 
> Yeah, diffstat tells me that you probably have the right approach (more 
> lines deleted than added). I''d prefer fewer but bigger patches.
e.g.,
> merge the first three ''cleanup'' patches into one.
Here is the second round of patches. Please apply them in order.

The current split is a cleanup patch, followed by the consolidation
patch.  Localy I actually have the cleanup patch broken out into the
error handling changes and other changes - as per the split in the
changelog entry. Let me know if you want me to send the broken out
patches.

I''ve done a little bit of testing, and I can trace both a core
and a running domain. Testing and feedback welcome.

-- 
Horms





_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Xen devel - Mar 2006 - [PATCH] gdbserver-xen: fix corefile access

[Xen-devel] [PATCH] gdbserver-xen: fix corefile access

Re: [Xen-devel] [PATCH] gdbserver-xen: fix corefile access

Re: [Xen-devel] [PATCH] gdbserver-xen: fix corefile access

Re: [Xen-devel] [PATCH] gdbserver-xen: fix corefile access

Re: [Xen-devel] [PATCH] gdbserver-xen: fix corefile access

Re: [Xen-devel] [PATCH] gdbserver-xen: fix corefile access

Re: [Xen-devel] [PATCH] gdbserver-xen: fix corefile access

Re: [Xen-devel] [PATCH] gdbserver-xen: fix corefile access

Re: [Xen-devel] [PATCH] gdbserver-xen: fix corefile access

Re: [Xen-devel] [PATCH] gdbserver-xen: fix corefile access