[review] gdb: Support printf 'z' size modifier
diff mbox series

Message ID gerrit.1572968811000.Ib6c44d88aa5bce265d757e4c0698881803dd186f@gnutoolchain-gerrit.osci.io
State New
Headers show
Series
  • [review] gdb: Support printf 'z' size modifier
Related show

Commit Message

Sourceware to Gerrit sync (Code Review) Nov. 5, 2019, 3:46 p.m. 
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(-)




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

Comments

Sourceware to Gerrit sync (Code Review) Nov. 5, 2019, 4:22 p.m. | #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.



-- 
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
Sourceware to Gerrit sync (Code Review) Nov. 5, 2019, 4:31 p.m. | #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.



-- 
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
Sourceware to Gerrit sync (Code Review) Nov. 6, 2019, 12:18 a.m. | #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.



-- 
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
Sourceware to Gerrit sync (Code Review) Nov. 7, 2019, 11:05 a.m. | #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.



-- 
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
Sourceware to Gerrit sync (Code Review) Nov. 12, 2019, 8:18 p.m. | #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


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

Patch
diff mbox series 

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