From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 28883 invoked by alias); 8 Jul 2019 18:13:01 -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 28875 invoked by uid 89); 8 Jul 2019 18:13:01 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.1 spammy=styling, HX-Spam-Relays-External:209.85.221.67, H*RU:209.85.221.67, quit X-HELO: mail-wr1-f67.google.com Received: from mail-wr1-f67.google.com (HELO mail-wr1-f67.google.com) (209.85.221.67) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 08 Jul 2019 18:12:59 +0000 Received: by mail-wr1-f67.google.com with SMTP id x4so18174886wrt.6 for ; Mon, 08 Jul 2019 11:12:58 -0700 (PDT) Return-Path: Received: from ?IPv6:2001:8a0:f913:f700:56ee:75ff:fe8d:232b? ([2001:8a0:f913:f700:56ee:75ff:fe8d:232b]) by smtp.gmail.com with ESMTPSA id c3sm5138546wrx.19.2019.07.08.11.12.56 (version=TLS1_3 cipher=AEAD-AES128-GCM-SHA256 bits=128/128); Mon, 08 Jul 2019 11:12:56 -0700 (PDT) Subject: Re: [RFA] Ensure GDB printf command can print convenience var strings without a target. To: Philippe Waroquiers , gdb-patches@sourceware.org References: <20190610211622.15237-1-philippe.waroquiers@skynet.be> From: Pedro Alves Message-ID: <6b413ba9-e390-d0a5-b323-976d449b5e36@redhat.com> Date: Mon, 08 Jul 2019 18:13:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: <20190610211622.15237-1-philippe.waroquiers@skynet.be> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2019-07/txt/msg00199.txt.bz2 Looks fine to me, with the nits below fixed. On 6/10/19 10:16 PM, Philippe Waroquiers wrote: > Without this patch, GDB printf command calls malloc on the target, > writes the convenience var content to the target, > re-reads the content from the target, and then locally printf the string. > > This implies inferior calls, and does not work when there is no inferior, > or when the inferior is a core dump. > > With this patch, printf command can printf string convenience variables > without inferior function calls. > Ada string convenience variables can also be printed. > > gdb/ChangeLog > > 2019-06-10 Philippe Waroquiers > > * NEWS: Mention that GDB printf and eval commands can now print > C-style and Ada-style convenience var strings without > calling the inferior. > * printcmd.c (printf_c_string): Locally print GDB internal var > instead of transiting via the inferior. > (printf_wide_c_string): Likewise. > > 2019-06-10 Philippe Waroquiers > > * gdb.base/printcmds.exp: Test printing C strings and > C wide strings convenience var without transiting via the inferior. > --- > gdb/NEWS | 7 ++ > gdb/printcmd.c | 143 +++++++++++++++++---------- > gdb/testsuite/gdb.base/printcmds.exp | 39 ++++++++ > 3 files changed, 136 insertions(+), 53 deletions(-) > > diff --git a/gdb/NEWS b/gdb/NEWS > index 9e1462b6bf..9d6a2de661 100644 > --- a/gdb/NEWS > +++ b/gdb/NEWS > @@ -98,6 +98,13 @@ apropos [-v] REGEXP > of matching commands and to use the highlight style to mark > the documentation parts matching REGEXP. > > +printf > +eval > + The GDB printf and eval commands can now print C-style and Ada-style > + convenience variables without calling functions in the program. > + This allows to do formatted printing of strings without having > + an inferior, or when debugging a core dump. Better say without having a _running_ inferior, since there's always an inferior. > + > show style > The "show style" and its subcommands are now styling > a style name in their output using its own style, to help > diff --git a/gdb/printcmd.c b/gdb/printcmd.c > index 9e84594fe6..d7b8b9a1c1 100644 > --- a/gdb/printcmd.c > +++ b/gdb/printcmd.c > @@ -23,6 +23,7 @@ > #include "gdbtypes.h" > #include "value.h" > #include "language.h" > +#include "c-lang.h" > #include "expression.h" > #include "gdbcore.h" > #include "gdbcmd.h" > @@ -2200,91 +2201,127 @@ print_variable_and_value (const char *name, struct symbol *var, > > /* Subroutine of ui_printf to simplify it. > Print VALUE to STREAM using FORMAT. > - VALUE is a C-style string on the target. */ > + VALUE is a C-style string on the target or a C-style string > + in a GDB internal variable. */ You could avoid the repetition: VALUE is a C-style string either on the target or in a GDB internal variable. */ > > static void > printf_c_string (struct ui_file *stream, const char *format, > struct value *value) > { > - gdb_byte *str; > - CORE_ADDR tem; > - int j; > + const gdb_byte *str; > > - tem = value_as_address (value); > - if (tem == 0) > + if (VALUE_LVAL (value) == lval_internalvar > + && c_is_string_type_p (value_type (value))) > { > - DIAGNOSTIC_PUSH > - DIAGNOSTIC_IGNORE_FORMAT_NONLITERAL > - fprintf_filtered (stream, format, "(null)"); > - DIAGNOSTIC_POP > - return; > - } > + gdb_byte *tem_str; You can declare tem_str at the point of initialization. > + size_t len = TYPE_LENGTH (value_type (value)); Spurious double space after len. > > - /* This is a %s argument. Find the length of the string. */ > - for (j = 0;; j++) > + /* Copy the internal var value to tem_str and append a terminating null TEM_STR > + character. This protects against corrupted C-style strings that lacks "strings that lack" > + the terminating null char. It also allows Ada style strings (not not "Ada style strings" -> "Ada-style strings" "not not" -> "not". > + null terminated) to be printed without problems. */ > + tem_str = (gdb_byte *) alloca (len + 1); > + memcpy (tem_str, value_contents (value), len); > + tem_str [len] = 0; > + str = tem_str; > + } > + else > { > - gdb_byte c; > + int len; > + CORE_ADDR tem; > + gdb_byte *tem_str; Ditto. > > - QUIT; > - read_memory (tem + j, &c, 1); > - if (c == 0) > - break; > - } > + tem = value_as_address (value); > + if (tem == 0) > + { > + DIAGNOSTIC_PUSH > + DIAGNOSTIC_IGNORE_FORMAT_NONLITERAL > + fprintf_filtered (stream, format, "(null)"); > + DIAGNOSTIC_POP Please align all these on the same column, like it was before. > + return; > + } > > - /* Copy the string contents into a string inside GDB. */ > - str = (gdb_byte *) alloca (j + 1); > - if (j != 0) > - read_memory (tem, str, j); > - str[j] = 0; > + /* This is a %s argument. Find the length of the string. */ > + for (len = 0;; len++) > + { > + gdb_byte c; > + > + QUIT; > + read_memory (tem + len, &c, 1); > + if (c == 0) > + break; > + } > + > + /* Copy the string contents into a string inside GDB. */ > + tem_str = (gdb_byte *) alloca (len + 1); > + if (len != 0) > + read_memory (tem, tem_str, len); > + tem_str[len] = 0; > + str = tem_str; I notice this renamed "j" -> "len", but the wide version did not get the same treatment. > + } > > DIAGNOSTIC_PUSH > - DIAGNOSTIC_IGNORE_FORMAT_NONLITERAL > - fprintf_filtered (stream, format, (char *) str); > + DIAGNOSTIC_IGNORE_FORMAT_NONLITERAL > + fprintf_filtered (stream, format, (char *) str); > DIAGNOSTIC_POP Ditto. > -} > + } > > /* Subroutine of ui_printf to simplify it. > Print VALUE to STREAM using FORMAT. > - VALUE is a wide C-style string on the target. */ > + VALUE is a wide C-style string on the target or a wide C-style string > + in a GDB internal variable. */ Same comments as in the non-wide version apply. > > static void > printf_wide_c_string (struct ui_file *stream, const char *format, > struct value *value) > { > - gdb_byte *str; > - CORE_ADDR tem; > + const gdb_byte *str; > int j; > struct gdbarch *gdbarch = get_type_arch (value_type (value)); > - enum bfd_endian byte_order = gdbarch_byte_order (gdbarch); > struct type *wctype = lookup_typename (current_language, gdbarch, > "wchar_t", NULL, 0); > int wcwidth = TYPE_LENGTH (wctype); > - gdb_byte *buf = (gdb_byte *) alloca (wcwidth); > > - tem = value_as_address (value); > - if (tem == 0) > + if (VALUE_LVAL (value) == lval_internalvar > + && c_is_string_type_p (value_type (value))) > { > - DIAGNOSTIC_PUSH > - DIAGNOSTIC_IGNORE_FORMAT_NONLITERAL > - fprintf_filtered (stream, format, "(null)"); > - DIAGNOSTIC_POP > - return; > + str = value_contents (value); > + j = TYPE_LENGTH (value_type (value)); > } > - > - /* This is a %s argument. Find the length of the string. */ > - for (j = 0;; j += wcwidth) > + else > { > - QUIT; > - read_memory (tem + j, buf, wcwidth); > - if (extract_unsigned_integer (buf, wcwidth, byte_order) == 0) > - break; > - } > + gdb_byte *tem_str; > + CORE_ADDR tem; > + gdb_byte *buf = (gdb_byte *) alloca (wcwidth); > > - /* Copy the string contents into a string inside GDB. */ > - str = (gdb_byte *) alloca (j + wcwidth); > - if (j != 0) > - read_memory (tem, str, j); > - memset (&str[j], 0, wcwidth); > + enum bfd_endian byte_order = gdbarch_byte_order (gdbarch); You could move this after the following if block, since byte_order won't be needed until then. > + > + tem = value_as_address (value); > + if (tem == 0) > + { > + DIAGNOSTIC_PUSH > + DIAGNOSTIC_IGNORE_FORMAT_NONLITERAL > + fprintf_filtered (stream, format, "(null)"); > + DIAGNOSTIC_POP > + return; > + } > + > + /* This is a %s argument. Find the length of the string. */ > + for (j = 0;; j += wcwidth) > + { > + QUIT; > + read_memory (tem + j, buf, wcwidth); > + if (extract_unsigned_integer (buf, wcwidth, byte_order) == 0) > + break; > + } > + > + /* Copy the string contents into a string inside GDB. */ > + tem_str = (gdb_byte *) alloca (j + wcwidth); > + if (j != 0) > + read_memory (tem, tem_str, j); > + memset (&tem_str[j], 0, wcwidth); > + str = tem_str; > + } > > auto_obstack output; > > diff --git a/gdb/testsuite/gdb.base/printcmds.exp b/gdb/testsuite/gdb.base/printcmds.exp > index f2d6ee229d..3b6562426e 100644 > --- a/gdb/testsuite/gdb.base/printcmds.exp > +++ b/gdb/testsuite/gdb.base/printcmds.exp > @@ -932,6 +932,32 @@ proc test_repeat_bytes {} { > } > } > > +proc test_printf_convenience_var {prefix do_wstring} { Needs an intro comment. > + > + with_test_prefix $prefix { > + gdb_test_no_output "set var \$cstr = \"abcde\"" "set \$cstr, conv var" > + gdb_test "printf \"cstr val = %s\\n\", \$cstr" "cstr val = abcde" \ > + "printf \$cstr, conv var" > + gdb_test_no_output "set var \$abcde = \"ABCDE\"" "set \$abcde, conv var" > + gdb_test "eval \"print \$%s\\n\", \$cstr" "= \"ABCDE\"" \ > + "indirect print abcde" Missing ", conv var" ? But see below. > + gdb_test "set language ada" ".*" "set language ada, conv var" gdb_test_no_output ? > + gdb_test_no_output "set var \$astr := \"fghij\"" "set \$astr, conv var" > + gdb_test "printf \"astr val = %s\\n\", \$astr" "astr val = fghij" \ > + "printf \$astr, conv var" > + gdb_test "set language auto" ".*" "set language auto, conv var" gdb_test_no_output ? > + gdb_test "printf \"astr val = %s\\n\", \$astr" "astr val = fghij" \ > + "printf \$astr, conv var, auto language" > + if ($do_wstring) { Use {} instead of (). > + gdb_test_no_output "set var \$wstr = L\"facile\"" \ > + "set \$wstr, conv var" > + gdb_test "printf \"wstr val = %ls\\n\", \$wstr" \ > + "wstr val = facile" "printf \$wstr, conv var" > + } All these "conv var" in the test names seem redundant, given the whole proc body is wrapped in with_test_prefix. How about replacing all that with: - with_test_prefix $prefix { + with_test_prefix "conv var: $prefix" { > + } > +} > + > + > # Start with a fresh gdb. > > gdb_exit > @@ -948,6 +974,11 @@ gdb_test "ptype \"abc\"" " = char \\\[4\\\]" > gdb_test "print \$cvar = \"abc\"" " = \"abc\"" > gdb_test "print sizeof (\$cvar)" " = 4" > > +# Similarly, printf of convenience var should work without a target. "of convenience var" -> "of a convenience var" or "of convenience vars". Or maybe even: printf of a string convenience var > +# At this point, we cannot create wide strings convenience var, as the > +# type wchar_t is not yet known, so skip the wide string tests. "create wide strings convenience var" -> "create a wide string convenience var" "wchar_t type" -> "wchar_t type" > +test_printf_convenience_var "no target" 0 > + > # GDB used to complete the explicit location options even when > # printing expressions. > gdb_test_no_output "complete p -function" > @@ -977,6 +1008,14 @@ if ![runto_main] then { > return 0 > } > > +# With a target, printf convenience var should of course work. "With a running target" "printf convenience vars" > +test_printf_convenience_var "with target" 1 > + > +# But it should also work when inferior function calls are forbidden. "But it" -> "It". > +gdb_test_no_output "set may-call-functions off" > +test_printf_convenience_var "with target, may-call-functions off" 1 > +gdb_test_no_output "set may-call-functions on" > + > test_integer_literals_accepted > test_integer_literals_rejected > test_float_accepted > Thanks, Pedro Alves