[RFA] let record_resume fail immediately on error

[RFA] let record_resume fail immediately on error

[Michael Snyder]

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

Might as well let record_resume call internal_error if it fails,
rather than setting a flag and trying to report on it later.

The flag doesn't really work anyway -- looks like you wanted
record_wait to return SIGABRT (not really the right thing either),
but it doesn't.  Instead we somehow return "no more reverse execution
history".

What do you think, Hui?


[-- Attachment #2: rre.txt --]
[-- Type: text/plain, Size: 1475 bytes --]

2009-09-07  Michael Snyder  <msnyder@vmware.com>

	* record.c (record_resume): Call internal_error immediately
	instead of later.

Index: record.c
===================================================================
RCS file: /cvs/src/src/gdb/record.c,v
retrieving revision 1.17
diff -u -p -r1.17 record.c
--- record.c	8 Sep 2009 00:50:42 -0000	1.17
+++ record.c	8 Sep 2009 03:56:09 -0000
@@ -516,7 +516,6 @@ record_close (int quitting)
 }
 
 static int record_resume_step = 0;
-static int record_resume_error;
 
 static void
 record_resume (struct target_ops *ops, ptid_t ptid, int step,
@@ -527,14 +526,9 @@ record_resume (struct target_ops *ops, p
   if (!RECORD_IS_REPLAY)
     {
       if (do_record_message (get_current_regcache ()))
-        {
-          record_resume_error = 0;
-        }
-      else
-        {
-          record_resume_error = 1;
-          return;
-        }
+	internal_error (__FILE__, __LINE__, 
+			_("record_resume: do_record_message failed."));
+
       record_beneath_to_resume (record_beneath_to_resume_ops, ptid, 1,
                                 siggnal);
     }
@@ -586,14 +580,6 @@ record_wait (struct target_ops *ops,
 
   if (!RECORD_IS_REPLAY)
     {
-      if (record_resume_error)
-	{
-	  /* If record_resume get error, return directly.  */
-	  status->kind = TARGET_WAITKIND_STOPPED;
-	  status->value.sig = TARGET_SIGNAL_ABRT;
-	  return inferior_ptid;
-	}
-
       if (record_resume_step)
 	{
 	  /* This is a single step.  */

Re: [RFA] let record_resume fail immediately on error

[Hui Zhu]

Thanks for your work.
I think this patch is OK.

Hui

On Tue, Sep 8, 2009 at 12:01, Michael Snyder<msnyder@vmware.com> wrote:
> Might as well let record_resume call internal_error if it fails,
> rather than setting a flag and trying to report on it later.
>
> The flag doesn't really work anyway -- looks like you wanted
> record_wait to return SIGABRT (not really the right thing either),
> but it doesn't.  Instead we somehow return "no more reverse execution
> history".
>
> What do you think, Hui?
>
>
> 2009-09-07  Michael Snyder  <msnyder@vmware.com>
>
>        * record.c (record_resume): Call internal_error immediately
>        instead of later.
>
> Index: record.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/record.c,v
> retrieving revision 1.17
> diff -u -p -r1.17 record.c
> --- record.c    8 Sep 2009 00:50:42 -0000       1.17
> +++ record.c    8 Sep 2009 03:56:09 -0000
> @@ -516,7 +516,6 @@ record_close (int quitting)
>  }
>
>  static int record_resume_step = 0;
> -static int record_resume_error;
>
>  static void
>  record_resume (struct target_ops *ops, ptid_t ptid, int step,
> @@ -527,14 +526,9 @@ record_resume (struct target_ops *ops, p
>   if (!RECORD_IS_REPLAY)
>     {
>       if (do_record_message (get_current_regcache ()))
> -        {
> -          record_resume_error = 0;
> -        }
> -      else
> -        {
> -          record_resume_error = 1;
> -          return;
> -        }
> +       internal_error (__FILE__, __LINE__,
> +                       _("record_resume: do_record_message failed."));
> +
>       record_beneath_to_resume (record_beneath_to_resume_ops, ptid, 1,
>                                 siggnal);
>     }
> @@ -586,14 +580,6 @@ record_wait (struct target_ops *ops,
>
>   if (!RECORD_IS_REPLAY)
>     {
> -      if (record_resume_error)
> -       {
> -         /* If record_resume get error, return directly.  */
> -         status->kind = TARGET_WAITKIND_STOPPED;
> -         status->value.sig = TARGET_SIGNAL_ABRT;
> -         return inferior_ptid;
> -       }
> -
>       if (record_resume_step)
>        {
>          /* This is a single step.  */
>
>

Re: [RFA] let record_resume fail immediately on error

[Joel Brobecker]

>    if (!RECORD_IS_REPLAY)
>      {
>        if (do_record_message (get_current_regcache ()))
> -        {
> -          record_resume_error = 0;
> -        }
> -      else
> -        {
> -          record_resume_error = 1;
> -          return;
> -        }
> +	internal_error (__FILE__, __LINE__, 
> +			_("record_resume: do_record_message failed."));
> +

Forgive me if I'm wrong, as I don't know the record.c code at all, but
I cannot help but think that the internal_error is suspicious here.
Why is this an internal_error?

-- 
Joel

Re: [RFA] let record_resume fail immediately on error

[Hui Zhu]

The "record_resume_error" in gdb-cvs is to make user after get a error
of record_message, they can "record stop" close the record and keep
debug the inferior.

Thanks,
Hui

On Tue, Sep 8, 2009 at 14:58, Joel Brobecker<brobecker@adacore.com> wrote:
>>    if (!RECORD_IS_REPLAY)
>>      {
>>        if (do_record_message (get_current_regcache ()))
>> -        {
>> -          record_resume_error = 0;
>> -        }
>> -      else
>> -        {
>> -          record_resume_error = 1;
>> -          return;
>> -        }
>> +     internal_error (__FILE__, __LINE__,
>> +                     _("record_resume: do_record_message failed."));
>> +
>
> Forgive me if I'm wrong, as I don't know the record.c code at all, but
> I cannot help but think that the internal_error is suspicious here.
> Why is this an internal_error?
>
> --
> Joel
>

Re: [RFA] let record_resume fail immediately on error

[Hui Zhu]

If GDB call error in record_resume, user cannot keep debug the inferior.

Hui

On Tue, Sep 8, 2009 at 15:23, Hui Zhu<teawater@gmail.com> wrote:
> The "record_resume_error" in gdb-cvs is to make user after get a error
> of record_message, they can "record stop" close the record and keep
> debug the inferior.
>
> Thanks,
> Hui
>
> On Tue, Sep 8, 2009 at 14:58, Joel Brobecker<brobecker@adacore.com> wrote:
>>>    if (!RECORD_IS_REPLAY)
>>>      {
>>>        if (do_record_message (get_current_regcache ()))
>>> -        {
>>> -          record_resume_error = 0;
>>> -        }
>>> -      else
>>> -        {
>>> -          record_resume_error = 1;
>>> -          return;
>>> -        }
>>> +     internal_error (__FILE__, __LINE__,
>>> +                     _("record_resume: do_record_message failed."));
>>> +
>>
>> Forgive me if I'm wrong, as I don't know the record.c code at all, but
>> I cannot help but think that the internal_error is suspicious here.
>> Why is this an internal_error?
>>
>> --
>> Joel
>>
>

Re: [RFA] let record_resume fail immediately on error

[Hui Zhu]

On Tue, Sep 8, 2009 at 15:25, Hui Zhu<teawater@gmail.com> wrote:
> If GDB call error in record_resume, user cannot keep debug the inferior.
>
> Hui
>
> On Tue, Sep 8, 2009 at 15:23, Hui Zhu<teawater@gmail.com> wrote:
>> The "record_resume_error" in gdb-cvs is to make user after get a error
>> of record_message, they can "record stop" close the record and keep
>> debug the inferior.
>>
>> Thanks,
>> Hui
>>
>> On Tue, Sep 8, 2009 at 14:58, Joel Brobecker<brobecker@adacore.com> wrote:
>>>>    if (!RECORD_IS_REPLAY)
>>>>      {
>>>>        if (do_record_message (get_current_regcache ()))
>>>> -        {
>>>> -          record_resume_error = 0;
>>>> -        }
>>>> -      else
>>>> -        {
>>>> -          record_resume_error = 1;
>>>> -          return;
>>>> -        }
>>>> +     internal_error (__FILE__, __LINE__,
>>>> +                     _("record_resume: do_record_message failed."));
>>>> +
>>>
>>> Forgive me if I'm wrong, as I don't know the record.c code at all, but
>>> I cannot help but think that the internal_error is suspicious here.
>>> Why is this an internal_error?
>>>
>>> --
>>> Joel
>>>
>>
>

Hi guys,

I make a patch that make "record_resume_error" work better.  I did
some test.  It seems better than before.

Please help me with it.

Thanks,
Hui

2009-09-08  Hui Zhu  <teawater@gmail.com>

	* record.c (record_wait): Change TARGET_SIGNAL_ABRT to
	TARGET_SIGNAL_0.
---
 record.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/record.c
+++ b/record.c
@@ -590,7 +590,7 @@ record_wait (struct target_ops *ops,
 	{
 	  /* If record_resume get error, return directly.  */
 	  status->kind = TARGET_WAITKIND_STOPPED;
-	  status->value.sig = TARGET_SIGNAL_ABRT;
+	  status->value.sig = TARGET_SIGNAL_0;
 	  return inferior_ptid;
 	}

Re: [RFA] let record_resume fail immediately on error

[Michael Snyder]

Joel Brobecker wrote:
>>    if (!RECORD_IS_REPLAY)
>>      {
>>        if (do_record_message (get_current_regcache ()))
>> -        {
>> -          record_resume_error = 0;
>> -        }
>> -      else
>> -        {
>> -          record_resume_error = 1;
>> -          return;
>> -        }
>> +	internal_error (__FILE__, __LINE__, 
>> +			_("record_resume: do_record_message failed."));
>> +
> 
> Forgive me if I'm wrong, as I don't know the record.c code at all, but
> I cannot help but think that the internal_error is suspicious here.
> Why is this an internal_error?

I'm certainly open to alternatives.

It's some kind of error, because we're in the record phase, and
"do_record_message" (which records the side effects of the instruction)
returned failure.  Most likely it hit some resource limit, but it is
possible that something else happened like trying to read from an
unreadable memory location.

You think simple error would be better?

Re: [RFA] let record_resume fail immediately on error

[Michael Snyder]

Hui Zhu wrote:
> The "record_resume_error" in gdb-cvs is to make user after get a error
> of record_message, they can "record stop" close the record and keep
> debug the inferior.

Oh, well then I don't think it is working as you intended.
Try it -- just add some code to do_record_message that will
make it return FAILURE after the 100th call.


> 
> On Tue, Sep 8, 2009 at 14:58, Joel Brobecker<brobecker@adacore.com> wrote:
>>>    if (!RECORD_IS_REPLAY)
>>>      {
>>>        if (do_record_message (get_current_regcache ()))
>>> -        {
>>> -          record_resume_error = 0;
>>> -        }
>>> -      else
>>> -        {
>>> -          record_resume_error = 1;
>>> -          return;
>>> -        }
>>> +     internal_error (__FILE__, __LINE__,
>>> +                     _("record_resume: do_record_message failed."));
>>> +
>> Forgive me if I'm wrong, as I don't know the record.c code at all, but
>> I cannot help but think that the internal_error is suspicious here.
>> Why is this an internal_error?
>>
>> --
>> Joel
>>

Re: [RFA] let record_resume fail immediately on error

[Michael Snyder]

Hui Zhu wrote:
> If GDB call error in record_resume, user cannot keep debug the inferior.

OK, what would you want to have happen, if record_resume fails?
You have to assume that you can't keep recording, so you would
have to at least drop out of "target record" and back to "target beneath".

> On Tue, Sep 8, 2009 at 15:23, Hui Zhu<teawater@gmail.com> wrote:
>> The "record_resume_error" in gdb-cvs is to make user after get a error
>> of record_message, they can "record stop" close the record and keep
>> debug the inferior.
>>
>> Thanks,
>> Hui
>>
>> On Tue, Sep 8, 2009 at 14:58, Joel Brobecker<brobecker@adacore.com> wrote:
>>>>    if (!RECORD_IS_REPLAY)
>>>>      {
>>>>        if (do_record_message (get_current_regcache ()))
>>>> -        {
>>>> -          record_resume_error = 0;
>>>> -        }
>>>> -      else
>>>> -        {
>>>> -          record_resume_error = 1;
>>>> -          return;
>>>> -        }
>>>> +     internal_error (__FILE__, __LINE__,
>>>> +                     _("record_resume: do_record_message failed."));
>>>> +
>>> Forgive me if I'm wrong, as I don't know the record.c code at all, but
>>> I cannot help but think that the internal_error is suspicious here.
>>> Why is this an internal_error?
>>>
>>> --
>>> Joel
>>>

Re: [RFA] let record_resume fail immediately on error

[Michael Snyder]

Hui Zhu wrote:
> On Tue, Sep 8, 2009 at 15:25, Hui Zhu<teawater@gmail.com> wrote:
>> If GDB call error in record_resume, user cannot keep debug the inferior.
>>
>> Hui
>>
>> On Tue, Sep 8, 2009 at 15:23, Hui Zhu<teawater@gmail.com> wrote:
>>> The "record_resume_error" in gdb-cvs is to make user after get a error
>>> of record_message, they can "record stop" close the record and keep
>>> debug the inferior.
>>>
>>> Thanks,
>>> Hui
>>>
>>> On Tue, Sep 8, 2009 at 14:58, Joel Brobecker<brobecker@adacore.com> wrote:
>>>>>    if (!RECORD_IS_REPLAY)
>>>>>      {
>>>>>        if (do_record_message (get_current_regcache ()))
>>>>> -        {
>>>>> -          record_resume_error = 0;
>>>>> -        }
>>>>> -      else
>>>>> -        {
>>>>> -          record_resume_error = 1;
>>>>> -          return;
>>>>> -        }
>>>>> +     internal_error (__FILE__, __LINE__,
>>>>> +                     _("record_resume: do_record_message failed."));
>>>>> +
>>>> Forgive me if I'm wrong, as I don't know the record.c code at all, but
>>>> I cannot help but think that the internal_error is suspicious here.
>>>> Why is this an internal_error?
>>>>
>>>> --
>>>> Joel
>>>>
> 
> Hi guys,
> 
> I make a patch that make "record_resume_error" work better.  I did
> some test.  It seems better than before.

Could you explain what you mean by better?
I mean, what behavior are you looking for here?

Here is the behavior that I see --- I am making a recording,
I say "continue", and after a while this "record_resume_error"
is triggered, and gdb stops and says "No more reverse-execution history."

I would never expect to see that message during recording.


> 2009-09-08  Hui Zhu  <teawater@gmail.com>
> 
> 	* record.c (record_wait): Change TARGET_SIGNAL_ABRT to
> 	TARGET_SIGNAL_0.
> ---
>  record.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> --- a/record.c
> +++ b/record.c
> @@ -590,7 +590,7 @@ record_wait (struct target_ops *ops,
>  	{
>  	  /* If record_resume get error, return directly.  */
>  	  status->kind = TARGET_WAITKIND_STOPPED;
> -	  status->value.sig = TARGET_SIGNAL_ABRT;
> +	  status->value.sig = TARGET_SIGNAL_0;
>  	  return inferior_ptid;
>  	}

Re: [RFA] let record_resume fail immediately on error

[Joel Brobecker]

> It's some kind of error, because we're in the record phase, and
> "do_record_message" (which records the side effects of the instruction)
> returned failure.  Most likely it hit some resource limit, but it is
> possible that something else happened like trying to read from an
> unreadable memory location.
>
> You think simple error would be better?

To me, I see internal_errors as programing errors. From what you are
saying above, calling error would be more appropriate.

Taking a step back, if you had in fact hit a resource limit in
do_record_message, wouldn't it have been better to report the error
there? The error could then be a lot more precise and informative.
However, I see that do_record_message is a simple catch_errors wrapper,
so the error would be trapped, and your error-condition-checking code
would not trigger (catch_errors return zero in that case)... In any
case, these are just idle thoughts - improvements if any can be done
as part of another patch.

-- 
Joel

Re: [RFA] let record_resume fail immediately on error

[Hui Zhu]

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

On Wed, Sep 9, 2009 at 00:55, Michael Snyder<msnyder@vmware.com> wrote:
> Hui Zhu wrote:
>>
>> On Tue, Sep 8, 2009 at 15:25, Hui Zhu<teawater@gmail.com> wrote:
>>>
>>> If GDB call error in record_resume, user cannot keep debug the inferior.
>>>
>>> Hui
>>>
>>> On Tue, Sep 8, 2009 at 15:23, Hui Zhu<teawater@gmail.com> wrote:
>>>>
>>>> The "record_resume_error" in gdb-cvs is to make user after get a error
>>>> of record_message, they can "record stop" close the record and keep
>>>> debug the inferior.
>>>>
>>>> Thanks,
>>>> Hui
>>>>
>>>> On Tue, Sep 8, 2009 at 14:58, Joel Brobecker<brobecker@adacore.com>
>>>> wrote:
>>>>>>
>>>>>>   if (!RECORD_IS_REPLAY)
>>>>>>     {
>>>>>>       if (do_record_message (get_current_regcache ()))
>>>>>> -        {
>>>>>> -          record_resume_error = 0;
>>>>>> -        }
>>>>>> -      else
>>>>>> -        {
>>>>>> -          record_resume_error = 1;
>>>>>> -          return;
>>>>>> -        }
>>>>>> +     internal_error (__FILE__, __LINE__,
>>>>>> +                     _("record_resume: do_record_message failed."));
>>>>>> +
>>>>>
>>>>> Forgive me if I'm wrong, as I don't know the record.c code at all, but
>>>>> I cannot help but think that the internal_error is suspicious here.
>>>>> Why is this an internal_error?
>>>>>
>>>>> --
>>>>> Joel
>>>>>
>>
>> Hi guys,
>>
>> I make a patch that make "record_resume_error" work better.  I did
>> some test.  It seems better than before.
>
> Could you explain what you mean by better?
> I mean, what behavior are you looking for here?
>
> Here is the behavior that I see --- I am making a recording,
> I say "continue", and after a while this "record_resume_error"
> is triggered, and gdb stops and says "No more reverse-execution history."
>
> I would never expect to see that message during recording.

In before, GDB cannot throw error in record_resume (It just in my
memory, maybe I am wrong).
If we try to do it, it will get:
(gdb) c
Continuing.
Cannot execute this command while the selected thread is running.
So I add record_resume_error to let record_wait to handle it.

But record_wait cannot call error too.  So I let it:
	  /* If record_resume get error, return directly.  */
	  status->kind = TARGET_WAITKIND_STOPPED;
	  status->value.sig = TARGET_SIGNAL_0;
	  return inferior_ptid;

But I found that record_resume can call error now.  So I make a new
patch for it.

Could you help me try it?


Thanks,
Hui

2009-09-09  Hui Zhu  <teawater@gmail.com>

	* record.c (record_resume_error): Deleted.
	(record_resume): Call record_message.
	(record_wait): Deleted record_resume_error.

---
 record.c |   19 +------------------
 1 file changed, 1 insertion(+), 18 deletions(-)

--- a/record.c
+++ b/record.c
@@ -516,7 +516,6 @@ record_close (int quitting)
 }

 static int record_resume_step = 0;
-static int record_resume_error;

 static void
 record_resume (struct target_ops *ops, ptid_t ptid, int step,
@@ -526,15 +525,7 @@ record_resume (struct target_ops *ops, p

   if (!RECORD_IS_REPLAY)
     {
-      if (do_record_message (get_current_regcache ()))
-        {
-          record_resume_error = 0;
-        }
-      else
-        {
-          record_resume_error = 1;
-          return;
-        }
+      record_message (get_current_regcache ());
       record_beneath_to_resume (record_beneath_to_resume_ops, ptid, 1,
                                 siggnal);
     }
@@ -586,14 +577,6 @@ record_wait (struct target_ops *ops,

   if (!RECORD_IS_REPLAY)
     {
-      if (record_resume_error)
-	{
-	  /* If record_resume get error, return directly.  */
-	  status->kind = TARGET_WAITKIND_STOPPED;
-	  status->value.sig = TARGET_SIGNAL_ABRT;
-	  return inferior_ptid;
-	}
-
       if (record_resume_step)
 	{
 	  /* This is a single step.  */

[-- Attachment #2: prec-fix-error-handler.txt --]
[-- Type: text/plain, Size: 1166 bytes --]

---
 record.c |   19 +------------------
 1 file changed, 1 insertion(+), 18 deletions(-)

--- a/record.c
+++ b/record.c
@@ -516,7 +516,6 @@ record_close (int quitting)
 }
 
 static int record_resume_step = 0;
-static int record_resume_error;
 
 static void
 record_resume (struct target_ops *ops, ptid_t ptid, int step,
@@ -526,15 +525,7 @@ record_resume (struct target_ops *ops, p
 
   if (!RECORD_IS_REPLAY)
     {
-      if (do_record_message (get_current_regcache ()))
-        {
-          record_resume_error = 0;
-        }
-      else
-        {
-          record_resume_error = 1;
-          return;
-        }
+      record_message (get_current_regcache ());
       record_beneath_to_resume (record_beneath_to_resume_ops, ptid, 1,
                                 siggnal);
     }
@@ -586,14 +577,6 @@ record_wait (struct target_ops *ops,
 
   if (!RECORD_IS_REPLAY)
     {
-      if (record_resume_error)
-	{
-	  /* If record_resume get error, return directly.  */
-	  status->kind = TARGET_WAITKIND_STOPPED;
-	  status->value.sig = TARGET_SIGNAL_ABRT;
-	  return inferior_ptid;
-	}
-
       if (record_resume_step)
 	{
 	  /* This is a single step.  */

Re: [RFA] let record_resume fail immediately on error

[Hui Zhu]

PING.

On Wed, Sep 9, 2009 at 10:05, Hui Zhu <teawater@gmail.com> wrote:
> On Wed, Sep 9, 2009 at 00:55, Michael Snyder<msnyder@vmware.com> wrote:
>> Hui Zhu wrote:
>>>
>>> On Tue, Sep 8, 2009 at 15:25, Hui Zhu<teawater@gmail.com> wrote:
>>>>
>>>> If GDB call error in record_resume, user cannot keep debug the inferior.
>>>>
>>>> Hui
>>>>
>>>> On Tue, Sep 8, 2009 at 15:23, Hui Zhu<teawater@gmail.com> wrote:
>>>>>
>>>>> The "record_resume_error" in gdb-cvs is to make user after get a error
>>>>> of record_message, they can "record stop" close the record and keep
>>>>> debug the inferior.
>>>>>
>>>>> Thanks,
>>>>> Hui
>>>>>
>>>>> On Tue, Sep 8, 2009 at 14:58, Joel Brobecker<brobecker@adacore.com>
>>>>> wrote:
>>>>>>>
>>>>>>>   if (!RECORD_IS_REPLAY)
>>>>>>>     {
>>>>>>>       if (do_record_message (get_current_regcache ()))
>>>>>>> -        {
>>>>>>> -          record_resume_error = 0;
>>>>>>> -        }
>>>>>>> -      else
>>>>>>> -        {
>>>>>>> -          record_resume_error = 1;
>>>>>>> -          return;
>>>>>>> -        }
>>>>>>> +     internal_error (__FILE__, __LINE__,
>>>>>>> +                     _("record_resume: do_record_message failed."));
>>>>>>> +
>>>>>>
>>>>>> Forgive me if I'm wrong, as I don't know the record.c code at all, but
>>>>>> I cannot help but think that the internal_error is suspicious here.
>>>>>> Why is this an internal_error?
>>>>>>
>>>>>> --
>>>>>> Joel
>>>>>>
>>>
>>> Hi guys,
>>>
>>> I make a patch that make "record_resume_error" work better.  I did
>>> some test.  It seems better than before.
>>
>> Could you explain what you mean by better?
>> I mean, what behavior are you looking for here?
>>
>> Here is the behavior that I see --- I am making a recording,
>> I say "continue", and after a while this "record_resume_error"
>> is triggered, and gdb stops and says "No more reverse-execution history."
>>
>> I would never expect to see that message during recording.
>
> In before, GDB cannot throw error in record_resume (It just in my
> memory, maybe I am wrong).
> If we try to do it, it will get:
> (gdb) c
> Continuing.
> Cannot execute this command while the selected thread is running.
> So I add record_resume_error to let record_wait to handle it.
>
> But record_wait cannot call error too.  So I let it:
>          /* If record_resume get error, return directly.  */
>          status->kind = TARGET_WAITKIND_STOPPED;
>          status->value.sig = TARGET_SIGNAL_0;
>          return inferior_ptid;
>
> But I found that record_resume can call error now.  So I make a new
> patch for it.
>
> Could you help me try it?
>
>
> Thanks,
> Hui
>
> 2009-09-09  Hui Zhu  <teawater@gmail.com>
>
>        * record.c (record_resume_error): Deleted.
>        (record_resume): Call record_message.
>        (record_wait): Deleted record_resume_error.
>
> ---
>  record.c |   19 +------------------
>  1 file changed, 1 insertion(+), 18 deletions(-)
>
> --- a/record.c
> +++ b/record.c
> @@ -516,7 +516,6 @@ record_close (int quitting)
>  }
>
>  static int record_resume_step = 0;
> -static int record_resume_error;
>
>  static void
>  record_resume (struct target_ops *ops, ptid_t ptid, int step,
> @@ -526,15 +525,7 @@ record_resume (struct target_ops *ops, p
>
>   if (!RECORD_IS_REPLAY)
>     {
> -      if (do_record_message (get_current_regcache ()))
> -        {
> -          record_resume_error = 0;
> -        }
> -      else
> -        {
> -          record_resume_error = 1;
> -          return;
> -        }
> +      record_message (get_current_regcache ());
>       record_beneath_to_resume (record_beneath_to_resume_ops, ptid, 1,
>                                 siggnal);
>     }
> @@ -586,14 +577,6 @@ record_wait (struct target_ops *ops,
>
>   if (!RECORD_IS_REPLAY)
>     {
> -      if (record_resume_error)
> -       {
> -         /* If record_resume get error, return directly.  */
> -         status->kind = TARGET_WAITKIND_STOPPED;
> -         status->value.sig = TARGET_SIGNAL_ABRT;
> -         return inferior_ptid;
> -       }
> -
>       if (record_resume_step)
>        {
>          /* This is a single step.  */
>

Re: [RFA] let record_resume fail immediately on error

[Hui Zhu]

Hi Joel,

Sorry to disturb you.  Ping
http://sourceware.org/ml/gdb-patches/2009-09/msg00231.html

Thanks,
Hui

On Wed, Sep 9, 2009 at 10:05, Hui Zhu <teawater@gmail.com> wrote:
> On Wed, Sep 9, 2009 at 00:55, Michael Snyder<msnyder@vmware.com> wrote:
>> Hui Zhu wrote:
>>>
>>> On Tue, Sep 8, 2009 at 15:25, Hui Zhu<teawater@gmail.com> wrote:
>>>>
>>>> If GDB call error in record_resume, user cannot keep debug the inferior.
>>>>
>>>> Hui
>>>>
>>>> On Tue, Sep 8, 2009 at 15:23, Hui Zhu<teawater@gmail.com> wrote:
>>>>>
>>>>> The "record_resume_error" in gdb-cvs is to make user after get a error
>>>>> of record_message, they can "record stop" close the record and keep
>>>>> debug the inferior.
>>>>>
>>>>> Thanks,
>>>>> Hui
>>>>>
>>>>> On Tue, Sep 8, 2009 at 14:58, Joel Brobecker<brobecker@adacore.com>
>>>>> wrote:
>>>>>>>
>>>>>>>   if (!RECORD_IS_REPLAY)
>>>>>>>     {
>>>>>>>       if (do_record_message (get_current_regcache ()))
>>>>>>> -        {
>>>>>>> -          record_resume_error = 0;
>>>>>>> -        }
>>>>>>> -      else
>>>>>>> -        {
>>>>>>> -          record_resume_error = 1;
>>>>>>> -          return;
>>>>>>> -        }
>>>>>>> +     internal_error (__FILE__, __LINE__,
>>>>>>> +                     _("record_resume: do_record_message failed."));
>>>>>>> +
>>>>>>
>>>>>> Forgive me if I'm wrong, as I don't know the record.c code at all, but
>>>>>> I cannot help but think that the internal_error is suspicious here.
>>>>>> Why is this an internal_error?
>>>>>>
>>>>>> --
>>>>>> Joel
>>>>>>
>>>
>>> Hi guys,
>>>
>>> I make a patch that make "record_resume_error" work better.  I did
>>> some test.  It seems better than before.
>>
>> Could you explain what you mean by better?
>> I mean, what behavior are you looking for here?
>>
>> Here is the behavior that I see --- I am making a recording,
>> I say "continue", and after a while this "record_resume_error"
>> is triggered, and gdb stops and says "No more reverse-execution history."
>>
>> I would never expect to see that message during recording.
>
> In before, GDB cannot throw error in record_resume (It just in my
> memory, maybe I am wrong).
> If we try to do it, it will get:
> (gdb) c
> Continuing.
> Cannot execute this command while the selected thread is running.
> So I add record_resume_error to let record_wait to handle it.
>
> But record_wait cannot call error too.  So I let it:
>          /* If record_resume get error, return directly.  */
>          status->kind = TARGET_WAITKIND_STOPPED;
>          status->value.sig = TARGET_SIGNAL_0;
>          return inferior_ptid;
>
> But I found that record_resume can call error now.  So I make a new
> patch for it.
>
> Could you help me try it?
>
>
> Thanks,
> Hui
>
> 2009-09-09  Hui Zhu  <teawater@gmail.com>
>
>        * record.c (record_resume_error): Deleted.
>        (record_resume): Call record_message.
>        (record_wait): Deleted record_resume_error.
>
> ---
>  record.c |   19 +------------------
>  1 file changed, 1 insertion(+), 18 deletions(-)
>
> --- a/record.c
> +++ b/record.c
> @@ -516,7 +516,6 @@ record_close (int quitting)
>  }
>
>  static int record_resume_step = 0;
> -static int record_resume_error;
>
>  static void
>  record_resume (struct target_ops *ops, ptid_t ptid, int step,
> @@ -526,15 +525,7 @@ record_resume (struct target_ops *ops, p
>
>   if (!RECORD_IS_REPLAY)
>     {
> -      if (do_record_message (get_current_regcache ()))
> -        {
> -          record_resume_error = 0;
> -        }
> -      else
> -        {
> -          record_resume_error = 1;
> -          return;
> -        }
> +      record_message (get_current_regcache ());
>       record_beneath_to_resume (record_beneath_to_resume_ops, ptid, 1,
>                                 siggnal);
>     }
> @@ -586,14 +577,6 @@ record_wait (struct target_ops *ops,
>
>   if (!RECORD_IS_REPLAY)
>     {
> -      if (record_resume_error)
> -       {
> -         /* If record_resume get error, return directly.  */
> -         status->kind = TARGET_WAITKIND_STOPPED;
> -         status->value.sig = TARGET_SIGNAL_ABRT;
> -         return inferior_ptid;
> -       }
> -
>       if (record_resume_step)
>        {
>          /* This is a single step.  */
>

Re: [RFA] let record_resume fail immediately on error

[Michael Snyder]

Personally, I think this one isn't critical for 7.0.
Waiting for 7.1 will allow us more time to assess it.

Hui Zhu wrote:
> Hi Joel,
> 
> Sorry to disturb you.  Ping
> http://sourceware.org/ml/gdb-patches/2009-09/msg00231.html
> 
> Thanks,
> Hui
> 
> On Wed, Sep 9, 2009 at 10:05, Hui Zhu <teawater@gmail.com> wrote:
>> On Wed, Sep 9, 2009 at 00:55, Michael Snyder<msnyder@vmware.com> wrote:
>>> Hui Zhu wrote:
>>>> On Tue, Sep 8, 2009 at 15:25, Hui Zhu<teawater@gmail.com> wrote:
>>>>> If GDB call error in record_resume, user cannot keep debug the inferior.
>>>>>
>>>>> Hui
>>>>>
>>>>> On Tue, Sep 8, 2009 at 15:23, Hui Zhu<teawater@gmail.com> wrote:
>>>>>> The "record_resume_error" in gdb-cvs is to make user after get a error
>>>>>> of record_message, they can "record stop" close the record and keep
>>>>>> debug the inferior.
>>>>>>
>>>>>> Thanks,
>>>>>> Hui
>>>>>>
>>>>>> On Tue, Sep 8, 2009 at 14:58, Joel Brobecker<brobecker@adacore.com>
>>>>>> wrote:
>>>>>>>>   if (!RECORD_IS_REPLAY)
>>>>>>>>     {
>>>>>>>>       if (do_record_message (get_current_regcache ()))
>>>>>>>> -        {
>>>>>>>> -          record_resume_error = 0;
>>>>>>>> -        }
>>>>>>>> -      else
>>>>>>>> -        {
>>>>>>>> -          record_resume_error = 1;
>>>>>>>> -          return;
>>>>>>>> -        }
>>>>>>>> +     internal_error (__FILE__, __LINE__,
>>>>>>>> +                     _("record_resume: do_record_message failed."));
>>>>>>>> +
>>>>>>> Forgive me if I'm wrong, as I don't know the record.c code at all, but
>>>>>>> I cannot help but think that the internal_error is suspicious here.
>>>>>>> Why is this an internal_error?
>>>>>>>
>>>>>>> --
>>>>>>> Joel
>>>>>>>
>>>> Hi guys,
>>>>
>>>> I make a patch that make "record_resume_error" work better.  I did
>>>> some test.  It seems better than before.
>>> Could you explain what you mean by better?
>>> I mean, what behavior are you looking for here?
>>>
>>> Here is the behavior that I see --- I am making a recording,
>>> I say "continue", and after a while this "record_resume_error"
>>> is triggered, and gdb stops and says "No more reverse-execution history."
>>>
>>> I would never expect to see that message during recording.
>> In before, GDB cannot throw error in record_resume (It just in my
>> memory, maybe I am wrong).
>> If we try to do it, it will get:
>> (gdb) c
>> Continuing.
>> Cannot execute this command while the selected thread is running.
>> So I add record_resume_error to let record_wait to handle it.
>>
>> But record_wait cannot call error too.  So I let it:
>>          /* If record_resume get error, return directly.  */
>>          status->kind = TARGET_WAITKIND_STOPPED;
>>          status->value.sig = TARGET_SIGNAL_0;
>>          return inferior_ptid;
>>
>> But I found that record_resume can call error now.  So I make a new
>> patch for it.
>>
>> Could you help me try it?
>>
>>
>> Thanks,
>> Hui
>>
>> 2009-09-09  Hui Zhu  <teawater@gmail.com>
>>
>>        * record.c (record_resume_error): Deleted.
>>        (record_resume): Call record_message.
>>        (record_wait): Deleted record_resume_error.
>>
>> ---
>>  record.c |   19 +------------------
>>  1 file changed, 1 insertion(+), 18 deletions(-)
>>
>> --- a/record.c
>> +++ b/record.c
>> @@ -516,7 +516,6 @@ record_close (int quitting)
>>  }
>>
>>  static int record_resume_step = 0;
>> -static int record_resume_error;
>>
>>  static void
>>  record_resume (struct target_ops *ops, ptid_t ptid, int step,
>> @@ -526,15 +525,7 @@ record_resume (struct target_ops *ops, p
>>
>>   if (!RECORD_IS_REPLAY)
>>     {
>> -      if (do_record_message (get_current_regcache ()))
>> -        {
>> -          record_resume_error = 0;
>> -        }
>> -      else
>> -        {
>> -          record_resume_error = 1;
>> -          return;
>> -        }
>> +      record_message (get_current_regcache ());
>>       record_beneath_to_resume (record_beneath_to_resume_ops, ptid, 1,
>>                                 siggnal);
>>     }
>> @@ -586,14 +577,6 @@ record_wait (struct target_ops *ops,
>>
>>   if (!RECORD_IS_REPLAY)
>>     {
>> -      if (record_resume_error)
>> -       {
>> -         /* If record_resume get error, return directly.  */
>> -         status->kind = TARGET_WAITKIND_STOPPED;
>> -         status->value.sig = TARGET_SIGNAL_ABRT;
>> -         return inferior_ptid;
>> -       }
>> -
>>       if (record_resume_step)
>>        {
>>          /* This is a single step.  */
>>

Re: [RFA] let record_resume fail immediately on error

[Hui Zhu]

OK.  Thanks, Michael.

Hui

On Sun, Sep 27, 2009 at 02:33, Michael Snyder <msnyder@vmware.com> wrote:
> Personally, I think this one isn't critical for 7.0.
> Waiting for 7.1 will allow us more time to assess it.
>
> Hui Zhu wrote:
>>
>> Hi Joel,
>>
>> Sorry to disturb you.  Ping
>> http://sourceware.org/ml/gdb-patches/2009-09/msg00231.html
>>
>> Thanks,
>> Hui
>>
>> On Wed, Sep 9, 2009 at 10:05, Hui Zhu <teawater@gmail.com> wrote:
>>>
>>> On Wed, Sep 9, 2009 at 00:55, Michael Snyder<msnyder@vmware.com> wrote:
>>>>
>>>> Hui Zhu wrote:
>>>>>
>>>>> On Tue, Sep 8, 2009 at 15:25, Hui Zhu<teawater@gmail.com> wrote:
>>>>>>
>>>>>> If GDB call error in record_resume, user cannot keep debug the
>>>>>> inferior.
>>>>>>
>>>>>> Hui
>>>>>>
>>>>>> On Tue, Sep 8, 2009 at 15:23, Hui Zhu<teawater@gmail.com> wrote:
>>>>>>>
>>>>>>> The "record_resume_error" in gdb-cvs is to make user after get a
>>>>>>> error
>>>>>>> of record_message, they can "record stop" close the record and keep
>>>>>>> debug the inferior.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Hui
>>>>>>>
>>>>>>> On Tue, Sep 8, 2009 at 14:58, Joel Brobecker<brobecker@adacore.com>
>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>>  if (!RECORD_IS_REPLAY)
>>>>>>>>>    {
>>>>>>>>>      if (do_record_message (get_current_regcache ()))
>>>>>>>>> -        {
>>>>>>>>> -          record_resume_error = 0;
>>>>>>>>> -        }
>>>>>>>>> -      else
>>>>>>>>> -        {
>>>>>>>>> -          record_resume_error = 1;
>>>>>>>>> -          return;
>>>>>>>>> -        }
>>>>>>>>> +     internal_error (__FILE__, __LINE__,
>>>>>>>>> +                     _("record_resume: do_record_message
>>>>>>>>> failed."));
>>>>>>>>> +
>>>>>>>>
>>>>>>>> Forgive me if I'm wrong, as I don't know the record.c code at all,
>>>>>>>> but
>>>>>>>> I cannot help but think that the internal_error is suspicious here.
>>>>>>>> Why is this an internal_error?
>>>>>>>>
>>>>>>>> --
>>>>>>>> Joel
>>>>>>>>
>>>>> Hi guys,
>>>>>
>>>>> I make a patch that make "record_resume_error" work better.  I did
>>>>> some test.  It seems better than before.
>>>>
>>>> Could you explain what you mean by better?
>>>> I mean, what behavior are you looking for here?
>>>>
>>>> Here is the behavior that I see --- I am making a recording,
>>>> I say "continue", and after a while this "record_resume_error"
>>>> is triggered, and gdb stops and says "No more reverse-execution
>>>> history."
>>>>
>>>> I would never expect to see that message during recording.
>>>
>>> In before, GDB cannot throw error in record_resume (It just in my
>>> memory, maybe I am wrong).
>>> If we try to do it, it will get:
>>> (gdb) c
>>> Continuing.
>>> Cannot execute this command while the selected thread is running.
>>> So I add record_resume_error to let record_wait to handle it.
>>>
>>> But record_wait cannot call error too.  So I let it:
>>>         /* If record_resume get error, return directly.  */
>>>         status->kind = TARGET_WAITKIND_STOPPED;
>>>         status->value.sig = TARGET_SIGNAL_0;
>>>         return inferior_ptid;
>>>
>>> But I found that record_resume can call error now.  So I make a new
>>> patch for it.
>>>
>>> Could you help me try it?
>>>
>>>
>>> Thanks,
>>> Hui
>>>
>>> 2009-09-09  Hui Zhu  <teawater@gmail.com>
>>>
>>>       * record.c (record_resume_error): Deleted.
>>>       (record_resume): Call record_message.
>>>       (record_wait): Deleted record_resume_error.
>>>
>>> ---
>>>  record.c |   19 +------------------
>>>  1 file changed, 1 insertion(+), 18 deletions(-)
>>>
>>> --- a/record.c
>>> +++ b/record.c
>>> @@ -516,7 +516,6 @@ record_close (int quitting)
>>>  }
>>>
>>>  static int record_resume_step = 0;
>>> -static int record_resume_error;
>>>
>>>  static void
>>>  record_resume (struct target_ops *ops, ptid_t ptid, int step,
>>> @@ -526,15 +525,7 @@ record_resume (struct target_ops *ops, p
>>>
>>>  if (!RECORD_IS_REPLAY)
>>>    {
>>> -      if (do_record_message (get_current_regcache ()))
>>> -        {
>>> -          record_resume_error = 0;
>>> -        }
>>> -      else
>>> -        {
>>> -          record_resume_error = 1;
>>> -          return;
>>> -        }
>>> +      record_message (get_current_regcache ());
>>>      record_beneath_to_resume (record_beneath_to_resume_ops, ptid, 1,
>>>                                siggnal);
>>>    }
>>> @@ -586,14 +577,6 @@ record_wait (struct target_ops *ops,
>>>
>>>  if (!RECORD_IS_REPLAY)
>>>    {
>>> -      if (record_resume_error)
>>> -       {
>>> -         /* If record_resume get error, return directly.  */
>>> -         status->kind = TARGET_WAITKIND_STOPPED;
>>> -         status->value.sig = TARGET_SIGNAL_ABRT;
>>> -         return inferior_ptid;
>>> -       }
>>> -
>>>      if (record_resume_step)
>>>       {
>>>         /* This is a single step.  */
>>>
>
>

Re: [RFA] let record_resume fail immediately on error

[Hui Zhu]

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

Hi Joel and Michael,

I think we handler record really not very well.

For example:
cat 1.c
#include <sys/types.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <errno.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <stdint.h>

int
main(int argc,char *argv[],char *envp[])
{
	asm ("rdtsc");

	return (0);
}

Without the fix error patch:
we will get:
gdb ./a.out
(gdb) start
During symbol reading, DW_AT_name missing from DW_TAG_base_type.
Temporary breakpoint 1 at 0x8048352: file 4.c, line 14.
Starting program: /home/teawater/gdb/bgdbno/gdb/a.out

Temporary breakpoint 1, main (argc=<value optimized out>, argv=<value
optimized out>, envp=<value optimized out>)
    at 4.c:14
14		asm ("rdtsc");
(gdb) record
(gdb) c
Continuing.
Process record doesn't support instruction rdtsc.
Process record doesn't support instruction 0xf31 at address 0x8048352.
Process record: failed to record execution log.

Program received signal SIGABRT, Aborted.
main (argc=<value optimized out>, argv=<value optimized out>,
envp=<value optimized out>) at 4.c:14
14		asm ("rdtsc");
(gdb) c
Continuing.

Program terminated with signal SIGABRT, Aborted.
The program no longer exists.
(gdb) record stop
Process record is not started.

The inferior is killed by gdb, user cannot close the record and keep
debug the inferior.

With the fix error patch:
(gdb) start
During symbol reading, DW_AT_name missing from DW_TAG_base_type.
Temporary breakpoint 1 at 0x8048352: file 4.c, line 14.
Starting program: /home/teawater/gdb/bgdbno/gdb/a.out

Temporary breakpoint 1, main (argc=<value optimized out>, argv=<value
optimized out>, envp=<value optimized out>)
    at 4.c:14
14		asm ("rdtsc");
(gdb) record
(gdb) c
Continuing.
Process record doesn't support instruction rdtsc.
Process record doesn't support instruction 0xf31 at address 0x8048352.
main (argc=<value optimized out>, argv=<value optimized out>,
envp=<value optimized out>) at 4.c:14
14		asm ("rdtsc");
Process record: failed to record execution log.
(gdb) c
Continuing.
Process record doesn't support instruction rdtsc.
Process record doesn't support instruction 0xf31 at address 0x8048352.
main (argc=<value optimized out>, argv=<value optimized out>,
envp=<value optimized out>) at 4.c:14
14		asm ("rdtsc");
Process record: failed to record execution log.
(gdb)
Continuing.
Process record doesn't support instruction rdtsc.
Process record doesn't support instruction 0xf31 at address 0x8048352.
main (argc=<value optimized out>, argv=<value optimized out>,
envp=<value optimized out>) at 4.c:14
14		asm ("rdtsc");
Process record: failed to record execution log.
(gdb)
Continuing.
Process record doesn't support instruction rdtsc.
Process record doesn't support instruction 0xf31 at address 0x8048352.
main (argc=<value optimized out>, argv=<value optimized out>,
envp=<value optimized out>) at 4.c:14
14		asm ("rdtsc");
Process record: failed to record execution log.
(gdb)
Continuing.
Process record doesn't support instruction rdtsc.
Process record doesn't support instruction 0xf31 at address 0x8048352.
main (argc=<value optimized out>, argv=<value optimized out>,
envp=<value optimized out>) at 4.c:14
14		asm ("rdtsc");
Process record: failed to record execution log.
(gdb)
Continuing.
Process record doesn't support instruction rdtsc.
Process record doesn't support instruction 0xf31 at address 0x8048352.
main (argc=<value optimized out>, argv=<value optimized out>,
envp=<value optimized out>) at 4.c:14
14		asm ("rdtsc");
Process record: failed to record execution log.
(gdb)
Continuing.
Process record doesn't support instruction rdtsc.
Process record doesn't support instruction 0xf31 at address 0x8048352.
main (argc=<value optimized out>, argv=<value optimized out>,
envp=<value optimized out>) at 4.c:14
14		asm ("rdtsc");
Process record: failed to record execution log.
(gdb)
Continuing.
Process record doesn't support instruction rdtsc.
Process record doesn't support instruction 0xf31 at address 0x8048352.
main (argc=<value optimized out>, argv=<value optimized out>,
envp=<value optimized out>) at 4.c:14
14		asm ("rdtsc");
Process record: failed to record execution log.
(gdb) record stop
Process record is stoped and all execution log is deleted.
(gdb) c
Continuing.

User can keep debug the inferior after close the record.

So maybe we need this patch in 7.0.  What do you think about it?

Thanks,
Hui

BTW, I make a new patch follow the gdb update:

2009-09-28  Hui Zhu  <teawater@gmail.com>

	* record.c (record_resume_error): Deleted.
	(record_resume): Call record_message.
	(record_wait): Deleted record_resume_error.

---
 record.c |   23 +++++------------------
 1 file changed, 5 insertions(+), 18 deletions(-)

--- a/record.c
+++ b/record.c
@@ -566,7 +566,6 @@ record_close (int quitting)
 }

 static int record_resume_step = 0;
-static int record_resume_error;

 static void
 record_resume (struct target_ops *ops, ptid_t ptid, int step,
@@ -576,15 +575,11 @@ record_resume (struct target_ops *ops, p

   if (!RECORD_IS_REPLAY)
     {
-      if (do_record_message (get_current_regcache (), signal))
-        {
-          record_resume_error = 0;
-        }
-      else
-        {
-          record_resume_error = 1;
-          return;
-        }
+      struct record_message_args args;
+
+      args.regcache = get_current_regcache ();
+      args.signal = signal;
+      record_message (&args);
       record_beneath_to_resume (record_beneath_to_resume_ops, ptid, 1,
                                 signal);
     }
@@ -636,14 +631,6 @@ record_wait (struct target_ops *ops,

   if (!RECORD_IS_REPLAY)
     {
-      if (record_resume_error)
-	{
-	  /* If record_resume get error, return directly.  */
-	  status->kind = TARGET_WAITKIND_STOPPED;
-	  status->value.sig = TARGET_SIGNAL_ABRT;
-	  return inferior_ptid;
-	}
-
       if (record_resume_step)
 	{
 	  /* This is a single step.  */


On Sun, Sep 27, 2009 at 02:33, Michael Snyder <msnyder@vmware.com> wrote:
> Personally, I think this one isn't critical for 7.0.
> Waiting for 7.1 will allow us more time to assess it.
>
> Hui Zhu wrote:
>>
>> Hi Joel,
>>
>> Sorry to disturb you.  Ping
>> http://sourceware.org/ml/gdb-patches/2009-09/msg00231.html
>>
>> Thanks,
>> Hui
>>
>> On Wed, Sep 9, 2009 at 10:05, Hui Zhu <teawater@gmail.com> wrote:
>>>
>>> On Wed, Sep 9, 2009 at 00:55, Michael Snyder<msnyder@vmware.com> wrote:
>>>>
>>>> Hui Zhu wrote:
>>>>>
>>>>> On Tue, Sep 8, 2009 at 15:25, Hui Zhu<teawater@gmail.com> wrote:
>>>>>>
>>>>>> If GDB call error in record_resume, user cannot keep debug the
>>>>>> inferior.
>>>>>>
>>>>>> Hui
>>>>>>
>>>>>> On Tue, Sep 8, 2009 at 15:23, Hui Zhu<teawater@gmail.com> wrote:
>>>>>>>
>>>>>>> The "record_resume_error" in gdb-cvs is to make user after get a
>>>>>>> error
>>>>>>> of record_message, they can "record stop" close the record and keep
>>>>>>> debug the inferior.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Hui
>>>>>>>
>>>>>>> On Tue, Sep 8, 2009 at 14:58, Joel Brobecker<brobecker@adacore.com>
>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>>  if (!RECORD_IS_REPLAY)
>>>>>>>>>    {
>>>>>>>>>      if (do_record_message (get_current_regcache ()))
>>>>>>>>> -        {
>>>>>>>>> -          record_resume_error = 0;
>>>>>>>>> -        }
>>>>>>>>> -      else
>>>>>>>>> -        {
>>>>>>>>> -          record_resume_error = 1;
>>>>>>>>> -          return;
>>>>>>>>> -        }
>>>>>>>>> +     internal_error (__FILE__, __LINE__,
>>>>>>>>> +                     _("record_resume: do_record_message
>>>>>>>>> failed."));
>>>>>>>>> +
>>>>>>>>
>>>>>>>> Forgive me if I'm wrong, as I don't know the record.c code at all,
>>>>>>>> but
>>>>>>>> I cannot help but think that the internal_error is suspicious here.
>>>>>>>> Why is this an internal_error?
>>>>>>>>
>>>>>>>> --
>>>>>>>> Joel
>>>>>>>>
>>>>> Hi guys,
>>>>>
>>>>> I make a patch that make "record_resume_error" work better.  I did
>>>>> some test.  It seems better than before.
>>>>
>>>> Could you explain what you mean by better?
>>>> I mean, what behavior are you looking for here?
>>>>
>>>> Here is the behavior that I see --- I am making a recording,
>>>> I say "continue", and after a while this "record_resume_error"
>>>> is triggered, and gdb stops and says "No more reverse-execution
>>>> history."
>>>>
>>>> I would never expect to see that message during recording.
>>>
>>> In before, GDB cannot throw error in record_resume (It just in my
>>> memory, maybe I am wrong).
>>> If we try to do it, it will get:
>>> (gdb) c
>>> Continuing.
>>> Cannot execute this command while the selected thread is running.
>>> So I add record_resume_error to let record_wait to handle it.
>>>
>>> But record_wait cannot call error too.  So I let it:
>>>         /* If record_resume get error, return directly.  */
>>>         status->kind = TARGET_WAITKIND_STOPPED;
>>>         status->value.sig = TARGET_SIGNAL_0;
>>>         return inferior_ptid;
>>>
>>> But I found that record_resume can call error now.  So I make a new
>>> patch for it.
>>>
>>> Could you help me try it?
>>>
>>>
>>> Thanks,
>>> Hui
>>>
>>> 2009-09-09  Hui Zhu  <teawater@gmail.com>
>>>
>>>       * record.c (record_resume_error): Deleted.
>>>       (record_resume): Call record_message.
>>>       (record_wait): Deleted record_resume_error.
>>>
>>> ---
>>>  record.c |   19 +------------------
>>>  1 file changed, 1 insertion(+), 18 deletions(-)
>>>
>>> --- a/record.c
>>> +++ b/record.c
>>> @@ -516,7 +516,6 @@ record_close (int quitting)
>>>  }
>>>
>>>  static int record_resume_step = 0;
>>> -static int record_resume_error;
>>>
>>>  static void
>>>  record_resume (struct target_ops *ops, ptid_t ptid, int step,
>>> @@ -526,15 +525,7 @@ record_resume (struct target_ops *ops, p
>>>
>>>  if (!RECORD_IS_REPLAY)
>>>    {
>>> -      if (do_record_message (get_current_regcache ()))
>>> -        {
>>> -          record_resume_error = 0;
>>> -        }
>>> -      else
>>> -        {
>>> -          record_resume_error = 1;
>>> -          return;
>>> -        }
>>> +      record_message (get_current_regcache ());
>>>      record_beneath_to_resume (record_beneath_to_resume_ops, ptid, 1,
>>>                                siggnal);
>>>    }
>>> @@ -586,14 +577,6 @@ record_wait (struct target_ops *ops,
>>>
>>>  if (!RECORD_IS_REPLAY)
>>>    {
>>> -      if (record_resume_error)
>>> -       {
>>> -         /* If record_resume get error, return directly.  */
>>> -         status->kind = TARGET_WAITKIND_STOPPED;
>>> -         status->value.sig = TARGET_SIGNAL_ABRT;
>>> -         return inferior_ptid;
>>> -       }
>>> -
>>>      if (record_resume_step)
>>>       {
>>>         /* This is a single step.  */
>>>
>
>

[-- Attachment #2: prec-fix-error-handler.txt --]
[-- Type: text/plain, Size: 1280 bytes --]

---
 record.c |   23 +++++------------------
 1 file changed, 5 insertions(+), 18 deletions(-)

--- a/record.c
+++ b/record.c
@@ -566,7 +566,6 @@ record_close (int quitting)
 }
 
 static int record_resume_step = 0;
-static int record_resume_error;
 
 static void
 record_resume (struct target_ops *ops, ptid_t ptid, int step,
@@ -576,15 +575,11 @@ record_resume (struct target_ops *ops, p
 
   if (!RECORD_IS_REPLAY)
     {
-      if (do_record_message (get_current_regcache (), signal))
-        {
-          record_resume_error = 0;
-        }
-      else
-        {
-          record_resume_error = 1;
-          return;
-        }
+      struct record_message_args args;
+
+      args.regcache = get_current_regcache ();
+      args.signal = signal;
+      record_message (&args);
       record_beneath_to_resume (record_beneath_to_resume_ops, ptid, 1,
                                 signal);
     }
@@ -636,14 +631,6 @@ record_wait (struct target_ops *ops,
 
   if (!RECORD_IS_REPLAY)
     {
-      if (record_resume_error)
-	{
-	  /* If record_resume get error, return directly.  */
-	  status->kind = TARGET_WAITKIND_STOPPED;
-	  status->value.sig = TARGET_SIGNAL_ABRT;
-	  return inferior_ptid;
-	}
-
       if (record_resume_step)
 	{
 	  /* This is a single step.  */

Re: [RFA] let record_resume fail immediately on error

[Joel Brobecker]

> With the fix error patch:
[...]
> (gdb) c
> Continuing.
> Process record doesn't support instruction rdtsc.
> Process record doesn't support instruction 0xf31 at address 0x8048352.
> main (argc=<value optimized out>, argv=<value optimized out>,
> envp=<value optimized out>) at 4.c:14
> 14		asm ("rdtsc");
> Process record: failed to record execution log.
> (gdb)
> Continuing.
> Process record doesn't support instruction rdtsc.
> Process record doesn't support instruction 0xf31 at address 0x8048352.
> main (argc=<value optimized out>, argv=<value optimized out>,
> envp=<value optimized out>) at 4.c:14
> 14		asm ("rdtsc");
> Process record: failed to record execution log.

I am going to defer to Michael who understands the issues much better
than I do; I am a little confused by what you are suggesting. On the
one hand, I understand that the problem you're trying to deal with
is the case when the inferior exits. Are you talking about normal
exiting as well as abnormal exiting like in your case? In any event,
what you show in your example looks like a bandaid that will only
handle a specific situation: In other words, it works around the issue
because you no longer let the inferior exit.

-- 
Joel

Re: [RFA] let record_resume fail immediately on error

[Hui Zhu]

On Tue, Sep 29, 2009 at 00:07, Joel Brobecker <brobecker@adacore.com> wrote:
>> With the fix error patch:
> [...]
>> (gdb) c
>> Continuing.
>> Process record doesn't support instruction rdtsc.
>> Process record doesn't support instruction 0xf31 at address 0x8048352.
>> main (argc=<value optimized out>, argv=<value optimized out>,
>> envp=<value optimized out>) at 4.c:14
>> 14            asm ("rdtsc");
>> Process record: failed to record execution log.
>> (gdb)
>> Continuing.
>> Process record doesn't support instruction rdtsc.
>> Process record doesn't support instruction 0xf31 at address 0x8048352.
>> main (argc=<value optimized out>, argv=<value optimized out>,
>> envp=<value optimized out>) at 4.c:14
>> 14            asm ("rdtsc");
>> Process record: failed to record execution log.
>
> I am going to defer to Michael who understands the issues much better
> than I do; I am a little confused by what you are suggesting. On the
> one hand, I understand that the problem you're trying to deal with
> is the case when the inferior exits. Are you talking about normal
> exiting as well as abnormal exiting like in your case? In any event,
> what you show in your example looks like a bandaid that will only
> handle a specific situation: In other words, it works around the issue
> because you no longer let the inferior exit.

Sorry I didn't talk it very clear.  Let me try it.

Sometime, prec will get a error for example some insn didn't support.
But it didn't affect the inferior, so customer can close the prec and
keep debug the inferior.

But prec has the bug that will make the inferior exit when get the
error in prec part (like the example that I post in prev mail).  It is
not right because inferior is OK.  So I make the patch for it.

After patch the patch, the inferior will not exit in this prec part error.


Thanks,
Hui

Re: [RFA] let record_resume fail immediately on error

[Joel Brobecker]

> Sorry I didn't talk it very clear.  Let me try it.

In other words: If an error occurs during recording, somehow
the inferior "runs away", meaning runs until completion?
Do we lose the process record?

Based on the transcript of the session *with* the patch you propose,
it looks like GDB is now just stuck on that instruction that it does
not know how to record. Is that really progress?

-- 
Joel

Re: [RFA] let record_resume fail immediately on error

[Hui Zhu]

On Wed, Sep 30, 2009 at 05:29, Joel Brobecker <brobecker@adacore.com> wrote:
>> Sorry I didn't talk it very clear.  Let me try it.
>
> In other words: If an error occurs during recording, somehow
> the inferior "runs away", meaning runs until completion?
> Do we lose the process record?

If the error happen, prec will stop the inferior.

But make the inferior is not right.  It because I use signal in
record_wait.  So I remove it now.

>
> Based on the transcript of the session *with* the patch you propose,
> it looks like GDB is now just stuck on that instruction that it does
> not know how to record. Is that really progress?

The insn nonsupport error is a error that easy to reproduce.   I think
the current prec will make the inferior exit with other error.

Thanks,
Hui

Re: [RFA] let record_resume fail immediately on error

[Joel Brobecker]

Hui,

It looks like you might be waiting for input from someone, or for
approval? I confess that I'm still completely confused as to what
the problem is and how you're resolving it.  I don't want to be
the one slowing you down, so if Michael is happy, I'm happy.  But
if you'd like me to take a look, can you try to explain the issue
in a different way?

For instance, I asked:

> In other words: If an error occurs during recording, somehow
> the inferior "runs away", meaning runs until completion?
> Do we lose the process record?

I was mentioning this as being the current behavior, which
presumably is wrong.  Am I correct?

I also asked:

> Based on the transcript of the session *with* the patch you propose,
> it looks like GDB is now just stuck on that instruction that it does
> not know how to record. Is that really progress?

I am now refering to the situation *AFTER* your patch is applied.
I couldn't understand the answer to sent or how it was relevant to
my question.

-- 
Joel

Re: [RFA] let record_resume fail immediately on error

[Hui Zhu]

Hi Joel,

I think explain is very hard to make you happy with this patch.  I am
really not good at it.  Sorry for it.

Could you please try the example?

For example:
cat 1.c
#include <sys/types.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <errno.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <stdint.h>

int
main(int argc,char *argv[],char *envp[])
{
       asm ("rdtsc");

       return (0);
}

Without the fix error patch:
we will get:
gdb ./a.out
(gdb) start
During symbol reading, DW_AT_name missing from DW_TAG_base_type.
Temporary breakpoint 1 at 0x8048352: file 4.c, line 14.
Starting program: /home/teawater/gdb/bgdbno/gdb/a.out

Temporary breakpoint 1, main (argc=<value optimized out>, argv=<value
optimized out>, envp=<value optimized out>)
   at 4.c:14
14              asm ("rdtsc");
(gdb) record
(gdb) c
Continuing.
Process record doesn't support instruction rdtsc.
Process record doesn't support instruction 0xf31 at address 0x8048352.
Process record: failed to record execution log.

Program received signal SIGABRT, Aborted.
main (argc=<value optimized out>, argv=<value optimized out>,
envp=<value optimized out>) at 4.c:14
14              asm ("rdtsc");
(gdb) c
Continuing.

Program terminated with signal SIGABRT, Aborted.
The program no longer exists.
(gdb) record stop
Process record is not started.


Thanks,
Hui

On Wed, Oct 14, 2009 at 10:10, Joel Brobecker <brobecker@adacore.com> wrote:
> Hui,
>
> It looks like you might be waiting for input from someone, or for
> approval? I confess that I'm still completely confused as to what
> the problem is and how you're resolving it.  I don't want to be
> the one slowing you down, so if Michael is happy, I'm happy.  But
> if you'd like me to take a look, can you try to explain the issue
> in a different way?
>
> For instance, I asked:
>
>> In other words: If an error occurs during recording, somehow
>> the inferior "runs away", meaning runs until completion?
>> Do we lose the process record?
>
> I was mentioning this as being the current behavior, which
> presumably is wrong.  Am I correct?
>
> I also asked:
>
>> Based on the transcript of the session *with* the patch you propose,
>> it looks like GDB is now just stuck on that instruction that it does
>> not know how to record. Is that really progress?
>
> I am now refering to the situation *AFTER* your patch is applied.
> I couldn't understand the answer to sent or how it was relevant to
> my question.
>
> --
> Joel
>

Re: [RFA] let record_resume fail immediately on error

[Joel Brobecker]

> (gdb) record
> (gdb) c
> Continuing.
> Process record doesn't support instruction rdtsc.
> Process record doesn't support instruction 0xf31 at address 0x8048352.
> Process record: failed to record execution log.
> 
> Program received signal SIGABRT, Aborted.

OK - I can see that there is a SIGABRT, and so I suspect that this
SIGABRT is a consequence of the problem you're trying to fix.  Can you
explain the sequence of events that occur inside GDB that cause this
SIGABRT?  Can you also explain how you are fixing this problem? Again,
from a copy of the GDB session *after* your patch, it looks like GDB
is stuck on the unsupported instruction, and I'm not sure that this
is an improvement.  So I'm assuming that I am not understanding what
your fix is doing.

-- 
Joel

Re: [RFA] let record_resume fail immediately on error

[Hui Zhu]

The important part is:
(gdb) c
Continuing.

Program terminated with signal SIGABRT, Aborted.
The program no longer exists.
(gdb) record stop
Process record is not started.

The inferior is dead.

"it looks like GDB
> is stuck on the unsupported instruction, and I'm not sure that this
> is an improvement."

Stop is better than dead, right?

Hui


On Wed, Oct 14, 2009 at 10:42, Joel Brobecker <brobecker@adacore.com> wrote:
>> (gdb) record
>> (gdb) c
>> Continuing.
>> Process record doesn't support instruction rdtsc.
>> Process record doesn't support instruction 0xf31 at address 0x8048352.
>> Process record: failed to record execution log.
>>
>> Program received signal SIGABRT, Aborted.
>
> OK - I can see that there is a SIGABRT, and so I suspect that this
> SIGABRT is a consequence of the problem you're trying to fix.  Can you
> explain the sequence of events that occur inside GDB that cause this
> SIGABRT?  Can you also explain how you are fixing this problem? Again,
> from a copy of the GDB session *after* your patch, it looks like GDB
> is stuck on the unsupported instruction, and I'm not sure that this
> is an improvement.  So I'm assuming that I am not understanding what
> your fix is doing.
>
> --
> Joel
>

Re: [RFA] let record_resume fail immediately on error

[Joel Brobecker]

> Stop is better than dead, right?

Hmmmm, I suppose I forgot that you can still go back while your stuck.
So, yeah, that's better.

-- 
Joel

Re: [RFA] let record_resume fail immediately on error

[Hui Zhu]

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

Thanks Joel,

I think stop with sigtrap looks not very well.  So I change it to sigint.

Please help me with it.

Thanks,
Hui

2009-10-15  Hui Zhu  <teawater@gmail.com>

	* record.c (record_resume_error): Deleted.
	(record_resume): Call record_message.
	(record_wait): Deleted record_resume_error.
	Set status when do_record_message need stop the inferior.
---
 record.c |   25 +++++++------------------
 1 file changed, 7 insertions(+), 18 deletions(-)

--- a/record.c
+++ b/record.c
@@ -566,7 +566,6 @@ record_close (int quitting)
 }

 static int record_resume_step = 0;
-static int record_resume_error;

 static void
 record_resume (struct target_ops *ops, ptid_t ptid, int step,
@@ -576,15 +575,11 @@ record_resume (struct target_ops *ops, p

   if (!RECORD_IS_REPLAY)
     {
-      if (do_record_message (get_current_regcache (), signal))
-        {
-          record_resume_error = 0;
-        }
-      else
-        {
-          record_resume_error = 1;
-          return;
-        }
+      struct record_message_args args;
+
+      args.regcache = get_current_regcache ();
+      args.signal = signal;
+      record_message (&args);
       record_beneath_to_resume (record_beneath_to_resume_ops, ptid, 1,
                                 signal);
     }
@@ -636,14 +631,6 @@ record_wait (struct target_ops *ops,

   if (!RECORD_IS_REPLAY)
     {
-      if (record_resume_error)
-	{
-	  /* If record_resume get error, return directly.  */
-	  status->kind = TARGET_WAITKIND_STOPPED;
-	  status->value.sig = TARGET_SIGNAL_ABRT;
-	  return inferior_ptid;
-	}
-
       if (record_resume_step)
 	{
 	  /* This is a single step.  */
@@ -689,6 +676,8 @@ record_wait (struct target_ops *ops,
 		      if (!do_record_message (get_current_regcache (),
 					      TARGET_SIGNAL_0))
 			{
+                          status->kind = TARGET_WAITKIND_STOPPED;
+                          status->value.sig = TARGET_SIGNAL_INT;
                           break;
 			}
 		      record_beneath_to_resume (record_beneath_to_resume_ops,

[-- Attachment #2: prec-fix-error-handler.txt --]
[-- Type: text/plain, Size: 1671 bytes --]

---
 record.c |   25 +++++++------------------
 1 file changed, 7 insertions(+), 18 deletions(-)

--- a/record.c
+++ b/record.c
@@ -566,7 +566,6 @@ record_close (int quitting)
 }
 
 static int record_resume_step = 0;
-static int record_resume_error;
 
 static void
 record_resume (struct target_ops *ops, ptid_t ptid, int step,
@@ -576,15 +575,11 @@ record_resume (struct target_ops *ops, p
 
   if (!RECORD_IS_REPLAY)
     {
-      if (do_record_message (get_current_regcache (), signal))
-        {
-          record_resume_error = 0;
-        }
-      else
-        {
-          record_resume_error = 1;
-          return;
-        }
+      struct record_message_args args;
+
+      args.regcache = get_current_regcache ();
+      args.signal = signal;
+      record_message (&args);
       record_beneath_to_resume (record_beneath_to_resume_ops, ptid, 1,
                                 signal);
     }
@@ -636,14 +631,6 @@ record_wait (struct target_ops *ops,
 
   if (!RECORD_IS_REPLAY)
     {
-      if (record_resume_error)
-	{
-	  /* If record_resume get error, return directly.  */
-	  status->kind = TARGET_WAITKIND_STOPPED;
-	  status->value.sig = TARGET_SIGNAL_ABRT;
-	  return inferior_ptid;
-	}
-
       if (record_resume_step)
 	{
 	  /* This is a single step.  */
@@ -689,6 +676,8 @@ record_wait (struct target_ops *ops,
 		      if (!do_record_message (get_current_regcache (),
 					      TARGET_SIGNAL_0))
 			{
+                          status->kind = TARGET_WAITKIND_STOPPED;
+                          status->value.sig = TARGET_SIGNAL_INT;
                           break;
 			}
 		      record_beneath_to_resume (record_beneath_to_resume_ops,

Re: [RFA] let record_resume fail immediately on error

[Joel Brobecker]

> I think stop with sigtrap looks not very well.  So I change it to sigint.

From a user perspective, either is confusing and, IMO, wrong.  The program
did not receive any of these signals, so it seems like we're pretending
that they did.  Can't we just have an error message "Error: unsupported
instruction, cannot continue from there", and just display the frame
where we stopped?

-- 
Joel

Re: [RFA] let record_resume fail immediately on error

[Michael Snyder]

Joel Brobecker wrote:
>> I think stop with sigtrap looks not very well.  So I change it to sigint.
> 
> From a user perspective, either is confusing and, IMO, wrong.  The program
> did not receive any of these signals, so it seems like we're pretending
> that they did.  Can't we just have an error message "Error: unsupported
> instruction, cannot continue from there", and just display the frame
> where we stopped?

Hui, try returning signal zero.  GDB will just report that you
stopped, without saying anything about a signal.

Old trick of mine.  ;-)

Re: [RFA] let record_resume fail immediately on error

[Hui Zhu]

> Hui, try returning signal zero.  GDB will just report that you
> stopped, without saying anything about a signal.
>
> Old trick of mine.  ;-)

Cool!  The signal 0 make the output like following:
The next instruction is syscall exit_group.  It will make the program
exit.  Do you want to stop the program?([y] or n)
Process record: inferior program stopped.

[process 25901] #1 stopped.
0xb7fe3405 in __kernel_vsyscall ()
(gdb) rc

Or:
Process record doesn't support instruction rdtsc.
Process record doesn't support instruction 0xf31 at address 0x8048391.
Process record: failed to record execution log.

[process 25947] #1 stopped.
main (argc=<value optimized out>, argv=<value optimized out>,
envp=<value optimized out>) at 2.c:16
16		asm ("rdtsc");

I make a new patch for it.  Please help me with it.

Thanks,
Hui

2009-10-16  Hui Zhu  <teawater@gmail.com>

	* record.c (record_resume_error): Deleted.
	(record_resume): Call record_message.
	(record_wait): Deleted record_resume_error.
	Set status when do_record_message need stop the inferior.
---
 record.c |   25 +++++++------------------
 1 file changed, 7 insertions(+), 18 deletions(-)

--- a/record.c
+++ b/record.c
@@ -643,7 +643,6 @@ record_close (int quitting)
 }

 static int record_resume_step = 0;
-static int record_resume_error;

 static void
 record_resume (struct target_ops *ops, ptid_t ptid, int step,
@@ -653,15 +652,11 @@ record_resume (struct target_ops *ops, p

   if (!RECORD_IS_REPLAY)
     {
-      if (do_record_message (get_current_regcache (), signal))
-        {
-          record_resume_error = 0;
-        }
-      else
-        {
-          record_resume_error = 1;
-          return;
-        }
+      struct record_message_args args;
+
+      args.regcache = get_current_regcache ();
+      args.signal = signal;
+      record_message (&args);
       record_beneath_to_resume (record_beneath_to_resume_ops, ptid, 1,
                                 signal);
     }
@@ -713,14 +708,6 @@ record_wait (struct target_ops *ops,

   if (!RECORD_IS_REPLAY)
     {
-      if (record_resume_error)
-	{
-	  /* If record_resume get error, return directly.  */
-	  status->kind = TARGET_WAITKIND_STOPPED;
-	  status->value.sig = TARGET_SIGNAL_ABRT;
-	  return inferior_ptid;
-	}
-
       if (record_resume_step)
 	{
 	  /* This is a single step.  */
@@ -766,6 +753,8 @@ record_wait (struct target_ops *ops,
 		      if (!do_record_message (get_current_regcache (),
 					      TARGET_SIGNAL_0))
 			{
+                          status->kind = TARGET_WAITKIND_STOPPED;
+                          status->value.sig = TARGET_SIGNAL_0;
                           break;
 			}
 		      record_beneath_to_resume (record_beneath_to_resume_ops,

Re: [RFA] let record_resume fail immediately on error

[Hui Zhu]

Update follow cvs-head.

Please help me with it.

Thanks,
Hui

2009-10-20  Hui Zhu  <teawater@gmail.com>

	* record.c (record_resume_error): Deleted.
	(record_resume): Call record_message.
	(record_wait): Deleted record_resume_error.
	Set status when do_record_message need stop the inferior.

---
 record.c |   29 ++++++++++-------------------
 1 file changed, 10 insertions(+), 19 deletions(-)

--- a/record.c
+++ b/record.c
@@ -679,7 +679,6 @@ record_close (int quitting)
 }

 static int record_resume_step = 0;
-static int record_resume_error;

 static void
 record_resume (struct target_ops *ops, ptid_t ptid, int step,
@@ -689,15 +688,11 @@ record_resume (struct target_ops *ops, p

   if (!RECORD_IS_REPLAY)
     {
-      if (do_record_message (get_current_regcache (), signal))
-        {
-          record_resume_error = 0;
-        }
-      else
-        {
-          record_resume_error = 1;
-          return;
-        }
+      struct record_message_args args;
+
+      args.regcache = get_current_regcache ();
+      args.signal = signal;
+      record_message (&args);
       record_beneath_to_resume (record_beneath_to_resume_ops, ptid, 1,
                                 signal);
     }
@@ -749,14 +744,6 @@ record_wait (struct target_ops *ops,

   if (!RECORD_IS_REPLAY)
     {
-      if (record_resume_error)
-	{
-	  /* If record_resume get error, return directly.  */
-	  status->kind = TARGET_WAITKIND_STOPPED;
-	  status->value.sig = TARGET_SIGNAL_ABRT;
-	  return inferior_ptid;
-	}
-
       if (record_resume_step)
 	{
 	  /* This is a single step.  */
@@ -802,7 +789,11 @@ record_wait (struct target_ops *ops,
 			 Therefore we will not return to gdb.
 		         Record the insn and resume.  */
 		      if (!do_record_message (regcache, TARGET_SIGNAL_0))
-			break;
+  			{
+                           status->kind = TARGET_WAITKIND_STOPPED;
+                           status->value.sig = TARGET_SIGNAL_0;
+                           break;
+  			}

 		      record_beneath_to_resume (record_beneath_to_resume_ops,
 						ptid, 1,

Re: [RFA] let record_resume fail immediately on error

[Hui Zhu]

Ping.

On Tue, Oct 20, 2009 at 11:17, Hui Zhu <teawater@gmail.com> wrote:
> Update follow cvs-head.
>
> Please help me with it.
>
> Thanks,
> Hui
>
> 2009-10-20  Hui Zhu  <teawater@gmail.com>
>
>        * record.c (record_resume_error): Deleted.
>        (record_resume): Call record_message.
>        (record_wait): Deleted record_resume_error.
>        Set status when do_record_message need stop the inferior.
>
> ---
>  record.c |   29 ++++++++++-------------------
>  1 file changed, 10 insertions(+), 19 deletions(-)
>
> --- a/record.c
> +++ b/record.c
> @@ -679,7 +679,6 @@ record_close (int quitting)
>  }
>
>  static int record_resume_step = 0;
> -static int record_resume_error;
>
>  static void
>  record_resume (struct target_ops *ops, ptid_t ptid, int step,
> @@ -689,15 +688,11 @@ record_resume (struct target_ops *ops, p
>
>   if (!RECORD_IS_REPLAY)
>     {
> -      if (do_record_message (get_current_regcache (), signal))
> -        {
> -          record_resume_error = 0;
> -        }
> -      else
> -        {
> -          record_resume_error = 1;
> -          return;
> -        }
> +      struct record_message_args args;
> +
> +      args.regcache = get_current_regcache ();
> +      args.signal = signal;
> +      record_message (&args);
>       record_beneath_to_resume (record_beneath_to_resume_ops, ptid, 1,
>                                 signal);
>     }
> @@ -749,14 +744,6 @@ record_wait (struct target_ops *ops,
>
>   if (!RECORD_IS_REPLAY)
>     {
> -      if (record_resume_error)
> -       {
> -         /* If record_resume get error, return directly.  */
> -         status->kind = TARGET_WAITKIND_STOPPED;
> -         status->value.sig = TARGET_SIGNAL_ABRT;
> -         return inferior_ptid;
> -       }
> -
>       if (record_resume_step)
>        {
>          /* This is a single step.  */
> @@ -802,7 +789,11 @@ record_wait (struct target_ops *ops,
>                         Therefore we will not return to gdb.
>                         Record the insn and resume.  */
>                      if (!do_record_message (regcache, TARGET_SIGNAL_0))
> -                       break;
> +                       {
> +                           status->kind = TARGET_WAITKIND_STOPPED;
> +                           status->value.sig = TARGET_SIGNAL_0;
> +                           break;
> +                       }
>
>                      record_beneath_to_resume (record_beneath_to_resume_ops,
>                                                ptid, 1,
>

Re: [RFA] let record_resume fail immediately on error

[Hui Zhu]

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

Hi Michael,

Now we have a lot of patch in running.
Could you please help me review this patch first?

Thanks,
Hui

2009-11-03  Hui Zhu  <teawater@gmail.com>

	* record.c (record_resume_error): Deleted.
	(record_resume): Call record_message.
	(record_wait): Deleted record_resume_error.
	Set status when do_record_message need stop the inferior.

---
 record.c |   29 ++++++++++-------------------
 1 file changed, 10 insertions(+), 19 deletions(-)

--- a/record.c
+++ b/record.c
@@ -954,7 +954,6 @@ record_close (int quitting)
 }

 static int record_resume_step = 0;
-static int record_resume_error;

 /* "to_resume" target method.  Resume the process record target.  */

@@ -966,15 +965,11 @@ record_resume (struct target_ops *ops, p

   if (!RECORD_IS_REPLAY)
     {
-      if (do_record_message (get_current_regcache (), signal))
-        {
-          record_resume_error = 0;
-        }
-      else
-        {
-          record_resume_error = 1;
-          return;
-        }
+      struct record_message_args args;
+
+      args.regcache = get_current_regcache ();
+      args.signal = signal;
+      record_message (&args);
       record_beneath_to_resume (record_beneath_to_resume_ops, ptid, 1,
                                 signal);
     }
@@ -1038,14 +1033,6 @@ record_wait (struct target_ops *ops,

   if (!RECORD_IS_REPLAY && ops != &record_core_ops)
     {
-      if (record_resume_error)
-	{
-	  /* If record_resume get error, return directly.  */
-	  status->kind = TARGET_WAITKIND_STOPPED;
-	  status->value.sig = TARGET_SIGNAL_ABRT;
-	  return inferior_ptid;
-	}
-
       if (record_resume_step)
 	{
 	  /* This is a single step.  */
@@ -1091,7 +1078,11 @@ record_wait (struct target_ops *ops,
 			 Therefore we will not return to gdb.
 		         Record the insn and resume.  */
 		      if (!do_record_message (regcache, TARGET_SIGNAL_0))
-			break;
+  			{
+                           status->kind = TARGET_WAITKIND_STOPPED;
+                           status->value.sig = TARGET_SIGNAL_0;
+                           break;
+  			}

 		      record_beneath_to_resume (record_beneath_to_resume_ops,
 						ptid, 1,

[-- Attachment #2: prec-fix-error-handler.txt --]
[-- Type: text/plain, Size: 1797 bytes --]

---
 record.c |   29 ++++++++++-------------------
 1 file changed, 10 insertions(+), 19 deletions(-)

--- a/record.c
+++ b/record.c
@@ -954,7 +954,6 @@ record_close (int quitting)
 }
 
 static int record_resume_step = 0;
-static int record_resume_error;
 
 /* "to_resume" target method.  Resume the process record target.  */
 
@@ -966,15 +965,11 @@ record_resume (struct target_ops *ops, p
 
   if (!RECORD_IS_REPLAY)
     {
-      if (do_record_message (get_current_regcache (), signal))
-        {
-          record_resume_error = 0;
-        }
-      else
-        {
-          record_resume_error = 1;
-          return;
-        }
+      struct record_message_args args;
+
+      args.regcache = get_current_regcache ();
+      args.signal = signal;
+      record_message (&args);
       record_beneath_to_resume (record_beneath_to_resume_ops, ptid, 1,
                                 signal);
     }
@@ -1038,14 +1033,6 @@ record_wait (struct target_ops *ops,
 
   if (!RECORD_IS_REPLAY && ops != &record_core_ops)
     {
-      if (record_resume_error)
-	{
-	  /* If record_resume get error, return directly.  */
-	  status->kind = TARGET_WAITKIND_STOPPED;
-	  status->value.sig = TARGET_SIGNAL_ABRT;
-	  return inferior_ptid;
-	}
-
       if (record_resume_step)
 	{
 	  /* This is a single step.  */
@@ -1091,7 +1078,11 @@ record_wait (struct target_ops *ops,
 			 Therefore we will not return to gdb.
 		         Record the insn and resume.  */
 		      if (!do_record_message (regcache, TARGET_SIGNAL_0))
-			break;
+  			{
+                           status->kind = TARGET_WAITKIND_STOPPED;
+                           status->value.sig = TARGET_SIGNAL_0;
+                           break;
+  			}
 
 		      record_beneath_to_resume (record_beneath_to_resume_ops,
 						ptid, 1,

Re: [RFA] let record_resume fail immediately on error

[Michael Snyder]

Hui Zhu wrote:
> Hi Michael,
> 
> Now we have a lot of patch in running.
> Could you please help me review this patch first?

OK, could you help me understand it?

You have an error-handler routine called do_record_message,
which calls record_message by way of catch_errors.  You call
do_record_message from two places, record_wait and record_resume.

But now you want to bypass the error-handler routine when
you call record_message from record_resume, but keep it in
place for record_wait.  Now you have two different ways of
calling record_message.  Seems inconsistant.  No explanation,
no comments explaining the difference.



> 2009-11-03  Hui Zhu  <teawater@gmail.com>
> 
> 	* record.c (record_resume_error): Deleted.
> 	(record_resume): Call record_message.
> 	(record_wait): Deleted record_resume_error.
> 	Set status when do_record_message need stop the inferior.
> 
> ---
>  record.c |   29 ++++++++++-------------------
>  1 file changed, 10 insertions(+), 19 deletions(-)
> 
> --- a/record.c
> +++ b/record.c
> @@ -954,7 +954,6 @@ record_close (int quitting)
>  }
> 
>  static int record_resume_step = 0;
> -static int record_resume_error;
> 
>  /* "to_resume" target method.  Resume the process record target.  */
> 
> @@ -966,15 +965,11 @@ record_resume (struct target_ops *ops, p
> 
>    if (!RECORD_IS_REPLAY)
>      {
> -      if (do_record_message (get_current_regcache (), signal))
> -        {
> -          record_resume_error = 0;
> -        }
> -      else
> -        {
> -          record_resume_error = 1;
> -          return;
> -        }
> +      struct record_message_args args;
> +
> +      args.regcache = get_current_regcache ();
> +      args.signal = signal;
> +      record_message (&args);
>        record_beneath_to_resume (record_beneath_to_resume_ops, ptid, 1,
>                                  signal);
>      }
> @@ -1038,14 +1033,6 @@ record_wait (struct target_ops *ops,
> 
>    if (!RECORD_IS_REPLAY && ops != &record_core_ops)
>      {
> -      if (record_resume_error)
> -	{
> -	  /* If record_resume get error, return directly.  */
> -	  status->kind = TARGET_WAITKIND_STOPPED;
> -	  status->value.sig = TARGET_SIGNAL_ABRT;
> -	  return inferior_ptid;
> -	}
> -
>        if (record_resume_step)
>  	{
>  	  /* This is a single step.  */
> @@ -1091,7 +1078,11 @@ record_wait (struct target_ops *ops,
>  			 Therefore we will not return to gdb.
>  		         Record the insn and resume.  */
>  		      if (!do_record_message (regcache, TARGET_SIGNAL_0))
> -			break;
> +  			{
> +                           status->kind = TARGET_WAITKIND_STOPPED;
> +                           status->value.sig = TARGET_SIGNAL_0;
> +                           break;
> +  			}
> 
>  		      record_beneath_to_resume (record_beneath_to_resume_ops,
>  						ptid, 1,

Re: [RFA] let record_resume fail immediately on error

[Hui Zhu]

>> Now we have a lot of patch in running.
>> Could you please help me review this patch first?
>
> OK, could you help me understand it?
>
> You have an error-handler routine called do_record_message,
> which calls record_message by way of catch_errors.  You call
> do_record_message from two places, record_wait and record_resume.
>
> But now you want to bypass the error-handler routine when
> you call record_message from record_resume, but keep it in
> place for record_wait.  Now you have two different ways of
> calling record_message.  Seems inconsistant.  No explanation,
> no comments explaining the difference.
>

OK.  I will try to talk clear about it.

I make do_record_message because in before, call a error in
record_resume and record_wait will make inferior cannot keep running.

Now, I found that call error in record_resume will not affect
anything.  So I change do_record_message to record_message in
record_resume.

But in record_wait, call error still make inferior cannot keep
running, so I keep do_record_message.

Thanks,
Hui


>
>
>> 2009-11-03  Hui Zhu  <teawater@gmail.com>
>>
>>        * record.c (record_resume_error): Deleted.
>>        (record_resume): Call record_message.
>>        (record_wait): Deleted record_resume_error.
>>        Set status when do_record_message need stop the inferior.
>>
>> ---
>>  record.c |   29 ++++++++++-------------------
>>  1 file changed, 10 insertions(+), 19 deletions(-)
>>
>> --- a/record.c
>> +++ b/record.c
>> @@ -954,7 +954,6 @@ record_close (int quitting)
>>  }
>>
>>  static int record_resume_step = 0;
>> -static int record_resume_error;
>>
>>  /* "to_resume" target method.  Resume the process record target.  */
>>
>> @@ -966,15 +965,11 @@ record_resume (struct target_ops *ops, p
>>
>>   if (!RECORD_IS_REPLAY)
>>     {
>> -      if (do_record_message (get_current_regcache (), signal))
>> -        {
>> -          record_resume_error = 0;
>> -        }
>> -      else
>> -        {
>> -          record_resume_error = 1;
>> -          return;
>> -        }
>> +      struct record_message_args args;
>> +
>> +      args.regcache = get_current_regcache ();
>> +      args.signal = signal;
>> +      record_message (&args);
>>       record_beneath_to_resume (record_beneath_to_resume_ops, ptid, 1,
>>                                 signal);
>>     }
>> @@ -1038,14 +1033,6 @@ record_wait (struct target_ops *ops,
>>
>>   if (!RECORD_IS_REPLAY && ops != &record_core_ops)
>>     {
>> -      if (record_resume_error)
>> -       {
>> -         /* If record_resume get error, return directly.  */
>> -         status->kind = TARGET_WAITKIND_STOPPED;
>> -         status->value.sig = TARGET_SIGNAL_ABRT;
>> -         return inferior_ptid;
>> -       }
>> -
>>       if (record_resume_step)
>>        {
>>          /* This is a single step.  */
>> @@ -1091,7 +1078,11 @@ record_wait (struct target_ops *ops,
>>                         Therefore we will not return to gdb.
>>                         Record the insn and resume.  */
>>                      if (!do_record_message (regcache, TARGET_SIGNAL_0))
>> -                       break;
>> +                       {
>> +                           status->kind = TARGET_WAITKIND_STOPPED;
>> +                           status->value.sig = TARGET_SIGNAL_0;
>> +                           break;
>> +                       }
>>
>>                      record_beneath_to_resume
>> (record_beneath_to_resume_ops,
>>                                                ptid, 1,
>
>

Re: [RFA] let record_resume fail immediately on error

[Hui Zhu]

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

Hi Michael and Tom,

I make a new prec-fix-error-handler.txt according to the Tom's idea in
http://sourceware.org/ml/gdb-patches/2009-11/msg00152.html

It add a new argument "catch" to do_record_message.  If catch is true,
it will call "record_message" with catch errors.  If not, it will call
"record_message" directly.

In "record_resume", it call do_record_message with "catch = 0".
In "record_wait", it call do_record_message with "catch = 1".

Please help me review it.

Thanks,
Hui

2009-11-10  Hui Zhu  <teawater@gmail.com>

	* record.c (do_record_message): Add new argument "catch".
	(record_resume_error): Deleted.
	(record_resume): Call do_record_message with catch is 1.
	(record_wait): Deleted record_resume_error.
	Set status when do_record_message need stop the inferior.

---
 record.c |   35 +++++++++++++----------------------
 1 file changed, 13 insertions(+), 22 deletions(-)

--- a/record.c
+++ b/record.c
@@ -647,13 +647,17 @@ record_message (void *args)

 static int
 do_record_message (struct regcache *regcache,
-		   enum target_signal signal)
+		   enum target_signal signal, int catch)
 {
   struct record_message_args args;

   args.regcache = regcache;
   args.signal = signal;
-  return catch_errors (record_message, &args, NULL, RETURN_MASK_ALL);
+
+  if (catch)
+    return catch_errors (record_message, &args, NULL, RETURN_MASK_ALL);
+
+  return record_message (&args);
 }

 /* Set to 1 if record_store_registers and record_xfer_partial
@@ -954,7 +958,6 @@ record_close (int quitting)
 }

 static int record_resume_step = 0;
-static int record_resume_error;

 /* "to_resume" target method.  Resume the process record target.  */

@@ -966,15 +969,7 @@ record_resume (struct target_ops *ops, p

   if (!RECORD_IS_REPLAY)
     {
-      if (do_record_message (get_current_regcache (), signal))
-        {
-          record_resume_error = 0;
-        }
-      else
-        {
-          record_resume_error = 1;
-          return;
-        }
+      do_record_message (get_current_regcache (), signal, 0);
       record_beneath_to_resume (record_beneath_to_resume_ops, ptid, 1,
                                 signal);
     }
@@ -1038,14 +1033,6 @@ record_wait (struct target_ops *ops,

   if (!RECORD_IS_REPLAY && ops != &record_core_ops)
     {
-      if (record_resume_error)
-	{
-	  /* If record_resume get error, return directly.  */
-	  status->kind = TARGET_WAITKIND_STOPPED;
-	  status->value.sig = TARGET_SIGNAL_ABRT;
-	  return inferior_ptid;
-	}
-
       if (record_resume_step)
 	{
 	  /* This is a single step.  */
@@ -1090,8 +1077,12 @@ record_wait (struct target_ops *ops,
 		         stepping, therefore gdb will not stop.
 			 Therefore we will not return to gdb.
 		         Record the insn and resume.  */
-		      if (!do_record_message (regcache, TARGET_SIGNAL_0))
-			break;
+		      if (!do_record_message (regcache, TARGET_SIGNAL_0, 1))
+  			{
+                           status->kind = TARGET_WAITKIND_STOPPED;
+                           status->value.sig = TARGET_SIGNAL_0;
+                           break;
+  			}

 		      record_beneath_to_resume (record_beneath_to_resume_ops,
 						ptid, 1,


>
> OK.  I will try to talk clear about it.
>
> I make do_record_message because in before, call a error in
> record_resume and record_wait will make inferior cannot keep running.
>
> Now, I found that call error in record_resume will not affect
> anything.  So I change do_record_message to record_message in
> record_resume.
>
> But in record_wait, call error still make inferior cannot keep
> running, so I keep do_record_message.
>
> Thanks,
> Hui
>
>
> >
> >
> >> 2009-11-03  Hui Zhu  <teawater@gmail.com>
> >>
> >>        * record.c (record_resume_error): Deleted.
> >>        (record_resume): Call record_message.
> >>        (record_wait): Deleted record_resume_error.
> >>        Set status when do_record_message need stop the inferior.
> >>
> >> ---
> >>  record.c |   29 ++++++++++-------------------
> >>  1 file changed, 10 insertions(+), 19 deletions(-)
> >>
> >> --- a/record.c
> >> +++ b/record.c
> >> @@ -954,7 +954,6 @@ record_close (int quitting)
> >>  }
> >>
> >>  static int record_resume_step = 0;
> >> -static int record_resume_error;
> >>
> >>  /* "to_resume" target method.  Resume the process record target.  */
> >>
> >> @@ -966,15 +965,11 @@ record_resume (struct target_ops *ops, p
> >>
> >>   if (!RECORD_IS_REPLAY)
> >>     {
> >> -      if (do_record_message (get_current_regcache (), signal))
> >> -        {
> >> -          record_resume_error = 0;
> >> -        }
> >> -      else
> >> -        {
> >> -          record_resume_error = 1;
> >> -          return;
> >> -        }
> >> +      struct record_message_args args;
> >> +
> >> +      args.regcache = get_current_regcache ();
> >> +      args.signal = signal;
> >> +      record_message (&args);
> >>       record_beneath_to_resume (record_beneath_to_resume_ops, ptid, 1,
> >>                                 signal);
> >>     }
> >> @@ -1038,14 +1033,6 @@ record_wait (struct target_ops *ops,
> >>
> >>   if (!RECORD_IS_REPLAY && ops != &record_core_ops)
> >>     {
> >> -      if (record_resume_error)
> >> -       {
> >> -         /* If record_resume get error, return directly.  */
> >> -         status->kind = TARGET_WAITKIND_STOPPED;
> >> -         status->value.sig = TARGET_SIGNAL_ABRT;
> >> -         return inferior_ptid;
> >> -       }
> >> -
> >>       if (record_resume_step)
> >>        {
> >>          /* This is a single step.  */
> >> @@ -1091,7 +1078,11 @@ record_wait (struct target_ops *ops,
> >>                         Therefore we will not return to gdb.
> >>                         Record the insn and resume.  */
> >>                      if (!do_record_message (regcache, TARGET_SIGNAL_0))
> >> -                       break;
> >> +                       {
> >> +                           status->kind = TARGET_WAITKIND_STOPPED;
> >> +                           status->value.sig = TARGET_SIGNAL_0;
> >> +                           break;
> >> +                       }
> >>
> >>                      record_beneath_to_resume
> >> (record_beneath_to_resume_ops,
> >>                                                ptid, 1,
> >
> >

[-- Attachment #2: prec-fix-error-handler.txt --]
[-- Type: text/plain, Size: 2377 bytes --]

---
 record.c |   35 +++++++++++++----------------------
 1 file changed, 13 insertions(+), 22 deletions(-)

--- a/record.c
+++ b/record.c
@@ -647,13 +647,17 @@ record_message (void *args)
 
 static int
 do_record_message (struct regcache *regcache,
-		   enum target_signal signal)
+		   enum target_signal signal, int catch)
 {
   struct record_message_args args;
 
   args.regcache = regcache;
   args.signal = signal;
-  return catch_errors (record_message, &args, NULL, RETURN_MASK_ALL);
+
+  if (catch)
+    return catch_errors (record_message, &args, NULL, RETURN_MASK_ALL);
+
+  return record_message (&args);
 }
 
 /* Set to 1 if record_store_registers and record_xfer_partial
@@ -954,7 +958,6 @@ record_close (int quitting)
 }
 
 static int record_resume_step = 0;
-static int record_resume_error;
 
 /* "to_resume" target method.  Resume the process record target.  */
 
@@ -966,15 +969,7 @@ record_resume (struct target_ops *ops, p
 
   if (!RECORD_IS_REPLAY)
     {
-      if (do_record_message (get_current_regcache (), signal))
-        {
-          record_resume_error = 0;
-        }
-      else
-        {
-          record_resume_error = 1;
-          return;
-        }
+      do_record_message (get_current_regcache (), signal, 0);
       record_beneath_to_resume (record_beneath_to_resume_ops, ptid, 1,
                                 signal);
     }
@@ -1038,14 +1033,6 @@ record_wait (struct target_ops *ops,
 
   if (!RECORD_IS_REPLAY && ops != &record_core_ops)
     {
-      if (record_resume_error)
-	{
-	  /* If record_resume get error, return directly.  */
-	  status->kind = TARGET_WAITKIND_STOPPED;
-	  status->value.sig = TARGET_SIGNAL_ABRT;
-	  return inferior_ptid;
-	}
-
       if (record_resume_step)
 	{
 	  /* This is a single step.  */
@@ -1090,8 +1077,12 @@ record_wait (struct target_ops *ops,
 		         stepping, therefore gdb will not stop.
 			 Therefore we will not return to gdb.
 		         Record the insn and resume.  */
-		      if (!do_record_message (regcache, TARGET_SIGNAL_0))
-			break;
+		      if (!do_record_message (regcache, TARGET_SIGNAL_0, 1))
+  			{
+                           status->kind = TARGET_WAITKIND_STOPPED;
+                           status->value.sig = TARGET_SIGNAL_0;
+                           break;
+  			}
 
 		      record_beneath_to_resume (record_beneath_to_resume_ops,
 						ptid, 1,

Re: [RFA] let record_resume fail immediately on error

[Tom Tromey]

>>>>> ">" == Hui Zhu <teawater@gmail.com> writes:

>> It add a new argument "catch" to do_record_message.  If catch is true,
>> it will call "record_message" with catch errors.  If not, it will call
>> "record_message" directly.

I don't like this much either.  It is trivial for callers that want to
catch errors to catch them.  They can use TRY_CATCH or catch_errors(...,
record_message).

Callers that don't want to catch errors should just call
do_record_message directly.  This is both for type safety and also
because such calls are plain old C, and should therefore look like it.

Tom

Re: [RFA] let record_resume fail immediately on error

[Hui Zhu]

The callers are very lazy, they like call a function directly.  :)

Thanks,
Hui

On Wed, Nov 11, 2009 at 06:04, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> ">" == Hui Zhu <teawater@gmail.com> writes:
>
>>> It add a new argument "catch" to do_record_message.  If catch is true,
>>> it will call "record_message" with catch errors.  If not, it will call
>>> "record_message" directly.
>
> I don't like this much either.  It is trivial for callers that want to
> catch errors to catch them.  They can use TRY_CATCH or catch_errors(...,
> record_message).
>
> Callers that don't want to catch errors should just call
> do_record_message directly.  This is both for type safety and also
> because such calls are plain old C, and should therefore look like it.
>
> Tom
>

Re: [RFA] let record_resume fail immediately on error

[Hui Zhu]

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

Hi guys,

I update this patch follow cvs-head and do a small change about the testsuite.
Please help me review it.

Thanks,
Hui

2009-11-24  Hui Zhu  <teawater@gmail.com>

	* record.c (do_record_message): Add new argument "catch".
	(record_resume_error): Deleted.
	(record_resume): Call do_record_message with catch is 1.
	(record_wait): Deleted record_resume_error.
	Set status when do_record_message need stop the inferior.

2009-11-24  Hui Zhu  <teawater@gmail.com>

	* gdb.reverse/sigall-reverse.exp: Adjust.

---
 record.c                                 |   35 +++++++++++--------------------
 testsuite/gdb.reverse/sigall-reverse.exp |    2 -
 2 files changed, 14 insertions(+), 23 deletions(-)

--- a/record.c
+++ b/record.c
@@ -650,13 +650,17 @@ record_message (void *args)

 static int
 do_record_message (struct regcache *regcache,
-		   enum target_signal signal)
+		   enum target_signal signal, int catch)
 {
   struct record_message_args args;

   args.regcache = regcache;
   args.signal = signal;
-  return catch_errors (record_message, &args, NULL, RETURN_MASK_ALL);
+
+  if (catch)
+    return catch_errors (record_message, &args, NULL, RETURN_MASK_ALL);
+
+  return record_message (&args);
 }

 /* Set to 1 if record_store_registers and record_xfer_partial
@@ -983,7 +987,6 @@ record_close (int quitting)
 }

 static int record_resume_step = 0;
-static int record_resume_error;

 /* "to_resume" target method.  Resume the process record target.  */

@@ -995,15 +998,7 @@ record_resume (struct target_ops *ops, p

   if (!RECORD_IS_REPLAY)
     {
-      if (do_record_message (get_current_regcache (), signal))
-        {
-          record_resume_error = 0;
-        }
-      else
-        {
-          record_resume_error = 1;
-          return;
-        }
+      do_record_message (get_current_regcache (), signal, 0);
       record_beneath_to_resume (record_beneath_to_resume_ops, ptid, 1,
                                 signal);
     }
@@ -1067,14 +1062,6 @@ record_wait (struct target_ops *ops,

   if (!RECORD_IS_REPLAY && ops != &record_core_ops)
     {
-      if (record_resume_error)
-	{
-	  /* If record_resume get error, return directly.  */
-	  status->kind = TARGET_WAITKIND_STOPPED;
-	  status->value.sig = TARGET_SIGNAL_ABRT;
-	  return inferior_ptid;
-	}
-
       if (record_resume_step)
 	{
 	  /* This is a single step.  */
@@ -1130,8 +1117,12 @@ record_wait (struct target_ops *ops,
 		    {
 		      /* This must be a single-step trap.  Record the
 		         insn and issue another step.  */
-		      if (!do_record_message (regcache, TARGET_SIGNAL_0))
-			break;
+		      if (!do_record_message (regcache, TARGET_SIGNAL_0, 1))
+  			{
+                           status->kind = TARGET_WAITKIND_STOPPED;
+                           status->value.sig = TARGET_SIGNAL_0;
+                           break;
+  			}

 		      record_beneath_to_resume (record_beneath_to_resume_ops,
 						ptid, 1,
--- a/testsuite/gdb.reverse/sigall-reverse.exp
+++ b/testsuite/gdb.reverse/sigall-reverse.exp
@@ -262,7 +262,7 @@ gdb_test "continue" \
     "get signal TERM"
 gdb_test "continue" "Breakpoint.*handle_TERM.*" "send signal TERM"

-gdb_test "continue" "Program received .*" "continue to sigall exit" \
+gdb_test "continue" "\[process \[0-9\]+ .*" "continue to sigall exit" \
     "The next instruction is syscall exit_group.* program...y. or n. " \
     "yes"

[-- Attachment #2: prec-fix-error-handler.txt --]
[-- Type: text/plain, Size: 2891 bytes --]

---
 record.c                                 |   35 +++++++++++--------------------
 testsuite/gdb.reverse/sigall-reverse.exp |    2 -
 2 files changed, 14 insertions(+), 23 deletions(-)

--- a/record.c
+++ b/record.c
@@ -650,13 +650,17 @@ record_message (void *args)
 
 static int
 do_record_message (struct regcache *regcache,
-		   enum target_signal signal)
+		   enum target_signal signal, int catch)
 {
   struct record_message_args args;
 
   args.regcache = regcache;
   args.signal = signal;
-  return catch_errors (record_message, &args, NULL, RETURN_MASK_ALL);
+
+  if (catch)
+    return catch_errors (record_message, &args, NULL, RETURN_MASK_ALL);
+
+  return record_message (&args);
 }
 
 /* Set to 1 if record_store_registers and record_xfer_partial
@@ -983,7 +987,6 @@ record_close (int quitting)
 }
 
 static int record_resume_step = 0;
-static int record_resume_error;
 
 /* "to_resume" target method.  Resume the process record target.  */
 
@@ -995,15 +998,7 @@ record_resume (struct target_ops *ops, p
 
   if (!RECORD_IS_REPLAY)
     {
-      if (do_record_message (get_current_regcache (), signal))
-        {
-          record_resume_error = 0;
-        }
-      else
-        {
-          record_resume_error = 1;
-          return;
-        }
+      do_record_message (get_current_regcache (), signal, 0);
       record_beneath_to_resume (record_beneath_to_resume_ops, ptid, 1,
                                 signal);
     }
@@ -1067,14 +1062,6 @@ record_wait (struct target_ops *ops,
 
   if (!RECORD_IS_REPLAY && ops != &record_core_ops)
     {
-      if (record_resume_error)
-	{
-	  /* If record_resume get error, return directly.  */
-	  status->kind = TARGET_WAITKIND_STOPPED;
-	  status->value.sig = TARGET_SIGNAL_ABRT;
-	  return inferior_ptid;
-	}
-
       if (record_resume_step)
 	{
 	  /* This is a single step.  */
@@ -1130,8 +1117,12 @@ record_wait (struct target_ops *ops,
 		    {
 		      /* This must be a single-step trap.  Record the
 		         insn and issue another step.  */
-		      if (!do_record_message (regcache, TARGET_SIGNAL_0))
-			break;
+		      if (!do_record_message (regcache, TARGET_SIGNAL_0, 1))
+  			{
+                           status->kind = TARGET_WAITKIND_STOPPED;
+                           status->value.sig = TARGET_SIGNAL_0;
+                           break;
+  			}
 
 		      record_beneath_to_resume (record_beneath_to_resume_ops,
 						ptid, 1,
--- a/testsuite/gdb.reverse/sigall-reverse.exp
+++ b/testsuite/gdb.reverse/sigall-reverse.exp
@@ -262,7 +262,7 @@ gdb_test "continue" \
     "get signal TERM"
 gdb_test "continue" "Breakpoint.*handle_TERM.*" "send signal TERM"
 
-gdb_test "continue" "Program received .*" "continue to sigall exit" \
+gdb_test "continue" "\[process \[0-9\]+ .*" "continue to sigall exit" \
     "The next instruction is syscall exit_group.* program...y. or n. " \
     "yes"
 

Re: [RFA] let record_resume fail immediately on error

[Tom Tromey]

>>>>> ">" == Hui Zhu <teawater@gmail.com> writes:

>>  static int
>>  do_record_message (struct regcache *regcache,
>> -		   enum target_signal signal)
>> +		   enum target_signal signal, int catch)

This is still a very weird way to write what should be an ordinary
function call.

Tom

Re: [RFA] let record_resume fail immediately on error

[Hui Zhu]

Why "do_record_message" cannot be a caller?
It be a caller in a long time.

I call "record_message".  You don't like it.
I put it to "do_record_message".  You don't like it too.

I don't like use TRY_CATCH or catch_errors directly.

Thanks,
Hui



On Wed, Nov 25, 2009 at 01:13, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> ">" == Hui Zhu <teawater@gmail.com> writes:
>
>>>  static int
>>>  do_record_message (struct regcache *regcache,
>>> -               enum target_signal signal)
>>> +               enum target_signal signal, int catch)
>
> This is still a very weird way to write what should be an ordinary
> function call.
>
> Tom
>

Re: [RFA] let record_resume fail immediately on error

[Joel Brobecker]

> Why "do_record_message" cannot be a caller?
> It be a caller in a long time.
> I call "record_message".  You don't like it.
> I put it to "do_record_message".  You don't like it too.

It did not seem to me that the feedback was on the function name,
but rather on the interface of the function.

> I don't like use TRY_CATCH or catch_errors directly.

I confess that I don't like catch_errors, because of the need to
artificially create a container type that contains all the function
parameters. But TRY_CATCH, on the other hand, solves that problem.
What is it that you don't like about TRY_CATCH.  As far as I can tell,
the syntax is very close to C++, no?

In any case, if you really want to have either possibilities
(record_message with or without exception protection), how about
two functions, named: record_message and safe_record_message.
The latter is just a TRY_CATCH wrapper around the former.
Perhaps that could be an acceptable compromise...

-- 
Joel

Re: [RFA] let record_resume fail immediately on error

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

>> I don't like use TRY_CATCH or catch_errors directly.

Joel> I confess that I don't like catch_errors, because of the need to
Joel> artificially create a container type that contains all the function
Joel> parameters.

In this code, we already have the container type and untyped trampoline
function.

The problem I have with it is that there are direct calls to the untyped
trampoline function.

I think the general approach for using catch_errors in gdb ought to be:

* Have a properly-typed function that does all the work.
* If you need catch_errors, introduce a new type to hold the actual
  arguments, and write an untyped trampoline function.  Then, *only*
  call this trampoline function via catch_errors.
* ... However, prefer TRY_CATCH in most cases, because it does not
  require a new type and is generally safer (though not completely
  safe).

And FWIW, I think this rule is generally followed in practice.

My reason for the above is that it is generally best to write a
type-safe program and let the compiler diagnose errors.  We can't do
this for catch_errors, due to limitations in C, but we can at least
limit the damage.


Also, this patch introduces an argument indicating whether or not to
catch.  This is bad, because it is confusing, but also particularly bad
in this case because the actual argument is always a constant.

Tom

Re: [RFA] let record_resume fail immediately on error

[Hui Zhu]

Hi guys,

What about following change:
Change record_message to record_message (struct regcache *regcache,
enum target_signal signal)

static int
record_message_wrapper_safe(struct regcache *regcache,
		   enum target_signal signal)
{
  struct record_message_args args;

  args.regcache = regcache;
  args.signal = signal;
  return catch_errors (record_message_wrapper, &args, NULL, RETURN_MASK_ALL);
}

static int
record_message_wrapper (void *args)
{
  return record_message (args->regcache, args->signal);
}

record_resume will call record_message.
record_wait will call record_message_wrapper_safe

Do you think it's OK?

Thanks,
Hui


On Thu, Nov 26, 2009 at 01:58, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:
>
>>> I don't like use TRY_CATCH or catch_errors directly.
>
> Joel> I confess that I don't like catch_errors, because of the need to
> Joel> artificially create a container type that contains all the function
> Joel> parameters.
>
> In this code, we already have the container type and untyped trampoline
> function.
>
> The problem I have with it is that there are direct calls to the untyped
> trampoline function.
>
> I think the general approach for using catch_errors in gdb ought to be:
>
> * Have a properly-typed function that does all the work.
> * If you need catch_errors, introduce a new type to hold the actual
>  arguments, and write an untyped trampoline function.  Then, *only*
>  call this trampoline function via catch_errors.
> * ... However, prefer TRY_CATCH in most cases, because it does not
>  require a new type and is generally safer (though not completely
>  safe).
>
> And FWIW, I think this rule is generally followed in practice.
>
> My reason for the above is that it is generally best to write a
> type-safe program and let the compiler diagnose errors.  We can't do
> this for catch_errors, due to limitations in C, but we can at least
> limit the damage.
>
>
> Also, this patch introduces an argument indicating whether or not to
> catch.  This is bad, because it is confusing, but also particularly bad
> in this case because the actual argument is always a constant.
>
> Tom
>

Re: [RFA] let record_resume fail immediately on error

[Hui Zhu]

Hey guys,

I know I must be so anxious with this patch.  Sorry for it.

I have another patch follow this patch that is reviewing too.  Update
patch to fit with code always a hard thing for me.
If you can give me some comment about this idea, it will really help me a lot.
Thanks a lot.

Hui

On Thu, Nov 26, 2009 at 14:39, Hui Zhu <teawater@gmail.com> wrote:
> Hi guys,
>
> What about following change:
> Change record_message to record_message (struct regcache *regcache,
> enum target_signal signal)
>
> static int
> record_message_wrapper_safe(struct regcache *regcache,
>                   enum target_signal signal)
> {
>  struct record_message_args args;
>
>  args.regcache = regcache;
>  args.signal = signal;
>  return catch_errors (record_message_wrapper, &args, NULL, RETURN_MASK_ALL);
> }
>
> static int
> record_message_wrapper (void *args)
> {
>  return record_message (args->regcache, args->signal);
> }
>
> record_resume will call record_message.
> record_wait will call record_message_wrapper_safe
>
> Do you think it's OK?
>
> Thanks,
> Hui
>
>
> On Thu, Nov 26, 2009 at 01:58, Tom Tromey <tromey@redhat.com> wrote:
>>>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:
>>
>>>> I don't like use TRY_CATCH or catch_errors directly.
>>
>> Joel> I confess that I don't like catch_errors, because of the need to
>> Joel> artificially create a container type that contains all the function
>> Joel> parameters.
>>
>> In this code, we already have the container type and untyped trampoline
>> function.
>>
>> The problem I have with it is that there are direct calls to the untyped
>> trampoline function.
>>
>> I think the general approach for using catch_errors in gdb ought to be:
>>
>> * Have a properly-typed function that does all the work.
>> * If you need catch_errors, introduce a new type to hold the actual
>>  arguments, and write an untyped trampoline function.  Then, *only*
>>  call this trampoline function via catch_errors.
>> * ... However, prefer TRY_CATCH in most cases, because it does not
>>  require a new type and is generally safer (though not completely
>>  safe).
>>
>> And FWIW, I think this rule is generally followed in practice.
>>
>> My reason for the above is that it is generally best to write a
>> type-safe program and let the compiler diagnose errors.  We can't do
>> this for catch_errors, due to limitations in C, but we can at least
>> limit the damage.
>>
>>
>> Also, this patch introduces an argument indicating whether or not to
>> catch.  This is bad, because it is confusing, but also particularly bad
>> in this case because the actual argument is always a constant.
>>
>> Tom
>>
>

Re: [RFA] let record_resume fail immediately on error

[Tom Tromey]

>>>>> ">" == Hui Zhu <teawater@gmail.com> writes:

>> What about following change:
>> Change record_message to record_message (struct regcache *regcache,
>> enum target_signal signal)
[...]

>> Do you think it's OK?

I think that addresses my complaints.  Thanks.

Tom

Re: [RFA] let record_resume fail immediately on error

[Hui Zhu]

Thanks Tom,

I make a new patch according it.  Please help me review it.

Thanks,
Hui

2009-12-02  Hui Zhu  <teawater@gmail.com>

	* record.c (record_message): Change argument.
	(record_message_wrapper): New function.
	(do_record_message): Change it name to
	"record_message_wrapper_safe".
	Let it call "record_message_wrapper".
	(record_resume_error): Deleted.
	(record_resume): Call "record_message".
	(record_wait): Deleted record_resume_error.
	Call "record_message_wrapper_safe".
	Set status when do_record_message need stop the inferior.
2009-12-02  Hui Zhu  <teawater@gmail.com>

	* gdb.reverse/sigall-reverse.exp: Adjust.

---
 record.c                                 |   70 ++++++++++++++-----------------
 testsuite/gdb.reverse/sigall-reverse.exp |    2
 2 files changed, 34 insertions(+), 38 deletions(-)

--- a/record.c
+++ b/record.c
@@ -572,17 +572,11 @@ record_arch_list_cleanups (void *ignore)
    record the running message of inferior and set them to
    record_arch_list, and add it to record_list.  */

-struct record_message_args {
-  struct regcache *regcache;
-  enum target_signal signal;
-};
-
 static int
-record_message (void *args)
+record_message (struct regcache *regcache, enum target_signal signal)
 {
   int ret;
-  struct record_message_args *myargs = args;
-  struct gdbarch *gdbarch = get_regcache_arch (myargs->regcache);
+  struct gdbarch *gdbarch = get_regcache_arch (regcache);
   struct cleanup *old_cleanups = make_cleanup (record_arch_list_cleanups, 0);

   record_arch_list_head = NULL;
@@ -616,18 +610,18 @@ record_message (void *args)
   if (record_list != &record_first)    /* FIXME better way to check */
     {
       gdb_assert (record_list->type == record_end);
-      record_list->u.end.sigval = myargs->signal;
+      record_list->u.end.sigval = signal;
     }

-  if (myargs->signal == TARGET_SIGNAL_0
+  if (signal == TARGET_SIGNAL_0
       || !gdbarch_process_record_signal_p (gdbarch))
     ret = gdbarch_process_record (gdbarch,
-				  myargs->regcache,
-				  regcache_read_pc (myargs->regcache));
+				  regcache,
+				  regcache_read_pc (regcache));
   else
     ret = gdbarch_process_record_signal (gdbarch,
-					 myargs->regcache,
-					 myargs->signal);
+					 regcache,
+					 signal);

   if (ret > 0)
     error (_("Process record: inferior program stopped."));
@@ -648,15 +642,29 @@ record_message (void *args)
   return 1;
 }

+struct record_message_args {
+  struct regcache *regcache;
+  enum target_signal signal;
+};
+
 static int
-do_record_message (struct regcache *regcache,
-		   enum target_signal signal)
+record_message_wrapper (void *args)
+{
+  struct record_message_args *record_args = args;
+
+  return record_message (record_args->regcache, record_args->signal);
+}
+
+static int
+record_message_wrapper_safe (struct regcache *regcache,
+                             enum target_signal signal)
 {
   struct record_message_args args;

   args.regcache = regcache;
   args.signal = signal;
-  return catch_errors (record_message, &args, NULL, RETURN_MASK_ALL);
+
+  return catch_errors (record_message_wrapper, &args, NULL, RETURN_MASK_ALL);
 }

 /* Set to 1 if record_store_registers and record_xfer_partial
@@ -983,7 +991,6 @@ record_close (int quitting)
 }

 static int record_resume_step = 0;
-static int record_resume_error;

 /* "to_resume" target method.  Resume the process record target.  */

@@ -995,15 +1002,7 @@ record_resume (struct target_ops *ops, p

   if (!RECORD_IS_REPLAY)
     {
-      if (do_record_message (get_current_regcache (), signal))
-        {
-          record_resume_error = 0;
-        }
-      else
-        {
-          record_resume_error = 1;
-          return;
-        }
+      record_message (get_current_regcache (), signal);
       record_beneath_to_resume (record_beneath_to_resume_ops, ptid, 1,
                                 signal);
     }
@@ -1067,14 +1066,6 @@ record_wait (struct target_ops *ops,

   if (!RECORD_IS_REPLAY && ops != &record_core_ops)
     {
-      if (record_resume_error)
-	{
-	  /* If record_resume get error, return directly.  */
-	  status->kind = TARGET_WAITKIND_STOPPED;
-	  status->value.sig = TARGET_SIGNAL_ABRT;
-	  return inferior_ptid;
-	}
-
       if (record_resume_step)
 	{
 	  /* This is a single step.  */
@@ -1130,8 +1121,13 @@ record_wait (struct target_ops *ops,
 		    {
 		      /* This must be a single-step trap.  Record the
 		         insn and issue another step.  */
-		      if (!do_record_message (regcache, TARGET_SIGNAL_0))
-			break;
+		      if (!record_message_wrapper_safe (regcache,
+                                                        TARGET_SIGNAL_0))
+  			{
+                           status->kind = TARGET_WAITKIND_STOPPED;
+                           status->value.sig = TARGET_SIGNAL_0;
+                           break;
+  			}

 		      record_beneath_to_resume (record_beneath_to_resume_ops,
 						ptid, 1,
--- a/testsuite/gdb.reverse/sigall-reverse.exp
+++ b/testsuite/gdb.reverse/sigall-reverse.exp
@@ -262,7 +262,7 @@ gdb_test "continue" \
     "get signal TERM"
 gdb_test "continue" "Breakpoint.*handle_TERM.*" "send signal TERM"

-gdb_test "continue" "Program received .*" "continue to sigall exit" \
+gdb_test "continue" "\[process \[0-9\]+ .*" "continue to sigall exit" \
     "The next instruction is syscall exit_group.* program...y. or n. " \
     "yes"

Re: [RFA] let record_resume fail immediately on error

[Hui Zhu]

Ping.

Thanks,
Hui

On Wed, Dec 2, 2009 at 11:22, Hui Zhu <teawater@gmail.com> wrote:
> Thanks Tom,
>
> I make a new patch according it.  Please help me review it.
>
> Thanks,
> Hui
>
> 2009-12-02  Hui Zhu  <teawater@gmail.com>
>
>        * record.c (record_message): Change argument.
>        (record_message_wrapper): New function.
>        (do_record_message): Change it name to
>        "record_message_wrapper_safe".
>        Let it call "record_message_wrapper".
>        (record_resume_error): Deleted.
>        (record_resume): Call "record_message".
>        (record_wait): Deleted record_resume_error.
>        Call "record_message_wrapper_safe".
>        Set status when do_record_message need stop the inferior.
> 2009-12-02  Hui Zhu  <teawater@gmail.com>
>
>        * gdb.reverse/sigall-reverse.exp: Adjust.
>
> ---
>  record.c                                 |   70 ++++++++++++++-----------------
>  testsuite/gdb.reverse/sigall-reverse.exp |    2
>  2 files changed, 34 insertions(+), 38 deletions(-)
>
> --- a/record.c
> +++ b/record.c
> @@ -572,17 +572,11 @@ record_arch_list_cleanups (void *ignore)
>    record the running message of inferior and set them to
>    record_arch_list, and add it to record_list.  */
>
> -struct record_message_args {
> -  struct regcache *regcache;
> -  enum target_signal signal;
> -};
> -
>  static int
> -record_message (void *args)
> +record_message (struct regcache *regcache, enum target_signal signal)
>  {
>   int ret;
> -  struct record_message_args *myargs = args;
> -  struct gdbarch *gdbarch = get_regcache_arch (myargs->regcache);
> +  struct gdbarch *gdbarch = get_regcache_arch (regcache);
>   struct cleanup *old_cleanups = make_cleanup (record_arch_list_cleanups, 0);
>
>   record_arch_list_head = NULL;
> @@ -616,18 +610,18 @@ record_message (void *args)
>   if (record_list != &record_first)    /* FIXME better way to check */
>     {
>       gdb_assert (record_list->type == record_end);
> -      record_list->u.end.sigval = myargs->signal;
> +      record_list->u.end.sigval = signal;
>     }
>
> -  if (myargs->signal == TARGET_SIGNAL_0
> +  if (signal == TARGET_SIGNAL_0
>       || !gdbarch_process_record_signal_p (gdbarch))
>     ret = gdbarch_process_record (gdbarch,
> -                                 myargs->regcache,
> -                                 regcache_read_pc (myargs->regcache));
> +                                 regcache,
> +                                 regcache_read_pc (regcache));
>   else
>     ret = gdbarch_process_record_signal (gdbarch,
> -                                        myargs->regcache,
> -                                        myargs->signal);
> +                                        regcache,
> +                                        signal);
>
>   if (ret > 0)
>     error (_("Process record: inferior program stopped."));
> @@ -648,15 +642,29 @@ record_message (void *args)
>   return 1;
>  }
>
> +struct record_message_args {
> +  struct regcache *regcache;
> +  enum target_signal signal;
> +};
> +
>  static int
> -do_record_message (struct regcache *regcache,
> -                  enum target_signal signal)
> +record_message_wrapper (void *args)
> +{
> +  struct record_message_args *record_args = args;
> +
> +  return record_message (record_args->regcache, record_args->signal);
> +}
> +
> +static int
> +record_message_wrapper_safe (struct regcache *regcache,
> +                             enum target_signal signal)
>  {
>   struct record_message_args args;
>
>   args.regcache = regcache;
>   args.signal = signal;
> -  return catch_errors (record_message, &args, NULL, RETURN_MASK_ALL);
> +
> +  return catch_errors (record_message_wrapper, &args, NULL, RETURN_MASK_ALL);
>  }
>
>  /* Set to 1 if record_store_registers and record_xfer_partial
> @@ -983,7 +991,6 @@ record_close (int quitting)
>  }
>
>  static int record_resume_step = 0;
> -static int record_resume_error;
>
>  /* "to_resume" target method.  Resume the process record target.  */
>
> @@ -995,15 +1002,7 @@ record_resume (struct target_ops *ops, p
>
>   if (!RECORD_IS_REPLAY)
>     {
> -      if (do_record_message (get_current_regcache (), signal))
> -        {
> -          record_resume_error = 0;
> -        }
> -      else
> -        {
> -          record_resume_error = 1;
> -          return;
> -        }
> +      record_message (get_current_regcache (), signal);
>       record_beneath_to_resume (record_beneath_to_resume_ops, ptid, 1,
>                                 signal);
>     }
> @@ -1067,14 +1066,6 @@ record_wait (struct target_ops *ops,
>
>   if (!RECORD_IS_REPLAY && ops != &record_core_ops)
>     {
> -      if (record_resume_error)
> -       {
> -         /* If record_resume get error, return directly.  */
> -         status->kind = TARGET_WAITKIND_STOPPED;
> -         status->value.sig = TARGET_SIGNAL_ABRT;
> -         return inferior_ptid;
> -       }
> -
>       if (record_resume_step)
>        {
>          /* This is a single step.  */
> @@ -1130,8 +1121,13 @@ record_wait (struct target_ops *ops,
>                    {
>                      /* This must be a single-step trap.  Record the
>                         insn and issue another step.  */
> -                     if (!do_record_message (regcache, TARGET_SIGNAL_0))
> -                       break;
> +                     if (!record_message_wrapper_safe (regcache,
> +                                                        TARGET_SIGNAL_0))
> +                       {
> +                           status->kind = TARGET_WAITKIND_STOPPED;
> +                           status->value.sig = TARGET_SIGNAL_0;
> +                           break;
> +                       }
>
>                      record_beneath_to_resume (record_beneath_to_resume_ops,
>                                                ptid, 1,
> --- a/testsuite/gdb.reverse/sigall-reverse.exp
> +++ b/testsuite/gdb.reverse/sigall-reverse.exp
> @@ -262,7 +262,7 @@ gdb_test "continue" \
>     "get signal TERM"
>  gdb_test "continue" "Breakpoint.*handle_TERM.*" "send signal TERM"
>
> -gdb_test "continue" "Program received .*" "continue to sigall exit" \
> +gdb_test "continue" "\[process \[0-9\]+ .*" "continue to sigall exit" \
>     "The next instruction is syscall exit_group.* program...y. or n. " \
>     "yes"
>

Re: [RFA] let record_resume fail immediately on error

[Hui Zhu]

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

Ping.

Thanks,
Hui

On Wed, Dec 2, 2009 at 11:22, Hui Zhu <teawater@gmail.com> wrote:
> Thanks Tom,
>
> I make a new patch according it.  Please help me review it.
>
> Thanks,
> Hui
>
> 2009-12-02  Hui Zhu  <teawater@gmail.com>
>
>        * record.c (record_message): Change argument.
>        (record_message_wrapper): New function.
>        (do_record_message): Change it name to
>        "record_message_wrapper_safe".
>        Let it call "record_message_wrapper".
>        (record_resume_error): Deleted.
>        (record_resume): Call "record_message".
>        (record_wait): Deleted record_resume_error.
>        Call "record_message_wrapper_safe".
>        Set status when do_record_message need stop the inferior.
> 2009-12-02  Hui Zhu  <teawater@gmail.com>
>
>        * gdb.reverse/sigall-reverse.exp: Adjust.
>
> ---
>  record.c                                 |   70 ++++++++++++++-----------------
>  testsuite/gdb.reverse/sigall-reverse.exp |    2
>  2 files changed, 34 insertions(+), 38 deletions(-)
>
> --- a/record.c
> +++ b/record.c
> @@ -572,17 +572,11 @@ record_arch_list_cleanups (void *ignore)
>    record the running message of inferior and set them to
>    record_arch_list, and add it to record_list.  */
>
> -struct record_message_args {
> -  struct regcache *regcache;
> -  enum target_signal signal;
> -};
> -
>  static int
> -record_message (void *args)
> +record_message (struct regcache *regcache, enum target_signal signal)
>  {
>   int ret;
> -  struct record_message_args *myargs = args;
> -  struct gdbarch *gdbarch = get_regcache_arch (myargs->regcache);
> +  struct gdbarch *gdbarch = get_regcache_arch (regcache);
>   struct cleanup *old_cleanups = make_cleanup (record_arch_list_cleanups, 0);
>
>   record_arch_list_head = NULL;
> @@ -616,18 +610,18 @@ record_message (void *args)
>   if (record_list != &record_first)    /* FIXME better way to check */
>     {
>       gdb_assert (record_list->type == record_end);
> -      record_list->u.end.sigval = myargs->signal;
> +      record_list->u.end.sigval = signal;
>     }
>
> -  if (myargs->signal == TARGET_SIGNAL_0
> +  if (signal == TARGET_SIGNAL_0
>       || !gdbarch_process_record_signal_p (gdbarch))
>     ret = gdbarch_process_record (gdbarch,
> -                                 myargs->regcache,
> -                                 regcache_read_pc (myargs->regcache));
> +                                 regcache,
> +                                 regcache_read_pc (regcache));
>   else
>     ret = gdbarch_process_record_signal (gdbarch,
> -                                        myargs->regcache,
> -                                        myargs->signal);
> +                                        regcache,
> +                                        signal);
>
>   if (ret > 0)
>     error (_("Process record: inferior program stopped."));
> @@ -648,15 +642,29 @@ record_message (void *args)
>   return 1;
>  }
>
> +struct record_message_args {
> +  struct regcache *regcache;
> +  enum target_signal signal;
> +};
> +
>  static int
> -do_record_message (struct regcache *regcache,
> -                  enum target_signal signal)
> +record_message_wrapper (void *args)
> +{
> +  struct record_message_args *record_args = args;
> +
> +  return record_message (record_args->regcache, record_args->signal);
> +}
> +
> +static int
> +record_message_wrapper_safe (struct regcache *regcache,
> +                             enum target_signal signal)
>  {
>   struct record_message_args args;
>
>   args.regcache = regcache;
>   args.signal = signal;
> -  return catch_errors (record_message, &args, NULL, RETURN_MASK_ALL);
> +
> +  return catch_errors (record_message_wrapper, &args, NULL, RETURN_MASK_ALL);
>  }
>
>  /* Set to 1 if record_store_registers and record_xfer_partial
> @@ -983,7 +991,6 @@ record_close (int quitting)
>  }
>
>  static int record_resume_step = 0;
> -static int record_resume_error;
>
>  /* "to_resume" target method.  Resume the process record target.  */
>
> @@ -995,15 +1002,7 @@ record_resume (struct target_ops *ops, p
>
>   if (!RECORD_IS_REPLAY)
>     {
> -      if (do_record_message (get_current_regcache (), signal))
> -        {
> -          record_resume_error = 0;
> -        }
> -      else
> -        {
> -          record_resume_error = 1;
> -          return;
> -        }
> +      record_message (get_current_regcache (), signal);
>       record_beneath_to_resume (record_beneath_to_resume_ops, ptid, 1,
>                                 signal);
>     }
> @@ -1067,14 +1066,6 @@ record_wait (struct target_ops *ops,
>
>   if (!RECORD_IS_REPLAY && ops != &record_core_ops)
>     {
> -      if (record_resume_error)
> -       {
> -         /* If record_resume get error, return directly.  */
> -         status->kind = TARGET_WAITKIND_STOPPED;
> -         status->value.sig = TARGET_SIGNAL_ABRT;
> -         return inferior_ptid;
> -       }
> -
>       if (record_resume_step)
>        {
>          /* This is a single step.  */
> @@ -1130,8 +1121,13 @@ record_wait (struct target_ops *ops,
>                    {
>                      /* This must be a single-step trap.  Record the
>                         insn and issue another step.  */
> -                     if (!do_record_message (regcache, TARGET_SIGNAL_0))
> -                       break;
> +                     if (!record_message_wrapper_safe (regcache,
> +                                                        TARGET_SIGNAL_0))
> +                       {
> +                           status->kind = TARGET_WAITKIND_STOPPED;
> +                           status->value.sig = TARGET_SIGNAL_0;
> +                           break;
> +                       }
>
>                      record_beneath_to_resume (record_beneath_to_resume_ops,
>                                                ptid, 1,
> --- a/testsuite/gdb.reverse/sigall-reverse.exp
> +++ b/testsuite/gdb.reverse/sigall-reverse.exp
> @@ -262,7 +262,7 @@ gdb_test "continue" \
>     "get signal TERM"
>  gdb_test "continue" "Breakpoint.*handle_TERM.*" "send signal TERM"
>
> -gdb_test "continue" "Program received .*" "continue to sigall exit" \
> +gdb_test "continue" "\[process \[0-9\]+ .*" "continue to sigall exit" \
>     "The next instruction is syscall exit_group.* program...y. or n. " \
>     "yes"
>

[-- Attachment #2: prec-fix-error-handler.txt --]
[-- Type: text/plain, Size: 4755 bytes --]

---
 record.c                                 |   70 ++++++++++++++-----------------
 testsuite/gdb.reverse/sigall-reverse.exp |    2 
 2 files changed, 34 insertions(+), 38 deletions(-)

--- a/record.c
+++ b/record.c
@@ -572,17 +572,11 @@ record_arch_list_cleanups (void *ignore)
    record the running message of inferior and set them to
    record_arch_list, and add it to record_list.  */
 
-struct record_message_args {
-  struct regcache *regcache;
-  enum target_signal signal;
-};
-
 static int
-record_message (void *args)
+record_message (struct regcache *regcache, enum target_signal signal)
 {
   int ret;
-  struct record_message_args *myargs = args;
-  struct gdbarch *gdbarch = get_regcache_arch (myargs->regcache);
+  struct gdbarch *gdbarch = get_regcache_arch (regcache);
   struct cleanup *old_cleanups = make_cleanup (record_arch_list_cleanups, 0);
 
   record_arch_list_head = NULL;
@@ -616,18 +610,18 @@ record_message (void *args)
   if (record_list != &record_first)    /* FIXME better way to check */
     {
       gdb_assert (record_list->type == record_end);
-      record_list->u.end.sigval = myargs->signal;
+      record_list->u.end.sigval = signal;
     }
 
-  if (myargs->signal == TARGET_SIGNAL_0
+  if (signal == TARGET_SIGNAL_0
       || !gdbarch_process_record_signal_p (gdbarch))
     ret = gdbarch_process_record (gdbarch,
-				  myargs->regcache,
-				  regcache_read_pc (myargs->regcache));
+				  regcache,
+				  regcache_read_pc (regcache));
   else
     ret = gdbarch_process_record_signal (gdbarch,
-					 myargs->regcache,
-					 myargs->signal);
+					 regcache,
+					 signal);
 
   if (ret > 0)
     error (_("Process record: inferior program stopped."));
@@ -648,15 +642,29 @@ record_message (void *args)
   return 1;
 }
 
+struct record_message_args {
+  struct regcache *regcache;
+  enum target_signal signal;
+};
+
 static int
-do_record_message (struct regcache *regcache,
-		   enum target_signal signal)
+record_message_wrapper (void *args)
+{
+  struct record_message_args *record_args = args;
+
+  return record_message (record_args->regcache, record_args->signal);
+}
+
+static int
+record_message_wrapper_safe (struct regcache *regcache,
+                             enum target_signal signal)
 {
   struct record_message_args args;
 
   args.regcache = regcache;
   args.signal = signal;
-  return catch_errors (record_message, &args, NULL, RETURN_MASK_ALL);
+
+  return catch_errors (record_message_wrapper, &args, NULL, RETURN_MASK_ALL);
 }
 
 /* Set to 1 if record_store_registers and record_xfer_partial
@@ -983,7 +991,6 @@ record_close (int quitting)
 }
 
 static int record_resume_step = 0;
-static int record_resume_error;
 
 /* "to_resume" target method.  Resume the process record target.  */
 
@@ -995,15 +1002,7 @@ record_resume (struct target_ops *ops, p
 
   if (!RECORD_IS_REPLAY)
     {
-      if (do_record_message (get_current_regcache (), signal))
-        {
-          record_resume_error = 0;
-        }
-      else
-        {
-          record_resume_error = 1;
-          return;
-        }
+      record_message (get_current_regcache (), signal);
       record_beneath_to_resume (record_beneath_to_resume_ops, ptid, 1,
                                 signal);
     }
@@ -1067,14 +1066,6 @@ record_wait (struct target_ops *ops,
 
   if (!RECORD_IS_REPLAY && ops != &record_core_ops)
     {
-      if (record_resume_error)
-	{
-	  /* If record_resume get error, return directly.  */
-	  status->kind = TARGET_WAITKIND_STOPPED;
-	  status->value.sig = TARGET_SIGNAL_ABRT;
-	  return inferior_ptid;
-	}
-
       if (record_resume_step)
 	{
 	  /* This is a single step.  */
@@ -1130,8 +1121,13 @@ record_wait (struct target_ops *ops,
 		    {
 		      /* This must be a single-step trap.  Record the
 		         insn and issue another step.  */
-		      if (!do_record_message (regcache, TARGET_SIGNAL_0))
-			break;
+		      if (!record_message_wrapper_safe (regcache,
+                                                        TARGET_SIGNAL_0))
+  			{
+                           status->kind = TARGET_WAITKIND_STOPPED;
+                           status->value.sig = TARGET_SIGNAL_0;
+                           break;
+  			}
 
 		      record_beneath_to_resume (record_beneath_to_resume_ops,
 						ptid, 1,
--- a/testsuite/gdb.reverse/sigall-reverse.exp
+++ b/testsuite/gdb.reverse/sigall-reverse.exp
@@ -262,7 +262,7 @@ gdb_test "continue" \
     "get signal TERM"
 gdb_test "continue" "Breakpoint.*handle_TERM.*" "send signal TERM"
 
-gdb_test "continue" "Program received .*" "continue to sigall exit" \
+gdb_test "continue" "\[process \[0-9\]+ .*" "continue to sigall exit" \
     "The next instruction is syscall exit_group.* program...y. or n. " \
     "yes"
 

Re: [RFA] let record_resume fail immediately on error

[Tom Tromey]

>>>>> ">" == Hui Zhu <teawater@gmail.com> writes:

>> 2009-12-02  Hui Zhu  <teawater@gmail.com>
>> * record.c (record_message): Change argument.
>> (record_message_wrapper): New function.
>> (do_record_message): Change it name to
>> "record_message_wrapper_safe".
>> Let it call "record_message_wrapper".
>> (record_resume_error): Deleted.
>> (record_resume): Call "record_message".
>> (record_wait): Deleted record_resume_error.
>> Call "record_message_wrapper_safe".
>> Set status when do_record_message need stop the inferior.

This is ok, thanks.

>> +gdb_test "continue" "\[process \[0-9\]+ .*" "continue to sigall exit" \

Should this be "signal" and not "sigall"?

Tom

Re: [RFA] let record_resume fail immediately on error

[Hui Zhu]

Thanks a lot.

Fixed and checked in.

Hui

On Tue, Dec 22, 2009 at 04:44, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> ">" == Hui Zhu <teawater@gmail.com> writes:
>
>>> 2009-12-02  Hui Zhu  <teawater@gmail.com>
>>> * record.c (record_message): Change argument.
>>> (record_message_wrapper): New function.
>>> (do_record_message): Change it name to
>>> "record_message_wrapper_safe".
>>> Let it call "record_message_wrapper".
>>> (record_resume_error): Deleted.
>>> (record_resume): Call "record_message".
>>> (record_wait): Deleted record_resume_error.
>>> Call "record_message_wrapper_safe".
>>> Set status when do_record_message need stop the inferior.
>
> This is ok, thanks.
>
>>> +gdb_test "continue" "\[process \[0-9\]+ .*" "continue to sigall exit" \
>
> Should this be "signal" and not "sigall"?
>
> Tom
>