public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
From: Andrew Burgess <aburgess@redhat.com>
To: gdb-patches@sourceware.org
Cc: Simon Marchi <simark@simark.ca>, Andrew Burgess <aburgess@redhat.com>
Subject: [PATCH 1/4] gdb: fix printf of wchar_t early in a gdb session
Date: Thu,  1 Jun 2023 10:27:50 +0100	[thread overview]
Message-ID: <fcec99feb4d222965225d2c087b84e49dbb3cd40.1685611212.git.aburgess@redhat.com> (raw)
In-Reply-To: <cover.1685611212.git.aburgess@redhat.com>

Given this test program:

  #include <wchar.h>

  const wchar_t wide_str[] = L"wide string";

  int
  main (void)
  {
    return 0;
  }

I observed this GDB behaviour:

  $ gdb -q /tmp/printf-wchar_t
  Reading symbols from /tmp/printf-wchar_t...
  (gdb) start
  Temporary breakpoint 1 at 0x40110a: file /tmp/printf-wchar_t.c, line 8.
  Starting program: /tmp/printf-wchar_t

  Temporary breakpoint 1, main () at /tmp/printf-wchar_t.c:8
  25	  return 0;
  (gdb) printf "%ls\n", wide_str

  (gdb)

Notice that the printf results in a blank line rather than the
expected 'wide string' output.

I tracked the problem down to printf_wide_c_string (in printcmd.c), in
this function we do this:

  struct type *wctype = lookup_typename (current_language,
					 "wchar_t", NULL, 0);
  int wcwidth = wctype->length ();

the problem here is that 'wchar_t' is a typedef.  If we look at the
comment on type::length() we see this:

  /* Note that if thistype is a TYPEDEF type, you have to call check_typedef.
     But check_typedef does set the TYPE_LENGTH of the TYPEDEF type,
     so you only have to call check_typedef once.  Since value::allocate
     calls check_typedef, X->type ()->length () is safe.  */

What this means is that after calling lookup_typename we should call
check_typedef in order to ensure that the length of the typedef has
been setup correctly.  We are not doing this in printf_wide_c_string,
and so wcwidth is incorrectly calculated as 0.  This is what leads GDB
to print an empty string.

We can see in c_string_operation::evaluate (in c-lang.c) an example of
calling check_typedef specifically to fix this exact issue.

Initially I did fix this problem by adding a check_typedef call into
printf_wide_c_string, but then I figured why not move the
check_typedef call up into lookup_typename itself, that feels like it
should be harmless when looking up a non-typedef type, but will avoid
bugs like this when looking up a typedef.  So that's what I did.

I can then remove the extra check_typedef call from c-lang.c, I don't
see any other places where we had extra check_typedef calls.  This
doesn't mean we definitely had bugs -- so long as we never checked the
length, or, if we knew that check_typedef had already been called,
then we would be fine.

I don't see any test regressions after this change, and my new test
case is now passing.
---
 gdb/c-lang.c                              |  3 ---
 gdb/gdbtypes.c                            | 11 ++++++----
 gdb/gdbtypes.h                            | 14 ++++++++++--
 gdb/testsuite/gdb.base/printf-wchar_t.c   | 26 +++++++++++++++++++++++
 gdb/testsuite/gdb.base/printf-wchar_t.exp | 26 +++++++++++++++++++++++
 5 files changed, 71 insertions(+), 9 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/printf-wchar_t.c
 create mode 100644 gdb/testsuite/gdb.base/printf-wchar_t.exp

diff --git a/gdb/c-lang.c b/gdb/c-lang.c
index 484f81e455b..677e76ee71b 100644
--- a/gdb/c-lang.c
+++ b/gdb/c-lang.c
@@ -615,9 +615,6 @@ c_string_operation::evaluate (struct type *expect_type,
       internal_error (_("unhandled c_string_type"));
     }
 
-  /* Ensure TYPE_LENGTH is valid for TYPE.  */
-  check_typedef (type);
-
   /* If the caller expects an array of some integral type,
      satisfy them.  If something odder is expected, rely on the
      caller to cast.  */
diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index 10e585066f7..9eecf357152 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -1648,9 +1648,7 @@ type_name_or_error (struct type *type)
 	 objfile ? objfile_name (objfile) : "<arch>");
 }
 
-/* Lookup a typedef or primitive type named NAME, visible in lexical
-   block BLOCK.  If NOERR is nonzero, return zero if NAME is not
-   suitably defined.  */
+/* See gdbtypes.h.  */
 
 struct type *
 lookup_typename (const struct language_defn *language,
@@ -1662,7 +1660,12 @@ lookup_typename (const struct language_defn *language,
   sym = lookup_symbol_in_language (name, block, VAR_DOMAIN,
 				   language->la_language, NULL).symbol;
   if (sym != NULL && sym->aclass () == LOC_TYPEDEF)
-    return sym->type ();
+    {
+      struct type *type = sym->type ();
+      /* Ensure the length of TYPE is valid.  */
+      check_typedef (type);
+      return type;
+    }
 
   if (noerr)
     return NULL;
diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
index 319a7731bca..f56de4544b5 100644
--- a/gdb/gdbtypes.h
+++ b/gdb/gdbtypes.h
@@ -2586,8 +2586,18 @@ extern void check_stub_method_group (struct type *, int);
 
 extern char *gdb_mangle_name (struct type *, int, int);
 
-extern struct type *lookup_typename (const struct language_defn *,
-				     const char *, const struct block *, int);
+/* Lookup a typedef or primitive type named NAME, visible in lexical block
+   BLOCK.  If NOERR is nonzero, return zero if NAME is not suitably
+   defined.
+
+   If this function finds a suitable type then check_typedef is called on
+   the type, this ensures that if the type being returned is a typedef
+   then the length of the type will be correct.  The original typedef will
+   still be returned, not the result of calling check_typedef.  */
+
+extern struct type *lookup_typename (const struct language_defn *language,
+				     const char *name,
+				     const struct block *block, int noerr);
 
 extern struct type *lookup_template_type (const char *, struct type *,
 					  const struct block *);
diff --git a/gdb/testsuite/gdb.base/printf-wchar_t.c b/gdb/testsuite/gdb.base/printf-wchar_t.c
new file mode 100644
index 00000000000..4d13fd3c961
--- /dev/null
+++ b/gdb/testsuite/gdb.base/printf-wchar_t.c
@@ -0,0 +1,26 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2023 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include <wchar.h>
+
+const wchar_t wide_str[] = L"wide string";
+
+int
+main (void)
+{
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/printf-wchar_t.exp b/gdb/testsuite/gdb.base/printf-wchar_t.exp
new file mode 100644
index 00000000000..85c6edf292c
--- /dev/null
+++ b/gdb/testsuite/gdb.base/printf-wchar_t.exp
@@ -0,0 +1,26 @@
+# Copyright 2023 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+standard_testfile
+
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile]} {
+    return -1
+}
+
+if {![runto_main]} {
+    return -1
+}
+
+gdb_test {printf "%ls\n", wide_str} "^wide string"
-- 
2.25.4


  reply	other threads:[~2023-06-01  9:28 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-06-01  9:27 [PATCH 0/4] Some alloca removal and a printf bug fix Andrew Burgess
2023-06-01  9:27 ` Andrew Burgess [this message]
2023-06-02 16:54   ` [PATCH 1/4] gdb: fix printf of wchar_t early in a gdb session Tom Tromey
2023-06-01  9:27 ` [PATCH 2/4] gdb: remove two uses of alloca from printcmd.c Andrew Burgess
2023-06-01  9:27 ` [PATCH 3/4] gdb: remove last alloca call " Andrew Burgess
2023-06-01  9:27 ` [PATCH 4/4] gdb: check max-value-size when reading strings for printf Andrew Burgess
2023-06-02 17:05   ` Tom Tromey
2023-06-05  9:43   ` Andrew Burgess
2023-07-04 13:20     ` Andrew Burgess
2023-07-04 13:24       ` Eli Zaretskii
2023-06-02 17:06 ` [PATCH 0/4] Some alloca removal and a printf bug fix Tom Tromey
2023-07-07 14:34   ` Andrew Burgess

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=fcec99feb4d222965225d2c087b84e49dbb3cd40.1685611212.git.aburgess@redhat.com \
    --to=aburgess@redhat.com \
    --cc=gdb-patches@sourceware.org \
    --cc=simark@simark.ca \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).