Only allow closure lookup by address if there are threads displaced-stepping

Since commit 1e5ccb9c5ff4fd8ade4a8694676f99f4abf2d679, we have an assertion in
displaced_step_buffers::copy_insn_closure_by_addr that makes sure a closure
is available whenever we have a match between the provided address argument and
the buffer address.

That is fine, but the report in PR30872 shows this assertion triggering when
it really shouldn't. After some investigation, here's what I found out.

The 32-bit Arm architecture is the only one that calls
gdbarch_displaced_step_copy_insn_closure_by_addr directly, and that's because
32-bit Arm needs to figure out the thumb state of the original instruction
that we displaced-stepped through the displaced-step buffer.

Before the assertion was put in place by commit
1e5ccb9c5ff4fd8ade4a8694676f99f4abf2d679, there was the possibility of
getting nullptr back, which meant we were not doing a displaced-stepping
operation.

Now, with the assertion in place, this is running into issues.

It looks like displaced_step_buffers::copy_insn_closure_by_addr is
being used to return a couple different answers depending on the
state we're in:

1 - If we are actively displaced-stepping, then copy_insn_closure_by_addr
is supposed to return a valid closure for us, so we can determine the
thumb mode.

2 - If we are not actively displaced-stepping, then copy_insn_closure_by_addr
should return nullptr to signal that there isn't any displaced-step buffers
in use, because we don't have a valid closure (but we should always have
this).

Since the displaced-step buffers are always allocated, but not always used,
that means the buffers will always contain data. In particular, the buffer
addr field cannot be used to determine if the buffer is active or not.

For instance, we cannot set the buffer addr field to 0x0, as that can be a
valid PC in some cases.

My understanding is that the current_thread field should be a good candidate
to signal that a particular displaced-step buffer is active or not. If it is
nullptr, we have no threads using that buffer to displaced-step.  Otherwise,
it is an active buffer in use by a particular thread.

The following fix modifies the displaced_step_buffers::copy_insn_closure_by_addr
function so we only attempt to return a closure if the buffer has an assigned
current_thread and if the buffer address matches the address argument.

Alternatively, I think we could use a function to answer the question of
whether we're actively displaced-stepping (so we have an active buffer) or
not.

I've also added a testcase that exercises the problem. It should reproduce
reliably on Arm, as that is the only architecture that faces this problem
at the moment.

Regression-tested on Ubuntu 20.04. OK?

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

1 files changed, 2 insertions, 1 deletions

diff --git a/gdb/displaced-stepping.c b/gdb/displaced-stepping.c
index bc59ef0..41c3c99 100644
--- a/gdb/displaced-stepping.c
+++ b/gdb/displaced-stepping.c
@@ -277,7 +277,8 @@ displaced_step_buffers::copy_insn_closure_by_addr (CORE_ADDR addr)
 {
   for (const displaced_step_buffer &buffer : m_buffers)
     {
-      if (addr == buffer.addr)
+      /* Make sure we have active buffers to compare to.  */
+      if (buffer.current_thread != nullptr && addr == buffer.addr)
       {
 	/* The closure information should always be available. */
 	gdb_assert (buffer.copy_insn_closure.get () != nullptr);