| Message ID | 20191230162440.21111-1-luis.machado@linaro.org |
|---|---|
| State | New, archived |
| Headers |
Received: (qmail 119091 invoked by alias); 30 Dec 2019 16:24:56 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 119082 invoked by uid 89); 30 Dec 2019 16:24:56 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.1 spammy=issued, signals, pc, reaching X-HELO: mail-pg1-f174.google.com Received: from mail-pg1-f174.google.com (HELO mail-pg1-f174.google.com) (209.85.215.174) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 30 Dec 2019 16:24:54 +0000 Received: by mail-pg1-f174.google.com with SMTP id x7so18222991pgl.11 for <gdb-patches@sourceware.org>; Mon, 30 Dec 2019 08:24:53 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=from:to:subject:date:message-id; bh=uiWWfHy6KFwTNqhmftvsW1v7tcpiQM57W907Jq3G6SM=; b=UOlDTam4ZKugT49IyrtmtSjRJK/PT9+jVLlklRE+ZbQbb1jT38IdiLHk65fWKcntp2 tBoQklIqCIFIR5TNoIF6iy8MnpoAPdSJFwA9+iOypOAP3ePWRfNFDDvLcyRj8BCegF/F BvGxkDj29nHjtRmcf1PN2b1qXrxYt/9mmDuQ3I3nCT8t1wqiM4i/Ip10r/48uZHn94Fw oUnfaHB7B2ZGsyPHxXdJ7T7tgWI/BMIRE8Jf2h/H1i5/gHy9ocTBL0pK0/nKnTNhhkMe Mp2botOiJOQJlu41myHFZH1tFOz8h0NiPVs61EdvJWvQeof7R8TlNGFWsqaBaJcLukoD yh7Q== Return-Path: <luis.machado@linaro.org> Received: from localhost.localdomain ([191.251.164.209]) by smtp.gmail.com with ESMTPSA id v4sm28379599pff.174.2019.12.30.08.24.50 for <gdb-patches@sourceware.org> (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 30 Dec 2019 08:24:51 -0800 (PST) From: Luis Machado <luis.machado@linaro.org> To: gdb-patches@sourceware.org Subject: [PATCH] [RFC][AArch64] Fix some erroneous behavior in gdb.base/step-over-syscall.exp Date: Mon, 30 Dec 2019 13:24:40 -0300 Message-Id: <20191230162440.21111-1-luis.machado@linaro.org> X-IsSubscribed: yes |
Commit Message
Luis Machado
Dec. 30, 2019, 4:24 p.m. UTC
I've been chasing/investigating this particular bug for a while now, and this is a tentative patch to fix the problem. Although this may not be aarch64-specific, i can reproduce it consitently in this particular architecture. In summary, this bug is a mix of random signal handling (SIGCHLD) and fork/vfork handling when single-stepping. Forks and vforks are handled in 3 to 4 steps. Fork 1 - We get a PTRACE_EVENT_FORK event. 2 - We single-step the inferior to get a SIGTRAP. 3 - Inferior is ready to be resumed as we wish. Vfork 1 - We get a PTRACE_EVENT_VFORK event. 2 - We continue the inferior to get a PTRACE_EVENT_VFORK_DONE event. 3 - We single-step the inferior to get a SIGTRAP. 4 - Inferior is ready to be resumed as we wish. The problem manifests itself when we are sitting at a syscall instruction and we try to instruction-step past it. There may or may not be a breakpoint inserted at the syscall instruction location. The expected outcome is that we step a single instruction and the resulting PC points at the next instruction ($pc + 4 in aarch64). What i see is that we end up in $pc + 8, that is, one instruction further than we should've been. This happens because, when forking/vforking, the child exits too quickly (in the particular case of gdb.base/step-over-syscall.exp) and a SIGCHLD signal is issued to the parent. Sometimes this signal arrives before GDB is done handling the fork/vfork situation. Usually in step 2 for fork and step 3 for vfork. In turn, this causes GDB to handle that SIGCHLD as a random signal that must be passed to the inferior, which is correct. But this particular code in infrun.c doesn't cope with this particular situation and GDB inserts a HP step-resume breakpoint and registers its wish for the inferior to be stepped yet again. Thus we end up in $pc + 8, even though we shouldn't have stepped again in this particular situation. The delivery of SIGCHLD in between fork/vfork handling steps is sensitive to timing. If we tweak things a little, enable debugging output or do any other thing that affects the timing, we may not see this behavior. This bug doesn't show up as failures in gdb.base/step-over-syscall.exp due to the way the test is written. As long as the PC is the same from the previous test to the next, GDB thinks it is good. The proposed patch doesn't cause a change in the number of PASSes/FAIL's. The proposed patch adds a variable waiting_for_fork_sigtrap that gets set just before the last step of fork/vfork handling and gets reset when we end up seeing that SIGTRAP we wanted. This variable is used in the random_signal handling of handle_signal_stop, where there are a couple conditional blocks that handle signals reaching the inferior at an unexpected time. If waiting_for_fork_sigtrap is set, we don't set GDB to do the additional single-step it wants to do. I gave this a thought and tried to find a better place to put the check in, but this seemed the most appropriate place to me. I also tried to solve this without having to introduce a new variable. But it seems we can't distinguish between a random signal reaching GDB in the middle of single-stepping and a random signal reaching GDB in the middle of single-stepping AND handling fork/vfork. Thoughts? gdb/ChangeLog: 2019-12-30 Luis Machado <luis.machado@linaro.org> * inferior.h (class inferior) <waiting_for_fork_sigtrap>: New member. Set to false by default. * infrun.c (handle_inferior_event): Set waiting_for_fork_sigtrap when a fork or vfork_done is detected. (handle_signal_stop): Don't step again if waiting_for_fork_sigtrap is true. Reset waiting_for_fork_sigtrap to false when nexti/stepi is detected. Change-Id: I2849e0dc49ad9c0a026daa8ced4610aa0ddbe637 --- gdb/inferior.h | 12 ++++++++++++ gdb/infrun.c | 31 +++++++++++++++++++++++++++++-- 2 files changed, 41 insertions(+), 2 deletions(-)
Comments
Ping? On 12/30/19 1:24 PM, Luis Machado wrote: > I've been chasing/investigating this particular bug for a while now, and this > is a tentative patch to fix the problem. > > Although this may not be aarch64-specific, i can reproduce it consitently in > this particular architecture. > > In summary, this bug is a mix of random signal handling (SIGCHLD) and > fork/vfork handling when single-stepping. > > Forks and vforks are handled in 3 to 4 steps. > > Fork > > 1 - We get a PTRACE_EVENT_FORK event. > 2 - We single-step the inferior to get a SIGTRAP. > 3 - Inferior is ready to be resumed as we wish. > > Vfork > > 1 - We get a PTRACE_EVENT_VFORK event. > 2 - We continue the inferior to get a PTRACE_EVENT_VFORK_DONE event. > 3 - We single-step the inferior to get a SIGTRAP. > 4 - Inferior is ready to be resumed as we wish. > > The problem manifests itself when we are sitting at a syscall instruction and > we try to instruction-step past it. There may or may not be a breakpoint > inserted at the syscall instruction location. > > The expected outcome is that we step a single instruction and the resulting PC > points at the next instruction ($pc + 4 in aarch64). > > What i see is that we end up in $pc + 8, that is, one instruction further than > we should've been. > > This happens because, when forking/vforking, the child exits too quickly (in the > particular case of gdb.base/step-over-syscall.exp) and a SIGCHLD signal is > issued to the parent. Sometimes this signal arrives before GDB is done handling > the fork/vfork situation. Usually in step 2 for fork and step 3 for vfork. > > In turn, this causes GDB to handle that SIGCHLD as a random signal that must be > passed to the inferior, which is correct. But this particular code in infrun.c > doesn't cope with this particular situation and GDB inserts a HP step-resume > breakpoint and registers its wish for the inferior to be stepped yet again. > > Thus we end up in $pc + 8, even though we shouldn't have stepped again in this > particular situation. > > The delivery of SIGCHLD in between fork/vfork handling steps is sensitive to > timing. If we tweak things a little, enable debugging output or do any other > thing that affects the timing, we may not see this behavior. > > This bug doesn't show up as failures in gdb.base/step-over-syscall.exp due to > the way the test is written. As long as the PC is the same from the previous > test to the next, GDB thinks it is good. The proposed patch doesn't cause a > change in the number of PASSes/FAIL's. > > The proposed patch adds a variable waiting_for_fork_sigtrap that gets set just > before the last step of fork/vfork handling and gets reset when we end up > seeing that SIGTRAP we wanted. > > This variable is used in the random_signal handling of handle_signal_stop, > where there are a couple conditional blocks that handle signals reaching the > inferior at an unexpected time. If waiting_for_fork_sigtrap is set, we don't > set GDB to do the additional single-step it wants to do. > > I gave this a thought and tried to find a better place to put the check in, > but this seemed the most appropriate place to me. > > I also tried to solve this without having to introduce a new variable. > But it seems we can't distinguish between a random signal reaching GDB in > the middle of single-stepping and a random signal reaching GDB in the middle > of single-stepping AND handling fork/vfork. > > Thoughts? > > gdb/ChangeLog: > > 2019-12-30 Luis Machado <luis.machado@linaro.org> > > * inferior.h (class inferior) <waiting_for_fork_sigtrap>: New member. > Set to false by default. > * infrun.c (handle_inferior_event): Set waiting_for_fork_sigtrap when > a fork or vfork_done is detected. > (handle_signal_stop): Don't step again if waiting_for_fork_sigtrap is > true. > Reset waiting_for_fork_sigtrap to false when nexti/stepi is detected. > > Change-Id: I2849e0dc49ad9c0a026daa8ced4610aa0ddbe637 > --- > gdb/inferior.h | 12 ++++++++++++ > gdb/infrun.c | 31 +++++++++++++++++++++++++++++-- > 2 files changed, 41 insertions(+), 2 deletions(-) > > diff --git a/gdb/inferior.h b/gdb/inferior.h > index 3bd9e8c3d7..2c23d7302b 100644 > --- a/gdb/inferior.h > +++ b/gdb/inferior.h > @@ -447,6 +447,18 @@ public: > exits or execs. */ > bool pending_detach = false; > > + /* True if we are still waiting for the final SIGTRAP when handling > + fork or vfork events. This is used in case a signal gets caught amidst > + handling fork and vfork events. > + > + For fork events we first get a PTRACE_EVENT_FORK, then SINGLESTEP the > + process and then get a SIGTRAP. > + > + For vfork events we first get a PTRACE_EVENT_VFORK, then we CONTINUE > + the process, get a PTRACE_EVENT_VFORK_DONE event, SINGLESTEP the process > + again and then get a SIGTRAP. */ > + bool waiting_for_fork_sigtrap = false; > + > /* True if this inferior is a vfork parent waiting for a vfork child > not under our control to be done with the shared memory region, > either by exiting or execing. */ > diff --git a/gdb/infrun.c b/gdb/infrun.c > index 7ddd21dd09..c3316a891a 100644 > --- a/gdb/infrun.c > +++ b/gdb/infrun.c > @@ -4937,6 +4937,10 @@ Cannot fill $_exitsignal with the correct signal number.\n")); > struct regcache *regcache = get_thread_regcache (ecs->event_thread); > struct gdbarch *gdbarch = regcache->arch (); > > + if (ecs->ws.kind == TARGET_WAITKIND_FORKED) > + /* We should see a SIGTRAP next time we stop. */ > + current_inferior ()->waiting_for_fork_sigtrap = true; > + > /* If checking displaced stepping is supported, and thread > ecs->ptid is displaced stepping. */ > if (displaced_step_in_progress_thread (ecs->event_thread)) > @@ -5094,6 +5098,8 @@ Cannot fill $_exitsignal with the correct signal number.\n")); > > context_switch (ecs); > > + /* We should see a SIGTRAP next time we stop. */ > + current_inferior ()->waiting_for_fork_sigtrap = true; > current_inferior ()->waiting_for_vfork_done = 0; > current_inferior ()->pspace->breakpoints_not_allowed = 0; > > @@ -5912,7 +5918,15 @@ handle_signal_stop (struct execution_control_state *ecs) > "breakpoint\n"); > > insert_hp_step_resume_breakpoint_at_frame (frame); > - ecs->event_thread->step_after_step_resume_breakpoint = 1; > + > + /* If we're still waiting for the final fork/vfork SIGTRAP and we > + got interrupted by a random signal, don't step further after > + returning from the signal handler. */ > + if (current_inferior ()->waiting_for_fork_sigtrap) > + ecs->event_thread->step_after_step_resume_breakpoint = 0; > + else > + ecs->event_thread->step_after_step_resume_breakpoint = 1; > + > /* Reset trap_expected to ensure breakpoints are re-inserted. */ > ecs->event_thread->control.trap_expected = 0; > > @@ -5947,7 +5961,15 @@ handle_signal_stop (struct execution_control_state *ecs) > > clear_step_over_info (); > insert_hp_step_resume_breakpoint_at_frame (frame); > - ecs->event_thread->step_after_step_resume_breakpoint = 1; > + > + /* If we're still waiting for the final fork/vfork SIGTRAP and we > + got interrupted by a random signal, don't step further after > + returning from the signal handler. */ > + if (current_inferior ()->waiting_for_fork_sigtrap) > + ecs->event_thread->step_after_step_resume_breakpoint = 0; > + else > + ecs->event_thread->step_after_step_resume_breakpoint = 1; > + > /* Reset trap_expected to ensure breakpoints are re-inserted. */ > ecs->event_thread->control.trap_expected = 0; > keep_going (ecs); > @@ -6691,6 +6713,11 @@ process_event_stop_test (struct execution_control_state *ecs) > one instruction. */ > if (debug_infrun) > fprintf_unfiltered (gdb_stdlog, "infrun: stepi/nexti\n"); > + > + /* If we were waiting for a final fork/vfork SIGTRAP after a > + single-step, this should be it. Clear the flag. */ > + if (current_inferior ()->waiting_for_fork_sigtrap) > + current_inferior ()->waiting_for_fork_sigtrap = false; > end_stepping_range (ecs); > return; > } >
> On 30 Dec 2019, at 16:24, Luis Machado <luis.machado@linaro.org> wrote: > > I've been chasing/investigating this particular bug for a while now, and this > is a tentative patch to fix the problem. > > Although this may not be aarch64-specific, i can reproduce it consitently in > this particular architecture. > > In summary, this bug is a mix of random signal handling (SIGCHLD) and > fork/vfork handling when single-stepping. > > Forks and vforks are handled in 3 to 4 steps. > > Fork > > 1 - We get a PTRACE_EVENT_FORK event. > 2 - We single-step the inferior to get a SIGTRAP. > 3 - Inferior is ready to be resumed as we wish. > > Vfork > > 1 - We get a PTRACE_EVENT_VFORK event. > 2 - We continue the inferior to get a PTRACE_EVENT_VFORK_DONE event. > 3 - We single-step the inferior to get a SIGTRAP. > 4 - Inferior is ready to be resumed as we wish. > > The problem manifests itself when we are sitting at a syscall instruction and > we try to instruction-step past it. There may or may not be a breakpoint > inserted at the syscall instruction location. > > The expected outcome is that we step a single instruction and the resulting PC > points at the next instruction ($pc + 4 in aarch64). > > What i see is that we end up in $pc + 8, that is, one instruction further than > we should've been. > > This happens because, when forking/vforking, the child exits too quickly (in the > particular case of gdb.base/step-over-syscall.exp) and a SIGCHLD signal is > issued to the parent. Sometimes this signal arrives before GDB is done handling > the fork/vfork situation. Usually in step 2 for fork and step 3 for vfork. > > In turn, this causes GDB to handle that SIGCHLD as a random signal that must be > passed to the inferior, which is correct. But this particular code in infrun.c > doesn't cope with this particular situation and GDB inserts a HP step-resume > breakpoint and registers its wish for the inferior to be stepped yet again. > > Thus we end up in $pc + 8, even though we shouldn't have stepped again in this > particular situation. I’ve tried debugging this issue in the past, and didn’t figure out the issue. This _sounds_ like a reasonable explanation of why. > > The delivery of SIGCHLD in between fork/vfork handling steps is sensitive to > timing. If we tweak things a little, enable debugging output or do any other > thing that affects the timing, we may not see this behavior. > > This bug doesn't show up as failures in gdb.base/step-over-syscall.exp due to > the way the test is written. As long as the PC is the same from the previous > test to the next, GDB thinks it is good. The proposed patch doesn't cause a > change in the number of PASSes/FAIL's. > Is it possible to add an explicit test to test for the issue? (possibly as a new .exp file). > The proposed patch adds a variable waiting_for_fork_sigtrap that gets set just > before the last step of fork/vfork handling and gets reset when we end up > seeing that SIGTRAP we wanted. > > This variable is used in the random_signal handling of handle_signal_stop, > where there are a couple conditional blocks that handle signals reaching the > inferior at an unexpected time. If waiting_for_fork_sigtrap is set, we don't > set GDB to do the additional single-step it wants to do. > > I gave this a thought and tried to find a better place to put the check in, > but this seemed the most appropriate place to me. > > I also tried to solve this without having to introduce a new variable. > But it seems we can't distinguish between a random signal reaching GDB in > the middle of single-stepping and a random signal reaching GDB in the middle > of single-stepping AND handling fork/vfork. > > Thoughts? > > gdb/ChangeLog: > > 2019-12-30 Luis Machado <luis.machado@linaro.org> > > * inferior.h (class inferior) <waiting_for_fork_sigtrap>: New member. > Set to false by default. > * infrun.c (handle_inferior_event): Set waiting_for_fork_sigtrap when > a fork or vfork_done is detected. > (handle_signal_stop): Don't step again if waiting_for_fork_sigtrap is > true. > Reset waiting_for_fork_sigtrap to false when nexti/stepi is detected. > > Change-Id: I2849e0dc49ad9c0a026daa8ced4610aa0ddbe637 > --- > gdb/inferior.h | 12 ++++++++++++ > gdb/infrun.c | 31 +++++++++++++++++++++++++++++-- > 2 files changed, 41 insertions(+), 2 deletions(-) > > diff --git a/gdb/inferior.h b/gdb/inferior.h > index 3bd9e8c3d7..2c23d7302b 100644 > --- a/gdb/inferior.h > +++ b/gdb/inferior.h > @@ -447,6 +447,18 @@ public: > exits or execs. */ > bool pending_detach = false; > > + /* True if we are still waiting for the final SIGTRAP when handling > + fork or vfork events. This is used in case a signal gets caught amidst > + handling fork and vfork events. > + > + For fork events we first get a PTRACE_EVENT_FORK, then SINGLESTEP the > + process and then get a SIGTRAP. > + > + For vfork events we first get a PTRACE_EVENT_VFORK, then we CONTINUE > + the process, get a PTRACE_EVENT_VFORK_DONE event, SINGLESTEP the process > + again and then get a SIGTRAP. */ > + bool waiting_for_fork_sigtrap = false; > + What happens with a multithreaded process that forks/vforks? Should this bool be held per thread instead? (A test would be nice to have here). > /* True if this inferior is a vfork parent waiting for a vfork child > not under our control to be done with the shared memory region, > either by exiting or execing. */ > diff --git a/gdb/infrun.c b/gdb/infrun.c > index 7ddd21dd09..c3316a891a 100644 > --- a/gdb/infrun.c > +++ b/gdb/infrun.c > @@ -4937,6 +4937,10 @@ Cannot fill $_exitsignal with the correct signal number.\n")); > struct regcache *regcache = get_thread_regcache (ecs->event_thread); > struct gdbarch *gdbarch = regcache->arch (); > > + if (ecs->ws.kind == TARGET_WAITKIND_FORKED) > + /* We should see a SIGTRAP next time we stop. */ > + current_inferior ()->waiting_for_fork_sigtrap = true; > + > /* If checking displaced stepping is supported, and thread > ecs->ptid is displaced stepping. */ > if (displaced_step_in_progress_thread (ecs->event_thread)) > @@ -5094,6 +5098,8 @@ Cannot fill $_exitsignal with the correct signal number.\n")); > > context_switch (ecs); > > + /* We should see a SIGTRAP next time we stop. */ > + current_inferior ()->waiting_for_fork_sigtrap = true; > current_inferior ()->waiting_for_vfork_done = 0; > current_inferior ()->pspace->breakpoints_not_allowed = 0; > > @@ -5912,7 +5918,15 @@ handle_signal_stop (struct execution_control_state *ecs) > "breakpoint\n"); > > insert_hp_step_resume_breakpoint_at_frame (frame); > - ecs->event_thread->step_after_step_resume_breakpoint = 1; > + > + /* If we're still waiting for the final fork/vfork SIGTRAP and we > + got interrupted by a random signal, don't step further after > + returning from the signal handler. */ > + if (current_inferior ()->waiting_for_fork_sigtrap) > + ecs->event_thread->step_after_step_resume_breakpoint = 0; > + else > + ecs->event_thread->step_after_step_resume_breakpoint = 1; > + Simpler to write as this? ecs->event_thread->step_after_step_resume_breakpoint = !current_inferior ()->waiting_for_fork_sigtrap; > /* Reset trap_expected to ensure breakpoints are re-inserted. */ > ecs->event_thread->control.trap_expected = 0; > > @@ -5947,7 +5961,15 @@ handle_signal_stop (struct execution_control_state *ecs) > > clear_step_over_info (); > insert_hp_step_resume_breakpoint_at_frame (frame); > - ecs->event_thread->step_after_step_resume_breakpoint = 1; > + > + /* If we're still waiting for the final fork/vfork SIGTRAP and we > + got interrupted by a random signal, don't step further after > + returning from the signal handler. */ > + if (current_inferior ()->waiting_for_fork_sigtrap) > + ecs->event_thread->step_after_step_resume_breakpoint = 0; > + else > + ecs->event_thread->step_after_step_resume_breakpoint = 1; > + Ditto. > /* Reset trap_expected to ensure breakpoints are re-inserted. */ > ecs->event_thread->control.trap_expected = 0; > keep_going (ecs); > @@ -6691,6 +6713,11 @@ process_event_stop_test (struct execution_control_state *ecs) > one instruction. */ > if (debug_infrun) > fprintf_unfiltered (gdb_stdlog, "infrun: stepi/nexti\n"); > + > + /* If we were waiting for a final fork/vfork SIGTRAP after a > + single-step, this should be it. Clear the flag. */ > + if (current_inferior ()->waiting_for_fork_sigtrap) > + current_inferior ()->waiting_for_fork_sigtrap = false; > end_stepping_range (ecs); > return; > } > -- > 2.17.1 >
On 1/9/20 12:21 PM, Alan Hayward wrote: > > >> On 30 Dec 2019, at 16:24, Luis Machado <luis.machado@linaro.org> wrote: >> >> I've been chasing/investigating this particular bug for a while now, and this >> is a tentative patch to fix the problem. >> >> Although this may not be aarch64-specific, i can reproduce it consitently in >> this particular architecture. >> >> In summary, this bug is a mix of random signal handling (SIGCHLD) and >> fork/vfork handling when single-stepping. >> >> Forks and vforks are handled in 3 to 4 steps. >> >> Fork >> >> 1 - We get a PTRACE_EVENT_FORK event. >> 2 - We single-step the inferior to get a SIGTRAP. >> 3 - Inferior is ready to be resumed as we wish. >> >> Vfork >> >> 1 - We get a PTRACE_EVENT_VFORK event. >> 2 - We continue the inferior to get a PTRACE_EVENT_VFORK_DONE event. >> 3 - We single-step the inferior to get a SIGTRAP. >> 4 - Inferior is ready to be resumed as we wish. >> >> The problem manifests itself when we are sitting at a syscall instruction and >> we try to instruction-step past it. There may or may not be a breakpoint >> inserted at the syscall instruction location. >> >> The expected outcome is that we step a single instruction and the resulting PC >> points at the next instruction ($pc + 4 in aarch64). >> >> What i see is that we end up in $pc + 8, that is, one instruction further than >> we should've been. >> >> This happens because, when forking/vforking, the child exits too quickly (in the >> particular case of gdb.base/step-over-syscall.exp) and a SIGCHLD signal is >> issued to the parent. Sometimes this signal arrives before GDB is done handling >> the fork/vfork situation. Usually in step 2 for fork and step 3 for vfork. >> >> In turn, this causes GDB to handle that SIGCHLD as a random signal that must be >> passed to the inferior, which is correct. But this particular code in infrun.c >> doesn't cope with this particular situation and GDB inserts a HP step-resume >> breakpoint and registers its wish for the inferior to be stepped yet again. >> >> Thus we end up in $pc + 8, even though we shouldn't have stepped again in this >> particular situation. > > I’ve tried debugging this issue in the past, and didn’t figure out the issue. > This _sounds_ like a reasonable explanation of why. > >> >> The delivery of SIGCHLD in between fork/vfork handling steps is sensitive to >> timing. If we tweak things a little, enable debugging output or do any other >> thing that affects the timing, we may not see this behavior. >> >> This bug doesn't show up as failures in gdb.base/step-over-syscall.exp due to >> the way the test is written. As long as the PC is the same from the previous >> test to the next, GDB thinks it is good. The proposed patch doesn't cause a >> change in the number of PASSes/FAIL's. >> > > Is it possible to add an explicit test to test for the issue? (possibly as a new > .exp file). I'll have to think about that. The timing-sensitive nature of this problem is within the kernel. The kernel lets the child process run and then notifies the parent. I can delay the child, but i can't delay the kernel notifying the parent. If i could defer the kernel notification to the parent enough so the child exits meanwhile, that would work. > > >> The proposed patch adds a variable waiting_for_fork_sigtrap that gets set just >> before the last step of fork/vfork handling and gets reset when we end up >> seeing that SIGTRAP we wanted. >> >> This variable is used in the random_signal handling of handle_signal_stop, >> where there are a couple conditional blocks that handle signals reaching the >> inferior at an unexpected time. If waiting_for_fork_sigtrap is set, we don't >> set GDB to do the additional single-step it wants to do. >> >> I gave this a thought and tried to find a better place to put the check in, >> but this seemed the most appropriate place to me. >> >> I also tried to solve this without having to introduce a new variable. >> But it seems we can't distinguish between a random signal reaching GDB in >> the middle of single-stepping and a random signal reaching GDB in the middle >> of single-stepping AND handling fork/vfork. >> >> Thoughts? >> >> gdb/ChangeLog: >> >> 2019-12-30 Luis Machado <luis.machado@linaro.org> >> >> * inferior.h (class inferior) <waiting_for_fork_sigtrap>: New member. >> Set to false by default. >> * infrun.c (handle_inferior_event): Set waiting_for_fork_sigtrap when >> a fork or vfork_done is detected. >> (handle_signal_stop): Don't step again if waiting_for_fork_sigtrap is >> true. >> Reset waiting_for_fork_sigtrap to false when nexti/stepi is detected. >> >> Change-Id: I2849e0dc49ad9c0a026daa8ced4610aa0ddbe637 >> --- >> gdb/inferior.h | 12 ++++++++++++ >> gdb/infrun.c | 31 +++++++++++++++++++++++++++++-- >> 2 files changed, 41 insertions(+), 2 deletions(-) >> >> diff --git a/gdb/inferior.h b/gdb/inferior.h >> index 3bd9e8c3d7..2c23d7302b 100644 >> --- a/gdb/inferior.h >> +++ b/gdb/inferior.h >> @@ -447,6 +447,18 @@ public: >> exits or execs. */ >> bool pending_detach = false; >> >> + /* True if we are still waiting for the final SIGTRAP when handling >> + fork or vfork events. This is used in case a signal gets caught amidst >> + handling fork and vfork events. >> + >> + For fork events we first get a PTRACE_EVENT_FORK, then SINGLESTEP the >> + process and then get a SIGTRAP. >> + >> + For vfork events we first get a PTRACE_EVENT_VFORK, then we CONTINUE >> + the process, get a PTRACE_EVENT_VFORK_DONE event, SINGLESTEP the process >> + again and then get a SIGTRAP. */ >> + bool waiting_for_fork_sigtrap = false; >> + > > > What happens with a multithreaded process that forks/vforks? Should this bool be > held per thread instead? (A test would be nice to have here). > > I'll have to try that. Unlike waiting for vfork-done, where the parent can't move until the child is done with the shared memory region, waiting for this final SIGTRAP doesn't have this particular restriction. Maybe Pedro has a better insight into this particular case. >> /* True if this inferior is a vfork parent waiting for a vfork child >> not under our control to be done with the shared memory region, >> either by exiting or execing. */ >> diff --git a/gdb/infrun.c b/gdb/infrun.c >> index 7ddd21dd09..c3316a891a 100644 >> --- a/gdb/infrun.c >> +++ b/gdb/infrun.c >> @@ -4937,6 +4937,10 @@ Cannot fill $_exitsignal with the correct signal number.\n")); >> struct regcache *regcache = get_thread_regcache (ecs->event_thread); >> struct gdbarch *gdbarch = regcache->arch (); >> >> + if (ecs->ws.kind == TARGET_WAITKIND_FORKED) >> + /* We should see a SIGTRAP next time we stop. */ >> + current_inferior ()->waiting_for_fork_sigtrap = true; >> + >> /* If checking displaced stepping is supported, and thread >> ecs->ptid is displaced stepping. */ >> if (displaced_step_in_progress_thread (ecs->event_thread)) >> @@ -5094,6 +5098,8 @@ Cannot fill $_exitsignal with the correct signal number.\n")); >> >> context_switch (ecs); >> >> + /* We should see a SIGTRAP next time we stop. */ >> + current_inferior ()->waiting_for_fork_sigtrap = true; >> current_inferior ()->waiting_for_vfork_done = 0; >> current_inferior ()->pspace->breakpoints_not_allowed = 0; >> >> @@ -5912,7 +5918,15 @@ handle_signal_stop (struct execution_control_state *ecs) >> "breakpoint\n"); >> >> insert_hp_step_resume_breakpoint_at_frame (frame); >> - ecs->event_thread->step_after_step_resume_breakpoint = 1; >> + >> + /* If we're still waiting for the final fork/vfork SIGTRAP and we >> + got interrupted by a random signal, don't step further after >> + returning from the signal handler. */ >> + if (current_inferior ()->waiting_for_fork_sigtrap) >> + ecs->event_thread->step_after_step_resume_breakpoint = 0; >> + else >> + ecs->event_thread->step_after_step_resume_breakpoint = 1; >> + > > Simpler to write as this? > ecs->event_thread->step_after_step_resume_breakpoint = !current_inferior ()->waiting_for_fork_sigtrap; > > I'll change it. >> /* Reset trap_expected to ensure breakpoints are re-inserted. */ >> ecs->event_thread->control.trap_expected = 0; >> >> @@ -5947,7 +5961,15 @@ handle_signal_stop (struct execution_control_state *ecs) >> >> clear_step_over_info (); >> insert_hp_step_resume_breakpoint_at_frame (frame); >> - ecs->event_thread->step_after_step_resume_breakpoint = 1; >> + >> + /* If we're still waiting for the final fork/vfork SIGTRAP and we >> + got interrupted by a random signal, don't step further after >> + returning from the signal handler. */ >> + if (current_inferior ()->waiting_for_fork_sigtrap) >> + ecs->event_thread->step_after_step_resume_breakpoint = 0; >> + else >> + ecs->event_thread->step_after_step_resume_breakpoint = 1; >> + > > Ditto. > I'll change it as well.
diff --git a/gdb/inferior.h b/gdb/inferior.h index 3bd9e8c3d7..2c23d7302b 100644 --- a/gdb/inferior.h +++ b/gdb/inferior.h @@ -447,6 +447,18 @@ public: exits or execs. */ bool pending_detach = false; + /* True if we are still waiting for the final SIGTRAP when handling + fork or vfork events. This is used in case a signal gets caught amidst + handling fork and vfork events. + + For fork events we first get a PTRACE_EVENT_FORK, then SINGLESTEP the + process and then get a SIGTRAP. + + For vfork events we first get a PTRACE_EVENT_VFORK, then we CONTINUE + the process, get a PTRACE_EVENT_VFORK_DONE event, SINGLESTEP the process + again and then get a SIGTRAP. */ + bool waiting_for_fork_sigtrap = false; + /* True if this inferior is a vfork parent waiting for a vfork child not under our control to be done with the shared memory region, either by exiting or execing. */ diff --git a/gdb/infrun.c b/gdb/infrun.c index 7ddd21dd09..c3316a891a 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -4937,6 +4937,10 @@ Cannot fill $_exitsignal with the correct signal number.\n")); struct regcache *regcache = get_thread_regcache (ecs->event_thread); struct gdbarch *gdbarch = regcache->arch (); + if (ecs->ws.kind == TARGET_WAITKIND_FORKED) + /* We should see a SIGTRAP next time we stop. */ + current_inferior ()->waiting_for_fork_sigtrap = true; + /* If checking displaced stepping is supported, and thread ecs->ptid is displaced stepping. */ if (displaced_step_in_progress_thread (ecs->event_thread)) @@ -5094,6 +5098,8 @@ Cannot fill $_exitsignal with the correct signal number.\n")); context_switch (ecs); + /* We should see a SIGTRAP next time we stop. */ + current_inferior ()->waiting_for_fork_sigtrap = true; current_inferior ()->waiting_for_vfork_done = 0; current_inferior ()->pspace->breakpoints_not_allowed = 0; @@ -5912,7 +5918,15 @@ handle_signal_stop (struct execution_control_state *ecs) "breakpoint\n"); insert_hp_step_resume_breakpoint_at_frame (frame); - ecs->event_thread->step_after_step_resume_breakpoint = 1; + + /* If we're still waiting for the final fork/vfork SIGTRAP and we + got interrupted by a random signal, don't step further after + returning from the signal handler. */ + if (current_inferior ()->waiting_for_fork_sigtrap) + ecs->event_thread->step_after_step_resume_breakpoint = 0; + else + ecs->event_thread->step_after_step_resume_breakpoint = 1; + /* Reset trap_expected to ensure breakpoints are re-inserted. */ ecs->event_thread->control.trap_expected = 0; @@ -5947,7 +5961,15 @@ handle_signal_stop (struct execution_control_state *ecs) clear_step_over_info (); insert_hp_step_resume_breakpoint_at_frame (frame); - ecs->event_thread->step_after_step_resume_breakpoint = 1; + + /* If we're still waiting for the final fork/vfork SIGTRAP and we + got interrupted by a random signal, don't step further after + returning from the signal handler. */ + if (current_inferior ()->waiting_for_fork_sigtrap) + ecs->event_thread->step_after_step_resume_breakpoint = 0; + else + ecs->event_thread->step_after_step_resume_breakpoint = 1; + /* Reset trap_expected to ensure breakpoints are re-inserted. */ ecs->event_thread->control.trap_expected = 0; keep_going (ecs); @@ -6691,6 +6713,11 @@ process_event_stop_test (struct execution_control_state *ecs) one instruction. */ if (debug_infrun) fprintf_unfiltered (gdb_stdlog, "infrun: stepi/nexti\n"); + + /* If we were waiting for a final fork/vfork SIGTRAP after a + single-step, this should be it. Clear the flag. */ + if (current_inferior ()->waiting_for_fork_sigtrap) + current_inferior ()->waiting_for_fork_sigtrap = false; end_stepping_range (ecs); return; }