From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 32057 invoked by alias); 1 Aug 2017 09:35: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 32040 invoked by uid 89); 1 Aug 2017 09:35:07 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-11.3 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_2,GIT_PATCH_3,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 spammy= 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, 01 Aug 2017 09:35:04 +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 v719Ywbr031370 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT) for ; Tue, 1 Aug 2017 05:35:02 -0400 Received: by simark.ca (Postfix, from userid 112) id 184981EA05; Tue, 1 Aug 2017 05:34:58 -0400 (EDT) Received: from simark.ca (localhost [127.0.0.1]) by simark.ca (Postfix) with ESMTP id C026E1E5AF; Tue, 1 Aug 2017 05:34:56 -0400 (EDT) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Tue, 01 Aug 2017 09:35:00 -0000 From: Simon Marchi To: Sergio Durigan Junior Cc: GDB Patches , Pedro Alves , Eli Zaretskii Subject: Re: [PATCH v2] Implement the ability to set/unset environment variables to GDBserver when starting the inferior In-Reply-To: <87wp6o9fv8.fsf@redhat.com> References: <20170629194106.23070-1-sergiodj@redhat.com> <20170727033531.23066-1-sergiodj@redhat.com> <87wp6o9fv8.fsf@redhat.com> Message-ID: <4ad3e00be2b381fa85367f963ced1950@polymtl.ca> X-Sender: simon.marchi@polymtl.ca User-Agent: Roundcube Webmail/1.3.0 X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Tue, 1 Aug 2017 09:34:58 +0000 X-IsSubscribed: yes X-SW-Source: 2017-08/txt/msg00004.txt.bz2 Hi Sergio, On 2017-08-01 04:33, Sergio Durigan Junior wrote: >> There is this use case for which the behavior is different between >> native and remote, related to unset >> >> native: >> >> (gdb inf1) file /usr/bin/env >> (gdb inf1) unset environment DISPLAY >> (gdb inf1) r # DISPLAY is not there >> (gdb inf1) add-inferior -exec /usr/bin/env >> (gdb inf1) inferior 2 >> (gdb inf2) r # DISPLAY is there >> >> remote: >> >> (gdb inf1) tar ext :1234 >> (gdb inf1) file /usr/bin/env >> (gdb inf1) set remote exec-file /usr/bin/env >> (gdb inf1) unset environment DISPLAY >> (gdb inf1) r # DISPLAY is not there >> (gdb inf1) add-inferior -exec /usr/bin/env >> (gdb inf1) inferior 2 >> (gdb inf2) set remote exec-file /usr/bin/env >> (gdb inf2) r # DISPLAY is not there >> >> I think that's because in GDB, we make a new pristine copy of the host >> environment for every inferior, which we don't in gdbserver. > > Thanks for the review, Simon. > > Yes, you're right, these cases are currently different because of the > way we handle the environment on GDB and gdbserver. On gdbserver we > have 'our_environ', which is a global declared at server.c and that is > passed to all inferiors being started. > >> The way I understand the QEnvironmentReset is that the remote agent >> (gdbserver) should forget any previous modification to the environment >> made using QEnvironmentHexEncoded and QEnvironmentUnset and return the >> environment to its original state, when it was launched. This should >> allow supporting the use case above. To implement that properly, we >> would need to keep a copy of gdbserver's initial environment, which we >> could revert to when receiving a QEnvironmentReset. > > Yes, and we already do that on gdbserver; see the 'our_environ' global. Maybe I'm reading the code wrong, but that's not what I understand. gdb_environ is never reset to gdbserver's original state. So if the DISPLAY env var is present in the original environment and is unset with a QEnvironmentUnset, a QEnvironmentReset won't make it reappear with the current implementation. But we would want it to be back, to support the scenario illustrated above, wouldn't we? I originally talked about keeping a copy of the initial environment, but actually when receiving a QEnvironmentReset, I think gdbserver should simply do our_environ = gdb_environ::from_host_environ (); >> In any case, I just want to make sure that the packets we introduce >> are not the things that limit us. > > Sorry, I'm not sure I understood what you have in mind. Could you > explain in what ways we'd be limited by the new packets? Oh, I just wanted to say that if the gdbserver implementation is not perfect yet, it's not the end of the world because that can always change. But the behavior of the RSP packets is more difficult to change once it becomes a published interface, so we need to be careful that their documented behavior covers all the use cases we want to support. But I know you already know that, so I don't know why I said it in the first place :). >>> + >>> + /* Iterate through M_USER_UNSET_ENV_LIST and unset all variables. >>> */ >>> + void clear_user_set_env (); >> >> I wouldn't put the "Iterate through M_USER_UNSET_ENV_LIST" in the >> comment, since that's implementation details. The function >> documentation should focus on the visible effects or goals of the >> function (i.e. remove the user set/unset variables in that >> environment). > > Good point. I've updated the comment to: > > /* Unset all variables that were previously set by the user, i.e., > that were added by calling the SET method. */ > void clear_user_set_env (); Sounds good. >> Similar thing here, what we pass to str is a const char*, so it leads >> to an unnecessary std::string construction. Also, the interface of >> the function is not well-suited to encode arbitrary binary data, which >> could any byte. One could use >> >> str2hex (std::string (p, count), hex); >> >> but again why copy the data in the first place? So what about this: >> >> extern std::string bin2hex (const char *bin, int count); > > Not sure if it was a typo or if you really meant to propose overloading > the bin2hex method and have another version of it that returns a > std::string, but I like the idea. I don't think we need to treat the > first parameter as a 'const char *'; TBH, treating it like a 'const > gdb_byte *' (just like the original version does) is more clear. So I > went ahead and implemented it this way. Sorry, I should have been more explicit. I really meant to overload bin2hex, since there's no point in limiting this function's scope to hex-encoding text strings, it can handle any binary data (of which a text strings are a subset). But you are right, it should be const gdb_byte *. >>> --- a/gdb/gdbserver/server.c >>> +++ b/gdb/gdbserver/server.c >>> @@ -631,6 +631,75 @@ handle_general_set (char *own_buf) >>> return; >>> } >>> >>> + if (startswith (own_buf, "QEnvironmentReset")) >>> + { >>> + our_environ.clear_user_set_env (); >>> + >>> + write_ok (own_buf); >>> + return; >>> + } >>> + >>> + if (startswith (own_buf, "QEnvironmentHexEncoded:")) >>> + { >>> + const char *p = own_buf + sizeof ("QEnvironmentHexEncoded:") >>> - >>> 1; >> >> You can also use strlen to avoid having to do -1, but either is fine >> with me. > > I personally like using sizeof here and avoiding the call to strlen. Ok, but remember that the compilers optimize those calls to strlen ("literal") away to a constant. Thanks, Simon