From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.polymtl.ca (smtp.polymtl.ca [132.207.4.11]) by sourceware.org (Postfix) with ESMTPS id 3A3493851C14 for ; Mon, 19 Jul 2021 13:36:45 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 3A3493851C14 Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id 16JDaYcQ011437 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 19 Jul 2021 09:36:39 -0400 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 16JDaYcQ011437 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 18E0C1E01B; Mon, 19 Jul 2021 09:36:33 -0400 (EDT) Subject: Re: [PATCH] gdb: set current thread in btrace_compute_ftrace_1 To: "Metzger, Markus T" , Simon Marchi Cc: "gdb-patches@sourceware.org" References: <20210714213744.490264-1-simon.marchi@efficios.com> <98c4a011-426c-e003-35b0-1c3ce04790b5@polymtl.ca> <34077d8e-f2fa-7c67-ff2b-9ad68aa371f4@polymtl.ca> From: Simon Marchi Message-ID: Date: Mon, 19 Jul 2021 09:36:33 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.12.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Mon, 19 Jul 2021 13:36:34 +0000 X-Spam-Status: No, score=-9.8 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_NUMSUBJECT, NICE_REPLY_A, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) 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: Mon, 19 Jul 2021 13:36:47 -0000 > Hello Simon, > >>> It is indeed confusing who is responsible for switching / restoring the >>> current thread. After talking to Pedro about this problem a few times, >>> I think a guideline to make progressive changes could be: if a function >>> A (such as btrace_compute_ftrace_bts) takes a thread pointer as >>> parameter (or inferior, or frame), it's because it advertises that it >>> will be working on that thread and does not require that thread to be >>> the current one. If it did require the current thread to be the same >>> thread it receives as argument, then why have the parameter at all? So > > Because I don't like global state and didn't want to add to the problem. Sorry if that wasn't clear, that wasn't a "why did Markus do it like this" question but just part of the reasoning of why a function shouldn't use both a parameter _and_ rely on the global context. >>> if function A (who doesn't care about the global context) needs to call >>> a function B (like gdb_insn_length in our example) that relies on the >>> current global thread, then it needs to set the current thread to >>> prepare the call B. It also needs to restore the current thread that >>> was there on entry, because it could happen to be called from a function >>> that cares about the current thread. And that caller wouldn't expect >>> a function that doesn't care about the global context to modify the >>> global context. > > OK, that makes sense. Those switch-to would go away once we got rid of > global state so we'd want to mark them somehow. Exact. I guess we could imagine a system of attributes to mark functions as using the global context and have some static analysis making sure that when a function not using the global context calls one using the global context, a function setting the global context is called before. But, that's probably not realistic. > It would really be nice to put them right in front of the call that required it. > But that would mean a lot of switching back and forth. Indeed. I initially put the switch in the narrowest scope possible, that is in the try around the gdb_insn_length call. But then some other function call later also needed the current thread to be set. I think in this case it's reasonable to have just one switch for the whole function. >>From b153656b2b8890ab67ed697da64c8fc6fbfde03f Mon Sep 17 00:00:00 2001 >> From: Simon Marchi >> Date: Wed, 14 Jul 2021 16:31:09 -0400 >> Subject: [PATCH v2] gdb: set current thread in btrace_compute_ftrace_1 >> >> As documented in bug 28086, test gdb.btrace/enable-new-thread.exp >> started failing with commit 0618ae414979 ("gdb: optimize >> all_matching_threads_iterator"): >> >> (gdb) record btrace^M >> (gdb) PASS: gdb.btrace/enable-new-thread.exp: record btrace >> break 24^M >> Breakpoint 2 at 0x555555555175: file /home/smarchi/src/binutils- >> gdb/gdb/testsuite/gdb.btrace/enable-new-thread.c, line 24.^M >> (gdb) continue^M >> Continuing.^M >> /home/smarchi/src/binutils-gdb/gdb/inferior.c:303: internal-error: inferior* >> find_inferior_pid(process_stratum_target*, int): Assertion `pid != 0' failed.^M >> A problem internal to GDB has been detected,^M >> further debugging may prove unreliable.^M >> Quit this debugging session? (y or n) FAIL: gdb.btrace/enable-new-thread.exp: >> continue to breakpoint: cont to bp.1 (GDB internal error) >> >> Note that I only see the failure if GDB is compiled without libipt >> support. I presume that this makes it use BTS instead of PT, so >> exercises different code paths. > > Only BTS calls gdb_insn_length (). But, as you point out below, PT passes a > read-memory callback to the decoder. I wonder why we are not seeing a > similar problem there? > > static int > btrace_pt_readmem_callback (gdb_byte *buffer, size_t size, > const struct pt_asid *asid, uint64_t pc, > void *context) > { > int result, errcode; > > result = (int) size; > try > { > errcode = target_read_code ((CORE_ADDR) pc, buffer, size); > if (errcode != 0) > result = -pte_nomap; > } > catch (const gdb_exception_error &error) > { > result = -pte_nomap; > } > > return result; > } > > We turn the exception into a decoder error code. In the corresponding > test, gdb.btrace/enable-new-thread.exp, we only check that we are recording; > we are not looking at the actual trace. Good point. I didn't have much time to dig into why PT didn't fail, but it sounds like the test could be improved. Would you like to improve the test in such a way that it would have caught the issue with PT? >> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28086 >> Change-Id: I407fbfe41aab990068bd102491aa3709b0a034b3 >> --- >> gdb/btrace.c | 12 ++++++++++++ >> 1 file changed, 12 insertions(+) >> >> diff --git a/gdb/btrace.c b/gdb/btrace.c >> index 5e689c11d4bd..6fec8103d4ce 100644 >> --- a/gdb/btrace.c >> +++ b/gdb/btrace.c >> @@ -1052,6 +1052,12 @@ btrace_compute_ftrace_bts (struct thread_info *tp, >> const struct btrace_data_bts *btrace, >> std::vector &gaps) >> { >> + /* btrace_compute_ftrace_bts may end up doing target calls that require the >> + current thread to be TP, for example reading memory through >> gdb_insn_length. >> + Make sure TP is the current thread. */ >> + scoped_restore_current_thread restore_thread; >> + switch_to_thread (tp); >> + >> struct btrace_thread_info *btinfo; >> struct gdbarch *gdbarch; >> unsigned int blk; >> @@ -1427,6 +1433,12 @@ btrace_compute_ftrace_pt (struct thread_info *tp, >> const struct btrace_data_pt *btrace, >> std::vector &gaps) >> { >> + /* btrace_compute_ftrace_bts may end up doing target calls that require the > > The function name is no longer valid in this copy. Maybe just say 'We may end up ...'? Sorry, copy-pasto. You're right I don't need to name the function in the comment. That came from my previous comment that was in btrace_compute_ftrace_1. >> + current thread to be TP, for example reading memory through >> + btrace_pt_readmem_callback. Make sure TP is the current thread. */ >> + scoped_restore_current_thread restore_thread; >> + switch_to_thread (tp); >> + >> struct btrace_thread_info *btinfo; >> struct pt_insn_decoder *decoder; >> struct pt_config config; >> -- >> 2.32.0 > > LGTM. Thanks, I'll commit it with the comments adjusted. Simon