[review] gdb: Support printf 'z' size modifier

[review] gdb: Support printf 'z' size modifier

[Andrew Burgess (Code Review)]

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/511
......................................................................

gdb: Support printf 'z' size modifier

The gdb format mechanism doesn't currently support the 'z' size
modifier, there are a few places in GDB where this is used.  Instead
of removing these uses lets just add support to GDB for using 'z'.

I've not added a test for this as I ran into the issue when trying to
use some debug output.  Before this commit:

  (gdb) set debug dwarf-line 9
  (gdb) file test
  Reading symbols from test...
  Unrecognized format specifier 'z' in printf
  (No debugging symbols found in test)
  (gdb)

After this commit:

  (gdb) set debug dwarf-line 9
  (gdb) file test
  Reading symbols from test...
  Adding dir 1: /usr/include
  Adding file 1: test.c
  Adding file 2: stdc-predef.h
  Processing actual line 3: file 1, address 0x4004a0, is_stmt 1, discrim 0
  Processing actual line 4: file 1, address 0x4004a0, is_stmt 1, discrim 0
  Processing actual line 5: file 1, address 0x4004a0, is_stmt 0, discrim 0
  Processing actual line 5: file 1, address 0x4004a6, is_stmt 0, discrim 0
  Processing actual line 15: file 1, address 0x4003b0, is_stmt 1, discrim 0
  Processing actual line 16: file 1, address 0x4003b0, is_stmt 1, discrim 0
  Processing actual line 10: file 1, address 0x4003b0, is_stmt 1, discrim 0
  Processing actual line 10: file 1, address 0x4003b0, is_stmt 0, discrim 0
  Processing actual line 10: file 1, address 0x4003b7, is_stmt 0, discrim 0
  Adding dir 1: /usr/include
  Adding file 1: test.c
  Adding file 2: stdc-predef.h
  Processing actual line 3: file 1, address 0x4004a0, is_stmt 1, discrim 0
  Recording line 3, file test.c, address 0x4004a0
  Processing actual line 4: file 1, address 0x4004a0, is_stmt 1, discrim 0
  Recording line 4, file test.c, address 0x4004a0
  Processing actual line 5: file 1, address 0x4004a0, is_stmt 0, discrim 0
  Processing actual line 5: file 1, address 0x4004a6, is_stmt 0, discrim 0
  Processing actual line 15: file 1, address 0x4003b0, is_stmt 1, discrim 0
  Recording line 15, file test.c, address 0x4003b0
  Processing actual line 16: file 1, address 0x4003b0, is_stmt 1, discrim 0
  Recording line 16, file test.c, address 0x4003b0
  Processing actual line 10: file 1, address 0x4003b0, is_stmt 1, discrim 0
  Recording line 10, file test.c, address 0x4003b0
  Processing actual line 10: file 1, address 0x4003b0, is_stmt 0, discrim 0
  Processing actual line 10: file 1, address 0x4003b7, is_stmt 0, discrim 0
  (gdb)

gdb/ChangeLog:

	* gdbsupport/format.c (format_pieces::format_pieces): Support
	printf 'z' size modifier.

Change-Id: Ib6c44d88aa5bce265d757e4c0698881803dd186f
---
M gdb/ChangeLog
M gdb/gdbsupport/format.c
2 files changed, 8 insertions(+), 0 deletions(-)



diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 41daef6..d29fffe 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2019-11-05  Andrew Burgess  <andrew.burgess@embecosm.com>
+
+	* gdbsupport/format.c (format_pieces::format_pieces): Support
+	printf 'z' size modifier.
+
 2019-11-04  Christian Biesinger  <cbiesinger@google.com>
 
 	* psympriv.h: Add static_asserts for sizeof (general_symbol_info)
diff --git a/gdb/gdbsupport/format.c b/gdb/gdbsupport/format.c
index 1e80350..fea73c8 100644
--- a/gdb/gdbsupport/format.c
+++ b/gdb/gdbsupport/format.c
@@ -234,6 +234,9 @@
 	    else
 	      seen_big_d = 1;
 	  }
+	/* For size_t or ssize_t.  */
+	else if (*f == 'z')
+	  f++;
 
 	switch (*f)
 	  {

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Ib6c44d88aa5bce265d757e4c0698881803dd186f
Gerrit-Change-Number: 511
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Burgess <andrew.burgess@embecosm.com>
Gerrit-MessageType: newchange

[review] gdb: Support printf 'z' size modifier

[Tom Tromey (Code Review)]

Tom Tromey has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/511
......................................................................


Patch Set 1:

(1 comment)

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/511/1/gdb/gdbsupport/format.c 
File gdb/gdbsupport/format.c:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/511/1/gdb/gdbsupport/format.c@237 
PS1, Line 237: 
 23 | format_pieces::format_pieces (const char **arg, bool gdb_extensions)
    | ...
232 | 		seen_double_big_d = 1;
233 | 	      }
234 | 	    else
235 | 	      seen_big_d = 1;
236 | 	  }
237 > 	/* For size_t or ssize_t.  */
238 > 	else if (*f == 'z')
239 > 	  f++;
240 | 
241 | 	switch (*f)
242 | 	  {
243 | 	  case 'u':
244 | 	    if (seen_hash)

This seems necessary but perhaps not sufficient -- nothing
seems to use the 'z' flag to change the size of the expected
argument.



-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Ib6c44d88aa5bce265d757e4c0698881803dd186f
Gerrit-Change-Number: 511
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Burgess <andrew.burgess@embecosm.com>
Gerrit-CC: Tom Tromey <tromey@sourceware.org>
Gerrit-Comment-Date: Tue, 05 Nov 2019 16:22:26 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[review] gdb: Support printf 'z' size modifier

[Simon Marchi (Code Review)]

Simon Marchi has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/511
......................................................................


Patch Set 1:

(1 comment)

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/511/1//COMMIT_MSG 
Commit Message:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/511/1//COMMIT_MSG@13 
PS1, Line 13: 
 8 | 
 9 | The gdb format mechanism doesn't currently support the 'z' size
10 | modifier, there are a few places in GDB where this is used.  Instead
11 | of removing these uses lets just add support to GDB for using 'z'.
12 | 
13 > I've not added a test for this as I ran into the issue when trying to
14 > use some debug output.  Before this commit:
15 | 
16 |   (gdb) set debug dwarf-line 9
17 |   (gdb) file test
18 |   Reading symbols from test...
19 |   Unrecognized format specifier 'z' in printf

It would seem easy enough to add an entry in test_format_specifier, in unittests/format_pieces-selftests.c, I think it would be a good idea to add it.



-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Ib6c44d88aa5bce265d757e4c0698881803dd186f
Gerrit-Change-Number: 511
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Burgess <andrew.burgess@embecosm.com>
Gerrit-CC: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-CC: Tom Tromey <tromey@sourceware.org>
Gerrit-Comment-Date: Tue, 05 Nov 2019 16:31:43 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[review] gdb: Support printf 'z' size modifier

[Pedro Alves (Code Review)]

Pedro Alves has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/511
......................................................................


Patch Set 1:

(1 comment)

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/511/1//COMMIT_MSG 
Commit Message:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/511/1//COMMIT_MSG@11 
PS1, Line 11: 
 6 | 
 7 | gdb: Support printf 'z' size modifier
 8 | 
 9 | The gdb format mechanism doesn't currently support the 'z' size
10 | modifier, there are a few places in GDB where this is used.  Instead
11 > of removing these uses lets just add support to GDB for using 'z'.
12 | 
13 | I've not added a test for this as I ran into the issue when trying to
14 | use some debug output.  Before this commit:
15 | 
16 |   (gdb) set debug dwarf-line 9

Just FYI, we didn't use to allow 'z' since it is a C99 feature (and thus C++11), and older compilers didn't support it, namely the MSVC runtime used by mingw.org.  I think that is passed us nowadays, and I think mingw nowadays has replacement printf that support C99 features.  And if it doesn't likely gnulib is taking care of it.



-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Ib6c44d88aa5bce265d757e4c0698881803dd186f
Gerrit-Change-Number: 511
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Burgess <andrew.burgess@embecosm.com>
Gerrit-CC: Pedro Alves <palves@redhat.com>
Gerrit-CC: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-CC: Tom Tromey <tromey@sourceware.org>
Gerrit-Comment-Date: Wed, 06 Nov 2019 00:18:15 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[review v2] gdb: Support printf 'z' size modifier

[Andrew Burgess (Code Review)]

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/511
......................................................................

gdb: Support printf 'z' size modifier

The gdb format mechanism doesn't currently support the 'z' size
modifier, there are a few places in GDB where this is used.  Instead
of removing these uses lets just add support to GDB for using 'z'.

I found this issue when trying to use some of the debug output.
Before this commit:

  (gdb) set debug dwarf-line 9
  (gdb) file test
  Reading symbols from test...
  Unrecognized format specifier 'z' in printf
  (No debugging symbols found in test)
  (gdb)

After this commit:

  (gdb) set debug dwarf-line 9
  (gdb) file test
  Reading symbols from test...
  Adding dir 1: /usr/include
  Adding file 1: test.c
  Adding file 2: stdc-predef.h
  Processing actual line 3: file 1, address 0x4004a0, is_stmt 1, discrim 0
  Processing actual line 4: file 1, address 0x4004a0, is_stmt 1, discrim 0
  .... lots of debug output ...
  Processing actual line 10: file 1, address 0x4003b7, is_stmt 0, discrim 0
  (gdb)

I've added a self test to cover the integer format size modifiers,
including the 'z' modifier.

gdb/ChangeLog:

	* gdbsupport/format.c (format_pieces::format_pieces): Support
	printf 'z' size modifier.
	* gdbsupport/format.h (enum argclass): Add size_t_arg.
	* printcmd.c (ui_printf):  Handle size_t_arg.
	* ui-out.c (ui_out::vmessage): Likewise.
	* unittests/format_pieces-selftests.c (test_format_int_sizes): New
	function.
	(run_tests): Call test_format_int_sizes.

gdb/gdbserver/ChangeLog:

	* ax.c (ax_printf): Handle size_t_arg.

Change-Id: Ib6c44d88aa5bce265d757e4c0698881803dd186f
---
M gdb/ChangeLog
M gdb/gdbserver/ChangeLog
M gdb/gdbserver/ax.c
M gdb/gdbsupport/format.c
M gdb/gdbsupport/format.h
M gdb/printcmd.c
M gdb/ui-out.c
M gdb/unittests/format_pieces-selftests.c
8 files changed, 106 insertions(+), 2 deletions(-)



diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 41daef6..860037f 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,14 @@
+2019-11-05  Andrew Burgess  <andrew.burgess@embecosm.com>
+
+	* gdbsupport/format.c (format_pieces::format_pieces): Support
+	printf 'z' size modifier.
+	* gdbsupport/format.h (enum argclass): Add size_t_arg.
+	* printcmd.c (ui_printf):  Handle size_t_arg.
+	* ui-out.c (ui_out::vmessage): Likewise.
+	* unittests/format_pieces-selftests.c (test_format_int_sizes): New
+	function.
+	(run_tests): Call test_format_int_sizes.
+
 2019-11-04  Christian Biesinger  <cbiesinger@google.com>
 
 	* psympriv.h: Add static_asserts for sizeof (general_symbol_info)
diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog
index 35684db..a69a320 100644
--- a/gdb/gdbserver/ChangeLog
+++ b/gdb/gdbserver/ChangeLog
@@ -1,3 +1,7 @@
+2019-11-07  Andrew Burgess  <andrew.burgess@embecosm.com>
+
+	* ax.c (ax_printf): Handle size_t_arg.
+
 2019-11-01  Christian Biesinger  <cbiesinger@google.com>
 
 	* configure: Regenerate.
diff --git a/gdb/gdbserver/ax.c b/gdb/gdbserver/ax.c
index 2c096ef..21fde5e 100644
--- a/gdb/gdbserver/ax.c
+++ b/gdb/gdbserver/ax.c
@@ -904,6 +904,14 @@
 	    break;
 	  }
 
+	case size_t_arg:
+	  {
+	    size_t val = args[i];
+
+	    printf (current_substring, val);
+	    break;
+	  }
+
 	case literal_piece:
 	  /* Print a portion of the format string that has no
 	     directives.  Note that this will not include any
diff --git a/gdb/gdbsupport/format.c b/gdb/gdbsupport/format.c
index 1e80350..2e2d90a 100644
--- a/gdb/gdbsupport/format.c
+++ b/gdb/gdbsupport/format.c
@@ -123,6 +123,7 @@
 	int seen_space = 0, seen_plus = 0;
 	int seen_big_l = 0, seen_h = 0, seen_big_h = 0;
 	int seen_big_d = 0, seen_double_big_d = 0;
+	int seen_size_t = 0;
 	int bad = 0;
 	int n_int_args = 0;
 
@@ -234,6 +235,12 @@
 	    else
 	      seen_big_d = 1;
 	  }
+	/* For size_t or ssize_t.  */
+	else if (*f == 'z')
+	  {
+	    seen_size_t = 1;
+	    f++;
+	  }
 
 	switch (*f)
 	  {
@@ -251,7 +258,9 @@
 
 	  case 'd':
 	  case 'i':
-	    if (lcount == 0)
+	    if (seen_size_t)
+	      this_argclass = size_t_arg;
+	    else if (lcount == 0)
 	      this_argclass = int_arg;
 	    else if (lcount == 1)
 	      this_argclass = long_arg;
diff --git a/gdb/gdbsupport/format.h b/gdb/gdbsupport/format.h
index e2a47ba..07374db 100644
--- a/gdb/gdbsupport/format.h
+++ b/gdb/gdbsupport/format.h
@@ -38,7 +38,7 @@
 enum argclass
   {
     literal_piece,
-    int_arg, long_arg, long_long_arg, ptr_arg,
+    int_arg, long_arg, long_long_arg, size_t_arg, ptr_arg,
     string_arg, wide_string_arg, wide_char_arg,
     double_arg, long_double_arg,
     dec32float_arg, dec64float_arg, dec128float_arg
diff --git a/gdb/printcmd.c b/gdb/printcmd.c
index 27aaf7a..323a5d0 100644
--- a/gdb/printcmd.c
+++ b/gdb/printcmd.c
@@ -2663,6 +2663,16 @@
 	      DIAGNOSTIC_POP
 	      break;
 	    }
+	  case size_t_arg:
+	    {
+	      size_t val = value_as_long (val_args[i]);
+
+	      DIAGNOSTIC_PUSH
+	      DIAGNOSTIC_IGNORE_FORMAT_NONLITERAL
+              fprintf_filtered (stream, current_substring, val);
+	      DIAGNOSTIC_POP
+	      break;
+	    }
 	  /* Handles floating-point values.  */
 	  case double_arg:
 	  case long_double_arg:
diff --git a/gdb/ui-out.c b/gdb/ui-out.c
index 6b0b5ac..80845f4 100644
--- a/gdb/ui-out.c
+++ b/gdb/ui-out.c
@@ -688,6 +688,24 @@
 	      }
 	  }
 	  break;
+	case size_t_arg:
+	  {
+	    size_t val = va_arg (args, size_t);
+	    switch (piece.n_int_args)
+	      {
+	      case 0:
+		call_do_message (style, current_substring, val);
+		break;
+	      case 1:
+		call_do_message (style, current_substring, intvals[0], val);
+		break;
+	      case 2:
+		call_do_message (style, current_substring,
+				 intvals[0], intvals[1], val);
+		break;
+	      }
+	  }
+	  break;
 	case double_arg:
 	  call_do_message (style, current_substring, va_arg (args, double));
 	  break;
diff --git a/gdb/unittests/format_pieces-selftests.c b/gdb/unittests/format_pieces-selftests.c
index ed83d96..3971201 100644
--- a/gdb/unittests/format_pieces-selftests.c
+++ b/gdb/unittests/format_pieces-selftests.c
@@ -76,12 +76,56 @@
     }, true);
 }
 
+/* Test the different size modifiers that can be applied to an integer
+   argument.  Test with different integer format specifiers too.  */
+
+static void
+test_format_int_sizes ()
+{
+  check ("Hello\\t %hu%lu%llu%zu", /* ARI: %ll */
+    {
+      format_piece ("Hello\t ", literal_piece, 0),
+      format_piece ("%hu", int_arg, 0),
+      format_piece ("%lu", long_arg, 0),
+      format_piece ("%llu", long_long_arg, 0), /* ARI: %ll */
+      format_piece ("%zu", size_t_arg, 0)
+    });
+
+  check ("Hello\\t %hx%lx%llx%zx", /* ARI: %ll */
+    {
+      format_piece ("Hello\t ", literal_piece, 0),
+      format_piece ("%hx", int_arg, 0),
+      format_piece ("%lx", long_arg, 0),
+      format_piece ("%llx", long_long_arg, 0), /* ARI: %ll */
+      format_piece ("%zx", size_t_arg, 0)
+    });
+
+  check ("Hello\\t %ho%lo%llo%zo", /* ARI: %ll */
+    {
+      format_piece ("Hello\t ", literal_piece, 0),
+      format_piece ("%ho", int_arg, 0),
+      format_piece ("%lo", long_arg, 0),
+      format_piece ("%llo", long_long_arg, 0), /* ARI: %ll */
+      format_piece ("%zo", size_t_arg, 0)
+    });
+
+  check ("Hello\\t %hd%ld%lld%zd", /* ARI: %ll */
+    {
+      format_piece ("Hello\t ", literal_piece, 0),
+      format_piece ("%hd", int_arg, 0),
+      format_piece ("%ld", long_arg, 0),
+      format_piece ("%lld", long_long_arg, 0), /* ARI: %ll */
+      format_piece ("%zd", size_t_arg, 0)
+    });
+}
+
 static void
 run_tests ()
 {
   test_escape_sequences ();
   test_format_specifier ();
   test_gdb_formats ();
+  test_format_int_sizes ();
 }
 
 } /* namespace format_pieces */

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Ib6c44d88aa5bce265d757e4c0698881803dd186f
Gerrit-Change-Number: 511
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Burgess <andrew.burgess@embecosm.com>
Gerrit-CC: Pedro Alves <palves@redhat.com>
Gerrit-CC: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-CC: Tom Tromey <tromey@sourceware.org>
Gerrit-MessageType: newpatchset

[review v2] gdb: Support printf 'z' size modifier

[Andrew Burgess (Code Review)]

Andrew Burgess has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/511
......................................................................


Patch Set 2:

(3 comments)

New version should hopefully correctly handle the argument as a different size and includes some selftests.

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/511/1//COMMIT_MSG 
Commit Message:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/511/1//COMMIT_MSG@11 
PS1, Line 11: 
 6 | 
 7 | gdb: Support printf 'z' size modifier
 8 | 
 9 | The gdb format mechanism doesn't currently support the 'z' size
10 | modifier, there are a few places in GDB where this is used.  Instead
11 > of removing these uses lets just add support to GDB for using 'z'.
12 | 
13 | I've not added a test for this as I ran into the issue when trying to
14 | use some debug output.  Before this commit:
15 | 
16 |   (gdb) set debug dwarf-line 9

> Just FYI, we didn't use to allow 'z' since it is a C99 feature (and thus C++11), and older compilers […]

I did consider just replacing all uses of %z with something else and casting the argument, there aren't that many uses so the patch would be pretty similar in size to what I've now posted.  Let me know if you'd prefer this approach.


https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/511/1//COMMIT_MSG@13 
PS1, Line 13: 
 8 | 
 9 | The gdb format mechanism doesn't currently support the 'z' size
10 | modifier, there are a few places in GDB where this is used.  Instead
11 | of removing these uses lets just add support to GDB for using 'z'.
12 | 
13 > I've not added a test for this as I ran into the issue when trying to
14 > use some debug output.  Before this commit:
15 | 
16 |   (gdb) set debug dwarf-line 9
17 |   (gdb) file test
18 |   Reading symbols from test...
19 |   Unrecognized format specifier 'z' in printf

> It would seem easy enough to add an entry in test_format_specifier, in unittests/format_pieces-selft […]

Done.


https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/511/1/gdb/gdbsupport/format.c 
File gdb/gdbsupport/format.c:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/511/1/gdb/gdbsupport/format.c@237 
PS1, Line 237: 
 23 | format_pieces::format_pieces (const char **arg, bool gdb_extensions)
    | ...
232 | 		seen_double_big_d = 1;
233 | 	      }
234 | 	    else
235 | 	      seen_big_d = 1;
236 | 	  }
237 > 	/* For size_t or ssize_t.  */
238 > 	else if (*f == 'z')
239 > 	  f++;
240 | 
241 | 	switch (*f)
242 | 	  {
243 | 	  case 'u':
244 | 	    if (seen_hash)

> This seems necessary but perhaps not sufficient -- nothing […]

Thanks for this.  I think I was fooled by it "just working" when treating the size_t as an int.  I believe I've now handled this correctly.



-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Ib6c44d88aa5bce265d757e4c0698881803dd186f
Gerrit-Change-Number: 511
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Burgess <andrew.burgess@embecosm.com>
Gerrit-Reviewer: Andrew Burgess <andrew.burgess@embecosm.com>
Gerrit-CC: Pedro Alves <palves@redhat.com>
Gerrit-CC: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-CC: Tom Tromey <tromey@sourceware.org>
Gerrit-Comment-Date: Thu, 07 Nov 2019 11:05:23 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Pedro Alves <palves@redhat.com>
Comment-In-Reply-To: Tom Tromey <tromey@sourceware.org>
Comment-In-Reply-To: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-MessageType: comment

[review v2] gdb: Support printf 'z' size modifier

[Kevin Buettner (Code Review)]

Kevin Buettner has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/511
......................................................................


Patch Set 2: Code-Review+2


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Ib6c44d88aa5bce265d757e4c0698881803dd186f
Gerrit-Change-Number: 511
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Burgess <andrew.burgess@embecosm.com>
Gerrit-Reviewer: Andrew Burgess <andrew.burgess@embecosm.com>
Gerrit-Reviewer: Kevin Buettner <kevinb@redhat.com>
Gerrit-CC: Joel Brobecker <brobecker@adacore.com>
Gerrit-CC: Pedro Alves <palves@redhat.com>
Gerrit-CC: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-CC: Tom Tromey <tromey@sourceware.org>
Gerrit-Comment-Date: Tue, 12 Nov 2019 20:18:59 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[pushed] gdb: Support printf 'z' size modifier

[Sourceware to Gerrit sync (Code Review)]

The original change was created by Andrew Burgess.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/511
......................................................................

gdb: Support printf 'z' size modifier

The gdb format mechanism doesn't currently support the 'z' size
modifier, there are a few places in GDB where this is used.  Instead
of removing these uses lets just add support to GDB for using 'z'.

I found this issue when trying to use some of the debug output.
Before this commit:

  (gdb) set debug dwarf-line 9
  (gdb) file test
  Reading symbols from test...
  Unrecognized format specifier 'z' in printf
  (No debugging symbols found in test)
  (gdb)

After this commit:

  (gdb) set debug dwarf-line 9
  (gdb) file test
  Reading symbols from test...
  Adding dir 1: /usr/include
  Adding file 1: test.c
  Adding file 2: stdc-predef.h
  Processing actual line 3: file 1, address 0x4004a0, is_stmt 1, discrim 0
  Processing actual line 4: file 1, address 0x4004a0, is_stmt 1, discrim 0
  .... lots of debug output ...
  Processing actual line 10: file 1, address 0x4003b7, is_stmt 0, discrim 0
  (gdb)

I've added a self test to cover the integer format size modifiers,
including the 'z' modifier.

gdb/ChangeLog:

	* gdbsupport/format.c (format_pieces::format_pieces): Support
	printf 'z' size modifier.
	* gdbsupport/format.h (enum argclass): Add size_t_arg.
	* printcmd.c (ui_printf):  Handle size_t_arg.
	* ui-out.c (ui_out::vmessage): Likewise.
	* unittests/format_pieces-selftests.c (test_format_int_sizes): New
	function.
	(run_tests): Call test_format_int_sizes.

gdb/gdbserver/ChangeLog:

	* ax.c (ax_printf): Handle size_t_arg.

Change-Id: Ib6c44d88aa5bce265d757e4c0698881803dd186f
---
M gdb/ChangeLog
M gdb/gdbserver/ChangeLog
M gdb/gdbserver/ax.c
M gdb/gdbsupport/format.c
M gdb/gdbsupport/format.h
M gdb/printcmd.c
M gdb/ui-out.c
M gdb/unittests/format_pieces-selftests.c
8 files changed, 106 insertions(+), 2 deletions(-)



diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 44ace04..4f40ea8 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,14 @@
+2019-11-12  Andrew Burgess  <andrew.burgess@embecosm.com>
+
+	* gdbsupport/format.c (format_pieces::format_pieces): Support
+	printf 'z' size modifier.
+	* gdbsupport/format.h (enum argclass): Add size_t_arg.
+	* printcmd.c (ui_printf):  Handle size_t_arg.
+	* ui-out.c (ui_out::vmessage): Likewise.
+	* unittests/format_pieces-selftests.c (test_format_int_sizes): New
+	function.
+	(run_tests): Call test_format_int_sizes.
+
 2019-11-12  Christian Biesinger  <cbiesinger@google.com>
 
 	* ada-exp.y (write_ambiguous_var): Update.
diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog
index 9d0afaa..e17a7ca 100644
--- a/gdb/gdbserver/ChangeLog
+++ b/gdb/gdbserver/ChangeLog
@@ -1,3 +1,7 @@
+2019-11-12  Andrew Burgess  <andrew.burgess@embecosm.com>
+
+	* ax.c (ax_printf): Handle size_t_arg.
+
 2019-11-06  Christian Biesinger  <cbiesinger@google.com>
 
 	* linux-tdep.c (linux_info_proc): Use strtok_r instead of strtok.
diff --git a/gdb/gdbserver/ax.c b/gdb/gdbserver/ax.c
index 2c096ef..21fde5e 100644
--- a/gdb/gdbserver/ax.c
+++ b/gdb/gdbserver/ax.c
@@ -904,6 +904,14 @@
 	    break;
 	  }
 
+	case size_t_arg:
+	  {
+	    size_t val = args[i];
+
+	    printf (current_substring, val);
+	    break;
+	  }
+
 	case literal_piece:
 	  /* Print a portion of the format string that has no
 	     directives.  Note that this will not include any
diff --git a/gdb/gdbsupport/format.c b/gdb/gdbsupport/format.c
index 1e80350..2e2d90a 100644
--- a/gdb/gdbsupport/format.c
+++ b/gdb/gdbsupport/format.c
@@ -123,6 +123,7 @@
 	int seen_space = 0, seen_plus = 0;
 	int seen_big_l = 0, seen_h = 0, seen_big_h = 0;
 	int seen_big_d = 0, seen_double_big_d = 0;
+	int seen_size_t = 0;
 	int bad = 0;
 	int n_int_args = 0;
 
@@ -234,6 +235,12 @@
 	    else
 	      seen_big_d = 1;
 	  }
+	/* For size_t or ssize_t.  */
+	else if (*f == 'z')
+	  {
+	    seen_size_t = 1;
+	    f++;
+	  }
 
 	switch (*f)
 	  {
@@ -251,7 +258,9 @@
 
 	  case 'd':
 	  case 'i':
-	    if (lcount == 0)
+	    if (seen_size_t)
+	      this_argclass = size_t_arg;
+	    else if (lcount == 0)
 	      this_argclass = int_arg;
 	    else if (lcount == 1)
 	      this_argclass = long_arg;
diff --git a/gdb/gdbsupport/format.h b/gdb/gdbsupport/format.h
index e2a47ba..07374db 100644
--- a/gdb/gdbsupport/format.h
+++ b/gdb/gdbsupport/format.h
@@ -38,7 +38,7 @@
 enum argclass
   {
     literal_piece,
-    int_arg, long_arg, long_long_arg, ptr_arg,
+    int_arg, long_arg, long_long_arg, size_t_arg, ptr_arg,
     string_arg, wide_string_arg, wide_char_arg,
     double_arg, long_double_arg,
     dec32float_arg, dec64float_arg, dec128float_arg
diff --git a/gdb/printcmd.c b/gdb/printcmd.c
index 27aaf7a..323a5d0 100644
--- a/gdb/printcmd.c
+++ b/gdb/printcmd.c
@@ -2663,6 +2663,16 @@
 	      DIAGNOSTIC_POP
 	      break;
 	    }
+	  case size_t_arg:
+	    {
+	      size_t val = value_as_long (val_args[i]);
+
+	      DIAGNOSTIC_PUSH
+	      DIAGNOSTIC_IGNORE_FORMAT_NONLITERAL
+              fprintf_filtered (stream, current_substring, val);
+	      DIAGNOSTIC_POP
+	      break;
+	    }
 	  /* Handles floating-point values.  */
 	  case double_arg:
 	  case long_double_arg:
diff --git a/gdb/ui-out.c b/gdb/ui-out.c
index 6b0b5ac..80845f4 100644
--- a/gdb/ui-out.c
+++ b/gdb/ui-out.c
@@ -688,6 +688,24 @@
 	      }
 	  }
 	  break;
+	case size_t_arg:
+	  {
+	    size_t val = va_arg (args, size_t);
+	    switch (piece.n_int_args)
+	      {
+	      case 0:
+		call_do_message (style, current_substring, val);
+		break;
+	      case 1:
+		call_do_message (style, current_substring, intvals[0], val);
+		break;
+	      case 2:
+		call_do_message (style, current_substring,
+				 intvals[0], intvals[1], val);
+		break;
+	      }
+	  }
+	  break;
 	case double_arg:
 	  call_do_message (style, current_substring, va_arg (args, double));
 	  break;
diff --git a/gdb/unittests/format_pieces-selftests.c b/gdb/unittests/format_pieces-selftests.c
index ed83d96..3971201 100644
--- a/gdb/unittests/format_pieces-selftests.c
+++ b/gdb/unittests/format_pieces-selftests.c
@@ -76,12 +76,56 @@
     }, true);
 }
 
+/* Test the different size modifiers that can be applied to an integer
+   argument.  Test with different integer format specifiers too.  */
+
+static void
+test_format_int_sizes ()
+{
+  check ("Hello\\t %hu%lu%llu%zu", /* ARI: %ll */
+    {
+      format_piece ("Hello\t ", literal_piece, 0),
+      format_piece ("%hu", int_arg, 0),
+      format_piece ("%lu", long_arg, 0),
+      format_piece ("%llu", long_long_arg, 0), /* ARI: %ll */
+      format_piece ("%zu", size_t_arg, 0)
+    });
+
+  check ("Hello\\t %hx%lx%llx%zx", /* ARI: %ll */
+    {
+      format_piece ("Hello\t ", literal_piece, 0),
+      format_piece ("%hx", int_arg, 0),
+      format_piece ("%lx", long_arg, 0),
+      format_piece ("%llx", long_long_arg, 0), /* ARI: %ll */
+      format_piece ("%zx", size_t_arg, 0)
+    });
+
+  check ("Hello\\t %ho%lo%llo%zo", /* ARI: %ll */
+    {
+      format_piece ("Hello\t ", literal_piece, 0),
+      format_piece ("%ho", int_arg, 0),
+      format_piece ("%lo", long_arg, 0),
+      format_piece ("%llo", long_long_arg, 0), /* ARI: %ll */
+      format_piece ("%zo", size_t_arg, 0)
+    });
+
+  check ("Hello\\t %hd%ld%lld%zd", /* ARI: %ll */
+    {
+      format_piece ("Hello\t ", literal_piece, 0),
+      format_piece ("%hd", int_arg, 0),
+      format_piece ("%ld", long_arg, 0),
+      format_piece ("%lld", long_long_arg, 0), /* ARI: %ll */
+      format_piece ("%zd", size_t_arg, 0)
+    });
+}
+
 static void
 run_tests ()
 {
   test_escape_sequences ();
   test_format_specifier ();
   test_gdb_formats ();
+  test_format_int_sizes ();
 }
 
 } /* namespace format_pieces */

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Ib6c44d88aa5bce265d757e4c0698881803dd186f
Gerrit-Change-Number: 511
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Burgess <andrew.burgess@embecosm.com>
Gerrit-Reviewer: Andrew Burgess <andrew.burgess@embecosm.com>
Gerrit-Reviewer: Kevin Buettner <kevinb@redhat.com>
Gerrit-CC: Joel Brobecker <brobecker@adacore.com>
Gerrit-CC: Pedro Alves <palves@redhat.com>
Gerrit-CC: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-CC: Tom Tromey <tromey@sourceware.org>
Gerrit-MessageType: newpatchset

[pushed] gdb: Support printf 'z' size modifier

[Sourceware to Gerrit sync (Code Review)]

Sourceware to Gerrit sync has submitted this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/511
......................................................................

gdb: Support printf 'z' size modifier

The gdb format mechanism doesn't currently support the 'z' size
modifier, there are a few places in GDB where this is used.  Instead
of removing these uses lets just add support to GDB for using 'z'.

I found this issue when trying to use some of the debug output.
Before this commit:

  (gdb) set debug dwarf-line 9
  (gdb) file test
  Reading symbols from test...
  Unrecognized format specifier 'z' in printf
  (No debugging symbols found in test)
  (gdb)

After this commit:

  (gdb) set debug dwarf-line 9
  (gdb) file test
  Reading symbols from test...
  Adding dir 1: /usr/include
  Adding file 1: test.c
  Adding file 2: stdc-predef.h
  Processing actual line 3: file 1, address 0x4004a0, is_stmt 1, discrim 0
  Processing actual line 4: file 1, address 0x4004a0, is_stmt 1, discrim 0
  .... lots of debug output ...
  Processing actual line 10: file 1, address 0x4003b7, is_stmt 0, discrim 0
  (gdb)

I've added a self test to cover the integer format size modifiers,
including the 'z' modifier.

gdb/ChangeLog:

	* gdbsupport/format.c (format_pieces::format_pieces): Support
	printf 'z' size modifier.
	* gdbsupport/format.h (enum argclass): Add size_t_arg.
	* printcmd.c (ui_printf):  Handle size_t_arg.
	* ui-out.c (ui_out::vmessage): Likewise.
	* unittests/format_pieces-selftests.c (test_format_int_sizes): New
	function.
	(run_tests): Call test_format_int_sizes.

gdb/gdbserver/ChangeLog:

	* ax.c (ax_printf): Handle size_t_arg.

Change-Id: Ib6c44d88aa5bce265d757e4c0698881803dd186f
---
M gdb/ChangeLog
M gdb/gdbserver/ChangeLog
M gdb/gdbserver/ax.c
M gdb/gdbsupport/format.c
M gdb/gdbsupport/format.h
M gdb/printcmd.c
M gdb/ui-out.c
M gdb/unittests/format_pieces-selftests.c
8 files changed, 106 insertions(+), 2 deletions(-)


diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 44ace04..4f40ea8 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,14 @@
+2019-11-12  Andrew Burgess  <andrew.burgess@embecosm.com>
+
+	* gdbsupport/format.c (format_pieces::format_pieces): Support
+	printf 'z' size modifier.
+	* gdbsupport/format.h (enum argclass): Add size_t_arg.
+	* printcmd.c (ui_printf):  Handle size_t_arg.
+	* ui-out.c (ui_out::vmessage): Likewise.
+	* unittests/format_pieces-selftests.c (test_format_int_sizes): New
+	function.
+	(run_tests): Call test_format_int_sizes.
+
 2019-11-12  Christian Biesinger  <cbiesinger@google.com>
 
 	* ada-exp.y (write_ambiguous_var): Update.
diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog
index 9d0afaa..e17a7ca 100644
--- a/gdb/gdbserver/ChangeLog
+++ b/gdb/gdbserver/ChangeLog
@@ -1,3 +1,7 @@
+2019-11-12  Andrew Burgess  <andrew.burgess@embecosm.com>
+
+	* ax.c (ax_printf): Handle size_t_arg.
+
 2019-11-06  Christian Biesinger  <cbiesinger@google.com>
 
 	* linux-tdep.c (linux_info_proc): Use strtok_r instead of strtok.
diff --git a/gdb/gdbserver/ax.c b/gdb/gdbserver/ax.c
index 2c096ef..21fde5e 100644
--- a/gdb/gdbserver/ax.c
+++ b/gdb/gdbserver/ax.c
@@ -904,6 +904,14 @@
 	    break;
 	  }
 
+	case size_t_arg:
+	  {
+	    size_t val = args[i];
+
+	    printf (current_substring, val);
+	    break;
+	  }
+
 	case literal_piece:
 	  /* Print a portion of the format string that has no
 	     directives.  Note that this will not include any
diff --git a/gdb/gdbsupport/format.c b/gdb/gdbsupport/format.c
index 1e80350..2e2d90a 100644
--- a/gdb/gdbsupport/format.c
+++ b/gdb/gdbsupport/format.c
@@ -123,6 +123,7 @@
 	int seen_space = 0, seen_plus = 0;
 	int seen_big_l = 0, seen_h = 0, seen_big_h = 0;
 	int seen_big_d = 0, seen_double_big_d = 0;
+	int seen_size_t = 0;
 	int bad = 0;
 	int n_int_args = 0;
 
@@ -234,6 +235,12 @@
 	    else
 	      seen_big_d = 1;
 	  }
+	/* For size_t or ssize_t.  */
+	else if (*f == 'z')
+	  {
+	    seen_size_t = 1;
+	    f++;
+	  }
 
 	switch (*f)
 	  {
@@ -251,7 +258,9 @@
 
 	  case 'd':
 	  case 'i':
-	    if (lcount == 0)
+	    if (seen_size_t)
+	      this_argclass = size_t_arg;
+	    else if (lcount == 0)
 	      this_argclass = int_arg;
 	    else if (lcount == 1)
 	      this_argclass = long_arg;
diff --git a/gdb/gdbsupport/format.h b/gdb/gdbsupport/format.h
index e2a47ba..07374db 100644
--- a/gdb/gdbsupport/format.h
+++ b/gdb/gdbsupport/format.h
@@ -38,7 +38,7 @@
 enum argclass
   {
     literal_piece,
-    int_arg, long_arg, long_long_arg, ptr_arg,
+    int_arg, long_arg, long_long_arg, size_t_arg, ptr_arg,
     string_arg, wide_string_arg, wide_char_arg,
     double_arg, long_double_arg,
     dec32float_arg, dec64float_arg, dec128float_arg
diff --git a/gdb/printcmd.c b/gdb/printcmd.c
index 27aaf7a..323a5d0 100644
--- a/gdb/printcmd.c
+++ b/gdb/printcmd.c
@@ -2663,6 +2663,16 @@
 	      DIAGNOSTIC_POP
 	      break;
 	    }
+	  case size_t_arg:
+	    {
+	      size_t val = value_as_long (val_args[i]);
+
+	      DIAGNOSTIC_PUSH
+	      DIAGNOSTIC_IGNORE_FORMAT_NONLITERAL
+              fprintf_filtered (stream, current_substring, val);
+	      DIAGNOSTIC_POP
+	      break;
+	    }
 	  /* Handles floating-point values.  */
 	  case double_arg:
 	  case long_double_arg:
diff --git a/gdb/ui-out.c b/gdb/ui-out.c
index 6b0b5ac..80845f4 100644
--- a/gdb/ui-out.c
+++ b/gdb/ui-out.c
@@ -688,6 +688,24 @@
 	      }
 	  }
 	  break;
+	case size_t_arg:
+	  {
+	    size_t val = va_arg (args, size_t);
+	    switch (piece.n_int_args)
+	      {
+	      case 0:
+		call_do_message (style, current_substring, val);
+		break;
+	      case 1:
+		call_do_message (style, current_substring, intvals[0], val);
+		break;
+	      case 2:
+		call_do_message (style, current_substring,
+				 intvals[0], intvals[1], val);
+		break;
+	      }
+	  }
+	  break;
 	case double_arg:
 	  call_do_message (style, current_substring, va_arg (args, double));
 	  break;
diff --git a/gdb/unittests/format_pieces-selftests.c b/gdb/unittests/format_pieces-selftests.c
index ed83d96..3971201 100644
--- a/gdb/unittests/format_pieces-selftests.c
+++ b/gdb/unittests/format_pieces-selftests.c
@@ -76,12 +76,56 @@
     }, true);
 }
 
+/* Test the different size modifiers that can be applied to an integer
+   argument.  Test with different integer format specifiers too.  */
+
+static void
+test_format_int_sizes ()
+{
+  check ("Hello\\t %hu%lu%llu%zu", /* ARI: %ll */
+    {
+      format_piece ("Hello\t ", literal_piece, 0),
+      format_piece ("%hu", int_arg, 0),
+      format_piece ("%lu", long_arg, 0),
+      format_piece ("%llu", long_long_arg, 0), /* ARI: %ll */
+      format_piece ("%zu", size_t_arg, 0)
+    });
+
+  check ("Hello\\t %hx%lx%llx%zx", /* ARI: %ll */
+    {
+      format_piece ("Hello\t ", literal_piece, 0),
+      format_piece ("%hx", int_arg, 0),
+      format_piece ("%lx", long_arg, 0),
+      format_piece ("%llx", long_long_arg, 0), /* ARI: %ll */
+      format_piece ("%zx", size_t_arg, 0)
+    });
+
+  check ("Hello\\t %ho%lo%llo%zo", /* ARI: %ll */
+    {
+      format_piece ("Hello\t ", literal_piece, 0),
+      format_piece ("%ho", int_arg, 0),
+      format_piece ("%lo", long_arg, 0),
+      format_piece ("%llo", long_long_arg, 0), /* ARI: %ll */
+      format_piece ("%zo", size_t_arg, 0)
+    });
+
+  check ("Hello\\t %hd%ld%lld%zd", /* ARI: %ll */
+    {
+      format_piece ("Hello\t ", literal_piece, 0),
+      format_piece ("%hd", int_arg, 0),
+      format_piece ("%ld", long_arg, 0),
+      format_piece ("%lld", long_long_arg, 0), /* ARI: %ll */
+      format_piece ("%zd", size_t_arg, 0)
+    });
+}
+
 static void
 run_tests ()
 {
   test_escape_sequences ();
   test_format_specifier ();
   test_gdb_formats ();
+  test_format_int_sizes ();
 }
 
 } /* namespace format_pieces */

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Ib6c44d88aa5bce265d757e4c0698881803dd186f
Gerrit-Change-Number: 511
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Burgess <andrew.burgess@embecosm.com>
Gerrit-Reviewer: Andrew Burgess <andrew.burgess@embecosm.com>
Gerrit-Reviewer: Kevin Buettner <kevinb@redhat.com>
Gerrit-CC: Joel Brobecker <brobecker@adacore.com>
Gerrit-CC: Pedro Alves <palves@redhat.com>
Gerrit-CC: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-CC: Tom Tromey <tromey@sourceware.org>
Gerrit-MessageType: merged

Re: [pushed] gdb: Support printf 'z' size modifier

[Eli Zaretskii]

> Date: Tue, 12 Nov 2019 18:53:15 -0500
> From: "Sourceware to Gerrit sync (Code Review)" <gerrit@gnutoolchain-gerrit.osci.io>
> Cc: Kevin Buettner <kevinb@redhat.com>,	Joel Brobecker <brobecker@adacore.com>,	Pedro Alves <palves@redhat.com>,	Simon Marchi <simon.marchi@polymtl.ca>,	Tom Tromey <tromey@sourceware.org>
> 
> gdb/ChangeLog:
> 
> 	* gdbsupport/format.c (format_pieces::format_pieces): Support
> 	printf 'z' size modifier.
> 	* gdbsupport/format.h (enum argclass): Add size_t_arg.
> 	* printcmd.c (ui_printf):  Handle size_t_arg.
> 	* ui-out.c (ui_out::vmessage): Likewise.
> 	* unittests/format_pieces-selftests.c (test_format_int_sizes): New
> 	function.
> 	(run_tests): Call test_format_int_sizes.

I believe this requires to use __USE_MINGW_ANSI_STDIO with the MinGW
builds, since %z is not universally supported by the Windows runtime.

Re: [pushed] gdb: Support printf 'z' size modifier

[Andrew Burgess]

* Eli Zaretskii <eliz@gnu.org> [2019-11-14 14:54:50 +0200]:

> > Date: Tue, 12 Nov 2019 18:53:15 -0500
> > From: "Sourceware to Gerrit sync (Code Review)" <gerrit@gnutoolchain-gerrit.osci.io>
> > Cc: Kevin Buettner <kevinb@redhat.com>,	Joel Brobecker <brobecker@adacore.com>,	Pedro Alves <palves@redhat.com>,	Simon Marchi <simon.marchi@polymtl.ca>,	Tom Tromey <tromey@sourceware.org>
> > 
> > gdb/ChangeLog:
> > 
> > 	* gdbsupport/format.c (format_pieces::format_pieces): Support
> > 	printf 'z' size modifier.
> > 	* gdbsupport/format.h (enum argclass): Add size_t_arg.
> > 	* printcmd.c (ui_printf):  Handle size_t_arg.
> > 	* ui-out.c (ui_out::vmessage): Likewise.
> > 	* unittests/format_pieces-selftests.c (test_format_int_sizes): New
> > 	function.
> > 	(run_tests): Call test_format_int_sizes.
> 
> I believe this requires to use __USE_MINGW_ANSI_STDIO with the MinGW
> builds, since %z is not universally supported by the Windows runtime.

I only stumbled onto this issue as I hit a use of %z (which wasn't
guarded with __USE_MINGW_ANSI_STDIO) and wanted it to work now that
these strings pass through GDB's formatting code.

I guess we're no worse off now than we were before, but obviously
that's not saying much if we were possibly broken before.

I guess there are a couple of solutions:

  1. Remove all uses of %z from GDB, and back out the %z support, or

  2. Have GDB translate %z into some other suitable format specifier
     for targets where %z is not supported.

Below is a patch that tries to take the second approach.

Feedback / thoughts welcome.

Thanks,
Andrew

----


commit fb431811b59597b63c5bb9bcf7bf8559991c52e1
Author: Andrew Burgess <andrew.burgess@embecosm.com>
Date:   Thu Nov 14 16:40:57 2019 +0000

    gdb: Support for %z format on MinGW
    
    Eli pointed out that the %z size specifier is not supported on all
    versions of MinGW.  This commit attempts to work around this by
    translating %z into some other suitable format specifier.  So
    something like %zd will become either %d, %ld, or %lld depending on
    whether the sizeof (size_t) matches the sizeof (int), sizeof (long),
    or sizeof (long long).
    
    For the long long case we might also translate to %I64d if
    USE_PRINTF_I64 is true.
    
    I don't have access to MinGW so this code is mostly untested - I did
    remove the '#if defined __MINGW32__ ....' check and try using %z in
    some formatted prints, this all seemed to work fine - on my machine
    sizeof (size_t) == sizeof (long).
    
    gdb/ChangeLog:
    
            * gdbsupport/format.c (format_pieces::format_pieces): Translate %z
            into some other suitable format size specifier if it is not
            supported.
    
    Change-Id: I20f5e4cb1a7ab88f00f5e42d3fd8ca4aef00993d

diff --git a/gdb/gdbsupport/format.c b/gdb/gdbsupport/format.c
index 2e2d90a9246..e316e840e22 100644
--- a/gdb/gdbsupport/format.c
+++ b/gdb/gdbsupport/format.c
@@ -375,6 +375,40 @@ format_pieces::format_pieces (const char **arg, bool gdb_extensions)
 	    strcpy (current_substring + length_before_ls, "s");
 	    current_substring += length_before_ls + 2;
 	  }
+#if defined __MINGW32__ && !defined __USE_MINGW_ANSI_STDIO
+	else if (this_argclass == size_t_arg)
+	  {
+	    /* Some versions of MinGW don't support the %z size format, so
+	       lets change to use some appropriate alternative.  */
+	    *current_substring++ = '%';
+	    if (sizeof (size_t) == sizeof (int))
+	      {
+		*current_substring++ = *(percent_loc + 2);
+		this_argclass = int_arg;
+	      }
+	    else if (sizeof (size_t) == sizeof (long))
+	      {
+		*current_substring++ = 'l';
+		*current_substring++ = *(percent_loc + 2);
+		this_argclass = long_arg;
+	      }
+	    else if (sizeof (size_t) == sizeof (long long))
+	      {
+		if (USE_PRINTF_I64)
+		  {
+		    strcpy (current_substring, "I64");
+		    current_substring += 3;
+		  }
+		else
+		  {
+		    strcpy (current_substring, "ll");
+		    current_substring += 2;
+		  }
+		*current_substring++ = *(percent_loc + 2);
+		this_argclass = long_long_arg;
+	      }
+	  }
+#endif	/* defined __MINGW32__ && !defined __USE_MINGW_ANSI_STDIO */
 	else
 	  {
 	    strncpy (current_substring, percent_loc, f - percent_loc);

Re: [pushed] gdb: Support printf 'z' size modifier

[Eli Zaretskii]

> Date: Thu, 14 Nov 2019 16:49:45 +0000
> From: Andrew Burgess <andrew.burgess@embecosm.com>
> Cc: noreply@gnutoolchain-gerrit.osci.io, simon.marchi@polymtl.ca,
> 	tromey@sourceware.org, palves@redhat.com, brobecker@adacore.com,
> 	kevinb@redhat.com, gdb-patches@sourceware.org
> 
> I guess there are a couple of solutions:
> 
>   1. Remove all uses of %z from GDB, and back out the %z support, or
> 
>   2. Have GDB translate %z into some other suitable format specifier
>      for targets where %z is not supported.
> 
> Below is a patch that tries to take the second approach.

Hmm... isn't it better to simply add -D__USE_MINGW_ANSI_STDIO=1 to the
cpp flags when building with MinGW?  I don't think there are versions
of MinGW that don't support that macro.

The effect of that macro is to link against a MinGW library that
provides replacement implementations for *printf functions, and the
replacements do support %z.

Re: [pushed] gdb: Support printf 'z' size modifier

[Pedro Alves]

On 11/14/19 4:49 PM, Andrew Burgess wrote:
> * Eli Zaretskii <eliz@gnu.org> [2019-11-14 14:54:50 +0200]:
> 

>> I believe this requires to use __USE_MINGW_ANSI_STDIO with the MinGW
>> builds, since %z is not universally supported by the Windows runtime.
> 
> I only stumbled onto this issue as I hit a use of %z (which wasn't
> guarded with __USE_MINGW_ANSI_STDIO) and wanted it to work now that
> these strings pass through GDB's formatting code.

Isn't gnulib taking care of this, by either enforcing __USE_MINGW_ANSI_STDIO,
or by replacing printf with it's own implementation?

Thanks,
Pedro Alves

Re: [pushed] gdb: Support printf 'z' size modifier

[Simon Marchi]

On 2019-11-14 11:59 a.m., Eli Zaretskii wrote:
>> Date: Thu, 14 Nov 2019 16:49:45 +0000
>> From: Andrew Burgess <andrew.burgess@embecosm.com>
>> Cc: noreply@gnutoolchain-gerrit.osci.io, simon.marchi@polymtl.ca,
>> 	tromey@sourceware.org, palves@redhat.com, brobecker@adacore.com,
>> 	kevinb@redhat.com, gdb-patches@sourceware.org
>>
>> I guess there are a couple of solutions:
>>
>>   1. Remove all uses of %z from GDB, and back out the %z support, or
>>
>>   2. Have GDB translate %z into some other suitable format specifier
>>      for targets where %z is not supported.
>>
>> Below is a patch that tries to take the second approach.
> 
> Hmm... isn't it better to simply add -D__USE_MINGW_ANSI_STDIO=1 to the
> cpp flags when building with MinGW?  I don't think there are versions
> of MinGW that don't support that macro.
> 
> The effect of that macro is to link against a MinGW library that
> provides replacement implementations for *printf functions, and the
> replacements do support %z.
> 

Apaprently, the __USE_MINGW_ANSI_STDIO macro has been marked as deprecated (IIUC, to
incite people not to define it directly), and I found this thread [1] on the mingw-users
mailing list where you asked the maintainer to reconsider this decision.

What is the status on that?  If defining __USE_MINGW_ANSI_STDIO is not the right way
of enabling this feature, then what is?  What is the status of this?

Simon

[1] https://osdn.net/projects/mingw/lists/archive/users/2019-January/thread.html#199
    Subject "Deprecation of __USE_MINGW_ANSI_STDIO"

Re: [pushed] gdb: Support printf 'z' size modifier

[Eli Zaretskii]

> Cc: noreply@gnutoolchain-gerrit.osci.io, simon.marchi@polymtl.ca,
>  tromey@sourceware.org, brobecker@adacore.com, kevinb@redhat.com,
>  gdb-patches@sourceware.org
> From: Pedro Alves <palves@redhat.com>
> Date: Thu, 14 Nov 2019 17:06:20 +0000
> 
> Isn't gnulib taking care of this, by either enforcing __USE_MINGW_ANSI_STDIO,
> or by replacing printf with it's own implementation?

Only if you import the printf module.  Which I'm not sure we want,
because it's a MinGW only issue, and MinGW already has a solution.

Re: [pushed] gdb: Support printf 'z' size modifier

[Eli Zaretskii]

> Cc: tromey@sourceware.org, palves@redhat.com, brobecker@adacore.com,
>         kevinb@redhat.com, gdb-patches@sourceware.org
> From: Simon Marchi <simon.marchi@polymtl.ca>
> Date: Thu, 14 Nov 2019 12:09:20 -0500
> 
> Apaprently, the __USE_MINGW_ANSI_STDIO macro has been marked as deprecated (IIUC, to
> incite people not to define it directly), and I found this thread [1] on the mingw-users
> mailing list where you asked the maintainer to reconsider this decision.
> 
> What is the status on that?  If defining __USE_MINGW_ANSI_STDIO is not the right way
> of enabling this feature, then what is?  What is the status of this?

The "new" way is to use the __MINGW_FEATURES__ macro.  But if you want
to do this in a way that will work with noth mingw.org's MinGW and
MinGW64, the __USE_MINGW_ANSI_STDIO is the way, and it is still
supported.

Re: [pushed] gdb: Support printf 'z' size modifier

[Pedro Alves]

On 11/14/19 6:27 PM, Eli Zaretskii wrote:
>> Cc: tromey@sourceware.org, palves@redhat.com, brobecker@adacore.com,
>>         kevinb@redhat.com, gdb-patches@sourceware.org
>> From: Simon Marchi <simon.marchi@polymtl.ca>
>> Date: Thu, 14 Nov 2019 12:09:20 -0500
>>
>> Apaprently, the __USE_MINGW_ANSI_STDIO macro has been marked as deprecated (IIUC, to
>> incite people not to define it directly), and I found this thread [1] on the mingw-users
>> mailing list where you asked the maintainer to reconsider this decision.
>>
>> What is the status on that?  If defining __USE_MINGW_ANSI_STDIO is not the right way
>> of enabling this feature, then what is?  What is the status of this?
> 
> The "new" way is to use the __MINGW_FEATURES__ macro.  But if you want
> to do this in a way that will work with noth mingw.org's MinGW and
> MinGW64, the __USE_MINGW_ANSI_STDIO is the way, and it is still
> supported.
> 

I think that the ideal place to do that would be in src/config/mh-mingw,
so that all projects in the tree, plus gcc, enabled it.
It's where we do -D__USE_MINGW_ACCESS.

Thanks,
Pedro Alves

Re: [pushed] gdb: Support printf 'z' size modifier

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew>     For the long long case we might also translate to %I64d if
Andrew>     USE_PRINTF_I64 is true.
    
FWIW, today I debugged a crash on an internal test case that turned out
to happen because format_pieces does not recognize %I64d, but format.c
also already changes %lld to %I64d.

So, we need to handle something here.  I am leaning toward just teaching
format.c about %I64d as well.

Tom

Re: [pushed] gdb: Support printf 'z' size modifier

[Eli Zaretskii]

> Cc: andrew.burgess@embecosm.com, tromey@sourceware.org,
>  brobecker@adacore.com, kevinb@redhat.com, gdb-patches@sourceware.org
> From: Pedro Alves <palves@redhat.com>
> Date: Thu, 14 Nov 2019 20:37:15 +0000
> 
> > The "new" way is to use the __MINGW_FEATURES__ macro.  But if you want
> > to do this in a way that will work with noth mingw.org's MinGW and
> > MinGW64, the __USE_MINGW_ANSI_STDIO is the way, and it is still
> > supported.
> > 
> 
> I think that the ideal place to do that would be in src/config/mh-mingw,
> so that all projects in the tree, plus gcc, enabled it.
> It's where we do -D__USE_MINGW_ACCESS.

That'd be fine, thanks.