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

[Takis Psarogiannakopoulos <takis at XFree86 dot Org>]

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