GDB 10.2 Release -- 2021-04-11 update

[Kevin Buettner]

On Tue, 13 Apr 2021 21:45:53 -0400
Simon Marchi <simon.marchi@polymtl.ca> wrote:

> On 2021-04-13 12:18 a.m., Kevin Buettner wrote:> On Mon, 12 Apr 2021 10:04:44 -0400
[...]
> > 
> > diff --git a/gdbserver/thread-db.cc b/gdbserver/thread-db.cc
> > index 055a0fa970f..9403b419710 100644
> > --- a/gdbserver/thread-db.cc
> > +++ b/gdbserver/thread-db.cc
> > @@ -44,10 +44,14 @@ struct thread_db
> >    /* Connection to the libthread_db library.  */
> >    td_thragent_t *thread_agent;
> >  
> > -  /* If this flag has been set, we've already asked GDB for all
> > -     symbols we might need; assume symbol cache misses are
> > -     failures.  */
> > -  int all_symbols_looked_up;
> > +  /* This flag is set after asking GDB for symbols that we need.  It
> > +     does not, however, indicate that all necessary symbols have found
> > +     as these may be split among several shared objects.  */
> > +  bool initialized;
> > +
> > +  /* Flag which indicates that it's okay to ask GDB for symbols via
> > +     a qSymbol reply.  */
> > +  bool qsymbol_reply_okay;
> >  
> >  #ifndef USE_LIBTHREAD_DB_DIRECTLY
> >    /* Handle of the libthread_db from dlopen.  */
> > @@ -359,19 +363,20 @@ thread_db_look_up_symbols (void)
> >    const char **sym_list;
> >    CORE_ADDR unused;
> >  
> > +  thread_db->qsymbol_reply_okay = true;
> > +
> >    for (sym_list = thread_db->td_symbol_list_p (); *sym_list; sym_list++)
> >      look_up_one_symbol (*sym_list, &unused, 1);
> >  
> > -  /* We're not interested in any other libraries loaded after this
> > -     point, only in symbols in libpthread.so.  */
> > -  thread_db->all_symbols_looked_up = 1;
> > +  thread_db->qsymbol_reply_okay = false;
> > +  thread_db->initialized = true;
> >  }
> >  
> >  int
> >  thread_db_look_up_one_symbol (const char *name, CORE_ADDR *addrp)
> >  {
> >    struct thread_db *thread_db = current_process ()->priv->thread_db;
> > -  int may_ask_gdb = !thread_db->all_symbols_looked_up;
> > +  int may_ask_gdb = thread_db->qsymbol_reply_okay || !thread_db->initialized;
> >  
> >    /* If we've passed the call to thread_db_look_up_symbols, then
> >       anything not in the cache must not exist; we're not interested
> > @@ -396,7 +401,7 @@ thread_db_get_tls_address (struct thread_info *thread, CORE_ADDR offset,
> >    thread_db = proc->priv->thread_db;
> >  
> >    /* If the thread layer is not (yet) initialized, fail.  */
> > -  if (thread_db == NULL || !thread_db->all_symbols_looked_up)
> > +  if (thread_db == NULL || !thread_db->initialized)
> >      return TD_ERR;
> >  
> >    /* If td_thr_tls_get_addr is missing rather do not expect td_thr_tlsbase
> > @@ -888,7 +893,7 @@ thread_db_notice_clone (struct thread_info *parent_thr, ptid_t child_ptid)
> >  
> >    /* If the thread layer isn't initialized, return.  It may just
> >       be that the program uses clone, but does not use libthread_db.  */
> > -  if (thread_db == NULL || !thread_db->all_symbols_looked_up)
> > +  if (thread_db == NULL || !thread_db->initialized)
> >      return;
> >  
> >    /* find_one_thread calls into libthread_db which accesses memory via
> >   
> 
> Thanks!
> 
> So from what I understand, the problem is that GDB offers GDBserver the
> opportunity to look up some symbols in between each objfile read in the
> initial library scan.  After GDB has loaded lipthread (but not ld-linux
> yet), libthread_db fails to work because GDB doesn't know the ld-linux
> symbols yet.  It sounds very much like the non-GDBserver case.
> 
> I see that you are trying to fix that GDBserver-side (although I don't
> quite get what you are trying to do, perhaps a quick explanation would
> help).

On the gdbserver side of things, I noticed that there's a flag
named "all_symbols_looked_up".  This flag is used for two purposes:

1) It's used by thread_db_get_tls_address() and
thread_db_notice_clone() to test whether the thread_db layer is
initialized enough to proceed.

2) It's used by thread_db_look_up_one_symbol() to find out whether
it's okay to send a qSymbol reply.  Regarding this particular change,
it currently looks like this:

-  int may_ask_gdb = !thread_db->all_symbols_looked_up;
+  int may_ask_gdb = thread_db->qsymbol_reply_okay || !thread_db->initialized;

However, I had originally (re)written that line as follows:

+  int may_ask_gdb = thread_db->qsymbol_reply_okay;

This looks correct to me, and it caused
gdb.threads/fork-plus-threads.exp to work as well as it does on a
glibc 2.32 machine.  Unfortunately, it caused gdb.threads/tls.exp
(and other tests too) to regress.

So, anyway, my thought was to split that one flag which serves
two purposes into separate flags.  This would allow us to process
additional qSymbol requests from GDB when new shared libraries
are loaded.

I'm not confident that this approach can be made to work, though
it looked promising for a while.


> Could we instead tackle it GDB-side, with the same idea as what
> I proposed the native debugging?  We could suppress the qsymbol packets
> sent in between each objfile loaded from the initial library scan.  We
> would have a single qsymbol sent after finishing loading all the initial
> libraries.

Sure, I'm willing to work on this, or at least help out...

> For example, I just hacked this, this makes it work for the simple
> non-stop attach case with gdbserver, where you run gdbserver with
> --attach and then connect with GDB (I haven't tried the other modes).
> The final qsymbol, that lets GDBserver load libthread_db, is sent in
> remote_target::start_remote.

I tested your patch today (against the test suite) using both
--target_board=native-extended-gdbserver/-m64 and
--target_board=native-gdbserver.  Unfortunately, I didn't see
a test which was helped by your patch.  (Though there was the
usual noise from racy tests.)

For the test that I've been focused on, I see the following results
on a glibc 2.32 machine:

[kev@f33-1 gdb]$ make check RUNTESTFLAGS="--target_board=native-extended-gdbserver/-m64" TESTS=gdb.threads/fork-plus-threads.exp
...
FAIL: gdb.threads/fork-plus-threads.exp: detach-on-fork=off: only inferior 1 left

		=== gdb Summary ===

# of expected passes		15
# of unexpected failures	1

With or without your patch, I see this result on a glibc 2.33 machine:

[kev@f33-3 gdb]$ make check RUNTESTFLAGS="--target_board=native-extended-gdbserver/-m64" TESTS=gdb.threads/fork-plus-threads.exp
...
FAIL: gdb.threads/fork-plus-threads.exp: detach-on-fork=off: inferior 1 exited
FAIL: gdb.threads/fork-plus-threads.exp: detach-on-fork=off: no threads left
FAIL: gdb.threads/fork-plus-threads.exp: detach-on-fork=off: only inferior 1 left
WARNING: Timed out waiting for EOF in server after monitor exit

		=== gdb Summary ===

# of expected passes		13
# of unexpected failures	3

(It also leaves a gdbserver running which must be killed with -KILL.)

The other thing that I should mention is that Florian Weimer has told
me that (he thinks) glibc 2.33 can be changed so that no GDB changes
are needed.  So, if we can't figure it out, we might want to ask
Florian to do this.

Florian has also said that things will change yet again in glibc 2.34. 
It's not clear to me that the fix we're investigating for glibc 2.33
will still be required.  Florian told me:  "So ideally we'd figure out
a way to load libthread_db even if there is no libpthread in the
inferior."  This sounds like a different problem than the one we're
currently investigating.

Kevin