From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id VO3JGgsHe1+TBAAAWB0awg (envelope-from ) for ; Mon, 05 Oct 2020 07:44:11 -0400 Received: by simark.ca (Postfix, from userid 112) id 609321EE0F; Mon, 5 Oct 2020 07:44:11 -0400 (EDT) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-1.0 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, MAILING_LIST_MULTI,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.2 Received: from sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id 0EEF11E58D for ; Mon, 5 Oct 2020 07:44:10 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 79B5338708A5; Mon, 5 Oct 2020 11:44:09 +0000 (GMT) Received: from mail-wr1-x442.google.com (mail-wr1-x442.google.com [IPv6:2a00:1450:4864:20::442]) by sourceware.org (Postfix) with ESMTPS id 13A35386F818 for ; Mon, 5 Oct 2020 11:44:07 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 13A35386F818 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=undo.io Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=mbarisione@undo.io Received: by mail-wr1-x442.google.com with SMTP id e17so480771wru.12 for ; Mon, 05 Oct 2020 04:44:07 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=undo-io.20150623.gappssmtp.com; s=20150623; h=mime-version:subject:from:in-reply-to:date:cc :content-transfer-encoding:message-id:references:to; bh=onZ90mztQOJbaXhDQ5M79TJqaVdrt793rD0B6fpzpJI=; b=G+pY/lOPLMKUkqT10z+Q+25R71CdvwDV0IKLYC/excwNMv24OJYcEKUCZQnUZJaR8B ++0qkGtSVu3qHu8c7NF848qwjFQZcSNK4LYwnZ6mwvLC4x2P/lRrm50ik5SOTmQbJNNq brH/3wS1i74wvQcbpdbqPypTgKSesJU3SvTDffNSBZewETzOUNd7yqkUKHRdZ9+E+ZN/ p5Ek8RKA+jYkpDiRf3CBLb3/ye/Qt6174Owepf9UDAUQs7wSWEkaA1d4dllAScZpBxm+ aQldz76SVNs7GJnjGEso2aeHL4Mbr6gGua1y7teDWTFK8TrI3kuNZ5FPYzVZyOzcMsQc uwmQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:subject:from:in-reply-to:date:cc :content-transfer-encoding:message-id:references:to; bh=onZ90mztQOJbaXhDQ5M79TJqaVdrt793rD0B6fpzpJI=; b=VatWADY6NncBIXlHzvUBbFGKZMrSK6CPKUVCi7xorznCmoQ+qO27NQmHAnpwt1HknW PtDwGtNE1u2CkLuAF9q9bkFazCjbCE/vfUeiKpmpimp1Uuf696TfSaXwjjzj6qzMhAcV FrF0qazqS1PYI8rVDZ7BNaQ7xyE9AAXy9kuabE0/Yp7D9m81ku7aTdVhKQbiYh/91TO3 nE7TqHEUGdexGIOIv+GYQrBnPNtBthh76hXm00+RK43p1Uf9hAY4r+cuxni69bP/kt2Q /t0ZAI7lim7TMRj4IMKo5zv8nL+jcLVXpR1p/iibyWDTFlHUQXaUWlgM8faIllBqkGW0 3h/Q== X-Gm-Message-State: AOAM531umOjs0YSumwlwAmF/+abKjqQiTfje0DEpYf4U4IN4B7wTE6CI 0KD4KqEKUuQmrotzIjH1J9wCdAUCoEdv5IvI X-Google-Smtp-Source: ABdhPJyUsMn45uMuFziknv4abZu3LiLbxI8Ju9zHcQ38fVRltFcPzK3mJD9ZyEhoejcO2nGag7UABA== X-Received: by 2002:a5d:498a:: with SMTP id r10mr5556146wrq.106.1601898245941; Mon, 05 Oct 2020 04:44:05 -0700 (PDT) Received: from [192.168.0.107] (cpc159317-cmbg20-2-0-cust117.5-4.cable.virginm.net. [81.111.29.118]) by smtp.gmail.com with ESMTPSA id d2sm12909349wro.34.2020.10.05.04.44.04 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Mon, 05 Oct 2020 04:44:04 -0700 (PDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.8\)) Subject: Re: [PATCH 2/2] Add a way to preserve redefined GDB commands for later invocation From: Marco Barisione In-Reply-To: <20201005102441.GG605036@embecosm.com> Date: Mon, 5 Oct 2020 12:44:03 +0100 Content-Transfer-Encoding: quoted-printable Message-Id: <27432854-8CFD-47A2-BB9C-62B8D921E1EA@undo.io> References: <20200914093925.5442-1-mbarisione@undo.io> <20200914093925.5442-3-mbarisione@undo.io> <20201005102441.GG605036@embecosm.com> To: Andrew Burgess X-Mailer: Apple Mail (2.3445.104.8) 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: , Cc: gdb-patches@sourceware.org Errors-To: gdb-patches-bounces@sourceware.org Sender: "Gdb-patches" On 5 Oct 2020, at 11:24, Andrew Burgess = wrote: >> +/* 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 = remaining >> + 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. This is briefly explained in the first email = (https://sourceware.org/pipermail/gdb-patches/2020-September/171829.html).= I didn=E2=80=99t want to do too much for this patch series in case the = whole approach was wrong. One of the things I didn=E2=80=99t do (for now) was this as I couldn't = find 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. 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 = command? In the latter case, wouldn=E2=80=99t it confusing that "-1" has a = different meaning inside or outside commands? Note that there=E2=80=99s another bit of useful work that I didn=E2=80=99t= do, that is the addition of a way of accessing all the untokenized arguments from a = command definition. > 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. 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. 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. > 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. Is this really what a user would want? Or would they want the ability to delete commands? > 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' = to 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. I don=E2=80=99t think it would be too difficult but it means that the = user 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? > 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. My goal with my patches was to get rid of hooks in my own code. > 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. Good point. > 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. Hm, difficult to say how difficult it could be but I will have a look. There may also be other subtle different behaviours we don=E2=80=99t = know about and which could break existing scripts. Is this acceptable? --=20 Marco Barisione