From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 13889 invoked by alias); 17 Sep 2014 18:20:16 -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 13879 invoked by uid 89); 17 Sep 2014 18:20:15 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.6 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_LOW,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-vc0-f178.google.com Received: from mail-vc0-f178.google.com (HELO mail-vc0-f178.google.com) (209.85.220.178) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Wed, 17 Sep 2014 18:20:14 +0000 Received: by mail-vc0-f178.google.com with SMTP id hy4so1639288vcb.23 for ; Wed, 17 Sep 2014 11:20:11 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type; bh=IqNB++IgrLMfDf7CUh3kf7c7G+hZxHpygBZoAmHqeHw=; b=CJlxkta/g8uS73aTSaNnjmpczJxsGlXWgZXpHCE8f56OGgEwKnq5iIsvOZL+uj0O7S Bg5zOQyLdq4YURmN5nTIp2cgE7RLLQSnAIeAFLaQE5zxWKoNy+Wi8tF4eJ7ZM8OfkFHp +RDncb2TcqE7v3uFjd8nPilo+fo2ZLWBHsqR8FnX3vCSpgIV9nQUs1NGlOMskbAw0pin osETP1ThF3JfOVSheLcO/fBkpJiQcgq7icK/CJDKX+WhBMGRl5g0LutBXnktLtTHRqpu CAVk5YYeWhoyX8bkQ9bjYJ6qrFn7xFSFqAgPqlVWJr7S9nMo2+qbEbzopbXp+A1sXGFf NexA== X-Gm-Message-State: ALoCoQnTv86yHl2pJHXI28FGhZUonMDa/agTsWhL6ralLtSoPToyTMlVvmon1VRw+equ3jpVwLoM MIME-Version: 1.0 X-Received: by 10.52.33.1 with SMTP id n1mr29722895vdi.28.1410978011814; Wed, 17 Sep 2014 11:20:11 -0700 (PDT) Received: by 10.52.181.65 with HTTP; Wed, 17 Sep 2014 11:20:11 -0700 (PDT) In-Reply-To: <541970D8.3080504@redhat.com> References: <21520.36381.756875.963606@ruffy2.mtv.corp.google.com> <20140911102659.GA17472@blade.nx> <5412DEB5.6020706@redhat.com> <21523.9502.168492.803068@ruffy2.mtv.corp.google.com> <54132B55.9000108@redhat.com> <21523.12189.134570.770432@ruffy2.mtv.corp.google.com> <5413305B.6020402@redhat.com> <21523.13993.986533.615240@ruffy2.mtv.corp.google.com> <54133939.70801@redhat.com> <20140915100736.GA13503@blade.nx> <54181295.6030302@redhat.com> <541970D8.3080504@redhat.com> Date: Wed, 17 Sep 2014 18:20:00 -0000 Message-ID: Subject: Re: [PATCH 3/9 v7] Introduce target_{stop,continue}_ptid From: Doug Evans To: Pedro Alves Cc: Gary Benson , gdb-patches Content-Type: text/plain; charset=UTF-8 X-IsSubscribed: yes X-SW-Source: 2014-09/txt/msg00593.txt.bz2 On Wed, Sep 17, 2014 at 4:30 AM, Pedro Alves wrote: > On 09/16/2014 10:18 PM, Doug Evans wrote: >> On Tue, Sep 16, 2014 at 3:36 AM, Pedro Alves wrote: >>> On 09/15/2014 05:00 PM, Doug Evans wrote: >>>> On Mon, Sep 15, 2014 at 3:07 AM, Gary Benson wrote: >>>>> Doug Evans wrote: >>>>>> On Fri, Sep 12, 2014 at 11:19 AM, Pedro Alves wrote: >>>>>>> On 09/12/2014 07:08 PM, Doug Evans wrote: >>>>>>>> Pedro Alves wrote: >>>>>>>>> I just now noticed the elephant in the room -- target_stop >>>>>>>>> is asynchronous, doesn't wait for a stop, while and >>>>>>>>> target_stop_ptid is synchronous. [...] >>>>>>>> >>>>>>>> If the above code is right, I think a clarifying comment is >>>>>>>> required somewhere. It's odd that one can call >>>>>>>> agent_run_command when the inferior may or may not be stopped >>>>>>>> yet. [Or is there a bug here? - if I'm reading the gdbserver >>>>>>>> version correctly it first waits for the inferior to stop] >>>>>>> >>>>>>> It's a bug. >>>>>>> >>>>>>> (Note that the GDB side interfaces with an out-of-tree >>>>>>> agent, not GDBserver's agent. I don't know the status of >>>>>>> that agent.) >>>>>> >>>>>> Data point that target_stop should be named target_stop_async? >>>>> >>>>> Ok, can I get a summary of this thread, I'm struggling to follow it. >>>>> >>>>> a) What should the functions be called: >>>>> - target_stop_async / target_stop_wait >>>>> - target_continue_async / target_continue_no_signal >>>>> - something else? >>>>> >>>>> b) Is there a bug here I need to address? >>>> >>>> At this point I think we're still in discussion mode, there are no >>>> conclusions/agreements yet, except for the agreement to use >>>> target_continue_with_no_signal instead of target_continue_ptid. >>>> >>>> To advance the discussion, >>>> the async case is the subtle case IMO. Evidently (and I'm just going >>>> by what I see, there may be more data here) someone (*1) looked at the >>>> name "target_stop" and thought it was async (which is probably what >>>> I'd do). The function comment doesn't specify. One could argue it's >>>> sufficient to just fix the function comment, but if we're going to >>>> have a mix of similar functions, some of which are async and some >>>> sync, then IMO we should also make the difference stand out in the >>>> code where it's read. I'd be happy with a convention where all async >>>> functions ended with _async or _no_wait (the latter reads better to >>>> me), but I'm guessing I'd be happy with a different convention as >>>> well. >>> >>> At first blush, this looks to me like looking for a generalization / hard >>> rule where one might not be necessary, or otherwise, we'd need more a >>> better/clearer rule/constrain on which methods this would apply to. E.g., >>> would you rename target_xfer_partial? Certainly memory accesses can >>> be expected to work in an asynchronous way, with read results being sent >>> over with asynchronous notifications (even though we don't do it like that). >> >> Are you asking about target_xfer_partial as it exists today, >> or a hypothetical version where it is used to request the transfer >> but the transfer happens asynchronously? [Or both? 1/2 :-)] >> I think it's more the latter, but I'll answer for both. >> >> When I think of a "read memory" operation, absent any info specifying >> sync/async, I'd expect that the request completes before control is >> returned to the caller. If it didn't wait for the request to complete that >> would be unexpected. Therefore I'd leave the name of >> target_xfer_partial as it exists today alone. >> That same reasoning to me says a hypothetical "memory read" >> request that was asynchronous should have something in its >> name that says to the reader "Hey, this is async! Heads up!". >> >>> What about target_resume? I'm not sure renaming that would be useful, >>> for example. >> >> I thought about target_resume. It was an semi-interesting case >> that immediately popped into my head at the time. >> And then I tried to think how the typical reader would interpret it. >> I'm not a typical reader, but I think(!) people would expect it to be >> asynchronous in the sense that the inferior is resumed and >> control returns to gdb. IOW target_resume doesn't also wait >> for the inferior to stop after it has been resumed. >> Therefore I see no need to rename it (say to target_resume_no_wait). >> >>> So I'd like to see a comprehensive list of functions >>> that'd you're proposing would be affected in order to be able to >>> comment further or even be able to see the same patterns you're seeing. >> >> I think these things can be approached on a by-demand basis. >> All I'm suggesting(!) [and it is just a suggestion] is having a convention >> for when the topic comes up. > > That's the thing. You've now shown that you've given some thought over > a couple cases, It's not clear to me what your point is here. I thought of target_resume earlier, and I hardly spent any time on it. I wouldn't have expected anyone else to either. I thought of target_xfer_partial when you mentioned it. > but when you first raised this, you didn't, which puts us > in a position of having to stop and go through the same exercises and > looking for patterns, checking what might or not make sense, etc. Let's assume for the sake of discussion I *had* also thought of target_xfer_partial earlier. Mentioning those two cases would have made any difference to the time being put into this discussion? Wow, noted for next time. > There's no way to tell whether the suggestion makes sense without that > info; IOW, the suggestion was too vague and ends up being > counterproductive (in the sense that we spend a bunch of time on it > instead of moving forward with other more important things). re: "more important things": Eh? The topic at hand is readable code and avoiding bugs, and yet if I go through gdb-patches@ for even the last week what will I find? The standard bits of hacking, bug fixing, and administrivia (e.g., vxworks cleanup). Characterizing this as spending time on less important things bothers me. gdb has several problems I wish people had spent a bit more time on. [OTOH, I'm not suggesting spending *too* much time on this. That should go without saying though.] > Please > don't take me wrong. I'm in "how to communicate better mode". I would > have liked to have been able to reply quicker to these emails, but I > just don't know how -- fyi, today's and yesterday's emails on this > subject took me a few hours. If the situation were reversed and I didn't have time at that moment, I'd have just said it's an interesting idea but let's table this discussion until we can collect more info. > I guess my thing is that I'm not particularly fond of appending > _async/_sync/_no_wait to target methods names throughout. Maybe > we'll find even better names as we move along. > >>> target_stop_and_wait (the existing target_stop_ptid) isn't really a >>> target method, unlike target_stop, target_resume, etc. -- it's a >>> helper method that sits on top of target methods. >> >> Whether or not a function is a target method is orthogonal to >> what its name suggests to the reader about what it does >> and how it behaves. > > I was just trying to say that we could call target_stop_ptid > pause_thread or something else without "target_" in the name even, > though I'm not suggesting that. Ah, thanks for clearing that up ... Still, it's a bit tangential to the discussion at hand. No worries though. >>> So I'd go with >>> clarifying target_stop's "asyncness" in its comments, name the >>> helper target_stop_and_wait. I think that's already better >>> than the status quo. And then we can discuss conventions and >>> (potentially wholesale) renaming separately. It's not a big deal >>> if we have to rename the new function again. >> >> This is a step in the right direction, and if that is all people are >> comfortable with right now that's fine by me. > > Thanks. Let's go forward with that, so the common/ work can > continue, and continue discussing the naming in parallel. FAOD, I was not suggesting blocking the common/ work for this. I'd expect some parallelization. >> [I've gotten target_stop's asyncness burned into memory, >> but I can't help but worry about the next reader.] > > We could even pick IMO more natural terms than _async or _no_wait, > e.g., target_request_stop. As long as it solves the problem in a reasonable way and we're consistent I think that'd be ok.