From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 98927 invoked by alias); 20 Mar 2017 22:22:43 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 98908 invoked by uid 89); 20 Mar 2017 22:22:42 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-23.8 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_HELO_PASS,SPF_SOFTFAIL autolearn=ham version=3.3.2 spammy= X-HELO: simark.ca Received: from simark.ca (HELO simark.ca) (158.69.221.121) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 20 Mar 2017 22:22:40 +0000 Received: by simark.ca (Postfix, from userid 33) id D8BDD1E81B; Mon, 20 Mar 2017 18:22:39 -0400 (EDT) To: Pedro Alves Subject: Re: [PATCH 3/3] windows: Use ptid from regcache in register fetch/store X-PHP-Originating-Script: 33:rcube.php MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Mon, 20 Mar 2017 22:22:00 -0000 From: Simon Marchi Cc: gdb-patches@sourceware.org, Simon Marchi In-Reply-To: <41e56d7a-3bf6-71d5-6f5e-03c721ee4ff9@redhat.com> References: <20170318170801.22988-1-simon.marchi@polymtl.ca> <20170318170801.22988-3-simon.marchi@polymtl.ca> <41e56d7a-3bf6-71d5-6f5e-03c721ee4ff9@redhat.com> Message-ID: <80d736666fe9a1484f57f8c60a1f64df@polymtl.ca> X-Sender: simon.marchi@polymtl.ca User-Agent: Roundcube Webmail/1.2.4 X-IsSubscribed: yes X-SW-Source: 2017-03/txt/msg00354.txt.bz2 On 2017-03-20 11:56, Pedro Alves wrote: > On 03/18/2017 05:08 PM, Simon Marchi wrote: >> From: Simon Marchi >> >> Use the ptid from the regcache so we don't depend on the current value >> of the inferior_ptid global. >> >> Also, change how the current thread is passed to sub-functions. The >> windows_fetch_inferior_registers function sets current_thread then >> calls >> do_windows_fetch_inferior_registers, which reads current_thread. This >> very much looks like passing a parameter through a global variable. I >> think it would be more straightforward to pass the thread as a >> parameter. >> >> gdb/ChangeLog: >> >> * windows-nat.c (do_windows_fetch_inferior_registers): Add >> windows_thread_info parameter and use it instead of >> current_thread. >> (windows_fetch_inferior_registers): Don't set current_thread, >> pass the thread to do_windows_fetch_inferior_registers. Use >> ptid from regcache instead of inferior_ptid. >> (do_windows_store_inferior_registers): Add windows_thread_info >> parameter and use it instead of current_thread. >> (windows_store_inferior_registers): Don't set current_thread, >> pass the thread to do_windows_store_inferior_registers. Use >> ptid from regcache instead of inferior_ptid. >> --- >> gdb/windows-nat.c | 43 +++++++++++++++++++++---------------------- >> 1 file changed, 21 insertions(+), 22 deletions(-) >> >> diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c >> index 9cc755f0d4..32a9ee62cf 100644 >> --- a/gdb/windows-nat.c >> +++ b/gdb/windows-nat.c >> @@ -460,18 +460,15 @@ windows_delete_thread (ptid_t ptid, DWORD >> exit_code) >> } >> >> static void >> -do_windows_fetch_inferior_registers (struct regcache *regcache, int >> r) >> +do_windows_fetch_inferior_registers (struct regcache *regcache, >> + windows_thread_info *th, int r) >> { >> char *context_offset = ((char *) ¤t_thread->context) + >> mappings[r]; > > Is this reference to "current_thread" still correct? Oops, I guess it should be th, like the rest: char *context_offset = ((char *) th->context) + mappings[r]; Fixed locally. >> @@ -537,25 +533,26 @@ static void >> windows_fetch_inferior_registers (struct target_ops *ops, >> struct regcache *regcache, int r) >> { >> - current_thread = thread_rec (ptid_get_tid (inferior_ptid), TRUE); >> + DWORD pid = ptid_get_tid (regcache_get_ptid (regcache)); >> + windows_thread_info *th = thread_rec (pid, TRUE); >> + >> /* Check if current_thread exists. Windows sometimes uses a >> non-existent >> thread id in its events. */ > > The comment is out of date now. Fixed locally. > Did you look for the history around these comments? I wonder whether > these NULL checks still make sense here if we always reference the > regcache's thread. The equivalent code in gdbserver doesn't seem to > have them. All I know is that this is the patch that introduced them: https://sourceware.org/ml/gdb-patches/2003-12/msg00479.html The PR 1048 seems to refer to a pre-bugzilla bug tracking system. Do we still have them somewhere? From what I understand, it's the use case where you attach to a process whose main thread has already exited. If the patch introduced these NULL checks, I suppose it's because they were necessary back then to work around the Windows bug. I have no idea if they are still necessary, or if the Microsoft people fixed it. In any case, the fact of whether the checks are needed is not impacted by the current patch: in the end, we call thread_rec with the same pid with which we would have called it before, so we should get the same result. I'll wait for your input on this before sending a new version. >> @@ -564,11 +561,13 @@ static void >> windows_store_inferior_registers (struct target_ops *ops, >> struct regcache *regcache, int r) >> { >> - current_thread = thread_rec (ptid_get_tid (inferior_ptid), TRUE); >> + DWORD pid = ptid_get_tid (regcache_get_ptid (regcache)); >> + windows_thread_info *th = thread_rec (pid, TRUE); >> + >> /* Check if current_thread exists. Windows sometimes uses a >> non-existent >> thread id in its events. */ > > Ditto. Fixed locally. Thanks, Simon