diff mbox

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

Message ID 20191114164945.GP11037@embecosm.com
State New, archived
Headers

Commit Message

Andrew Burgess Nov. 14, 2019, 4:49 p.m. UTC 
  * 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

Comments

Eli Zaretskii Nov. 14, 2019, 4:59 p.m. UTC | #1 
> 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.
Pedro Alves Nov. 14, 2019, 5:06 p.m. UTC | #2 
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
Simon Marchi Nov. 14, 2019, 5:09 p.m. UTC | #3 
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"
Eli Zaretskii Nov. 14, 2019, 6:18 p.m. UTC | #4 
> 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.
Eli Zaretskii Nov. 14, 2019, 6:27 p.m. UTC | #5 
> 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.
Pedro Alves Nov. 14, 2019, 8:37 p.m. UTC | #6 
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
Tom Tromey Nov. 14, 2019, 9:25 p.m. UTC | #7 
>>>>> "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
Eli Zaretskii Nov. 15, 2019, 7:37 a.m. UTC | #8 
> 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.
diff mbox

Patch

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