Message ID | 20191114164945.GP11037@embecosm.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 95581 invoked by alias); 14 Nov 2019 16:49:51 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 95561 invoked by uid 89); 14 Nov 2019 16:49:51 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-23.7 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.1 spammy=strncpy X-HELO: mail-wr1-f43.google.com Received: from mail-wr1-f43.google.com (HELO mail-wr1-f43.google.com) (209.85.221.43) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 14 Nov 2019 16:49:49 +0000 Received: by mail-wr1-f43.google.com with SMTP id r10so7280831wrx.3 for <gdb-patches@sourceware.org>; Thu, 14 Nov 2019 08:49:49 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=U1dpprwozMj6pbwOEiWJNc9Y0REQovQLI/OPiq9omjY=; b=gqu8d9aGtEopCT3byT+IE6h7AtyfZdpzkWuECkCiEuju469dnxR0KGqrcutAYIH6iQ 07DZ5Y69r0hI+yR+IwrDiX5mET9Lybf5lDK7tS70s+5fDEgdYmWIJ+WJjYoIK6x4Tchk zi0oGFenWPEAqPusRI3U2J8sDtLGUar+YjBMnucMBDlevcVM7f9AEqDsCBnS4LltsQgt PS1gQ0uGcyB9rs9kv3w9ipwqhWYGULuNmjlR77irmDHQ0mfmrYLIMIQ9wk2RP57jJ0a7 q2cKkYUMHxH9BTu9xSQxsPdBt99LTwonQQMCAyfV6z45OHDW7t17oaQF1giFkfLLr4yI 3mKw== Return-Path: <andrew.burgess@embecosm.com> Received: from localhost (host86-128-12-122.range86-128.btcentralplus.com. [86.128.12.122]) by smtp.gmail.com with ESMTPSA id a186sm6340223wmc.48.2019.11.14.08.49.46 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Thu, 14 Nov 2019 08:49:46 -0800 (PST) Date: Thu, 14 Nov 2019 16:49:45 +0000 From: Andrew Burgess <andrew.burgess@embecosm.com> To: Eli Zaretskii <eliz@gnu.org> 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 Subject: Re: [pushed] gdb: Support printf 'z' size modifier Message-ID: <20191114164945.GP11037@embecosm.com> References: <gerrit.1572968811000.Ib6c44d88aa5bce265d757e4c0698881803dd186f@gnutoolchain-gerrit.osci.io> <20191112235315.5BD6728171@gnutoolchain-gerrit.osci.io> <83y2wi62jp.fsf@gnu.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <83y2wi62jp.fsf@gnu.org> X-Fortune: If at first you don't succeed, you are running about average. X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] User-Agent: Mutt/1.9.2 (2017-12-15) X-IsSubscribed: yes |
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
> 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.
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
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"
> 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.
> 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.
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
>>>>> "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
> 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 --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);