From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-x443.google.com (mail-wr1-x443.google.com [IPv6:2a00:1450:4864:20::443]) by sourceware.org (Postfix) with ESMTPS id D70563858D37 for ; Mon, 5 Oct 2020 18:11:03 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org D70563858D37 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=embecosm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=andrew.burgess@embecosm.com Received: by mail-wr1-x443.google.com with SMTP id e18so4743071wrw.9 for ; Mon, 05 Oct 2020 11:11:03 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to; bh=ws721Q8TZGDJtAyorSopGuk2jNxYSsREA4IZqICwW/Q=; b=H8t41RffGOkscj/nguF1OvP6X4RvpdyCuOK0oJoNvZd2vU0BY/YPFph4NMg32Wa+KU OYcDFZRhgdGhWMi49rre3LzkADJc7dvisFSKXt/eNBhMpE2OfZlfaCL/OHpE56/1bF7c JQylQ6zFtuzp6ATrpVS+9cIcy6LpW9b3SHBLWB5QI1TQ8uO28RiOFA9W5LPc7TDkjAKe FwMl46ERepJNWSSAl4a8/OvxXBlooEhf70nhO99yFVI8oAKTl0M86E6665QlLdZdT8dr mIvappbEEt9/0qLUuprVKcCbhkDQDdFPtBsEBW9AFLcaDJoij6rL3hXxZlQYUpuV+3yg ihIg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to; bh=ws721Q8TZGDJtAyorSopGuk2jNxYSsREA4IZqICwW/Q=; b=MuGiaJGRSmSaIGB8Rz1j1Yaa9jAMtb+jev/g+vBZ2+uuMOJU1a0r1CaN51x9kPYB3Z z+lEX1f/P5GaE7U/Qxop4hLW6QtwBd70tqcj9W4DCyQCY3igvh3rpA5oeFP6uBT5sjRn EgIF8RcTEi2+9N/qKuODE8M4dR/mEm8pe2wKvcjox0cyMS78cW2ElW0WzNJlrDNh32L6 g87AxWBwTG/Uni2Az5wbSKTspht37HAtpH/fzNw3nGwWZEuihhoEhBa0wmlgsPue5WNE NyRR9kwUHEqahRMUoiwltKErSSGqegtEQlq5DfGomnDPSeHXeVWv3qnFYS7AJa5Bggby 1Ryg== X-Gm-Message-State: AOAM533NkPL0dn2moN2afo4AhUHgVRSEe3vJVQvUShH4WNtol1TxlmFm Hk5Fa5vMbn3883Yb/3KW8W2XVg== X-Google-Smtp-Source: ABdhPJx8Fhaj9ae/oGy0uedj3V490cl0+bzjKAlGAz0VWd3A1B/sLg9CoEhfysTk1XSv9D+GRxFTdg== X-Received: by 2002:a05:6000:108a:: with SMTP id y10mr680884wrw.41.1601921462864; Mon, 05 Oct 2020 11:11:02 -0700 (PDT) Received: from localhost (host109-151-14-50.range109-151.btcentralplus.com. [109.151.14.50]) by smtp.gmail.com with ESMTPSA id s6sm891200wrg.92.2020.10.05.11.11.01 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 05 Oct 2020 11:11:01 -0700 (PDT) Date: Mon, 5 Oct 2020 19:11:00 +0100 From: Andrew Burgess To: Marco Barisione Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 2/2] Add a way to preserve redefined GDB commands for later invocation Message-ID: <20201005181100.GI605036@embecosm.com> References: <20200914093925.5442-1-mbarisione@undo.io> <20200914093925.5442-3-mbarisione@undo.io> <20201005102441.GG605036@embecosm.com> <27432854-8CFD-47A2-BB9C-62B8D921E1EA@undo.io> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable In-Reply-To: <27432854-8CFD-47A2-BB9C-62B8D921E1EA@undo.io> X-Operating-System: Linux/5.8.12-100.fc31.x86_64 (x86_64) X-Uptime: 18:52:19 up 2 days, 9:04, X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] X-Spam-Status: No, score=-5.0 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 05 Oct 2020 18:11:05 -0000 * Marco Barisione [2020-10-05 12:44:03 +0100]: > On 5 Oct 2020, at 11:24, Andrew Burgess wro= te: > >> +/* Implementation of the "uplevel" command. */ > >> + > >> +static void > >> +uplevel_command (const char *arg, int from_tty) > >> +{ > >> + /* Extract the level argument and update arg to point to the remain= ing > >> + string. */ > >> + size_t level_end_offset; > >> + int level; > >> + try > >> + { > >> + level =3D std::stoi (arg, &level_end_offset); > >> + } > >> + catch (const std::exception &) > >> + { > >> + level =3D -1; > >> + } > >> + if (level < 0) > >> + error (_("uplevel command requires a non-negative number for the " > >> + "level argument.")); > >=20 > > I don't understand why the Python API allows for negative values while > > the CLI command does not. In my mind, negative values are the only > > really useful way to use this feature as it should be considered bad > > practice to assume that there are not some unknown number of overrides > > above you in the command stack. >=20 > This is briefly explained in the first email > (https://sourceware.org/pipermail/gdb-patches/2020-September/171829.html). >=20 > I didn=E2=80=99t want to do too much for this patch series in case the wh= ole approach > was wrong. > One of the things I didn=E2=80=99t do (for now) was this as I couldn't fi= nd an obvious > way for the "uplevel" command to know which command you are currently > executing. An option could be to implement something similar way to how > command arguments are kept around with scoped_user_args_level, so we could > keep a stack of all (user and non-user) commands which are being executed. > By checking the latest one you can know what "uplevel -1" would > apply to. I think this would be a fine approach. I'd pack the command pointer and arguments into a single object and store these in a newly renamed scoped_user_args_level I think. >=20 > Another question I couldn=E2=80=99t answer is what "uplevel -1 =E2=80=A6"= would do if used > outside a definition. > Should it show an error or should it just invoke the latest defined comma= nd? > In the latter case, wouldn=E2=80=99t it confusing that "-1" has a differe= nt meaning > inside or outside commands? Yeah I don't think the last case would be a good way to go, but what about executing the last but one version of the command, this would seem exactly inline with the behaviour inside a command, so: (gdb) define cmd_1 echo v1\n end (gdb) define cmd_1 echo v2\n uplevel -1 cmd_1 end (gdb) cmd_1 v2 v1 (gdb) uplevel 0 cmd_1 v1 (gdb) uplevel -1 cmd v1 >=20 > Note that there=E2=80=99s another bit of useful work that I didn=E2=80=99= t do, that is the > addition of a way of accessing all the untokenized arguments from a comma= nd > definition. >=20 > > I think that there is another useful check that should be added to > > this patch, I think we should prohibit calling a command further down > > the command stack. That is we should make this invalid: > >=20 > > define cmd_1 > > echo this is level 0 cmd_1\n > > uplevel 1 cmd_1 > > end > >=20 > > define cmd_1 > > echo this is level 1 cmd_1\n > > uplevel 0 cmd_1 > > end > >=20 > > What I'd like to see is that when we execute 'uplevel 1 cmd_1' GDB > > throws an error about '... requested command at higher level > > than current command...' or similar. > >=20 > > The above will eventually error with 'Max user call depth exceeded -- > > command aborted.', but I think explicitly catching this case would > > improve the user experience. >=20 > How about the case where a command on the same level is invoked? > I considered this (there should be a comment somewhere) but thought it=E2= =80=99s > not a real problem as there may be some legitimate cases (for instance > if the arguments are changed) and, if things go wrong, you just get a > max user call depth exceeded. But at the same level can't you just call the command by name with no 'uplevel' at all? >=20 > The same weird but legitimate case may in theory exist also for > invoking a command at a higher level, i.e. the arguments are > changed. Yeah... no. I'm sure it's possible to craft an example where this might work, but I think we should start off by blocking this, then if someone comes up with a really good example of why this is needed, and is the _only_ way to solve there problem then we can rip the check out. In general I think you could only call to a higher level if you had full control over how the commands are written and the order they are registered, in which case (I boldly claim) there would be a better way to rewrite the code to avoid calling to a higher level. IF you don't have full control over what was registered later, or the order in which parts are registered then this would never work. In short I think blocking this will help far more than it hurts. >=20 > > I also wonder how we might handle _really_ redefining a command now > > that past commands hang around. > >=20 > > When writing the above cmd_1 example the first thing I did was open > > GDB and try to write the commands at the CLI. I made a typo when > > writing the second version of cmd_1. So now I'm stuck with a chain: > >=20 > > working level 0 cmd_1 --> broken level 1 cmd_1 > >=20 > > Sure I can write a level 2 version that knows to skip over the broken > > level 1, but it might be nice if there was a flag somewhere that I > > could pass to say, no, really replace the last version of this command > > please. >=20 > Is this really what a user would want? Or would they want the ability > to delete commands? Deleting too might be useful, but I think the case I gave is not unique to me, I tried writing a command, I got it slightly wrong. Sure I can delete and start again, but being able to just do: (gdb) define --redefine cmd_1 ... would be nice. >=20 > > Also, when I define cmd_1 (at the CLI) I get asked: > >=20 > > Redefine command "cmd_1"? (y or n) > >=20 > > Maybe this could/should be changed to: > >=20 > > "cmd_1" already exists, type 'o' to override, 'r' to replace, or 'q' t= o quit: > >=20 > > I'm thinking about the pager prompt which is more than just y/n. I > > haven't looked how easy it would be to do something similar here. >=20 > I don=E2=80=99t think it would be too difficult but it means that the use= r must > decide what they want early and they can=E2=80=99t change their minds. > What happens if you accidentally replace a command but you wanted to > override it instead? Well, GDB offers a programmable environment, we don't allow undoing most (or hardly any) actions, so I think users are OK with the idea that some action might not be reversible. >=20 > > One last thing I note is that this new functionality is very similar > > to the existing "hook-" and "hookpost-" functionality, though I think > > this new approach is much better. >=20 > My goal with my patches was to get rid of hooks in my own code. >=20 > > I think as a minimum the documentation for the old hook approach > > should be updated to indicate that that technique is deprecated, and > > this new approach should be used instead. >=20 > Good point. >=20 > > Where I really think we should be going is adding a patch #3 to this > > series that replaces the implementation of the old functionality using > > your new code. Meaning, when someone writes: > >=20 > > define hook-echo > > echo before\n > > end > >=20 > > GDB would internally change this to effectively be: > >=20 > > define echo > > echo before\n > > uplevel -1 echo > > end > >=20 > > this will work for many trivial cases. There's a few nasty corners, > > like hooks for sub-commands (see the docs on hooking 'target remote'), > > the pseudo-command 'stop', and calling the same command from within a > > hook. >=20 > Hm, difficult to say how difficult it could be but I will have a look. >=20 > There may also be other subtle different behaviours we don=E2=80=99t know= about > and which could break existing scripts. Is this acceptable? Nothing is completely risk free, and I don't know how well tested the hooks support is, but it would be nice if we could fully replace hooks with your code (which is a much better approach). Thanks, Andrew >=20 > --=20 > Marco Barisione >=20