RE:Re: GDB 5.1/Core files and ptids (CONT)

RE:Re: GDB 5.1/Core files and ptids (CONT)

[Takis Psarogiannakopoulos]

On Fri, 18 Jan 2002, Kevin Buettner wrote:

>
> Yep, I agree.  As I pointed out to Takis in an earlier message, I think
> the right way to fix it is to modify both corelow.c on the GDB side
> and elf.c on the bfd side.
>
> My suggestion was that instead of naming sections .reg/PIDLWP where
> PIDLWP is a combined (numeric) pid and lwp identifier that these
> sections instead be named .reg/PID+LWP where PID is the pid and LWP is
> the lwp.
>
> When the LWP doesn't exist or is simply zero, we simply use .reg/PID
> as before.  (Or we could use .reg/PID+0.  It doesn't really matter
> so long as both sides are in agreement.)
>

Hi Kevin, 
Below is a core from omniORB. Take a look. My GDB 5.1 will say:

=========================================================================
(gdb) info threads
* 5 -> PID: 14546 DG/UX Kernel LWP: 4  0x08058b77 in fde_split
                   (linear=0x801de790, erratic=0x801de798) at frame.c:313
  4 -> PID: 14546 DG/UX Kernel LWP: 3  0x801ac4dc in __nsl_accept () from
                                                    /usr/dglib/libnsl.so.1
  3 -> PID: 14546 DG/UX Kernel LWP: 2  0x801d67eb in __dg_kfc () from
                                                 /usr/dglib/libthread.so.1
  2 -> PID: 14546 DG/UX Kernel LWP: 1  0x801d67eb in __dg_kfc () from
                                                 /usr/dglib/libthread.so.1
  1 -> PID: 14546 DG/UX Kernel LWP: 0  0x801d67eb in __dg_kfc () from
                                                 /usr/dglib/libthread.so.1
Current language:  auto; currently c


(gdb) info threads-status 
* 5 -> PID: 14546 DG/UX Kernel LWP: 4
                               eligible,dumping-core,caught-signal,detached
  4 -> PID: 14546 DG/UX Kernel LWP: 3 eligible
  3 -> PID: 14546 DG/UX Kernel LWP: 2  
                               frozen,cond-wait(0x80673c8)-mutex(0x80673a4)
  2 -> PID: 14546 DG/UX Kernel LWP: 1
                             frozen,cond-wait(0x8067334)-mutex(0x8067310)
  1 -> PID: 14546 DG/UX Kernel LWP: 0
                             frozen,cond-wait(0x80177184)-mutex(0x80177160)

(gdb) thread 1
[Switching to thread 1 (-> PID: 14546 DG/UX Kernel LWP: 0)]#0  0x801d67eb
                             in __dg_kfc () from /usr/dglib/libthread.so.1

(gdb) info lwp-status 
 DG/UX Kernel based LWP status space: 
   PID / LWP ID          :      14546 / 0 
   LWP Group ID          :      0
   Last on CPU ID        :      2
   LWP Kernel state      :
   LWP is waiting on the user-level condition variable at
              address 0x80177184, with associated mutex 0x80177160 
   LWP is marked as user frozen and cannot run at user level 
   Stack info            :      NO dedicated stack
   LWP sched is          :      LOCAL
   Sched policy          :      SCHED_OTHER
   Sched priority        :      2047
   Stop-on-store bmaps   : 
   Bit map num 0 is      :      0x0
   Bit map num 1 is      :      0x0
(gdb) quit

=====================================================================

As you can see if we put a conditional 

if ( (elf_tdata (abfd)->core_lwpid) == 0)

inside bfd/elf.c/elfcore_make_ptid_str(abfd) for DG/UX OS at least
we are loosing a thread. Below is what I worked out for GDB 5.1.
It will patch elf.c and corelow.c. according to your suggestions.
If this is acceptable someone should check it in at least for
the upcoming GDB 5.x.x.

=====================================================================

diff -rcN /target/GDB/gdb-5.1/bfd/elf.c gdb-5.1/bfd/elf.c
*** /target/GDB/gdb-5.1/bfd/elf.c       Sun Jan 20 16:54:02 2002
--- gdb-5.1/bfd/elf.c   Sat Jun 30 04:05:12 2001
***************
*** 5349,5365 ****
          + (elf_tdata (abfd)->core_pid));
  }

- static char *
- elfcore_make_ptid_str (abfd)
-      bfd *abfd;
- {
-    static char ptid_buf[40];
-
-    sprintf (ptid_buf, "%d+%ld", (elf_tdata (abfd)->core_pid),
-                       (elf_tdata (abfd)->core_lwpid) );
-    return ptid_buf;
- }
-
  /* If there isn't a section called NAME, make one, using
     data from SECT.  Note, this function will generate a
     reference to NAME, so you shouldn't deallocate or
--- 5349,5354 ----
***************
*** 5391,5398 ****
     actually creates up to two pseudosections:
     - For the single-threaded case, a section named NAME, unless
       such a section already exists.
!    - For the multi-threaded case, a section named "NAME/PID+LWPID",
where
!      PID is prepared by elfcore_make_ptid_str (abfd).
     Both pseudosections have identical contents. */
  boolean
  _bfd_elfcore_make_pseudosection (abfd, name, size, filepos)
--- 5380,5387 ----
     actually creates up to two pseudosections:
     - For the single-threaded case, a section named NAME, unless
       such a section already exists.
!    - For the multi-threaded case, a section named "NAME/PID", where
!      PID is elfcore_make_pid (abfd).
     Both pseudosections have identical contents. */
  boolean
  _bfd_elfcore_make_pseudosection (abfd, name, size, filepos)
***************
*** 5407,5413 ****

    /* Build the section name.  */

!   sprintf (buf, "%s/%s", name, elfcore_make_ptid_str (abfd));
    threaded_name = bfd_alloc (abfd, strlen (buf) + 1);
    if (threaded_name == NULL)
      return false;
--- 5396,5402 ----

    /* Build the section name.  */

!   sprintf (buf, "%s/%d", name, elfcore_make_pid (abfd));
    threaded_name = bfd_alloc (abfd, strlen (buf) + 1);
    if (threaded_name == NULL)
      return false;
***************
*** 5699,5705 ****

    /* Make a ".reg/999" section.  */

!   sprintf (buf, ".reg/%s", elfcore_make_ptid_str (abfd));
    name = bfd_alloc (abfd, strlen (buf) + 1);
    if (name == NULL)
      return false;
--- 5688,5694 ----

    /* Make a ".reg/999" section.  */

!   sprintf (buf, ".reg/%d", elfcore_make_pid (abfd));
    name = bfd_alloc (abfd, strlen (buf) + 1);
    if (name == NULL)
      return false;
***************
*** 5728,5734 ****

    /* Make a ".reg2/999" section */

!   sprintf (buf, ".reg2/%s", elfcore_make_ptid_str (abfd));
    name = bfd_alloc (abfd, strlen (buf) + 1);
    if (name == NULL)
      return false;
--- 5717,5723 ----

    /* Make a ".reg2/999" section */

!   sprintf (buf, ".reg2/%d", elfcore_make_pid (abfd));
    name = bfd_alloc (abfd, strlen (buf) + 1);
    if (name == NULL)
      return false;
diff -rcN /target/GDB/gdb-5.1/gdb/corelow.c gdb-5.1/gdb/corelow.c
*** /target/GDB/gdb-5.1/gdb/corelow.c   Sun Jan 20 17:00:59 2002
--- gdb-5.1/gdb/corelow.c       Tue May 15 00:03:36 2001
***************
*** 234,262 ****
  static void
  add_to_thread_list (bfd *abfd, asection *asect, PTR reg_sect_arg)
  {
!   int p_id, count;
!   long t_id;
    asection *reg_sect = (asection *) reg_sect_arg;

    if (strncmp (bfd_section_name (abfd, asect), ".reg/", 5) != 0)
      return;

!   count = sscanf(bfd_section_name (abfd, asect), ".reg/%d+%ld", &p_id,
&t_id);

! #if defined(DEBUG)
!   if ( count == 2 )
!     printf("Adding thread PID=%d+LWPID=%ld \n", p_id, t_id);
!   else if ( count == 1)
!     printf("Adding thread PID=%d+LWPID=? not found lwpid\n", p_id);
! #endif /* DEBUG */
!
!   add_thread ( ptid_build (p_id, t_id, 0) );

  /* Warning, Will Robinson, looking at BFD private data! */

    if (reg_sect != NULL
        && asect->filepos == reg_sect->filepos) /* Did we find .reg? */
!     inferior_ptid = ptid_build (p_id, t_id, 0);       /* Yes, make it
current */
  }

  /* This routine opens and sets up the core file bfd.  */
--- 234,254 ----
  static void
  add_to_thread_list (bfd *abfd, asection *asect, PTR reg_sect_arg)
  {
!   int thread_id;
    asection *reg_sect = (asection *) reg_sect_arg;

    if (strncmp (bfd_section_name (abfd, asect), ".reg/", 5) != 0)
      return;

!   thread_id = atoi (bfd_section_name (abfd, asect) + 5);

!   add_thread (pid_to_ptid (thread_id));

  /* Warning, Will Robinson, looking at BFD private data! */

    if (reg_sect != NULL
        && asect->filepos == reg_sect->filepos) /* Did we find .reg? */
!     inferior_ptid = pid_to_ptid (thread_id);  /* Yes, make it current */
  }

  /* This routine opens and sets up the core file bfd.  */
***************
*** 415,422 ****
    char *contents;

    if (PIDGET (inferior_ptid))
!       sprintf (section_name, "%s/%d+%ld", name,
!               PIDGET(inferior_ptid), TIDGET(inferior_ptid) );
    else
      strcpy (section_name, name);

--- 407,413 ----
    char *contents;

    if (PIDGET (inferior_ptid))
!     sprintf (section_name, "%s/%d", name, PIDGET (inferior_ptid));
    else
      strcpy (section_name, name);

========================================================================

Re: RE:Re: GDB 5.1/Core files and ptids (CONT)

[Kevin Buettner]

On Jan 20,  9:19am, Takis Psarogiannakopoulos wrote:

> [...] Below is what I worked out for GDB 5.1.
> It will patch elf.c and corelow.c. according to your suggestions.
> If this is acceptable someone should check it in at least for
> the upcoming GDB 5.x.x.

Takis,

I looked your patch over and it looks pretty good except that it's
reversed.  (When you did the diff, you did ``diff -c new old'' rather
than ``diff -c old new''.)  Could you please resend your patch in
the non-reversed form.  It'll make it easier to review.

Oh... also, could you include ChangeLog entries this time?

Thanks,

Kevin

P.S.  I'm not the maintainer of any of the files in question, so
someone else will need to approve Takis' changes.

RE:Re: GDB 5.1/Core files and ptids (CONT)

[Takis Psarogiannakopoulos]

Hi Kevin,

Unfortunately it seems that the change of mixed pids to ptids
has more problems that I thought in the start of this thread.
I am not sure after that change how any OS's that uses corelow.c
can debug a multi threaded core file!

Since GDB 5.2 or 5.1.1 or whatever is going to be out soon it is 
a good time to fix this one!

I remind that the current bfd routine is
======
static int
elfcore_make_pid (abfd)
      bfd *abfd;
{
   return ((elf_tdata (abfd)->core_lwpid << 16)
           + (elf_tdata (abfd)->core_pid));
}
=======

On Wed, 16 Jan 2002, Kevin Buettner wrote:

> Here are my suggestions:
> 
>  1) In bfd, the parts requiring elfcore_make_pid() are all contained in
>     elf.c.  I suggest that you rewrite elfcore_make_pid() to look 
>     something like this:
>  
>     static char *
>     elfcore_make_ptid_str (abfd)
>          bfd *abfd;
>     {
>       static char ptid_buf[40];
> 
>       if (elf_tdata (abfd)->core_lwpid == 0)
>         {
>           /* Non-threaded */
>           sprintf (ptid_buf, "%d", elf_tdata (abfd)->core_pid);
>         }
>        else
>         {
>           /* Threaded */   
>           sprintf (ptid_buf, "%d+%d", elf_tdata (abfd->core_pid),
>                                       elf_tdata (abfd->core_lwpid));
>         }
>       return ptid_buf;
>     }

Mmmm. This is not too good as it seems in first look. In lots of OS's (eg
DG/UX Unix) there is a LWP with id 0. In DG/UX moreover it may not even be
the main thread! 
Better change the above to ... < 0  or what ? probably even to 

static char *
elfcore_make_ptid_str (abfd)
     bfd *abfd;
{
   static char ptid_buf[40];
       
   sprintf (ptid_buf, "%d+%d", elf_tdata (abfd->core_pid),
                                       elf_tdata (abfd->core_lwpid));
   return ptid_buf;
}

Anyway this is debatable what should be. Binutil people should decide. 
Now in gdb/corelow.c (or in the core-dgux.c that is based in the 
current verison of gdb/corelow.c), function  add_to_thread_list
we find:

=========
  if (strncmp (bfd_section_name (abfd, asect), ".reg/", 5) != 0)
    return;

  thread_id = atoi (bfd_section_name (abfd, asect) + 5);

  add_thread (pid_to_ptid (thread_id));

======== 

You see it recognizes the threads by reading the various .reg sections.
And the result aka thread_id is an old mixed pid i.e. one of the form

#define CORE_MERGEPID(PID, TID)      (((PID) & 0xffff) | ((TID) << 16))

So doing an
    
    add_thread (pid_to_ptid (thread_id)); 

is a disaster because simply this is not the pid/tid!
Someone should before decode the pid/tid info using the old macros

#define CORE_PIDGET(PID)             (((PID) & 0xffff))
#define CORE_TIDGET(PID)             (((PID) & 0x7fffffff) >> 16)

  p_id = CORE_PIDGET(thread_id);
  t_id = CORE_TIDGET(thread_id);

and then do a call to add_thread as follows:

  add_thread ( MERGEPID(p_id, t_id) );


With a new elfcore_make_ptid_str the call to 

   thread_id = atoi (bfd_section_name (abfd, asect) + 5) 

is nonsense. Someome (perhaps the binutils people) must decide
what will be the thread string recognition and act in 
bfd/elf.c and gdb/corelow.c/add_to_thread_list aproppriately.

As I said GDB 5.1 in its currect form it doesnt seem to be able 
debug any mutithreaded cores. A quick fix to this bug leaving 
gdb-5.1/bfd as is:

define inside corelow.c the old macros:

#define CORE_PIDGET(PID)             (((PID) & 0xffff))
#define CORE_TIDGET(PID)             (((PID) & 0x7fffffff) >> 16)
#define CORE_MERGEPID(PID, TID)      (((PID) & 0xffff) | ((TID) << 16))

in accordance with bfd/elf.c/elfcore_make_pid(abfd).

Then the routine add_to_thread_list should be:

  thread_id = atoi (bfd_section_name (abfd, asect) + 5);
#if defined(DEBUG)
  warning("Adding thread %d inside core pid %d, tid %d \n", thread_id,
                        CORE_PIDGET(thread_id), CORE_TIDGET(thread_id));
#endif /* DEBUG */
  /* Decode the pid,tid from .reg/xxx section */
  p_id = CORE_PIDGET(thread_id);
  t_id = CORE_TIDGET(thread_id);

  /* Create and add the new ptid */
  add_thread ( MERGEPID(p_id, t_id) );

And again below the line 

  inferior_ptid = pid_to_ptid (thread_id);

should be changed!

Function: 

get_core_register_section

  if (PIDGET (inferior_ptid))
    sprintf (section_name, "%s/%d", name, PIDGET (inferior_ptid));
  else
    strcpy (section_name, name);

To: 
  int mixed;

  if (PIDGET (inferior_ptid))
  {

    mixed = CORE_MERGEPID ( PIDGET (inferior_ptid)
                                     TIDGET(inferior_ptid) );
    sprintf (section_name, "%s/%d", name, mixed);
  }
  else
    strcpy (section_name, name);

 
When you guys agree with the binutils for a solution to this, the
above hack in corelow.c can be removed. Until then gdb 5.1 is not
working for multi threaded core files without the above fix.
If corelow.c is left as is the new 5.2 will be also buggy.

Regards,
Takis