From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id A5337389AC93 for ; Mon, 10 Jan 2022 12:46:09 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org A5337389AC93 Received: from mail-wm1-f69.google.com (mail-wm1-f69.google.com [209.85.128.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-56-_IvNoeliONGe5gw_2JqQHw-1; Mon, 10 Jan 2022 07:46:05 -0500 X-MC-Unique: _IvNoeliONGe5gw_2JqQHw-1 Received: by mail-wm1-f69.google.com with SMTP id o18-20020a05600c511200b00345c1603997so2145460wms.1 for ; Mon, 10 Jan 2022 04:46:05 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:from:to:cc:subject:date:message-id:mime-version :content-transfer-encoding; bh=IPXdU0CJf1XxebbBt8CgzNvjlJHMq9eLiECRghNLXsE=; b=RoTdWEaFnYFW4dEZmoQXbYKSodJoXzxcnnBz9wkKHYC0FnIn1G5fKMbuD2KQHtKbqu oVE4nKBxLkR/ijkdH338W8xKBYq2WCucY72UAbtKuvSglfcOYbH6DWKv9XlgiEda8l/r 74MLzpOs6Y/wEryqlnXRcfl+ocABkXWI04P5sTZvsrB4GfwTvdlmdvABz5wRJkpqpl6V x9HDok613BL8MEhHLVWuj+3cQ5Md9vH4Vej8YORgMkIIhu1zmC8SBH7aGqq/MAuwfBVY 3+0CTWhle9q89A7OVpR1fah/O7Kb67cpppqVrSSydriPp3oSGUhUL7T8mR/QX6dYPd5g Xuig== X-Gm-Message-State: AOAM530zLXY7GGsvdJdESayNZrLVZLu9Vj0/jZhB9qZLgD1eP6mAMocC KkYjnTnH6/i2IlNy+KMnoV0pWaFQMh2UmlTANwQ7Fafb58omihC4b3fKdoqjYPjWn1ixrXRI4nG +6Q0xDCrg1ETw2AlTyRSJ1Oi9x7GIoFPJ3QRTzXiKrqf8xG09F7OHC33iVLf0/TjRkKo9yG9arg == X-Received: by 2002:a7b:c95a:: with SMTP id i26mr6210254wml.78.1641818764073; Mon, 10 Jan 2022 04:46:04 -0800 (PST) X-Google-Smtp-Source: ABdhPJwOIWT/BsXGOLa2hu/7SCplXJmKJ23/UctASmv1cZoUeviCXNUX4wcr4Cdz754mDbo1oX/r6Q== X-Received: by 2002:a7b:c95a:: with SMTP id i26mr6210217wml.78.1641818763538; Mon, 10 Jan 2022 04:46:03 -0800 (PST) Received: from localhost (host109-154-163-67.range109-154.btcentralplus.com. [109.154.163.67]) by smtp.gmail.com with ESMTPSA id r206sm2997527wma.2.2022.01.10.04.46.02 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 10 Jan 2022 04:46:03 -0800 (PST) From: Andrew Burgess To: gdb-patches@sourceware.org Cc: Andrew Burgess Subject: [PATCH] gdb/python: improve the auto help text for gdb.Parameter Date: Mon, 10 Jan 2022 12:45:58 +0000 Message-Id: <20220110124558.749570-1-aburgess@redhat.com> X-Mailer: git-send-email 2.25.4 MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Transfer-Encoding: 8bit Content-Type: text/plain; charset="US-ASCII" X-Spam-Status: No, score=-12.4 required=5.0 tests=BAYES_00, DKIM_INVALID, DKIM_SIGNED, GIT_PATCH_0, KAM_DMARC_NONE, KAM_DMARC_STATUS, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) 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, 10 Jan 2022 12:46:12 -0000 I'm aware of Tom's currently inflight series that also relates to gdb.Parameter[1], but this patch doesn't conflict. [1] https://sourceware.org/pipermail/gdb-patches/2022-January/184826.html --- This commit attempts to improve the help text that is generated for gdb.Parameter objects when the user fails to provide their own documentation. Documentation for a gdb.Parameter is currently pulled from two sources: the class documentation string, and the set_doc/show_doc class attributes. Thus, a fully documented parameter might look like this: class Param_All (gdb.Parameter): """This is the class documentation string.""" show_doc = "Show the state of this parameter" set_doc = "Set the state of this parameter" def get_set_string (self): val = "on" if (self.value == False): val = "off" return "Test Parameter has been set to " + val def __init__ (self, name): super (Param_All, self).__init__ (name, gdb.COMMAND_DATA, gdb.PARAM_BOOLEAN) self._value = True Param_All ('param-all') Then in GDB we see this: (gdb) help set param-all Set the state of this parameter This is the class documentation string. Which is fine. But, if the user skips both of the documentation parts like this: class Param_None (gdb.Parameter): def get_set_string (self): val = "on" if (self.value == False): val = "off" return "Test Parameter has been set to " + val def __init__ (self, name): super (Param_None, self).__init__ (name, gdb.COMMAND_DATA, gdb.PARAM_BOOLEAN) self._value = True Param_None ('param-none') Now in GDB we see this: (gdb) help set param-none This command is not documented. This command is not documented. That's not great, the duplicated text looks a bit weird. If we drop different parts we get different results. Here's what we get if the user drops the set_doc and show_doc attributes: (gdb) help set param-doc This command is not documented. This is the class documentation string. That kind of sucks, we say it's undocumented, then proceed to print the documentation. Finally, if we drop the class documentation but keep the set_doc and show_doc: (gdb) help set param-set-show Set the state of this parameter This command is not documented. That seems OK. So, I think there's room for improvement. With this patch, for the four cases above we now see this: # All values provided by the user, no change in this case: (gdb) help set param-all Set the state of this parameter This is the class documentation string. # Nothing provided by the user, the first string is now different: (gdb) help set param-none Set the current value of 'param-none'. This command is not documented. # Only the class documentation is provided, the first string is # changed as in the previous case: (gdb) help set param-doc Set the current value of 'param-doc'. This is the class documentation string. # Only the set_doc and show_doc are provided, this case is unchanged # from before the patch: (gdb) help set param-set-show Set the state of this parameter This command is not documented. The one place where this change might be considered a negative is when dealing with prefix commands. If we create a prefix command but don't supply the set_doc / show_doc strings, then this is what we saw before my patch: (gdb) python Param_None ('print param-none') (gdb) help set print set print, set pr, set p Generic command for setting how things print. List of set print subcommands: ... snip ... set print param-none -- This command is not documented. ... snip ... And after my patch: (gdb) python Param_None ('print param-none') (gdb) help set print set print, set pr, set p Generic command for setting how things print. List of set print subcommands: ... snip ... set print param-none -- Set the current value of 'print param-none'. ... snip ... This seems slightly less helpful than before, but I don't think its terrible. Additionally, I've changed what we print when the get_show_string method is not provided in Python. Back when gdb.Parameter was first added to GDB, we didn't provide a show function when registering the internal command object within GDB. As a result, GDB would make use of its "magic" mangling of the show_doc string to create a sentence that would display the current value (see deprecated_show_value_hack in cli/cli-setshow.c). However, when we added support for the get_show_string method to gdb.Parameter, there was an attempt to maintain backward compatibility by displaying the show_doc string with the current value appended, see get_show_value in py-param.c. Unfortunately, this isn't anywhere close to what deprecated_show_value_hack does, and the results are pretty poor, for example, this is GDB before my patch: (gdb) show param-none This command is not documented. off I think we can all agree that this is pretty bad. After my patch, we how show this: (gdb) show param-none The current value of 'param-none' is "off". Which at least is a real sentence, even if it's not very informative. This patch does change the way that the Python API behaves slightly, but only in the cases when the user has missed providing GDB with some information. In most cases I think the new behaviour is a lot better, there's the one case (noted above) which is a bit iffy, but I think is still OK. I've updated the existing gdb.python/py-parameter.exp test to cover the modified behaviour. Finally, I've updated the documentation to (I hope) make it clearer how the various bits of help text come together. --- gdb/cli/cli-decode.c | 14 ++++ gdb/cli/cli-decode.h | 6 ++ gdb/doc/python.texi | 41 ++++++++--- gdb/python/py-param.c | 84 ++++++++++++++++++++--- gdb/testsuite/gdb.python/py-parameter.exp | 36 +++++++--- 5 files changed, 151 insertions(+), 30 deletions(-) diff --git a/gdb/cli/cli-decode.c b/gdb/cli/cli-decode.c index 7266fb36d0d..af6828620eb 100644 --- a/gdb/cli/cli-decode.c +++ b/gdb/cli/cli-decode.c @@ -145,6 +145,20 @@ cmd_list_element::prefixname () const return prefixname; } +/* See cli/cli-decode.h. */ + +std::vector +cmd_list_element::command_components () const +{ + std::vector result; + + if (this->prefix != nullptr) + result = this->prefix->command_components (); + + result.emplace_back (std::string (this->name)); + return result; +} + /* Add element named NAME. Space for NAME and DOC must be allocated by the caller. CLASS is the top level category into which commands are broken down diff --git a/gdb/cli/cli-decode.h b/gdb/cli/cli-decode.h index 8f367bf4634..085c92de5bf 100644 --- a/gdb/cli/cli-decode.h +++ b/gdb/cli/cli-decode.h @@ -79,6 +79,12 @@ struct cmd_list_element For non-prefix commands, return an empty string. */ std::string prefixname () const; + /* Return a vector of strings describing the components of the full name + of this command. For example, if this command is 'set AA BB CC', + then the vector will contain 4 elements 'set', 'AA', 'BB', and 'CC' + in that order. */ + std::vector command_components () const; + /* Return true if this command is an alias of another command. */ bool is_alias () const { return this->alias_target != nullptr; } diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi index 6bd5f6b90ac..ff0103db8e3 100644 --- a/gdb/doc/python.texi +++ b/gdb/doc/python.texi @@ -4136,23 +4136,46 @@ If @var{parameter-class} is not @code{PARAM_ENUM}, then the presence of a fourth argument will cause an exception to be thrown. -The help text for the new parameter is taken from the Python -documentation string for the parameter's class, if there is one. If -there is no documentation string, a default value is used. +The help text for the new parameter includes the Python documentation +string from the parameter's class, if there is one. If there is no +documentation string, a default value is used. The documentation +string is included in the output of the parameters @code{help set} and +@code{help show} commands, and should be written taking this into +account. @end defun @defvar Parameter.set_doc If this attribute exists, and is a string, then its value is used as -the help text for this parameter's @code{set} command. The value is -examined when @code{Parameter.__init__} is invoked; subsequent changes -have no effect. +the first part of the help text for this parameter's @code{set} +command. The second part of the help text is taken from the +documentation string for the parameter's class, if there is one. + +The value of @code{set_doc} should give a brief summary specific to +the set action, this text is only displayed when the user runs the +@code{help set} command for this parameter. The class documentation +should be used to give a fuller description of what the parameter +does, this text is displayed for both the @code{help set} and +@code{help show} commands. + +The @code{set_doc} value is examined when @code{Parameter.__init__} is +invoked; subsequent changes have no effect. @end defvar @defvar Parameter.show_doc If this attribute exists, and is a string, then its value is used as -the help text for this parameter's @code{show} command. The value is -examined when @code{Parameter.__init__} is invoked; subsequent changes -have no effect. +the first part of the help text for this parameter's @code{show} +command. The second part of the help text is taken from the +documentation string for the parameter's class, if there is one. + +The value of @code{show_doc} should give a brief summary specific to +the show action, this text is only displayed when the user runs the +@code{help show} command for this parameter. The class documentation +should be used to give a fuller description of what the parameter +does, this text is displayed for both the @code{help set} and +@code{help show} commands. + +The @code{show_doc} value is examined when @code{Parameter.__init__} +is invoked; subsequent changes have no effect. @end defvar @defvar Parameter.value diff --git a/gdb/python/py-param.c b/gdb/python/py-param.c index 592ecbb41f9..3e1e0615b4d 100644 --- a/gdb/python/py-param.c +++ b/gdb/python/py-param.c @@ -331,14 +331,59 @@ set_attr (PyObject *obj, PyObject *attr_name, PyObject *val) return PyObject_GenericSetAttr (obj, attr_name, val); } +/* Build up the path to command C, but drop the first component of the + command prefix. This is only intended for use with the set/show + parameters this file deals with, the first prefix should always be + either 'set' or 'show'. + + As an example, if this full command is 'set prefix_a prefix_b command' + this function will return the string 'prefix_a prefix_b command'. */ + +static std::string +full_cmd_name_without_first_prefix (struct cmd_list_element *c) +{ + std::vector components + = c->command_components (); + gdb_assert (components.size () > 1); + std::string result = components[1]; + for (int i = 2; i < components.size (); ++i) + result += " " + components[i]; + return result; +} + +/* The different types of documentation string. */ + +enum doc_string_type +{ + doc_string_set, + doc_string_show, + doc_string_description +}; + /* A helper function which returns a documentation string for an object. */ static gdb::unique_xmalloc_ptr -get_doc_string (PyObject *object, PyObject *attr) +get_doc_string (PyObject *object, enum doc_string_type doc_type, + const char *cmd_name) { gdb::unique_xmalloc_ptr result; + PyObject *attr = nullptr; + switch (doc_type) + { + case doc_string_set: + attr = set_doc_cst; + break; + case doc_string_show: + attr = show_doc_cst; + break; + case doc_string_description: + attr = gdbpy_doc_cst; + break; + } + gdb_assert (attr != nullptr); + if (PyObject_HasAttr (object, attr)) { gdbpy_ref<> ds_obj (PyObject_GetAttr (object, attr)); @@ -350,8 +395,21 @@ get_doc_string (PyObject *object, PyObject *attr) gdbpy_print_stack (); } } - if (! result) - result.reset (xstrdup (_("This command is not documented."))); + + if (result == nullptr) + { + if (doc_type == doc_string_description) + result.reset (xstrdup (_("This command is not documented."))); + else + { + if (doc_type == doc_string_show) + result = xstrprintf (_("Show the current value of '%s'."), + cmd_name); + else + result = xstrprintf (_("Set the current value of '%s'."), + cmd_name); + } + } return result; } @@ -462,11 +520,15 @@ get_show_value (struct ui_file *file, int from_tty, } else { - /* We have to preserve the existing < GDB 7.3 API. If a - callback function does not exist, then attempt to read the - show_doc attribute. */ - show_doc_string = get_doc_string (obj, show_doc_cst); - fprintf_filtered (file, "%s %s\n", show_doc_string.get (), value); + /* If there is no 'get_show_string' callback then we want to show + something sensible here. In older versions of GDB (< 7.3) we + didn't support 'get_show_string', and instead we just made use of + GDB's builtin use of the show_doc. However, GDB's builtin + show_doc adjustment is not i18n friendly, so, instead, we just + print this generic string. */ + std::string cmd_path = full_cmd_name_without_first_prefix (c); + fprintf_filtered (file, _("The current value of '%s' is \"%s\".\n"), + cmd_path.c_str (), value); } } @@ -737,9 +799,9 @@ parmpy_init (PyObject *self, PyObject *args, PyObject *kwds) if (cmd_name == nullptr) return -1; - set_doc = get_doc_string (self, set_doc_cst); - show_doc = get_doc_string (self, show_doc_cst); - doc = get_doc_string (self, gdbpy_doc_cst); + set_doc = get_doc_string (self, doc_string_set, name); + show_doc = get_doc_string (self, doc_string_show, name); + doc = get_doc_string (self, doc_string_description, cmd_name.get ()); Py_INCREF (self); diff --git a/gdb/testsuite/gdb.python/py-parameter.exp b/gdb/testsuite/gdb.python/py-parameter.exp index b025c2562f0..1c84db7b78b 100644 --- a/gdb/testsuite/gdb.python/py-parameter.exp +++ b/gdb/testsuite/gdb.python/py-parameter.exp @@ -127,7 +127,10 @@ proc_with_prefix test_boolean_parameter { } { gdb_test "python print (test_param.value)" "False" \ "test boolean parameter value is False" gdb_test "help show print test-param" \ - "Show the state of the boolean test-param.*" "test show help" + [multi_line \ + "Show the state of the boolean test-param" \ + "When enabled, test param does something useful\\. When disabled, does nothing\\."] \ + "test show help" gdb_test "help set print test-param" \ "Set the state of the boolean test-param.*" "test set help" gdb_test "help set print" \ @@ -233,11 +236,14 @@ proc_with_prefix test_undocumented_parameter { } { gdb_test "python print (test_undoc_param.value)" \ "False" "test undocumented parameter value is False" gdb_test "help show print test-undoc-param" \ - "This command is not documented.*" "test show help" + [multi_line \ + "Show the current value of 'print test-undoc-param'\\." \ + "This command is not documented.*"] \ + "test show help" gdb_test "help set print test-undoc-param" \ "This command is not documented.*" "test set help" gdb_test "help set print" \ - "set print test-undoc-param -- This command is not documented.*" \ + "set print test-undoc-param -- Set the current value of 'print test-undoc-param'\\..*" \ "test general help" } @@ -255,19 +261,24 @@ proc_with_prefix test_really_undocumented_parameter { } { "end" gdb_test "show print test-nodoc-param" \ - "This command is not documented.*" "show parameter on" + "The current value of 'print test-nodoc-param' is \"on\"\\." \ + "show parameter on" gdb_test_no_output "set print test-nodoc-param off" \ "turn off parameter" gdb_test "show print test-nodoc-param" \ - "This command is not documented.*.*" "show parameter off" + "The current value of 'print test-nodoc-param' is \"off\"\\." \ + "show parameter off" gdb_test "python print (test_nodoc_param.value)" \ "False" "test really undocumented parameter value is False" gdb_test "help show print test-nodoc-param" \ - "This command is not documented.*" "test show help" + [multi_line \ + "Show the current value of 'print test-nodoc-param'\\." \ + "This command is not documented.*"] \ + "test show help" gdb_test "help set print test-nodoc-param" \ "This command is not documented.*" "test set help" gdb_test "help set print" \ - "set print test-nodoc-param -- This command is not documented.*" \ + "set print test-nodoc-param -- Set the current value of 'print test-nodoc-param'\\..*" \ "test general help" } @@ -290,14 +301,19 @@ proc_with_prefix test_deprecated_api_parameter { } { gdb_test "python print (test_param.value)" "True" \ "test deprecated API parameter value is True" gdb_test "show print test-param" \ - "State of the Test Parameter on.*" "show parameter on" + "The current value of 'print test-param' is \"on\"\\." \ + "show parameter on" gdb_test_no_output "set print test-param off" "turn off parameter" gdb_test "show print test-param" \ - "State of the Test Parameter off.*" "show parameter off" + "The current value of 'print test-param' is \"off\"\\." \ + "show parameter off" gdb_test "python print (test_param.value)" "False" \ "test deprecated API parameter value is False" gdb_test "help show print test-param" \ - "State of the Test Parameter.*" "test show help" + [multi_line \ + "State of the Test Parameter" \ + "When enabled, test param does something useful\\. When disabled, does nothing\\."] \ + "test show help" gdb_test "help set print test-param" \ "Set the state of the Test Parameter.*" "test set help" gdb_test "help set print" \ -- 2.25.4