Re: [RFA] let record_resume fail immediately on error

[Hui Zhu <teawater at gmail dot com>]

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: prec-fix-error-handler.txt
Description: Text document