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

[Michael Snyder <msnyder at redhat dot com>]

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