Re: [RFA/RFC] Prec multi-thread support [2/4] arch support

Hui Zhu
2009-11-26T07:20:29Z

On Thu, Nov 26, 2009 at 04:05, Michael Snyder <msnyder@vmware.com> wrote:
> Hui Zhu wrote:
>>
>> 1. In record_linux_system_call gdb_sys_clone, I set record_step to 0.
>> Then the record target will let inferior continue until to a
>> breakpoint.  Most of time, record just let inferior step.  But
>> sys_clone is very special.  It cannot work OK with
>> PTRACE_SINGLESTEP.(It make me hang this patch a log of months.)
>>
>> 2. Add new argument "addr" that point to the next code that need to be
>> analyzed to linux syscall record function "record_linux_system_call".
>
> I'm OK with #2.
> I need to spend some time looking at #1.

To reproduce this issue:
cat 2.c
#include <stdio.h>
#include <pthread.h>
#include <assert.h>

void td1(void * i)
{
while (1)
{
printf ("1\n");
sleep (1);
}

return;
}

void td2(void * i)
{
while (1)
{
printf ("2\n");
sleep (1);
}

return;
}

int
main(int argc,char *argv[],char *envp[])
{
pthread_t t1,t2;

pthread_create(&t1, NULL, (void*)td1, NULL);
pthread_create(&t2, NULL, (void*)td2, NULL);

while (1)
{
printf ("0\n");
sleep (1);
}

return (0);
}

teawater@pek-hzhu:~/gdb$ gcc -lpthread -g 2.c
teawater@pek-hzhu:~/gdb$ gdb ./a.out
GNU gdb (GDB) 7.0.50.20091126-cvs
Copyright (C) 2009 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law. Type "show copying"
and "show warranty" for details.
This GDB was configured as "i686-pc-linux-gnu".
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>...
Reading symbols from /home/teawater/gdb/a.out...done.
(gdb) start
Temporary breakpoint 1 at 0x8048495: file 2.c, line 32.
Starting program: /home/teawater/gdb/a.out
[Thread debugging using libthread_db enabled]

Temporary breakpoint 1, main (argc=<value optimized out>, argv=<value
optimized out>, envp=<value optimized out>)
at 2.c:32
32 pthread_create(&t1, NULL, (void*)td1, NULL);
(gdb) b clone
Breakpoint 2 at 0xb7f2ae00
(gdb) c
Continuing.

Breakpoint 2, 0xb7f2ae00 in clone () from /lib/tls/i686/cmov/libc.so.6
(gdb) disas
Dump of assembler code for function clone:
=> 0xb7f2ae00 <+0>: mov $0xffffffea,%eax
0xb7f2ae05 <+5>: mov 0x4(%esp),%ecx
0xb7f2ae09 <+9>: jecxz 0xb7f2ae78 <clone+120>
0xb7f2ae0b <+11>: mov 0x8(%esp),%ecx
0xb7f2ae0f <+15>: jecxz 0xb7f2ae78 <clone+120>
0xb7f2ae11 <+17>: and $0xfffffff0,%ecx
0xb7f2ae14 <+20>: sub $0x1c,%ecx
0xb7f2ae17 <+23>: mov 0x10(%esp),%eax
0xb7f2ae1b <+27>: mov %eax,0xc(%ecx)
0xb7f2ae1e <+30>: mov 0x4(%esp),%eax
0xb7f2ae22 <+34>: mov %eax,0x8(%ecx)
0xb7f2ae25 <+37>: movl $0x0,0x4(%ecx)
0xb7f2ae2c <+44>: push %ebx
0xb7f2ae2d <+45>: push %esi
0xb7f2ae2e <+46>: push %edi
0xb7f2ae2f <+47>: mov 0x24(%esp),%esi
0xb7f2ae33 <+51>: mov 0x20(%esp),%edx
0xb7f2ae37 <+55>: mov 0x18(%esp),%ebx
0xb7f2ae3b <+59>: mov 0x28(%esp),%edi
0xb7f2ae3f <+63>: mov $0x78,%eax
0xb7f2ae44 <+68>: mov %ebx,(%ecx)
0xb7f2ae46 <+70>: int $0x80
0xb7f2ae48 <+72>: pop %edi
0xb7f2ae49 <+73>: pop %esi
0xb7f2ae4a <+74>: pop %ebx
0xb7f2ae4b <+75>: test %eax,%eax
0xb7f2ae4d <+77>: jl 0xb7f2ae78 <clone+120>
0xb7f2ae4f <+79>: je 0xb7f2ae52 <clone+82>
---Type <return> to continue, or q <return> to quit---q
Quit
(gdb) b *0xb7f2ae46
Breakpoint 3 at 0xb7f2ae46
(gdb) c
Continuing.

Breakpoint 3, 0xb7f2ae46 in clone () from /lib/tls/i686/cmov/libc.so.6
(gdb) dis
(gdb) si

Program terminated with signal SIGTRAP, Trace/breakpoint trap.
The program no longer exists.

> But there's another change in here that you didn't mention --
> you added "if (record_debug) around some warnings.
>
> Would  you take that change out, please, and submit it separately?
> It isn't really a part of adding multi-thread support, since this
> warning shows up frequently in single-thread prec debugging.
>

Without this change, when debug the multi-thread inferior, gdb will
output a lot of warning like a big rain. :( And it looks didn't
affect revere debug.
I will post a separate patch for it.

Thanks,
Hui

>
>>
>> 2009-11-25  Hui Zhu  <teawater@gmail.com>
>>
>>        * amd64-linux-tdep.c (amd64_linux_syscall_record): Add new
>>        argument "addr".
>>        * i386-linux-tdep.c (i386_linux_intx80_sysenter_record): Ditto.
>>        * i386-tdep.c (i386_record_lea_modrm): Add "if (record_debug)"
>>        to segment register warning.
>>        (i386_process_record): Ditto.  Add new argument "addr" when
>>        call i386_intx80_record, i386_sysenter_record and
>>        i386_syscall_record.
>>        * i386-tdep.h (gdbarch_tdep): Add new argument "addr" when
>>        call i386_intx80_record, i386_sysenter_record and
>>        i386_syscall_record.
>>        * linux-record.c (inferior.h): New include.
>>        (record_linux_system_call): Add new argument "addr".
>>        Update code for gdb_sys_clone.
>>        * linux-record.h (record_linux_system_call): Add new argument
>>        "addr".
>>
>> ---
>>  amd64-linux-tdep.c |    4 ++--
>>  i386-linux-tdep.c  |    4 ++--
>>  i386-tdep.c        |   52
>> +++++++++++++++++++++++++++++-----------------------
>>  i386-tdep.h        |    6 +++---
>>  linux-record.c     |   13 +++++++++----
>>  linux-record.h     |    2 +-
>>  6 files changed, 46 insertions(+), 35 deletions(-)
>>
>> --- a/amd64-linux-tdep.c
>> +++ b/amd64-linux-tdep.c
>> @@ -1155,7 +1155,7 @@ static struct linux_record_tdep amd64_li
>>  #define RECORD_ARCH_GET_GS     0x1004
>>
>>  static int
>> -amd64_linux_syscall_record (struct regcache *regcache)
>> +amd64_linux_syscall_record (struct regcache *regcache, CORE_ADDR addr)
>>  {
>>   int ret;
>>   ULONGEST syscall_native;
>> @@ -1205,7 +1205,7 @@ amd64_linux_syscall_record (struct regca
>>     }
>>   else
>>     {
>> -      ret = record_linux_system_call (syscall_gdb, regcache,
>> +      ret = record_linux_system_call (syscall_gdb, addr, regcache,
>>                                       &amd64_linux_record_tdep);
>>       if (ret)
>>         return ret;
>> --- a/i386-linux-tdep.c
>> +++ b/i386-linux-tdep.c
>> @@ -411,7 +411,7 @@ i386_canonicalize_syscall (int syscall)
>>  static struct linux_record_tdep i386_linux_record_tdep;
>>
>>  static int
>> -i386_linux_intx80_sysenter_record (struct regcache *regcache)
>> +i386_linux_intx80_sysenter_record (struct regcache *regcache, CORE_ADDR
>> addr)
>>  {
>>   int ret;
>>   LONGEST syscall_native;
>> @@ -437,7 +437,7 @@ i386_linux_intx80_sysenter_record (struc
>>      return 0;
>>    }
>>
>> -  ret = record_linux_system_call (syscall_gdb, regcache,
>> +  ret = record_linux_system_call (syscall_gdb, addr, regcache,
>>                                  &i386_linux_record_tdep);
>>   if (ret)
>>     return ret;
>> --- a/i386-tdep.c
>> +++ b/i386-tdep.c
>> @@ -3160,10 +3160,11 @@ i386_record_lea_modrm (struct i386_recor
>>
>>   if (irp->override >= 0)
>>     {
>> -      warning (_("Process record ignores the memory change "
>> -                 "of instruction at address %s because it "
>> -                 "can't get the value of the segment register."),
>> -               paddress (gdbarch, irp->orig_addr));
>> +      if (record_debug)
>> +        warning (_("Process record ignores the memory change "
>> +                   "of instruction at address %s because it "
>> +                   "can't get the value of the segment register."),
>> +                 paddress (gdbarch, irp->orig_addr));
>>       return 0;
>>     }
>>
>> @@ -4042,11 +4043,12 @@ reswitch:
>>     case 0xa3:
>>       if (ir.override >= 0)
>>         {
>> -         warning (_("Process record ignores the memory change "
>> -                     "of instruction at address %s because "
>> -                     "it can't get the value of the segment "
>> -                     "register."),
>> -                   paddress (gdbarch, ir.orig_addr));
>> +          if (record_debug)
>> +           warning (_("Process record ignores the memory change "
>> +                       "of instruction at address %s because "
>> +                       "it can't get the value of the segment "
>> +                       "register."),
>> +                     paddress (gdbarch, ir.orig_addr));
>>        }
>>       else
>>        {
>> @@ -4467,11 +4469,12 @@ reswitch:
>>           if (ir.aflag && (es != ds))
>>             {
>>               /* addr += ((uint32_t) read_register (I386_ES_REGNUM)) << 4;
>> */
>> -              warning (_("Process record ignores the memory "
>> -                         "change of instruction at address %s "
>> -                         "because it can't get the value of the "
>> -                         "ES segment register."),
>> -                       paddress (gdbarch, ir.orig_addr));
>> +              if (record_debug)
>> +                warning (_("Process record ignores the memory "
>> +                           "change of instruction at address %s "
>> +                           "because it can't get the value of the "
>> +                           "ES segment register."),
>> +                         paddress (gdbarch, ir.orig_addr));
>>             }
>>           else
>>             {
>> @@ -4872,7 +4875,7 @@ reswitch:
>>            ir.addr -= 2;
>>            goto no_support;
>>          }
>> -       ret = gdbarch_tdep (gdbarch)->i386_intx80_record (ir.regcache);
>> +       ret = gdbarch_tdep (gdbarch)->i386_intx80_record (ir.regcache,
>> ir.addr);
>>        if (ret)
>>          return ret;
>>       }
>> @@ -4974,7 +4977,8 @@ reswitch:
>>            ir.addr -= 2;
>>            goto no_support;
>>          }
>> -       ret = gdbarch_tdep (gdbarch)->i386_sysenter_record (ir.regcache);
>> +       ret = gdbarch_tdep (gdbarch)->i386_sysenter_record (ir.regcache,
>> +                                                            ir.addr);
>>        if (ret)
>>          return ret;
>>       }
>> @@ -4999,7 +5003,8 @@ reswitch:
>>            ir.addr -= 2;
>>            goto no_support;
>>          }
>> -       ret = gdbarch_tdep (gdbarch)->i386_syscall_record (ir.regcache);
>> +       ret = gdbarch_tdep (gdbarch)->i386_syscall_record (ir.regcache,
>> +                                                           ir.addr);
>>        if (ret)
>>          return ret;
>>       }
>> @@ -5135,12 +5140,13 @@ reswitch:
>>              /* sidt */
>>              if (ir.override >= 0)
>>                {
>> -                 warning (_("Process record ignores the memory "
>> -                             "change of instruction at "
>> -                             "address %s because it can't get "
>> -                             "the value of the segment "
>> -                             "register."),
>> -                           paddress (gdbarch, ir.orig_addr));
>> +                  if (record_debug)
>> +                   warning (_("Process record ignores the memory "
>> +                               "change of instruction at "
>> +                               "address %s because it can't get "
>> +                               "the value of the segment "
>> +                               "register."),
>> +                             paddress (gdbarch, ir.orig_addr));
>>                }
>>              else
>>                {
>> --- a/i386-tdep.h
>> +++ b/i386-tdep.h
>> @@ -115,11 +115,11 @@ struct gdbarch_tdep
>>      in GDB is not same as I386 instructions.  */
>>   const int *record_regmap;
>>   /* Parse intx80 args.  */
>> -  int (*i386_intx80_record) (struct regcache *regcache);
>> +  int (*i386_intx80_record) (struct regcache *regcache, CORE_ADDR addr);
>>   /* Parse sysenter args.  */
>> -  int (*i386_sysenter_record) (struct regcache *regcache);
>> +  int (*i386_sysenter_record) (struct regcache *regcache, CORE_ADDR
>> addr);
>>   /* Parse syscall args.  */
>> -  int (*i386_syscall_record) (struct regcache *regcache);
>> +  int (*i386_syscall_record) (struct regcache *regcache, CORE_ADDR addr);
>>  };
>>
>>  /* Floating-point registers.  */
>> --- a/linux-record.c
>> +++ b/linux-record.c
>> @@ -21,6 +21,7 @@
>>  #include "target.h"
>>  #include "gdbtypes.h"
>>  #include "regcache.h"
>> +#include "inferior.h"
>>  #include "record.h"
>>  #include "linux-record.h"
>>
>> @@ -222,7 +223,7 @@ record_linux_msghdr (struct regcache *re
>>    Return -1 if something wrong.  */
>>
>>  int
>> -record_linux_system_call (enum gdb_syscall syscall,
>> +record_linux_system_call (enum gdb_syscall syscall, CORE_ADDR addr,
>>                          struct regcache *regcache,
>>                           struct linux_record_tdep *tdep)
>>  {
>> @@ -242,8 +243,9 @@ record_linux_system_call (enum gdb_sysca
>>         int q;
>>         target_terminal_ours ();
>>         q = yquery (_("The next instruction is syscall exit.  "
>> -                      "It will make the program exit.  "
>> -                      "Do you want to stop the program?"));
>> +                      "It will make the thread %s exit.  "
>> +                      "Do you want to stop the program?"),
>> +                    target_pid_to_str (inferior_ptid));
>>         target_terminal_inferior ();
>>         if (q)
>>           return 1;
>> @@ -1209,10 +1211,13 @@ record_linux_system_call (enum gdb_sysca
>>
>>     case gdb_sys_fsync:
>>     case gdb_sys_sigreturn:
>> -    case gdb_sys_clone:
>>     case gdb_sys_setdomainname:
>>       break;
>>
>> +    case gdb_sys_clone:
>> +      record_step = 0;
>> +      break;
>> +
>>     case gdb_sys_newuname:
>>       regcache_raw_read_unsigned (regcache, tdep->arg1, &tmpulongest);
>>       if (record_arch_list_add_mem ((CORE_ADDR) tmpulongest,
>> --- a/linux-record.h
>> +++ b/linux-record.h
>> @@ -534,7 +534,7 @@ enum gdb_syscall {
>>
>>  /* Record a linux syscall.  */
>>
>> -extern int record_linux_system_call (enum gdb_syscall num,
>> +extern int record_linux_system_call (enum gdb_syscall num, CORE_ADDR
>> addr,
>>                                     struct regcache *regcache,
>>                                     struct linux_record_tdep *tdep);
>>  #endif /* _LINUX_RECORD_H_ */
>
>