diff mbox

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

Message ID gerrit.1572968811000.Ib6c44d88aa5bce265d757e4c0698881803dd186f@gnutoolchain-gerrit.osci.io
State New, archived
Headers

Commit Message

Simon Marchi (Code Review) Nov. 5, 2019, 3:46 p.m. UTC 
  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(-)

Comments

Simon Marchi (Code Review) Nov. 5, 2019, 4:22 p.m. UTC | #1 
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.
Simon Marchi (Code Review) Nov. 5, 2019, 4:31 p.m. UTC | #2 
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.
Simon Marchi (Code Review) Nov. 6, 2019, 12:18 a.m. UTC | #3 
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.
Simon Marchi (Code Review) Nov. 7, 2019, 11:05 a.m. UTC | #4 
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.
Simon Marchi (Code Review) Nov. 12, 2019, 8:18 p.m. UTC | #5 
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
diff mbox

Patch

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)
 	  {