[Windows]Fix a bug which cause GDB.exe assert when try to run the inferior

[Windows]Fix a bug which cause GDB.exe assert when try to run the inferior

[asmwarrior]

I see GDB.exe(build from GIT HEAD) can't run any inferior now, it just stops and assert. See the log:

[debug]Starting program:
E:\code\lex\lessons\001_upn_calculator\bin\Debug\test.exe
[debug]../../binutils-gdb/gdb/target.c:1483: internal-error:
target_xfer_partial: Assertion `*xfered_len > 0' failed.
[debug]A problem internal to GDB has been detected,
[debug]further debugging may prove unreliable.
[debug]This application has requested the Runtime to terminate it in an
unusual way.
[debug]Please contact the application's support team for more information.


I found that the assert is here:  

  /* Check implementations of to_xfer_partial update *XFERED_LEN
     properly.  Do assertion after printing debug messages, so that we
     can find more clues on assertion failure from debugging messages.  */
  if (retval == TARGET_XFER_OK || retval == TARGET_XFER_E_UNAVAILABLE)
    gdb_assert (*xfered_len > 0);

Track down, I found that it is a bug introduced in this commit:
SHA-1: 9b409511d07fe375284701af34909fb539029caf
2014-02-11  Yao Qi  <yao@codesourcery.com>

* Return target_xfer_status in to_xfer_partial

This patch does the conversion of to_xfer_partial from

    LONGEST (*to_xfer_partial) (struct target_ops *ops,
				enum target_object object, const char *annex,
				gdb_byte *readbuf, const gdb_byte *writebuf,
				ULONGEST offset, ULONGEST len);

to

    enum target_xfer_status (*to_xfer_partial) (struct target_ops *ops,
				enum target_object object, const char *annex,
				gdb_byte *readbuf, const gdb_byte *writebuf,
				ULONGEST offset, ULONGEST len, ULONGEST *xfered_len);

It changes to_xfer_partial return the transfer status and the transfered
length by *XFERED_LEN.  Generally, the return status has three stats,

 - TARGET_XFER_OK,
 - TARGET_XFER_EOF,
 - TARGET_XFER_E_XXXX,

Especially the change on windows-nat.c
@@ -2536,27 +2538,30 @@ windows_xfer_shared_libraries (struct target_ops *ops,
     }
 
   obstack_free (&obstack, NULL);
-  return len;
+  *xfered_len = (ULONGEST) len;
+  return TARGET_XFER_OK;
 }

The original code return len(len could be 0), but the new code just return TARGET_XFER_OK.
If len is 0, it should return TARGET_XFER_EOF(it is 0 in enum target_xfer_status declaration.

So, a patch below is confirmed to fix the assert issue, an obvious fix, right? 

 gdb/windows-nat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index a570a1a..b76d94d 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -2480,7 +2480,7 @@ windows_xfer_shared_libraries (struct target_ops *ops,
 
   obstack_free (&obstack, NULL);
   *xfered_len = (ULONGEST) len;
-  return TARGET_XFER_OK;
+  return len ? TARGET_XFER_OK : TARGET_XFER_EOF;
 }
 
 static enum target_xfer_status


Thanks Yao for the hint, so cc to him.

Yuanhui Zhang

Re: [Windows]Fix a bug which cause GDB.exe assert when try to run the inferior

[Yao Qi]

On 02/20/2014 10:35 PM, asmwarrior wrote:
> The original code return len(len could be 0), but the new code just return TARGET_XFER_OK.
> If len is 0, it should return TARGET_XFER_EOF(it is 0 in enum target_xfer_status declaration.
> 

Yes, good catch!

> So, a patch below is confirmed to fix the assert issue, an obvious fix, right? 
> 

This fix is obvious to me.  Please wait for one day, if maintainers
have no comments, push it in.

>  gdb/windows-nat.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
> index a570a1a..b76d94d 100644
> --- a/gdb/windows-nat.c
> +++ b/gdb/windows-nat.c
> @@ -2480,7 +2480,7 @@ windows_xfer_shared_libraries (struct target_ops *ops,
>  
>    obstack_free (&obstack, NULL);
>    *xfered_len = (ULONGEST) len;
> -  return TARGET_XFER_OK;
> +  return len ? TARGET_XFER_OK : TARGET_XFER_EOF;
>  }
>  
>  static enum target_xfer_status

You still need a changelog entry.

-- 
Yao (齐尧)

Re: [Windows]Fix a bug which cause GDB.exe assert when try to run the inferior

[asmwarrior]

On 2014-2-21 9:19, Yao Qi wrote:
>>  gdb/windows-nat.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
>> index a570a1a..b76d94d 100644
>> --- a/gdb/windows-nat.c
>> +++ b/gdb/windows-nat.c
>> @@ -2480,7 +2480,7 @@ windows_xfer_shared_libraries (struct target_ops *ops,
>>  
>>    obstack_free (&obstack, NULL);
>>    *xfered_len = (ULONGEST) len;
>> -  return TARGET_XFER_OK;
>> +  return len ? TARGET_XFER_OK : TARGET_XFER_EOF;
>>  }
>>  
>>  static enum target_xfer_status
> You still need a changelog entry.
>
Hi, Yao, The change log could be below, fell free to adjust, thanks.

Windows: Fix target_xfer_partial: Assertion `*xfered_len > 0' failed.
The failure was introduced in commit 9b409511d07fe375284701af34909fb539029caf, 2014-02-11.
gdb/windows-nat.c (windows_xfer_shared_libraries) : If len is 0, it should return TARGET_XFER_EOF.

Yuanhui Zhang

Re: [Windows]Fix a bug which cause GDB.exe assert when try to run the inferior

[Yao Qi]

On 02/21/2014 02:11 PM, asmwarrior wrote:
> Windows: Fix target_xfer_partial: Assertion `*xfered_len > 0' failed.
> The failure was introduced in commit 9b409511d07fe375284701af34909fb539029caf, 2014-02-11.
> gdb/windows-nat.c (windows_xfer_shared_libraries) : If len is 0, it should return TARGET_XFER_EOF.

The format of the changelog entry isn't correct.  On the other hand,
changelog entry is about "what are changed", so "it should return
TARGET_XFER_EOF" doesn't describe "what are changed" clearly.  I suggest
something below.  Otherwise, I don't have comments.

gdb:

2014-02-21  Yuanhui Zhang  <asmwarrior@gmail.com>

	* windows-nat.c (windows_xfer_shared_libraries): Return
	TARGET_XFER_EOF if LEN is zero.

-- 
Yao (齐尧)

Re: [Windows]Fix a bug which cause GDB.exe assert when try to run the inferior

[Eli Zaretskii]

> Date: Fri, 21 Feb 2014 15:03:31 +0800
> From: Yao Qi <yao@codesourcery.com>
> CC: <gdb-patches@sourceware.org>
> 
> 2014-02-21  Yuanhui Zhang  <asmwarrior@gmail.com>
> 
> 	* windows-nat.c (windows_xfer_shared_libraries): Return
> 	TARGET_XFER_EOF if LEN is zero.

Indeed.  May I also suggest to point to the details of the underlying
problem, or include its summary, in the ChangeLog entry?

Re: [Windows]Fix a bug which cause GDB.exe assert when try to run the inferior

[Yao Qi]

On 02/21/2014 03:37 PM, Eli Zaretskii wrote:
>> Date: Fri, 21 Feb 2014 15:03:31 +0800
>> From: Yao Qi <yao@codesourcery.com>
>> CC: <gdb-patches@sourceware.org>
>>
>> 2014-02-21  Yuanhui Zhang  <asmwarrior@gmail.com>
>>
>> 	* windows-nat.c (windows_xfer_shared_libraries): Return
>> 	TARGET_XFER_EOF if LEN is zero.
> 
> Indeed.  May I also suggest to point to the details of the underlying
> problem, or include its summary, in the ChangeLog entry?
> 

I prefer to put the details of the underlying problem in commit log,
but I am fine to include its summary in the ChangeLog entry.

How about this?

In commit log:

A GDB internal error is found on native mingw32 target.

(gdb) run
../../binutils-gdb/gdb/target.c:1483: internal-error:
target_xfer_partial: Assertion `*xfered_len > 0' failed.
A problem internal to GDB has been detected,
further debugging may prove unreliable.
Quit this debugging session? (y or n)

This error was introduced by the following snippet in commit
9b409511d07fe375284701af34909fb539029caf

> @@ -2536,27 +2538,30 @@ windows_xfer_shared_libraries (struct target_ops *ops,
>      }
>  
>    obstack_free (&obstack, NULL);
> -  return len;
> +  *xfered_len = (ULONGEST) len;
> +  return TARGET_XFER_OK;
>  }

In the original code, len is returned, which could be 0, but after that
commit, only TARGET_XFER_OK is returned, which is wrong.  If len is 0,
TARGET_XFER_EOF should be returned.  (it is 0 in enum
target_xfer_status declaration).

gdb:

2014-02-21  Yuanhui Zhang  <asmwarrior@gmail.com>

	* windows-nat.c (windows_xfer_shared_libraries): Return
	TARGET_XFER_EOF if LEN is zero to fix an assert failure when
	requested object is TARGET_OBJECT_LIBRARIES.

-- 
Yao (齐尧)

Re: [Windows]Fix a bug which cause GDB.exe assert when try to run the inferior

[Eli Zaretskii]

> Date: Fri, 21 Feb 2014 15:57:17 +0800
> From: Yao Qi <yao@codesourcery.com>
> CC: <asmwarrior@gmail.com>, <gdb-patches@sourceware.org>
> 
> On 02/21/2014 03:37 PM, Eli Zaretskii wrote:
> >> Date: Fri, 21 Feb 2014 15:03:31 +0800
> >> From: Yao Qi <yao@codesourcery.com>
> >> CC: <gdb-patches@sourceware.org>
> >>
> >> 2014-02-21  Yuanhui Zhang  <asmwarrior@gmail.com>
> >>
> >> 	* windows-nat.c (windows_xfer_shared_libraries): Return
> >> 	TARGET_XFER_EOF if LEN is zero.
> > 
> > Indeed.  May I also suggest to point to the details of the underlying
> > problem, or include its summary, in the ChangeLog entry?
> > 
> 
> I prefer to put the details of the underlying problem in commit log,
> but I am fine to include its summary in the ChangeLog entry.
> 
> How about this?

Looks good to me, thanks.

Re: [Windows]Fix a bug which cause GDB.exe assert when try to run the inferior

[Yao Qi]

On 02/21/2014 03:57 PM, Yao Qi wrote:
> On 02/21/2014 03:37 PM, Eli Zaretskii wrote:
>>> Date: Fri, 21 Feb 2014 15:03:31 +0800
>>> From: Yao Qi <yao@codesourcery.com>
>>> CC: <gdb-patches@sourceware.org>
>>>
>>> 2014-02-21  Yuanhui Zhang  <asmwarrior@gmail.com>
>>>
>>> 	* windows-nat.c (windows_xfer_shared_libraries): Return
>>> 	TARGET_XFER_EOF if LEN is zero.
>>
>> Indeed.  May I also suggest to point to the details of the underlying
>> problem, or include its summary, in the ChangeLog entry?
>>
> 
> I prefer to put the details of the underlying problem in commit log,
> but I am fine to include its summary in the ChangeLog entry.
> 
> How about this?
> 
> In commit log:
> 
> A GDB internal error is found on native mingw32 target.
> 
> (gdb) run
> ../../binutils-gdb/gdb/target.c:1483: internal-error:
> target_xfer_partial: Assertion `*xfered_len > 0' failed.
> A problem internal to GDB has been detected,
> further debugging may prove unreliable.
> Quit this debugging session? (y or n)
> 
> This error was introduced by the following snippet in commit
> 9b409511d07fe375284701af34909fb539029caf
> 
>> @@ -2536,27 +2538,30 @@ windows_xfer_shared_libraries (struct target_ops *ops,
>>      }
>>  
>>    obstack_free (&obstack, NULL);
>> -  return len;
>> +  *xfered_len = (ULONGEST) len;
>> +  return TARGET_XFER_OK;
>>  }
> 
> In the original code, len is returned, which could be 0, but after that
> commit, only TARGET_XFER_OK is returned, which is wrong.  If len is 0,
> TARGET_XFER_EOF should be returned.  (it is 0 in enum
> target_xfer_status declaration).
> 
> gdb:
> 
> 2014-02-21  Yuanhui Zhang  <asmwarrior@gmail.com>
> 
> 	* windows-nat.c (windows_xfer_shared_libraries): Return
> 	TARGET_XFER_EOF if LEN is zero to fix an assert failure when
> 	requested object is TARGET_OBJECT_LIBRARIES.
> 

Please update your patch to replace "len ?" with "len != 0 ?" and commit
log we discussed here.  You can push it in.

Let me know if you don't have a commit access or have trouble on
committing, I can commit this patch for you.

-- 
Yao (齐尧)

Re: [Windows]Fix a bug which cause GDB.exe assert when try to run the inferior

[asmwarrior]

On 2014-2-24 14:45, Yao Qi wrote:
> > 
> > gdb:
> > 
> > 2014-02-21  Yuanhui Zhang  <asmwarrior@gmail.com>
> > 
> > 	* windows-nat.c (windows_xfer_shared_libraries): Return
> > 	TARGET_XFER_EOF if LEN is zero to fix an assert failure when
> > 	requested object is TARGET_OBJECT_LIBRARIES.
> > 
> Please update your patch to replace "len ?" with "len != 0 ?" and commit
> log we discussed here.  You can push it in.
>
> Let me know if you don't have a commit access or have trouble on
> committing, I can commit this patch for you.
Hi, Yao, thanks. I was thinking that I won't need further change, because it was a tiny patch.
Anyway, would you mind to commit for me? I don't have commit access.

 gdb/windows-nat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index a570a1a..b76d94d 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -2480,7 +2480,7 @@ windows_xfer_shared_libraries (struct target_ops *ops,
 
   obstack_free (&obstack, NULL);
   *xfered_len = (ULONGEST) len;
-  return TARGET_XFER_OK;
+  return len != 0 ? TARGET_XFER_OK : TARGET_XFER_EOF;
 }


The log message are much better than my original one.

gdb:
 
 2014-02-21  Yuanhui Zhang  <asmwarrior@gmail.com>
 
 	* windows-nat.c (windows_xfer_shared_libraries): Return
 	TARGET_XFER_EOF if LEN is zero to fix an assert failure when
 	requested object is TARGET_OBJECT_LIBRARIES.


Thanks for adjustment.

Yuanhui Zhang

Re: [Windows]Fix a bug which cause GDB.exe assert when try to run the inferior

[Yao Qi]

On 02/24/2014 03:10 PM, asmwarrior wrote:
> Hi, Yao, thanks. I was thinking that I won't need further change, because it was a tiny patch.

The patch is small, but we still have to make sure it is correct,
documented, and compliant to coding standard, etc.  There are something
not very interesting in the procedure, such as writing changelog or
commit log, but they guarantee GDB still maintainable after thousands
of commits in several years.

It is good to see your patch, and better to adjust patch to a
commit-able state as a result of reviews.

> Anyway, would you mind to commit for me? I don't have commit access.

Thanks for testing GDB for windows target, reporting this
regression, and posting your patch.

Patch below is what I pushed in.

-- 
Yao (齐尧)

From 5b8017c1ebd1b8105f6846dc15d5158e8ec3f450 Mon Sep 17 00:00:00 2001
From: Yuanhui Zhang <asmwarrior@gmail.com>
Date: Mon, 24 Feb 2014 15:22:10 +0800
Subject: [PATCH] Fix a GDB assert failure on windows

A GDB internal error is found on native mingw32 target.

(gdb) run
../../binutils-gdb/gdb/target.c:1483: internal-error:
target_xfer_partial: Assertion `*xfered_len > 0' failed.
A problem internal to GDB has been detected,
further debugging may prove unreliable.
Quit this debugging session? (y or n)

This error was introduced by the following snippet in commit
9b409511d07fe375284701af34909fb539029caf

> @@ -2536,27 +2538,30 @@ windows_xfer_shared_libraries (struct
target_ops *ops,
>      }
>
>    obstack_free (&obstack, NULL);
> -  return len;
> +  *xfered_len = (ULONGEST) len;
> +  return TARGET_XFER_OK;
>  }

In the original code, len is returned, which could be 0, but after that
commit, only TARGET_XFER_OK is returned, which is wrong.  If len is 0,
TARGET_XFER_EOF should be returned.  (it is 0 in enum
target_xfer_status declaration).

gdb:

2014-02-23  Yuanhui Zhang  <asmwarrior@gmail.com>

	* windows-nat.c (windows_xfer_shared_libraries): Return
	TARGET_XFER_EOF if LEN is zero to fix an assert failure when
	requested object is TARGET_OBJECT_LIBRARIES.
---
 gdb/windows-nat.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index 4366aab..6c45d0a 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -2501,7 +2501,7 @@ windows_xfer_shared_libraries (struct target_ops *ops,

   obstack_free (&obstack, NULL);
   *xfered_len = (ULONGEST) len;
-  return TARGET_XFER_OK;
+  return len != 0 ? TARGET_XFER_OK : TARGET_XFER_EOF;
 }

 static enum target_xfer_status
-- 
1.7.7.6

Re: [Windows]Fix a bug which cause GDB.exe assert when try to run the inferior

[Pedro Alves]

On 02/21/2014 07:03 AM, Yao Qi wrote:
> On 02/21/2014 02:11 PM, asmwarrior wrote:
>> Windows: Fix target_xfer_partial: Assertion `*xfered_len > 0' failed.
>> The failure was introduced in commit 9b409511d07fe375284701af34909fb539029caf, 2014-02-11.
>> gdb/windows-nat.c (windows_xfer_shared_libraries) : If len is 0, it should return TARGET_XFER_EOF.
> 
> The format of the changelog entry isn't correct.  On the other hand,
> changelog entry is about "what are changed", so "it should return
> TARGET_XFER_EOF" doesn't describe "what are changed" clearly.  I suggest
> something below.  Otherwise, I don't have comments.

Thanks You.  This is definitely OK.  Could you put it in?

-  return TARGET_XFER_OK;
+  return len ? TARGET_XFER_OK : TARGET_XFER_EOF;

(Please write 'len != 0' though.)

> 
> 2014-02-21  Yuanhui Zhang  <asmwarrior@gmail.com>
> 
> 	* windows-nat.c (windows_xfer_shared_libraries): Return
> 	TARGET_XFER_EOF if LEN is zero.
> 


-- 
Pedro Alves