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

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

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

[Michael Snyder]

Takis Psarogiannakopoulos wrote:
> 
> 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!

In fact, I think Takis is right.  I noticed while doing the
gcore work that the thread IDs from multi-threaded corefiles
on Solaris seemed to be broken, perhaps because corelow 
has not been made ptid-aware.

> 
> 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

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

[Kevin Buettner]

On Jan 18,  9:11am, Michael Snyder wrote:

> Takis Psarogiannakopoulos wrote:

> > 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!
> 
> In fact, I think Takis is right.  I noticed while doing the
> gcore work that the thread IDs from multi-threaded corefiles
> on Solaris seemed to be broken, perhaps because corelow 
> has not been made ptid-aware.

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.)

Kevin