From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 24982 invoked by alias); 16 Sep 2014 21:18:48 -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 24814 invoked by uid 89); 16 Sep 2014 21:18:47 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.7 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-f172.google.com Received: from mail-vc0-f172.google.com (HELO mail-vc0-f172.google.com) (209.85.220.172) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Tue, 16 Sep 2014 21:18:45 +0000 Received: by mail-vc0-f172.google.com with SMTP id hy10so506731vcb.3 for ; Tue, 16 Sep 2014 14:18:43 -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=o73H/BKHK7AJmd8h0b8IGjJzSH4+G9i4kIOyZEAEUhg=; b=lgBDOjthM9yeXRx2FcHZg5MBi9jHQh5GjHB8nMHf3c8/fqL5R1ltFsQtwPr4xC9VGL TiobCST20aOvex9Qa9zVEdsGrxZIymbP+FDYRVPtZpNgl/DbQXdteHBsAeSaL4xX0Gtf GZ/B6MeuWvzcH0OthxlhSRqCA4wAZLTiHxA5iZUvH6qiMFB4ZgO/F1W9fNmZLE5SjrtD A0VnMYTUe5y8nQ9ipX3MrdW+hcmqQ2XtLGe9MUm2uD1RwkI+B/zfMm0xq/KLXlhRER0m XKCdddZZySGqN567Yl0khw2Qua6J3sG2ENZKWNJByCqYGV+2l/n+NM3oeYc6sumlTOqV 9TUA== X-Gm-Message-State: ALoCoQmPsXMmga7oN8sdARVSVFneQ5C9xo06kI+ZZ3aEeYfaA07rPBk3mcCDRyKcb8x2Vj75nXrv MIME-Version: 1.0 X-Received: by 10.221.57.68 with SMTP id wf4mr11197664vcb.55.1410902323208; Tue, 16 Sep 2014 14:18:43 -0700 (PDT) Received: by 10.52.181.65 with HTTP; Tue, 16 Sep 2014 14:18:43 -0700 (PDT) In-Reply-To: <54181295.6030302@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> Date: Tue, 16 Sep 2014 21:18: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/msg00555.txt.bz2 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. > 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. > 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. [I've gotten target_stop's asyncness burned into memory, but I can't help but worry about the next reader.]