Message ID | 1553461783.1504.3.camel@skynet.be |
---|---|
State | New, archived |
Headers |
Received: (qmail 6002 invoked by alias); 24 Mar 2019 21:09:48 -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 5991 invoked by uid 89); 24 Mar 2019 21:09:48 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-24.2 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.1 spammy=HContent-Transfer-Encoding:8bit X-HELO: mailsec105.isp.belgacom.be Received: from mailsec105.isp.belgacom.be (HELO mailsec105.isp.belgacom.be) (195.238.20.101) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sun, 24 Mar 2019 21:09:45 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=skynet.be; i=@skynet.be; q=dns/txt; s=securemail; t=1553461785; x=1584997785; h=message-id:subject:from:to:date:in-reply-to:references: mime-version:content-transfer-encoding; bh=9k+051UfDMuhAJmMipl2Gd4fJjkVGgd+0RhtkkuymMw=; b=sJ/febhC+wPax+EdoG7PxmBl7xaFz7XLwwahCOFgWDwCz9fvfLfLA03S foCepRD6PHUrG9qEJKf4CWk1Cu7ihA==; Received: from 147.122-130-109.adsl-dyn.isp.belgacom.be (HELO md) ([109.130.122.147]) by relay.skynet.be with ESMTP/TLS/AES256-GCM-SHA384; 24 Mar 2019 22:09:43 +0100 Message-ID: <1553461783.1504.3.camel@skynet.be> Subject: Re: [RFA] Fix internal error with 'set debug infrun 1' under high load From: Philippe Waroquiers <philippe.waroquiers@skynet.be> To: Kevin Buettner <kevinb@redhat.com>, gdb-patches@sourceware.org Date: Sun, 24 Mar 2019 22:09:43 +0100 In-Reply-To: <20190324135043.043ef0d9@f29-4.lan> References: <20190324142505.27486-1-philippe.waroquiers@skynet.be> <20190324135043.043ef0d9@f29-4.lan> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 8bit X-IsSubscribed: yes |
Commit Message
Philippe Waroquiers
March 24, 2019, 9:09 p.m. UTC
On Sun, 2019-03-24 at 13:50 -0700, Kevin Buettner wrote: > Hi Philippe, > > There is definitely a bug in this section of code from infrun.c: > > else if (ws.kind == TARGET_WAITKIND_THREAD_EXITED > || ws.kind == TARGET_WAITKIND_EXITED > || ws.kind == TARGET_WAITKIND_SIGNALLED) > { > if (debug_infrun) > { > ptid_t ptid = ptid_t (ws.value.integer); > > fprintf_unfiltered (gdb_stdlog, > "infrun: %s exited while " > "stopping threads\n", > target_pid_to_str (ptid).c_str ()); > } > } > > This line... > > ptid_t ptid = ptid_t (ws.value.integer); > > ...doesn't make sense to me since ws.value.integer is supposed to > be the exit status for TARGET_WAITKIND_THREAD_EXITED and > TARGET_WAITKIND_EXITED. > > However, for TARGET_WAITKIND_SIGNALLED, the signal number is in > ws.value.sig (which, due to being part of a union occupies some > of the same bytes as ws.value.integer). > > So trying to find the ptid in that manner makes no sense at all. > > I'm guessing that the ptid values are bogus when it does work. > > Does it work when you use > > ptid_t ptid = ptid_t (event_pid); > > instead? I guess you mean to only print event_ptid. Yes, that is working (the proposed patch was printing both event_ptid and the ptid derived from ws.value.integer, assuming that sometimes ws.value.integer was something relevant). Here is the trace I obtain after a few trials under high load: infrun: stop_all_threads, pass=0, iterations=0 infrun: Thread 0x7ffff7fcfb40 (LWP 3587) not executing infrun: Thread 0x7ffff7310700 (LWP 3632) executing, need stop [Thread 0x7ffff7310700 (LWP 3632) exited] infrun: target_wait (-1.0.0, status) = infrun: 3587.3632.0 [LWP 3632], infrun: status->kind = thread exited, status = 0 infrun: LWP 3632 exited while stopping threads infrun: Thread 0x7ffff7fcfb40 (LWP 3587) not executing infrun: stop_all_threads, pass=1, iterations=1 infrun: Thread 0x7ffff7fcfb40 (LWP 3587) not executing infrun: stop_all_threads done The above is obtained with the patch:
Comments
On Sun, 24 Mar 2019 22:09:43 +0100 Philippe Waroquiers <philippe.waroquiers@skynet.be> wrote: > On Sun, 2019-03-24 at 13:50 -0700, Kevin Buettner wrote: > > Hi Philippe, > > > > There is definitely a bug in this section of code from infrun.c: > > > > else if (ws.kind == TARGET_WAITKIND_THREAD_EXITED > > || ws.kind == TARGET_WAITKIND_EXITED > > || ws.kind == TARGET_WAITKIND_SIGNALLED) > > { > > if (debug_infrun) > > { > > ptid_t ptid = ptid_t (ws.value.integer); > > > > fprintf_unfiltered (gdb_stdlog, > > "infrun: %s exited while " > > "stopping threads\n", > > target_pid_to_str (ptid).c_str ()); > > } > > } > > > > This line... > > > > ptid_t ptid = ptid_t (ws.value.integer); > > > > ...doesn't make sense to me since ws.value.integer is supposed to > > be the exit status for TARGET_WAITKIND_THREAD_EXITED and > > TARGET_WAITKIND_EXITED. > > > > However, for TARGET_WAITKIND_SIGNALLED, the signal number is in > > ws.value.sig (which, due to being part of a union occupies some > > of the same bytes as ws.value.integer). > > > > So trying to find the ptid in that manner makes no sense at all. > > > > I'm guessing that the ptid values are bogus when it does work. > > > > Does it work when you use > > > > ptid_t ptid = ptid_t (event_pid); > > > > instead? > I guess you mean to only print event_ptid. > > Yes, that is working (the proposed patch was printing both > event_ptid and the ptid derived from ws.value.integer, assuming > that sometimes ws.value.integer was something relevant). > > Here is the trace I obtain after a few trials under high load: > infrun: stop_all_threads, pass=0, iterations=0 > infrun: Thread 0x7ffff7fcfb40 (LWP 3587) not executing > infrun: Thread 0x7ffff7310700 (LWP 3632) executing, need stop > [Thread 0x7ffff7310700 (LWP 3632) exited] > infrun: target_wait (-1.0.0, status) = > infrun: 3587.3632.0 [LWP 3632], > infrun: status->kind = thread exited, status = 0 > infrun: LWP 3632 exited while stopping threads > infrun: Thread 0x7ffff7fcfb40 (LWP 3587) not executing > infrun: stop_all_threads, pass=1, iterations=1 > infrun: Thread 0x7ffff7fcfb40 (LWP 3587) not executing > infrun: stop_all_threads done > > The above is obtained with the patch: > > diff --git a/gdb/infrun.c b/gdb/infrun.c > index ad7892105a..7f1339a917 100644 > --- a/gdb/infrun.c > +++ b/gdb/infrun.c > @@ -4365,12 +4365,10 @@ stop_all_threads (void) > { > if (debug_infrun) > { > - ptid_t ptid = ptid_t (ws.value.integer); > - > fprintf_unfiltered (gdb_stdlog, > "infrun: %s exited while " > "stopping threads\n", > - target_pid_to_str (ptid).c_str ()); > + target_pid_to_str (event_ptid).c_str ()); > } > } > else > You make a good point about trying to make use of ws.value.integer. So, here are my suggestions: 1) Move TARGET_WAITKIND_SIGNALLED into another "else if" clause. It doesn't make sense for the debug message to indicate that the process has exited when it's actually been signalled. 2) Make the TARGET_WAITKIND_THREAD_EXITED / TARGET_WAITKIND_EXITED case print the exit status and make the TARGET_WAITKIND_SIGNALLED case print the signal. These are available (respectively) in ws.value.integer and ws.value.sig. Kevin
On Sun, 2019-03-24 at 14:35 -0700, Kevin Buettner wrote: > You make a good point about trying to make use of ws.value.integer. > > So, here are my suggestions: > > 1) Move TARGET_WAITKIND_SIGNALLED into another "else if" clause. It > doesn't make sense for the debug message to indicate that the process > has exited when it's actually been signalled. > > 2) Make the TARGET_WAITKIND_THREAD_EXITED / TARGET_WAITKIND_EXITED > case print the exit status and make the TARGET_WAITKIND_SIGNALLED case > print the signal. These are available (respectively) in ws.value.integer and > ws.value.sig. Thanks for the feedback, I will (some time this week) prepare a patch based on these suggestions. Philippe
diff --git a/gdb/infrun.c b/gdb/infrun.c index ad7892105a..7f1339a917 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -4365,12 +4365,10 @@ stop_all_threads (void) Â Â Â Â Â Â Â Â Â Â Â Â { Â Â Â Â Â Â Â Â Â Â Â Â Â Â if (debug_infrun) Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â { -Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ptid_t ptid = ptid_t (ws.value.integer); - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â fprintf_unfiltered (gdb_stdlog, Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â "infrun: %s exited while " Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â "stopping threads\n", -Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â target_pid_to_str (ptid).c_str ()); +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â target_pid_to_str (event_ptid).c_str ()); Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â } Â Â Â Â Â Â Â Â Â Â Â Â } Â Â Â Â Â Â Â Â Â Â else