* [RFA] windows-nat.c: Handle ERROR_PARTIAL_COPY in windows_xfer_memory function
@ 2013-09-01 22:10 Pierre Muller
0 siblings, 0 replies; 6+ messages in thread
From: Pierre Muller @ 2013-09-01 22:10 UTC (permalink / raw)
To: gdb-patches
This is the patch that Pedro suggested I send
after his commit to remove deprecated_xfer_memory
in windows-nat.c.
Pedro suggested that I submit this patch separately
(which I do here)... and with a gdbserver counterpart,
which I don't...
I tried, but finally realized that given the
read_memory / write_memory functions type defined
in target.h target_ops structure,
there is no way of passing information of partial
copy and of the length of this partial copy.
Indeed, the comments state that the return value is either 0 for success
or errno...
This is not compatible with returning information that only part of the
request length
was read/written.
Changing this semantics is too much work with high risks of breaking
things elsewhere...
Pierre Muller
GDB pascal language maintainer
PS: the use of plongest function is because I got an warning about %d
used for 'long long integer' type.
2013-09-01 Pierre Muller <muller@sourceware.org>
* windows-nat.c (windows_xfer_memory): Fix compilation failure
by use of plongest function.
Handle ERROR_PARTIAL_COPY error code.
Index: src/gdb/windows-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/windows-nat.c,v
retrieving revision 1.258
diff -u -p -r1.258 windows-nat.c
--- src/gdb/windows-nat.c 27 Aug 2013 11:36:09 -0000 1.258
+++ src/gdb/windows-nat.c 1 Sep 2013 21:20:51 -0000
@@ -2324,26 +2324,34 @@ windows_xfer_memory (gdb_byte *readbuf,
{
SIZE_T done = 0;
BOOL success;
+ DWORD lasterror = 0;
if (writebuf != NULL)
{
- DEBUG_MEM (("gdb: write target memory, %d bytes at %s\n",
- len, core_addr_to_string (memaddr)));
+ DEBUG_MEM (("gdb: write target memory, %s bytes at %s\n",
+ plongest (len), core_addr_to_string (memaddr)));
success = WriteProcessMemory (current_process_handle,
(LPVOID) (uintptr_t) memaddr, writebuf,
len, &done);
+ if (!success)
+ lasterror = GetLastError ();
FlushInstructionCache (current_process_handle,
(LPCVOID) (uintptr_t) memaddr, len);
}
else
{
- DEBUG_MEM (("gdb: read target memory, %d bytes at %s\n",
- len, core_addr_to_string (memaddr)));
+ DEBUG_MEM (("gdb: read target memory, %s bytes at %s\n",
+ plongest (len), core_addr_to_string (memaddr)));
success = ReadProcessMemory (current_process_handle,
(LPCVOID) (uintptr_t) memaddr, readbuf,
len, &done);
+ if (!success)
+ lasterror = GetLastError ();
}
- return success ? done : TARGET_XFER_E_IO;
+ if (!success && lasterror == ERROR_PARTIAL_COPY && done > 0)
+ return done;
+ else
+ return success ? done : TARGET_XFER_E_IO;
}
static void
^ permalink raw reply [flat|nested] 6+ messages in thread[parent not found: <5223bb46.c6c0420a.5a41.008dSMTPIN_ADDED_BROKEN@mx.google.com>]
* Re: [RFA] windows-nat.c: Handle ERROR_PARTIAL_COPY in windows_xfer_memory function
[not found] <5223bb46.c6c0420a.5a41.008dSMTPIN_ADDED_BROKEN@mx.google.com>
@ 2013-09-02 12:34 ` Pedro Alves
2013-09-02 12:48 ` Pierre Muller
2013-09-02 12:50 ` Pedro Alves
1 sibling, 1 reply; 6+ messages in thread
From: Pedro Alves @ 2013-09-02 12:34 UTC (permalink / raw)
To: Pierre Muller; +Cc: gdb-patches
On 09/01/2013 11:10 PM, Pierre Muller wrote:
>
> PS: the use of plongest function is because I got an warning about %d
> used for 'long long integer' type.
Then that's a regression from my deprecated_xfer_memory change.
Please keep logically independent changes as separate patches.
> 2013-09-01 Pierre Muller <muller@sourceware.org>
>
> * windows-nat.c (windows_xfer_memory): Fix compilation failure
> by use of plongest function.
This part is OK. Please split it out, and commit it.
--
Pedro Alves
^ permalink raw reply [flat|nested] 6+ messages in thread* RE: [RFA] windows-nat.c: Handle ERROR_PARTIAL_COPY in windows_xfer_memory function
2013-09-02 12:34 ` Pedro Alves
@ 2013-09-02 12:48 ` Pierre Muller
0 siblings, 0 replies; 6+ messages in thread
From: Pierre Muller @ 2013-09-02 12:48 UTC (permalink / raw)
To: 'Pedro Alves'; +Cc: gdb-patches
> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Pedro Alves
> Envoyé : lundi 2 septembre 2013 14:34
> À : Pierre Muller
> Cc : gdb-patches@sourceware.org
> Objet : Re: [RFA] windows-nat.c: Handle ERROR_PARTIAL_COPY in
> windows_xfer_memory function
>
> On 09/01/2013 11:10 PM, Pierre Muller wrote:
> >
> > PS: the use of plongest function is because I got an warning about %d
> > used for 'long long integer' type.
>
> Then that's a regression from my deprecated_xfer_memory change.
> Please keep logically independent changes as separate patches.
I just noticed the problem while
trying to update the patch...
You are off course right that this should be separate...
> > 2013-09-01 Pierre Muller <muller@sourceware.org>
> >
> > * windows-nat.c (windows_xfer_memory): Fix compilation failure
> > by use of plongest function.
>
> This part is OK. Please split it out, and commit it.
Done and committed,
thanks,
Pierre
PS: Do you want me to resubmit and new version of the ERROR_PARTIAL_COPY
RFA?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFA] windows-nat.c: Handle ERROR_PARTIAL_COPY in windows_xfer_memory function
[not found] <5223bb46.c6c0420a.5a41.008dSMTPIN_ADDED_BROKEN@mx.google.com>
2013-09-02 12:34 ` Pedro Alves
@ 2013-09-02 12:50 ` Pedro Alves
2013-09-02 13:05 ` Pierre Muller
1 sibling, 1 reply; 6+ messages in thread
From: Pedro Alves @ 2013-09-02 12:50 UTC (permalink / raw)
To: Pierre Muller; +Cc: gdb-patches
On 09/01/2013 11:10 PM, Pierre Muller wrote:
> This is the patch that Pedro suggested I send
> after his commit to remove deprecated_xfer_memory
> in windows-nat.c.
Thanks.
>
> Pedro suggested that I submit this patch separately
> (which I do here)... and with a gdbserver counterpart,
> which I don't...
>
> I tried, but finally realized that given the
> read_memory / write_memory functions type defined
> in target.h target_ops structure,
> there is no way of passing information of partial
> copy and of the length of this partial copy.
> Indeed, the comments state that the return value is either 0 for success
> or errno...
>
> This is not compatible with returning information that only part of the
> request length
> was read/written.
Well, we could just change that interface to make it possible...
The thing I don't like with doing this only on the native
side, is that we're trying to get to a point where we
can share the target backends between GDB and gdbserver:
<https://sourceware.org/gdb/wiki/LocalRemoteFeatureParity>.
Doing such a change on the GDB side only just means we're
pushing the feature-parity goal for the Windows port
further away...
> 2013-09-01 Pierre Muller <muller@sourceware.org>
>
...
> Handle ERROR_PARTIAL_COPY error code.
...
This part is OK too. (Please commit it separately from the
plongest fix.)
--
Pedro Alves
^ permalink raw reply [flat|nested] 6+ messages in thread* RE: [RFA] windows-nat.c: Handle ERROR_PARTIAL_COPY in windows_xfer_memory function
2013-09-02 12:50 ` Pedro Alves
@ 2013-09-02 13:05 ` Pierre Muller
2013-09-02 13:19 ` Pedro Alves
0 siblings, 1 reply; 6+ messages in thread
From: Pierre Muller @ 2013-09-02 13:05 UTC (permalink / raw)
To: 'Pedro Alves'; +Cc: gdb-patches
> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Pedro Alves
> Envoyé : lundi 2 septembre 2013 14:50
> À : Pierre Muller
> Cc : gdb-patches@sourceware.org
> Objet : Re: [RFA] windows-nat.c: Handle ERROR_PARTIAL_COPY in
> windows_xfer_memory function
>
> On 09/01/2013 11:10 PM, Pierre Muller wrote:
> > This is the patch that Pedro suggested I send
> > after his commit to remove deprecated_xfer_memory
> > in windows-nat.c.
>
> Thanks.
>
> >
> > Pedro suggested that I submit this patch separately
> > (which I do here)... and with a gdbserver counterpart,
> > which I don't...
> >
> > I tried, but finally realized that given the
> > read_memory / write_memory functions type defined
> > in target.h target_ops structure,
> > there is no way of passing information of partial
> > copy and of the length of this partial copy.
> > Indeed, the comments state that the return value is either 0 for success
> > or errno...
> >
> > This is not compatible with returning information that only part of the
> > request length
> > was read/written.
>
> Well, we could just change that interface to make it possible...
>
> The thing I don't like with doing this only on the native
> side, is that we're trying to get to a point where we
> can share the target backends between GDB and gdbserver:
Well, when you look at the code inside child_xfer_memory,
you can notice that the return value of ReadProcessMemory or
WriteProcessMemory
is discarded, which means that it does behave more or less like the
new windows-nat.c code (at least in case of ERROR_PARTIAL_COPY)
for other errors, it might also return garbage...
anyhow, the calling code compares the returned value to the requested length
(LEN value)
so that the risk of generating a successful read_memory despite a failure
of ReadProcessMemory function is small... (the uninitialized variable done
would need to return the value LEN..)
It could of course still happen theoretically...
> <https://sourceware.org/gdb/wiki/LocalRemoteFeatureParity>.
>
> Doing such a change on the GDB side only just means we're
> pushing the feature-parity goal for the Windows port
> further away...
>
> > 2013-09-01 Pierre Muller <muller@sourceware.org>
> >
> ...
> > Handle ERROR_PARTIAL_COPY error code.
> ...
>
> This part is OK too. (Please commit it separately from the
> plongest fix.)
Thanks for the approval,
patch committed,
Pierre Muller
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFA] windows-nat.c: Handle ERROR_PARTIAL_COPY in windows_xfer_memory function
2013-09-02 13:05 ` Pierre Muller
@ 2013-09-02 13:19 ` Pedro Alves
0 siblings, 0 replies; 6+ messages in thread
From: Pedro Alves @ 2013-09-02 13:19 UTC (permalink / raw)
To: Pierre Muller; +Cc: gdb-patches
On 09/02/2013 02:05 PM, Pierre Muller wrote:
>>> This is not compatible with returning information that only part of the
>>> request length
>>> was read/written.
>>
>> Well, we could just change that interface to make it possible...
>>
>> The thing I don't like with doing this only on the native
>> side, is that we're trying to get to a point where we
>> can share the target backends between GDB and gdbserver:
>
> Well, when you look at the code inside child_xfer_memory,
> you can notice that the return value of ReadProcessMemory or
> WriteProcessMemory
> is discarded, which means that it does behave more or less like the
> new windows-nat.c code (at least in case of ERROR_PARTIAL_COPY)
> for other errors, it might also return garbage...
> anyhow, the calling code compares the returned value to the requested length
> (LEN value)
That's brittle...
> so that the risk of generating a successful read_memory despite a failure
> of ReadProcessMemory function is small... (the uninitialized variable done
> would need to return the value LEN..)
> It could of course still happen theoretically...
This is really no argument for not fixing gdbserver... In fact,
it's an argument _for_ fixing it.
--
Pedro Alves
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-09-02 13:19 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-09-01 22:10 [RFA] windows-nat.c: Handle ERROR_PARTIAL_COPY in windows_xfer_memory function Pierre Muller
[not found] <5223bb46.c6c0420a.5a41.008dSMTPIN_ADDED_BROKEN@mx.google.com>
2013-09-02 12:34 ` Pedro Alves
2013-09-02 12:48 ` Pierre Muller
2013-09-02 12:50 ` Pedro Alves
2013-09-02 13:05 ` Pierre Muller
2013-09-02 13:19 ` Pedro Alves
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox