[Bug gdb/30935] Regression exposed by new gdb.python/py-progspace-events.exp test and native-extended-gdbserver board

["cvs-commit at gcc dot gnu.org" <sourceware-bugzilla@sourceware.org>]

https://sourceware.org/bugzilla/show_bug.cgi?id=30935

--- Comment #5 from cvs-commit at gcc dot gnu.org <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Simon Marchi <simark@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=1b28c0f488bb84c8cc9f680afa9680dc0b0e2b3d

commit 1b28c0f488bb84c8cc9f680afa9680dc0b0e2b3d
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Tue Oct 3 17:20:14 2023 +0100

    gdb: fix auxv cache clearing from new_objfile observer

    It was pointed out on the mailing list that a recently added
    test (gdb.python/py-progspace-events.exp) was failing when run with
    the native-extended-gdbserver board.  This test was added with this
    commit:

      commit 59912fb2d22f8a4bb0862487f12a5cc65b6a013f
      Date:   Tue Sep 19 11:45:36 2023 +0100

          gdb: add Python events for program space addition and removal

    It turns out though that the test is failing due to a existing bug
    in GDB, the new test just exposes the problem.  Additionally, the
    failure really doesn't even rely on the new functionality added in the
    above commit.  I reduced the test to a simple set of steps that
    reproduced the failure and tested against GDB 13, and the test passes;
    so the bug was introduced since then.  In fact, the bug was introduced
    with this commit:

      commit a2827364e2bf19910fa5a54364a594a5ba3033b8
      Date:   Fri Sep 8 15:48:16 2023 +0100

          gdb: remove final user of the executable_changed observer

    This commit changed how the per-inferior auxv data cache is managed,
    specifically, when the cache is cleared, and it is this that leads to
    the failure.

    This bug is interesting because it exposes a number of issues with
    GDB, I'll explain all of the problems I see, though ultimately, I only
    propose fixing one problem in this commit, which is enough to resolve
    the crash we are currently seeing.

    The crash that we are seeing manifests like this:

      ...
      [Inferior 2 (process 3970384) exited normally]
      +inferior 1
      [Switching to inferior 1 [process 3970383]
(/tmp/build/gdb/testsuite/outputs/gdb.python/py-progspace-events/py-progspace-events)]
      [Switching to thread 1.1 (Thread 3970383.3970383)]
      #0  breakpt () at
/tmp/build/gdb/testsuite/../../../src/gdb/testsuite/gdb.python/py-progspace-events.c:28
      28    { /* Nothing.  */ }
      (gdb) step
      +step
      terminate called after throwing an instance of 'gdb_exception_error'

      Fatal signal: Aborted
      ... etc ...

    What's happening is that GDB attempts to refill the auxv cache as a
    result of the gdbarch_has_shared_address_space call in
    program_space::~program_space, the backtrace looks like this:

      #0  0x00007fb4f419a9a5 in raise () from /lib64/libpthread.so.0
      #1  0x00000000008b635d in handle_fatal_signal (sig=6) at
../../src/gdb/event-top.c:912
      #2  <signal handler called>
      #3  0x00007fb4f38e3625 in raise () from /lib64/libc.so.6
      #4  0x00007fb4f38cc8d9 in abort () from /lib64/libc.so.6
      #5  0x00007fb4f3c70756 in __gnu_cxx::__verbose_terminate_handler() [clone
.cold] () from /lib64/libstdc++.so.6
      #6  0x00007fb4f3c7c6dc in __cxxabiv1::__terminate(void (*)()) () from
/lib64/libstdc++.so.6
      #7  0x00007fb4f3c7b6e9 in __cxa_call_terminate () from
/lib64/libstdc++.so.6
      #8  0x00007fb4f3c7c094 in __gxx_personality_v0 () from
/lib64/libstdc++.so.6
      #9  0x00007fb4f3a80c63 in _Unwind_RaiseException_Phase2 () from
/lib64/libgcc_s.so.1
      #10 0x00007fb4f3a8154e in _Unwind_Resume () from /lib64/libgcc_s.so.1
      #11 0x0000000000e8832d in target_read_alloc_1<unsigned char>
(ops=0x408a3a0, object=TARGET_OBJECT_AUXV, annex=0x0) at
../../src/gdb/target.c:2266
      #12 0x0000000000e73dea in target_read_alloc (ops=0x408a3a0,
object=TARGET_OBJECT_AUXV, annex=0x0) at ../../src/gdb/target.c:2315
      #13 0x000000000058248c in target_read_auxv_raw (ops=0x408a3a0) at
../../src/gdb/auxv.c:379
      #14 0x000000000058243d in target_read_auxv () at ../../src/gdb/auxv.c:368
      #15 0x000000000058255c in target_auxv_search (match=0x0,
valp=0x7ffdee17c598) at ../../src/gdb/auxv.c:415
      #16 0x0000000000a464bb in linux_is_uclinux () at
../../src/gdb/linux-tdep.c:433
      #17 0x0000000000a464f6 in linux_has_shared_address_space
(gdbarch=0x409a2d0) at ../../src/gdb/linux-tdep.c:440
      #18 0x0000000000510eae in gdbarch_has_shared_address_space
(gdbarch=0x409a2d0) at ../../src/gdb/gdbarch.c:4889
      #19 0x0000000000bc7558 in program_space::~program_space (this=0x4544aa0,
__in_chrg=<optimized out>) at ../../src/gdb/progspace.c:124
      #20 0x00000000009b245d in delete_inferior (inf=0x47b3de0) at
../../src/gdb/inferior.c:290
      #21 0x00000000009b2c10 in prune_inferiors () at
../../src/gdb/inferior.c:480
      #22 0x00000000009c5e3e in fetch_inferior_event () at
../../src/gdb/infrun.c:4558
      #23 0x000000000099b4dc in inferior_event_handler
(event_type=INF_REG_EVENT) at ../../src/gdb/inf-loop.c:42
      #24 0x0000000000cbc64f in remote_async_serial_handler (scb=0x4090a30,
context=0x408a6b0) at ../../src/gdb/remote.c:14859
      #25 0x0000000000d83d3a in run_async_handler_and_reschedule
(scb=0x4090a30) at ../../src/gdb/ser-base.c:138
      #26 0x0000000000d83e1f in fd_event (error=0, context=0x4090a30) at
../../src/gdb/ser-base.c:189

    So this is problem #1, if we throw an exception while deleting a
    program_space then this is not caught, and is going to crash GDB.

    Problem #2 becomes evident when we ask why GDB is throwing an error in
    this case; the error is thrown because the remote target, operating in
    non-async mode, can't read the auxv data while an inferior is running
    and GDB is waiting for a stop reply.  The problem here then, is why
    does GDB get into a position where it tries to interact with the
    remote target in this way, at this time?  The problem is caused by the
    prune_inferiors call which can be seen in the above backtrace.

    In prune_inferiors we check if the inferior is deletable, and if it
    is, we delete it.  The problem is, I think, we should also check if
    the target is currently in a state that would allow us to delete the
    inferior.  We don't currently have such a check available, we'd need
    to add one, but for the remote target, this would return false if the
    remote is in async mode and the remote is currently waiting for a stop
    reply.  With this change in place GDB would defer deleting the
    inferior until the remote target has stopped, at which point GDB would
    be able to refill the auxv cache successfully.

    And then, problem #3 becomes evident when we ask why GDB is needing to
    refill the auxv cache now when it didn't need to for GDB 13.  This is
    where the second commit mentioned above (a2827364e2bf) comes in.
    Prior to this commit, the auxv cache was cleared by the
    executable_changed observer, while after that commit the auxv cache
    was cleared by the new_objfile observer -- but only when the
    new_objfile observer is used in the special mode that actually means
    that all objfiles have been unloaded (I know, the overloading of the
    new_objfile observer is horrible, and unnecessary, but it's not really
    important for this bug).

    The difference arises because the new_objfile observer is triggered
    from clear_symtab_users, which in turn is called from
    program_space::~program_space.  The new_objfile observer for auxv does
    this:

      static void
      auxv_new_objfile_observer (struct objfile *objfile)
      {
        if (objfile == nullptr)
          invalidate_auxv_cache_inf (current_inferior ());
      }

    That is, when all the objfiles are unloaded, we clear the auxv cache
    for the current inferior.

    The problem is, then when we look at the prune_inferiors ->
    delete_inferior -> ~program_space path, we see that the current
    inferior is not going to be an inferior that exists within the
    program_space being deleted; delete_inferior removes the deleted
    inferior from the global inferior list, and then only deletes the
    program_space if program_space::empty() returns true, which is only
    the case if the current inferior isn't within the program_space to
    delete, and no other inferior exists within that program_space
    either.

    What this means is that when the new_objfile observer is called we
    can't rely on the current inferior having any relationship with the
    program space in which the objfiles were removed.  This was an error
    in the commit a2827364e2bf, the only thing we can rely on is the
    current program space.  As a result of this mistake, after commit
    a2827364e2bf, GDB was sometimes clearing the auxv cache for a random
    inferior.  In the native target case this was harmless as we can
    always refill the cache when needed, but in the remote target case, if
    we need to refill the cache when the remote target is executing, then
    we get the crash we observed.

    And additionally, if we think about this a little more, we see that
    commit a2827364e2bf made another mistake.  When all the objfiles are
    removed, they are removed from a program_space, a program_space might
    contain multiple inferiors, so surely, we should clear the auxv cache
    for all of the matching inferiors?

    Given these two insights, that the current_inferior is not relevant,
    only the current_program_space, and that we should be clearing the
    cache for all inferiors in the current_program_space, we can update
    auxv_new_objfile_observer to:

      if (objfile == nullptr)
        {
          for (inferior *inf : all_inferiors ())
            {
              if (inf->pspace == current_program_space)
                invalidate_auxv_cache_inf (inf);
            }
        }

    With this change we now correctly clear the auxv cache for the correct
    inferiors, and GDB no longer needs to refill the cache at an
    inconvenient time, this avoids the crash we were seeing.

    And finally, we reach problem #4.  Inspired by the observation that
    using the current_inferior from within the ~program_space function was
    not correct, I added some debug to see if current_inferior() was
    called anywhere else (below ~program_space), and the answer is yes,
    it's called a often.  Mostly the culprit is GDB doing:

      current_inferior ()->top_target ()-> ....

    But I think all of these calls are most likely doing the wrong thing,
    and only work because the top target in all these cases is shared
    between all inferiors, e.g. it's the native target, or the remote
    target for all inferiors.  But if we had a truly multi-connection
    setup, then we might start to see odd behaviour.

    Problem #1 I'm just ignoring for now, I guess at some point we might
    run into this again, and then we'd need to solve this.  But in this
    case I wasn't sure what a "good" solution would look like.  We need
    the auxv data in order to implement the linux_is_uclinux() function.
    If we can't get the auxv data then what should we do, assume yes, or
    assume no?  The right answer would probably be to propagate the error
    back up the stack, but then we reach ~program_space, and throwing
    exceptions from a destructor is problematic, so we'd need to catch and
    deal at this point.  The linux_is_uclinux() call is made from within
    gdbarch_has_shared_address_space(), which is used like:

      if (!gdbarch_has_shared_address_space (target_gdbarch ()))
        delete this->aspace;

    So, we would have to choose; delete the address space or not.  If we
    delete it on error, then we might delete an address space that is
    shared within another program space.  If we don't delete the address
    space, then we might leak it.  Neither choice is great.

    A better solution might be to have the address spaces be reference
    counted, then we could remove the gdbarch_has_shared_address_space
    call completely, and just rely on the reference count to auto-delete
    the address space when appropriate.

    The solution for problem #2 I already hinted at above, we should have
    a new target_can_delete_inferiors() call, which should be called from
    prune_inferiors, this would prevent GDB from trying to delete
    inferiors when a (remote) target is in a state where we know it can't
    delete the inferior.  Deleting an inferior often (always?) requires
    sending packets to the remote, and if the remote is waiting for a stop
    reply then this will never work, so the pruning should be deferred in
    this case.

    The solution for problem #3 is included in this commit.

    And, for problem #4, I'm not sure what the right solution is.  Maybe
    delete_inferior should ensure the inferior to be deleted is in place
    when ~program_space is called?  But that seems a little weird, as the
    current inferior would, in theory, still be using the current
    program_space...

    Anyway, after this commit, the gdb.python/py-progspace-events.exp test
    now passes when run with the native-extended-remote board.

    Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30935
    Approved-By: Simon Marchi <simon.marchi@efficios.com>
    Change-Id: I41f0e6e2d7ecc1e5e55ec170f37acd4052f46eaf

-- 
You are receiving this mail because:
You are on the CC list for the bug.