From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 56875 invoked by alias); 18 Feb 2020 21:12:22 -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 56867 invoked by uid 89); 18 Feb 2020 21:12:21 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-6.4 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.1 spammy=HX-Languages-Length:2528 X-HELO: smtp.polymtl.ca Received: from smtp.polymtl.ca (HELO smtp.polymtl.ca) (132.207.4.11) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 18 Feb 2020 21:12:20 +0000 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 01ILC5w7011844 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 18 Feb 2020 16:12:10 -0500 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 01ILC5w7011844 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=polymtl.ca; s=default; t=1582060331; bh=xMq/dZAQ8DBRa5PzIAA55fW7OnwGFZpukI6eNeAL5i0=; h=Subject:To:Cc:References:From:Date:In-Reply-To:From; b=ORuCEnB9v3WHlr9zqiUFJTqizsfWxi+xBSMLwpK4ov5ZMY5hbU83VSDCdiGf2Z2zt QVmG1Szf8ke/z7dX0z5VD0uBjBDX+nuZ7K0KdpT6OK4PzC3OQmmGJa7jlwb2oCKf8N vWRa0PBaIe9k2J6Gc7j7LEWYwqFUbw+n5RB8OBi8= Received: from [172.16.0.95] (192-222-181-218.qc.cable.ebox.net [192.222.181.218]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 566581E5FA; Tue, 18 Feb 2020 16:12:05 -0500 (EST) Subject: Re: [PATCH v3] Pass thread_info pointer to various inferior control functions To: Tom Tromey , Simon Marchi Cc: gdb-patches@sourceware.org References: <20200213230428.14476-1-simon.marchi@efficios.com> <87o8tv62jk.fsf@tromey.com> From: Simon Marchi Message-ID: <633e0b37-afb6-cd56-e9de-d6047987210f@polymtl.ca> Date: Tue, 18 Feb 2020 21:12:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.4.1 MIME-Version: 1.0 In-Reply-To: <87o8tv62jk.fsf@tromey.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2020-02/txt/msg00746.txt.bz2 On 2020-02-18 3:52 p.m., Tom Tromey wrote: >>>>>> "Simon" == Simon Marchi writes: > > Simon> I noticed that some functions in infcmd and infrun call each other and > Simon> all call inferior_thread, while they could just get the thread_info > Simon> pointer from their caller. That means less calls to inferior_thread, so > Simon> less reliance on global state, since inferior_thread reads > Simon> inferior_ptid. > > ... > > Simon> -set_step_frame (void) > Simon> +set_step_frame (thread_info *tp) > Simon> { > Simon> frame_info *frame = get_current_frame (); > > I like the idea of passing parameters rather than relying on global > state. However, in its current form, I think this patch may lull the > reader into a false sense of security. > > That is, it makes it seems like these functions operate on a thread that > you pass in. However, they don't actually. For example, in this one, I > think get_current_frame must be relying on the global inferior_ptid. > So, this would change the function from something that obviously has to > be relying on globals to something that un-obviously is. I agree. > I don't know whether that's a reason to reject it or not. > It just seemed like maybe a future source of bugs. > > I do welcome changes to reduce our reliance on globals. > Perhaps there's an argument that we can only achieve this incrementally > like this. That's what I think, we have to start somewhere, and for some time be in this weird state where we pass in things by parameter but also rely on globals. Perhaps a good way would be to add this parameter, but also assert at the entry of the function that the passed in parameter matches the global (of course, with a comment that says why). When we are convinced the code does not rely on globals anymore, we can remove the assertion. There's a precedent for such a thing here (altough I added it, so it might not count): https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gdb/target.c;h=470ef51d69ef5d712fc51a54f426c1cd3b79c977;hb=HEAD#l2020 So here, I would add this at the beginning of step_step_info, set_step_frame and prepare_one_step: /* This can be removed once this function does not rely on global state anymore. */ gdb_assert (inferior_ptid == tp->ptid); >Though, the follow-on question there is whether this is > something that will actually happen. I want to believe. > > Let me know what you think of this. Simon