From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id 0D4613858C83 for ; Tue, 19 Jul 2022 18:51:04 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 0D4613858C83 Received: from [10.0.0.11] (192-222-157-6.qc.cable.ebox.net [192.222.157.6]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 4F5D31E222; Tue, 19 Jul 2022 14:51:03 -0400 (EDT) Message-ID: <44ad453e-6196-d334-312f-d5d0414f4476@simark.ca> Date: Tue, 19 Jul 2022 14:51:02 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.11.0 Subject: Re: [PATCH] Enable multi process debugging for AIX Content-Language: en-US To: Aditya Kamath1 , Ulrich Weigand , Aditya Kamath1 via Gdb-patches Cc: Sangamesh Mallayya References: From: Simon Marchi In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-11.2 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 19 Jul 2022 18:51:09 -0000 > From 1eab0bb05b1db09dfb98d31cc630722b40c081c4 Mon Sep 17 00:00:00 2001 > From: Aditya Vidyadhar Kamath > Date: Tue, 19 Jul 2022 10:39:00 -0500 > Subject: [PATCH] Enable multi process debugging for AIX > > The attached proposed patch adds multi process debugging feature in AIX. > > Till now AIX supported debugging only one inferior at a time, > > now we can be able to debug multi process. > > Users can use set follow fork mode in child or parent and > > set detach on fork on or off to enable/disable simultaneous debugging of parent/child. There are a lot of unclear things about how AIX reports a fork event to the debugger. Is there some documentation on that somewhere? > --- > gdb/aix-thread.c | 4 ++++ > gdb/rs6000-aix-nat.c | 48 +++++++++++++++++++++++++++++++++++++++++--- > 2 files changed, 49 insertions(+), 3 deletions(-) > > diff --git a/gdb/aix-thread.c b/gdb/aix-thread.c > index d47f5132592..f66b5904ae8 100644 > --- a/gdb/aix-thread.c > +++ b/gdb/aix-thread.c > @@ -1088,6 +1088,10 @@ aix_thread_target::wait (ptid_t ptid, struct target_waitstatus *status, > pid-only ptids. */ > gdb_assert (ptid.is_pid ()); > > + /* In pd_activate to get PTHB_SUCCESS in pthread debug session init > + we need inferior_ptid set to update multiple threads. */ > + inferior_ptid = ptid; I would really prefer to avoid just setting inferior_ptid and hope things work magically, this is a step backwards. If some functions rely on the value of inferior_ptid on entry, change them so they don't rely on it anymore. > + > /* Check whether libpthdebug might be ready to be initialized. */ > if (!pd_active && status->kind () == TARGET_WAITKIND_STOPPED > && status->sig () == GDB_SIGNAL_TRAP) > diff --git a/gdb/rs6000-aix-nat.c b/gdb/rs6000-aix-nat.c > index f604f7d503e..55844ca6dab 100644 > --- a/gdb/rs6000-aix-nat.c > +++ b/gdb/rs6000-aix-nat.c > @@ -91,10 +91,16 @@ class rs6000_nat_target final : public inf_ptrace_target > > ptid_t wait (ptid_t, struct target_waitstatus *, target_wait_flags) override; > > + /* Fork detection related functions, For adding multi process debugging > + support. */ > + void follow_fork (inferior *, ptid_t, target_waitkind, bool, bool) override; > + > + void mourn_inferior () override; > + > protected: > > - void post_startup_inferior (ptid_t ptid) override > - { /* Nothing. */ } > + void post_startup_inferior (ptid_t ptid) override; > + //{ /* Nothing. */ } > > private: > enum target_xfer_status > @@ -246,6 +252,22 @@ fetch_register (struct regcache *regcache, int regno) > } > } > > +void rs6000_nat_target::post_startup_inferior(ptid_t ptid){ > + rs6000_ptrace64(PT_MULTI,ptid.pid(),NULL,1,NULL); > +} > + > +void > +rs6000_nat_target::follow_fork (inferior *child_inf, ptid_t child_ptid, > + target_waitkind fork_kind, bool follow_child, > + bool detach_fork) > +{ > + inf_ptrace_target::follow_fork(child_inf, child_ptid, fork_kind, > + follow_child, detach_fork); > +} > + > +void rs6000_nat_target::mourn_inferior(){ > + inf_ptrace_target::mourn_inferior(); > +} Maybe I'm mistaken, but the two methods above seem unnecessary, since inf_ptrace_target is the direct base of rs6000_nat_target. > /* Store register REGNO back into the inferior. */ > > static void > @@ -539,8 +561,28 @@ rs6000_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus, > if (status == 0x57c) > ourstatus->set_loaded (); > /* signal 0. I have no idea why wait(2) returns with this status word. */ > - else if (status == 0x7f) > + /* 0x17f and 0x137f in hexadecimal are status returned if > + if we follow parent, > + a switch is made to a child post parent execution > + and child continues its execution [user switches to child and > + presses continue]. */ > + else if (status == 0x7f || status == 0x17f || status == 0x137f) > ourstatus->set_spurious (); > + /* When a process completes execution and any fork process exits status. */ > + else if (WIFEXITED(status)) > + ourstatus->set_exited(0); Why is WIFEXITED now handled here? It used to be handled in the host_status_to_waitstatus call, which is still below. Why doesn't that work anymore? Also, why pass a literal 0 as the exit code, can't it be something else? > + /* 57e is the status number in AIX for fork event. > + If a breakpoint is attached to a parent or child then on fork, > + or after fork, once a breakpoint hits and the next or continue is > + pressed, post breakpoint status is 1406 but we need not set status > + to set_forked(), hence the condition find_inferior_pid() to set > + fork status only if a child is born. */ > + else if (status == 0x57e && find_inferior_pid(this,pid)== nullptr) These magic numbers make the code really difficult to follow. Is this really the way AIX intends users to use the status that waitpid returns? Isn't there a set of macros to inspect it? Worst case, I suppose we can make our own, like: #define AIX_EVENT_FORK 0x57e That would already help. > + { > + ourstatus->set_forked (ptid_t(pid)); > + /* On a fork event return parent process ID to target wait */ > + return ptid_t(current_inferior()->pid); You should not use current_inferior() here, any inferior could be the current one on entry. Not necessarily the one that has forked. How does AIX document the events sent to the ptracer in case of a fork? We need to be able to figure out the pid of the parent and the pid of the child before returning the fork event to the core. On some platforms, like FreeBSD, the kernel sends separate events for the parent and the child, so we need a bit of special handling to consume both events: https://gitlab.com/gnutools/binutils-gdb/-/blob/9afca381e2e46ccee433ce09001506e7683b273f/gdb/fbsd-nat.c#L1003-1035 I don't know if that's the case here, but it sounds similar. Simon