From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 10597 invoked by alias); 13 Mar 2017 15:34:07 -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 8620 invoked by uid 89); 13 Mar 2017 15:34:06 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-10.9 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.2 spammy=PID, tty, xfree, Share X-HELO: mail-wr0-f179.google.com Received: from mail-wr0-f179.google.com (HELO mail-wr0-f179.google.com) (209.85.128.179) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 13 Mar 2017 15:34:04 +0000 Received: by mail-wr0-f179.google.com with SMTP id u48so106288571wrc.0 for ; Mon, 13 Mar 2017 08:34:04 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:cc:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding; bh=nN4IMwNloVRKZCTERamIovfz79lM8lmk3wYgOCEvSho=; b=ArJzaYmX1HClZR+MVRL1c+rHPanG+oX5XUkXF4yI0MtOisJ8NgKewYr22vYiv9nKiP JFDCZB8tXW+q2ghvFcw8TmWjX2Jnke27rquNSgOLj2j3maDq3lm8A+QCVEAmzLVjD+mV ATWvHzPxGyHta3bykRigp/cc7s23/J7464qLb/qCJQ5E6P7P+XjcuVcT7YjthHW+BpNS +baWn0YYYqTWVQ6MVmpzm1PgE8lVSModATZVwsvVLhJaNglgFfSD+drjbRmnfyjsThgG K45cOQwPwugkKcmkd/+llBbAQwJ+QvAGUI3MiJAn+WYQ+8bTtQMTAim/aTnngJde2fKA 74VQ== X-Gm-Message-State: AMke39neMaTvcOfIsjXWtwui4tdX2jfQeixjXDmBy7GK7JjItki90+oSf0ALnNxy+ZRbb8ti X-Received: by 10.223.142.34 with SMTP id n31mr27711795wrb.11.1489419243086; Mon, 13 Mar 2017 08:34:03 -0700 (PDT) Received: from [192.168.0.101] ([37.189.166.198]) by smtp.gmail.com with ESMTPSA id o26sm25395634wro.44.2017.03.13.08.34.01 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 13 Mar 2017 08:34:02 -0700 (PDT) Subject: Re: [PATCH v3 5/6] Share fork_inferior et al with gdbserver To: Sergio Durigan Junior References: <1482464361-4068-1-git-send-email-sergiodj@redhat.com> <20170208032257.15443-1-sergiodj@redhat.com> <20170208032257.15443-6-sergiodj@redhat.com> <87bmtcg91v.fsf@redhat.com> Cc: GDB Patches , Luis Machado From: Pedro Alves Message-ID: <9f051eba-7b72-ba2b-5a7a-b4b6acff9579@redhat.com> Date: Mon, 13 Mar 2017 15:34:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <87bmtcg91v.fsf@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2017-03/txt/msg00190.txt.bz2 On 03/07/2017 07:51 PM, Sergio Durigan Junior wrote: > On Wednesday, February 15 2017, Pedro Alves wrote: > >>> +int >>> +fork_inferior (char *exec_file_arg, char *allargs, char **env, >>> + void (*traceme_fun) (void), void (*init_trace_fun) (int), >>> + void (*pre_trace_fun) (void), char *shell_file_arg, >>> + void (*exec_fun)(const char *file, char * const *argv, >>> + char * const *env)) >>> +{ >> >>> + /* Retain a copy of our environment variables, since the child will >>> + replace the value of environ and if we're vforked, we have to >>> + restore it. */ >>> + save_our_env = environ; >>> + >>> + /* Likewise the current UI. */ >>> + save_ui = current_ui; >> >> Hmm, making this compile on gdbserver is of course a hack, >> since there's not such thing as a "UI" concept on gdbserver... >> >>> + >>> + /* Tell the terminal handling subsystem what tty we plan to run on; >>> + it will just record the information for later. */ >>> + new_tty_prefork (inferior_io_terminal); >> >> I wonder about calling here instead a more generalized hook, and >> moving the save_ui saving and the new_tty_prefork calls to >> that hook's gdb implementation. Might be easier to do that after >> the series is in... > > Right, it is a hack indeed, and not a beautiful one. > > I implemented a bunch of new functions that solve this problem. Two of > them, tty_{pre,post}fork_hook, are responsible for saving/restoring the > current 'struct ui' and also for calling the new_tty_{pre,post}fork > functions (which are now static). One more function was needed: > switch_ui_postfork, which is responsible for switching the current_ui to > main_ui, as is done currently after we successfully fork. These 3 > functions are implemented only on GDB; they're stubs on gdbserver. Thanks. Sounds better, though the "ui" in switch_ui_postfork makes me wonder whether it's generalized enough, but I'll leave further comments (if any) for v4, which I haven't read yet. >>> --- a/gdb/gdbserver/inferiors.c >>> +++ b/gdb/gdbserver/inferiors.c >>> @@ -29,6 +29,8 @@ struct thread_info *current_thread; >>> >>> #define get_thread(inf) ((struct thread_info *)(inf)) >>> >>> +ptid_t inferior_ptid; >> >> What do we need this for? gdbserver already has >> a "current thread" global. Another one looks like asking >> for out-of-sync trouble. > > This is needed because fork_inferior et al reference this variable > directly, and so I moved the 'extern' declaration of it to commom/. > > A possible solution to this would be to create a get/set pair of > functions for it, but I'm not sure this would be a good idea due to (a) > the number of direct references to it, and (b) the fact that these > functions would probably end up being stubs on gdbserver as well. Or we can re-evaluate why do fork_inferior et al need to reference the variable directly. >>> -/* Add a process to the common process list, and set its private >>> - data. */ >>> +/* Update process represented by PID with necessary info. */ >>> >>> static struct process_info * >>> -linux_add_process (int pid, int attached) >>> +linux_update_process (int pid) >> >> I'm not sure I understand the need for this yet. I need >> to look deeper. "update what? why?" Or maybe the >> comments should be improved. :-) > > The reason these 'update' functions were created is because > fork_inferior already creates the process/thread structures, but we (the > caller) still need to fill in some of the fields of these structures > with more information. They are the same functions that existed before, > but now we work with an existing process/thread, while before we > *created* these structures. I think that that's only somewhat clear because you're looking at a diff. ISTM that someone looking at the resulting code with no other context has no clue what's being updated, and/or why. All one has to go by is "update". But, what kind of information is being updated? I guess what I'm saying is that "Update process" is so vague that it's borderline meaningless. >>> @@ -2870,62 +2893,91 @@ handle_v_run (char *own_buf) >>> new_argc++; >>> } >>> >>> - new_argv = (char **) calloc (new_argc + 2, sizeof (char *)); >>> - if (new_argv == NULL) >>> - { >>> - write_enn (own_buf); >>> - return 0; >>> - } >>> - >>> - i = 0; >>> - for (p = own_buf + strlen ("vRun;"); *p; p = next_p) >>> + for (i = 0, p = own_buf + strlen ("vRun;"); *p; p = next_p, ++i) >>> { >>> next_p = strchr (p, ';'); >>> if (next_p == NULL) >>> next_p = p + strlen (p); >>> >>> - if (i == 0 && p == next_p) >>> - new_argv[i] = NULL; >>> + if (p == next_p) >>> + new_argv.push_back ("''"); >>> else >>> { >>> /* FIXME: Fail request if out of memory instead of dying. */ >>> - new_argv[i] = (char *) xmalloc (1 + (next_p - p) / 2); >>> - hex2bin (p, (gdb_byte *) new_argv[i], (next_p - p) / 2); >>> - new_argv[i][(next_p - p) / 2] = '\0'; >>> + size_t len = 1 + (next_p - p) / 2; >>> + char *s = (char *) xmalloc (len); >>> + char *ss = (char *) xmalloc (len * 2); >>> + char *tmp_s, *tmp_ss; >>> + int need_quote; >>> + >>> + hex2bin (p, (gdb_byte *) s, (next_p - p) / 2); >>> + s[(next_p - p) / 2] = '\0'; >>> + >>> + tmp_s = s; >>> + tmp_ss = ss; >>> + need_quote = 0; >>> + while (*tmp_s != '\0') >>> + { >>> + switch (*tmp_s) >>> + { >>> + case '\n': >>> + *tmp_ss = '\''; >>> + ++tmp_ss; >>> + need_quote = 1; >>> + break; >>> + >>> + case '\'': >>> + *tmp_ss = '\\'; >>> + ++tmp_ss; >>> + break; >>> + >>> + default: >>> + break; >>> + } >>> + >>> + *tmp_ss = *tmp_s; >>> + ++tmp_ss; >>> + ++tmp_s; >>> + } >>> + >>> + if (need_quote) >>> + *tmp_ss++ = '\''; >> >> Hmm, is this quoting stuff being moved from somewhere, >> or it is new? > > This is new, even though GDB has a lot of places that do the same > thing... OK, but then what is the code doing? Can we at least add some comment? > >>> + >>> + *tmp_ss = '\0'; >>> + new_argv.push_back (ss); >>> + xfree (s); >>> } >>> >>> if (*next_p) >>> next_p++; >>> - i++; >>> } >>> - new_argv[i] = NULL; >> >>> >>> + /* Gather information about the environment. */ >>> + our_environ = make_environ (); >>> + init_environ (our_environ); >>> + >>> initialize_async_io (); >>> initialize_low (); >>> @@ -39,8 +39,14 @@ set spawn_id [remote_spawn target "$gdbserver stdio non-existing-program"] >>> set msg "gdbserver exits cleanly" >>> set saw_exiting 0 >>> expect { >>> - # This is what we get on ptrace-based targets. >>> - -re "stdin/stdout redirected.*No program to debug\r\nExiting\r\n$" { >>> + # This is what we get on ptrace-based targets with >>> + # startup-with-shell disabled. >>> + -re "stdin/stdout redirected.*gdbserver: Cannot exec >>> non-existing-program\r\ngdbserver: Error: No such file or >>> directory\r\n\r\nDuring startup program exited with code >>> 127\.\r\nExiting\r\n$" { >>> + set saw_exiting 1 >>> + exp_continue >> >> Shouldn't this be a part of the next patch? > > Not really. I put this here because without it a regreession is > introduced, and I wanted each patch to be regression-free. I'm confused then. The comment above says: "This is what we get on ptrace-based targets with startup-with-shell disabled" But the patch's intro said: "I decided to go ahead and implement a partial support for starting the inferior with a shell on gdbserver, although the full feature comes in the next patch. The user won't have the option to disable the startup-with-shell, and also won't be able to change which shell gdbserver will use (other than setting the $SHELL environment variable, that is)." So which one is right? Thanks, Pedro Alves