From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 875 invoked by alias); 4 Aug 2017 22:56:46 -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 865 invoked by uid 89); 4 Aug 2017 22:56:45 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-11.9 required=5.0 tests=BAYES_00,GIT_PATCH_2,GIT_PATCH_3,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=replied, illustrated, DISPLAY, 6316 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 04 Aug 2017 22:56:43 +0000 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 7CD15883BA; Fri, 4 Aug 2017 22:56:41 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 7CD15883BA Authentication-Results: ext-mx02.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx02.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=sergiodj@redhat.com Received: from localhost (unused-10-15-17-193.yyz.redhat.com [10.15.17.193]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 522AF5D9C0; Fri, 4 Aug 2017 22:56:41 +0000 (UTC) From: Sergio Durigan Junior To: Simon Marchi 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 References: <20170629194106.23070-1-sergiodj@redhat.com> <20170727033531.23066-1-sergiodj@redhat.com> <87wp6o9fv8.fsf@redhat.com> <4ad3e00be2b381fa85367f963ced1950@polymtl.ca> Date: Fri, 04 Aug 2017 22:56:00 -0000 In-Reply-To: <4ad3e00be2b381fa85367f963ced1950@polymtl.ca> (Simon Marchi's message of "Tue, 01 Aug 2017 11:34:56 +0200") Message-ID: <87r2wr7xhz.fsf@redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2017-08/txt/msg00090.txt.bz2 On Tuesday, August 01 2017, Simon Marchi wrote: > 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 (); You're right. This specific behaviour is not implemented yet. I guess I replied too early saying that this is working, but it's not. >>> 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 :). Aha, now it makes sense :-). And no worries, it's always good to make sure that we're on the right path. >>>> --- 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. Fair enough. I'll use strlen then :-). Thanks, -- Sergio GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36 Please send encrypted e-mail if possible http://sergiodj.net/