diff mbox

GDB 7.99.91 MinGW compilation error in cli-script.c

Message ID 83vaouns1q.fsf@gnu.org
State New
Headers show

Commit Message

Eli Zaretskii May 21, 2017, 3:33 p.m. UTC
> Cc: brobecker@adacore.com, simon.marchi@polymtl.ca, gdb-patches@sourceware.org
> From: Pedro Alves <palves@redhat.com>
> Date: Fri, 19 May 2017 12:23:06 +0100
> 
> On 05/19/2017 12:17 PM, Eli Zaretskii wrote:
> 
> > Fine with me, but Someone(TM) should figure out what to use instead of
> > "#if 0".
> > 
> 
> Something around
> __MINGW32__ / _GLIBCXX_USE_C99 / _GLIBCXX_HAVE_BROKEN_VSWPRINTF ?

Here's what I came up with.  OK to commit?

Comments

Pedro Alves May 22, 2017, 3:26 p.m. UTC | #1
On 05/21/2017 04:33 PM, Eli Zaretskii wrote:
>> Cc: brobecker@adacore.com, simon.marchi@polymtl.ca, gdb-patches@sourceware.org
>> From: Pedro Alves <palves@redhat.com>
>> Date: Fri, 19 May 2017 12:23:06 +0100
>>
>> On 05/19/2017 12:17 PM, Eli Zaretskii wrote:
>>
>>> Fine with me, but Someone(TM) should figure out what to use instead of
>>> "#if 0".
>>>
>>
>> Something around
>> __MINGW32__ / _GLIBCXX_USE_C99 / _GLIBCXX_HAVE_BROKEN_VSWPRINTF ?
> 
> Here's what I came up with.  OK to commit?

Is there still a reason for the "#include <_mingw.h>"?
I thought that was needed in the previous version for
__MINGW64_VERSION_MAJOR, but this version of the patch doesn't
use that symbol.

Otherwise, if

- older broken mingw releases get the replacement
- newer fixed mingw releases don't get the replacement
- mingw-w64 doesn't get the replacement (as it doesn't need one IIUC)

then it's fine with me.

It'd be useful for the archives if you expanded on which mingw versions
and compilers you tested this on.  Also likewise a short comment to
the effect in the code would be likewise handy for future readers
(please use /**/ style comments).

Thanks,
Pedro Alves
Eli Zaretskii May 22, 2017, 6:42 p.m. UTC | #2
> Cc: brobecker@adacore.com, simon.marchi@polymtl.ca, gdb-patches@sourceware.org
> From: Pedro Alves <palves@redhat.com>
> Date: Mon, 22 May 2017 16:26:19 +0100
> 
> Is there still a reason for the "#include <_mingw.h>"?

Yes, that's where _GLIBCXX_USE_C99 is defined.

> Otherwise, if
> 
> - older broken mingw releases get the replacement
> - newer fixed mingw releases don't get the replacement
> - mingw-w64 doesn't get the replacement (as it doesn't need one IIUC)
> 
> then it's fine with me.

Hmmm... I see that MinGW64 doesn't define _GLIBCXX_USE_C99 in its
system headers, so I guess I will have to add the
__MINGW64_VERSION_MAJOR guard as well.

> It'd be useful for the archives if you expanded on which mingw versions
> and compilers you tested this on.  Also likewise a short comment to
> the effect in the code would be likewise handy for future readers

OK, will do.

> (please use /**/ style comments).

I thought I did...
Pedro Alves May 23, 2017, 9:53 a.m. UTC | #3
On 05/22/2017 07:42 PM, Eli Zaretskii wrote:
>> Cc: brobecker@adacore.com, simon.marchi@polymtl.ca, gdb-patches@sourceware.org
>> From: Pedro Alves <palves@redhat.com>
>> Date: Mon, 22 May 2017 16:26:19 +0100
>>
>> Is there still a reason for the "#include <_mingw.h>"?
> 
> Yes, that's where _GLIBCXX_USE_C99 is defined.

Urgh...

> 
>> Otherwise, if
>>
>> - older broken mingw releases get the replacement
>> - newer fixed mingw releases don't get the replacement
>> - mingw-w64 doesn't get the replacement (as it doesn't need one IIUC)
>>
>> then it's fine with me.
> 
> Hmmm... I see that MinGW64 doesn't define _GLIBCXX_USE_C99 in its
> system headers, 

I'm surprised mingw does this, because that's a libstdc++
internal symbol...

> so I guess I will have to add the
> __MINGW64_VERSION_MAJOR guard as well.

Please also take a look at the fix for:

 https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58393

which suggests to me that newer compilers against older mingw
might actually be fixed, independently of the _GLIBCXX_USE_C99 hack?

> 
>> It'd be useful for the archives if you expanded on which mingw versions
>> and compilers you tested this on.  Also likewise a short comment to
>> the effect in the code would be likewise handy for future readers
> 
> OK, will do.
> 
>> (please use /**/ style comments).
> 
> I thought I did...

The comments you had added in the original patch used // style:

 +// For versions of mingw.org's MinGW runtime before 5.0, make sure
 +// libstdc++ headers don't omit portions that require C99.

Thanks,
Pedro Alves
Eli Zaretskii May 24, 2017, 2:44 a.m. UTC | #4
> Cc: brobecker@adacore.com, simon.marchi@polymtl.ca, gdb-patches@sourceware.org
> From: Pedro Alves <palves@redhat.com>
> Date: Tue, 23 May 2017 10:53:47 +0100
> 
> I'm surprised mingw does this, because that's a libstdc++
> internal symbol...

I'm guessing that was done because releases of MinGW runtime and the
MinGW port of GCC are not in sync.  So people who have a GCC
installation and upgrade to a later MinGW runtime expect to have this
problem solved, but the way you seem to assume tjis to happen is by
them having to build an updated GCC or wait for an update of the GCC
distribution.

> Please also take a look at the fix for:
> 
>  https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58393
> 
> which suggests to me that newer compilers against older mingw
> might actually be fixed, independently of the _GLIBCXX_USE_C99 hack?

mingw.org's MinGW currently doesn't offer GCC newer than 5.3.0, so
this solution is not yet available to MinGW users.  Maybe soon it will
be.
Pedro Alves May 25, 2017, 10:05 a.m. UTC | #5
On 05/24/2017 03:44 AM, Eli Zaretskii wrote:
>> Cc: brobecker@adacore.com, simon.marchi@polymtl.ca, gdb-patches@sourceware.org
>> From: Pedro Alves <palves@redhat.com>
>> Date: Tue, 23 May 2017 10:53:47 +0100
>>
>> I'm surprised mingw does this, because that's a libstdc++
>> internal symbol...
> 
> I'm guessing that was done because releases of MinGW runtime and the
> MinGW port of GCC are not in sync.  So people who have a GCC
> installation and upgrade to a later MinGW runtime expect to have this
> problem solved, but the way you seem to assume tjis to happen is by
> them having to build an updated GCC or wait for an update of the GCC
> distribution.

Your use of "have to wait" above doesn't make sense to me, to be
honest.  mingw.org maintainers chose to update the runtime and not
the compiler.  It's their choice.  Users have to update some component to
fix this, but the decision that the component that needs updating is
the runtime is mingw.org maintainer's.  It would have been equally possible to
provide an updated GCC release of the exact same GCC version / vintage
that defined _GLIBCXX_USE_C99 itself (or some other similar localized
hack or even a backport of the proper fix upstream) instead of
defining a libstdc++ internal symbol in the runtime and potentially
causing trouble when/if folks update to newer GCCs that
treat _GLIBCXX_USE_C99 differently, exactly because, as you say,
the runtime and the compiler are not released in sync.

> 
>> Please also take a look at the fix for:
>>
>>  https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58393
>>
>> which suggests to me that newer compilers against older mingw
>> might actually be fixed, independently of the _GLIBCXX_USE_C99 hack?
> 
> mingw.org's MinGW currently doesn't offer GCC newer than 5.3.0, so
> this solution is not yet available to MinGW users.  Maybe soon it will
> be.

The point was that the compiler fix will make std::to_string available
even when _GLIBCXX_USE_C99 ends up not defined, AFAICS.  Meaning that with
newer compilers we'll replace std::to_string when it's not necessary,
AFAICS, given:

 +#ifdef __MINGW32__
 +# include <_mingw.h>
 +# ifndef _GLIBCXX_USE_C99
 +#  undef REPLACE_TO_STRING
 +#  define REPLACE_TO_STRING 1
 +# endif
 +#endif

From the ChangeLog entry seen on that bugzilla url, I'd think that
skipping the replacement when GLIBCXX_USE_C99_STDIO is defined
would work.

But guess we'll just revisit when/if someone ever notices [to_string
replaced when it didn't need to], or when we stop supporting compilers
that had this broken, whichever comes first.

I'll go look at your newer patch.

Thanks,
Pedro Alves
Eli Zaretskii May 26, 2017, 7:57 a.m. UTC | #6
> Cc: brobecker@adacore.com, simon.marchi@polymtl.ca, gdb-patches@sourceware.org
> From: Pedro Alves <palves@redhat.com>
> Date: Thu, 25 May 2017 11:05:00 +0100
> 
> >> I'm surprised mingw does this, because that's a libstdc++
> >> internal symbol...
> > 
> > I'm guessing that was done because releases of MinGW runtime and the
> > MinGW port of GCC are not in sync.  So people who have a GCC
> > installation and upgrade to a later MinGW runtime expect to have this
> > problem solved, but the way you seem to assume tjis to happen is by
> > them having to build an updated GCC or wait for an update of the GCC
> > distribution.
> 
> Your use of "have to wait" above doesn't make sense to me, to be
> honest.  mingw.org maintainers chose to update the runtime and not
> the compiler.  It's their choice.  Users have to update some component to
> fix this, but the decision that the component that needs updating is
> the runtime is mingw.org maintainer's.  It would have been equally possible to
> provide an updated GCC release of the exact same GCC version / vintage
> that defined _GLIBCXX_USE_C99 itself (or some other similar localized
> hack or even a backport of the proper fix upstream) instead of
> defining a libstdc++ internal symbol in the runtime and potentially
> causing trouble when/if folks update to newer GCCs that
> treat _GLIBCXX_USE_C99 differently, exactly because, as you say,
> the runtime and the compiler are not released in sync.

I think there's some misunderstanding here.  The MinGW runtime needed
to be updated in any case, because the root cause of not defining
_GLIBCXX_USE_C99 for MinGW was a deficiency in the MinGW runtime: a
specific stdio function in the MS runtime wasn't behaving in
ANSI-compliant way, and needed a replacement for it to be added to
MinGW.  The _GLIBCXX_USE_C99 macro was undefined in the libstdc++
headers because of that problem, and the decision whether to define it
is made at libstdc++ build time, based on the MinGW runtime that was
present at that time.

Once that change in the MinGW runtime was made, there were two
possible ways of fixing the to_string problem in libstdc++:

  . build and release a new distribution of GCC, and tell everyone
    that they must upgrade their GCC, together with MinGW runtime; or

  . force _GLIBCXX_USE_C99 to be defined in the MinGW runtime headers

The MinGW maintainers decided to do the latter, most probably because
it is easier on both users and the maintainers.

> > mingw.org's MinGW currently doesn't offer GCC newer than 5.3.0, so
> > this solution is not yet available to MinGW users.  Maybe soon it will
> > be.
> 
> The point was that the compiler fix will make std::to_string available
> even when _GLIBCXX_USE_C99 ends up not defined, AFAICS.  Meaning that with
> newer compilers we'll replace std::to_string when it's not necessary,
> AFAICS, given:
> 
>  +#ifdef __MINGW32__
>  +# include <_mingw.h>
>  +# ifndef _GLIBCXX_USE_C99
>  +#  undef REPLACE_TO_STRING
>  +#  define REPLACE_TO_STRING 1
>  +# endif
>  +#endif
> 
> >From the ChangeLog entry seen on that bugzilla url, I'd think that
> skipping the replacement when GLIBCXX_USE_C99_STDIO is defined
> would work.

I'm okay with adding the newer symbols to the patch I already
installed, if that's what you want.  the latest GCC that's currently
available for MinGW is 5.3.0, where these newer symbols don't yet
appear.
Pedro Alves May 26, 2017, 11:52 a.m. UTC | #7
On 05/26/2017 08:57 AM, Eli Zaretskii wrote:
>> Cc: brobecker@adacore.com, simon.marchi@polymtl.ca, gdb-patches@sourceware.org
>> From: Pedro Alves <palves@redhat.com>
>> Date: Thu, 25 May 2017 11:05:00 +0100
>>
>>>> I'm surprised mingw does this, because that's a libstdc++
>>>> internal symbol...
>>>
>>> I'm guessing that was done because releases of MinGW runtime and the
>>> MinGW port of GCC are not in sync.  So people who have a GCC
>>> installation and upgrade to a later MinGW runtime expect to have this
>>> problem solved, but the way you seem to assume tjis to happen is by
>>> them having to build an updated GCC or wait for an update of the GCC
>>> distribution.
>>
>> Your use of "have to wait" above doesn't make sense to me, to be
>> honest.  mingw.org maintainers chose to update the runtime and not
>> the compiler.  It's their choice.  Users have to update some component to
>> fix this, but the decision that the component that needs updating is
>> the runtime is mingw.org maintainer's.  It would have been equally possible to
>> provide an updated GCC release of the exact same GCC version / vintage
>> that defined _GLIBCXX_USE_C99 itself (or some other similar localized
>> hack or even a backport of the proper fix upstream) instead of
>> defining a libstdc++ internal symbol in the runtime and potentially
>> causing trouble when/if folks update to newer GCCs that
>> treat _GLIBCXX_USE_C99 differently, exactly because, as you say,
>> the runtime and the compiler are not released in sync.
> 
> I think there's some misunderstanding here.  

Maybe.

> The MinGW runtime needed
> to be updated in any case, because the root cause of not defining
> _GLIBCXX_USE_C99 for MinGW was a deficiency in the MinGW runtime: a
> specific stdio function in the MS runtime wasn't behaving in
> ANSI-compliant way, and needed a replacement for it to be added to
> MinGW.  

My understanding of the issue is that libstdc++ had a too-coarse way
to tell whether the runtime supports C99, and that ended up disabling
std::to_string because some unrelated (to std::to_string) bits of C99
support are missing in mingw.org.

Specifically, wide string functions are not really necessary
for std::to_string.  Having non-conforming C99 math-related bits also
disabled std::to_string (see bug I pointed at), again
because _GLIBCXX_USE_C99 is too coarse-grained.

So forcing _GLIBCXX_USE_C99 exposes std::to_string, but also
exposes other C99 broken / non-conforming bits.

> The _GLIBCXX_USE_C99 macro was undefined in the libstdc++
> headers because of that problem, and the decision whether to define it
> is made at libstdc++ build time, based on the MinGW runtime that was
> present at that time.

Yes.

> 
> Once that change in the MinGW runtime was made, there were two
> possible ways of fixing the to_string problem in libstdc++:
> 
>   . build and release a new distribution of GCC, and tell everyone
>     that they must upgrade their GCC, together with MinGW runtime; or
> 
>   . force _GLIBCXX_USE_C99 to be defined in the MinGW runtime headers
> 
> The MinGW maintainers decided to do the latter, most probably because
> it is easier on both users and the maintainers.

I don't think the fix on the GCC side really requires a mingw runtime
update.  I may well be wrong, of course.

>>> mingw.org's MinGW currently doesn't offer GCC newer than 5.3.0, so
>>> this solution is not yet available to MinGW users.  Maybe soon it will
>>> be.
>>
>> The point was that the compiler fix will make std::to_string available
>> even when _GLIBCXX_USE_C99 ends up not defined, AFAICS.  Meaning that with
>> newer compilers we'll replace std::to_string when it's not necessary,
>> AFAICS, given:
>>
>>  +#ifdef __MINGW32__
>>  +# include <_mingw.h>
>>  +# ifndef _GLIBCXX_USE_C99
>>  +#  undef REPLACE_TO_STRING
>>  +#  define REPLACE_TO_STRING 1
>>  +# endif
>>  +#endif
>>
>> >From the ChangeLog entry seen on that bugzilla url, I'd think that
>> skipping the replacement when GLIBCXX_USE_C99_STDIO is defined
>> would work.
> 
> I'm okay with adding the newer symbols to the patch I already
> installed, if that's what you want.  the latest GCC that's currently
> available for MinGW is 5.3.0, where these newer symbols don't yet
> appear.
> 

I cloned the current mingw sources to see what's been done
over there, and here's what's there now (on the 5.0-active branch):

#if _ISOC99_SOURCE && __cplusplus >= 201103L && __GNUC__ < 6
 /* Due to a configuration defect in GCC versions prior to GCC-6, when
  * compiling C++11 code, the ISO-C99 functions may not be incorporated
  * into the appropriate namespace(s); we may be able to mitigate this,
  * by ensuring that these GCC configuration macros are defined.
  */
# define _GLIBCXX_USE_C99       1
# define _GLIBCXX_HAVE_WCSTOF   1
#endif

I would have helped a lot if I had been shown this.  Note the 
"__GNUC__ < 6" check.  So with newer compilers (yes, there's no
official binaries, but other people can and do build their own
compilers) _GLIBCXX_USE_C99 won't be hacked in.
So I think we'll define the replacement replacement when we needn't.
But in any case, I'll just ignore that.
If you're happy with the gdb fix in place now, I'm happy enough
with it too.

Thanks,
Pedro Alves
Eli Zaretskii May 26, 2017, 12:57 p.m. UTC | #8
> Cc: brobecker@adacore.com, simon.marchi@polymtl.ca, gdb-patches@sourceware.org
> From: Pedro Alves <palves@redhat.com>
> Date: Fri, 26 May 2017 12:52:27 +0100
> 
> My understanding of the issue is that libstdc++ had a too-coarse way
> to tell whether the runtime supports C99, and that ended up disabling
> std::to_string because some unrelated (to std::to_string) bits of C99
> support are missing in mingw.org.

That's another problem, but it doesn't matter in the case of MinGW,
because the root cause was indeed affecting std::to_string, AFAIU.

> I don't think the fix on the GCC side really requires a mingw runtime
> update.  I may well be wrong, of course.

I think you are wrong.  In the libstdc++ 5.3.0 distribution, the
offending defines are in include/c++/mingw32/bits/c++config.h, which
AFAIU is generated at libstdc++ build time.

> I cloned the current mingw sources to see what's been done
> over there, and here's what's there now (on the 5.0-active branch):

You could have just downloaded the MinGW runtime from here:

  https://sourceforge.net/projects/mingw/files/MinGW/Base/mingwrt/mingwrt-5.0/libmingwex-5.0-mingw32-dev.tar.xz/download

> #if _ISOC99_SOURCE && __cplusplus >= 201103L && __GNUC__ < 6
>  /* Due to a configuration defect in GCC versions prior to GCC-6, when
>   * compiling C++11 code, the ISO-C99 functions may not be incorporated
>   * into the appropriate namespace(s); we may be able to mitigate this,
>   * by ensuring that these GCC configuration macros are defined.
>   */
> # define _GLIBCXX_USE_C99       1
> # define _GLIBCXX_HAVE_WCSTOF   1
> #endif
> 
> I would have helped a lot if I had been shown this.  Note the 
> "__GNUC__ < 6" check.

Hey, that's unfair!  You expect me to look at the MinGW64 headers and
at related Bugzilla bugs, but you yourself cannot look in the MinGW
headers?

> If you're happy with the gdb fix in place now, I'm happy enough
> with it too.

I'm happy.  Any problems with MinGW GCC 6.x don't matter as long as
there's no such GCC version on the MinGW site.
Pedro Alves May 26, 2017, 1:58 p.m. UTC | #9
On 05/26/2017 01:57 PM, Eli Zaretskii wrote:

>> I would have helped a lot if I had been shown this.  Note the 
>> "__GNUC__ < 6" check.
> 
> Hey, that's unfair!  You expect me to look at the MinGW64 headers and
> at related Bugzilla bugs, but you yourself cannot look in the MinGW
> headers?

Sorry, I guess I'm a bit frustrated that we're spending a lot more time
on this than I think it's warranted.  Note I didn't ask you to search
for the bugs yourself, I took the time to research them, and provided
direct links which you can just open.  I didn't have that header handy,
and I had to go hunt for where to get it.

>> If you're happy with the gdb fix in place now, I'm happy enough
>> with it too.
> 
> I'm happy.  Any problems with MinGW GCC 6.x don't matter as long as
> there's no such GCC version on the MinGW site.

OK.

Thanks,
Pedro Alves
diff mbox

Patch

--- gdb/common/common-utils.h~0	2017-04-17 17:09:57.000000000 +0300
+++ gdb/common/common-utils.h	2017-05-21 10:01:22.219168100 +0300
@@ -22,6 +22,7 @@ 
 
 #include <string>
 #include <vector>
+#include <sstream>
 
 /* If possible, define FUNCTION_NAME, a macro containing the name of
    the function being defined.  Since this macro may not always be
@@ -63,6 +64,42 @@ 
 std::string string_printf (const char* fmt, ...)
   ATTRIBUTE_PRINTF (1, 2);
 
+/* Returns a string representation of VAL.  Replacement for C++11
+   std::to_string for hosts that lack it.  */
+
+namespace gdb {
+
+#define REPLACE_TO_STRING 0
+
+#ifdef __MINGW32__
+# include <_mingw.h>
+# ifndef _GLIBCXX_USE_C99
+#  undef REPLACE_TO_STRING
+#  define REPLACE_TO_STRING 1
+# endif
+#endif
+
+#if REPLACE_TO_STRING
+
+template <class T>
+inline std::string
+to_string (const T &val)
+{
+  std::stringstream ss;
+
+  ss << val;
+  return ss.str ();
+}
+
+#else /* !REPLACE_TO_STRING */
+
+using std::to_string;
+
+#endif
+
+#undef REPLACE_TO_STRING
+}
+
 /* Make a copy of the string at PTR with LEN characters
    (and add a null character at the end in the copy).
    Uses malloc to get the space.  Returns the address of the copy.  */
--- gdb/cli/cli-script.c~0	2017-04-17 17:09:57.000000000 +0300
+++ gdb/cli/cli-script.c	2017-05-21 09:36:17.610252500 +0300
@@ -806,7 +818,7 @@  user_args::insert_args (const char *line
 
       if (p[4] == 'c')
 	{
-	  new_line += std::to_string (m_args.size ());
+	  new_line += gdb::to_string (m_args.size ());
 	  line = p + 5;
 	}
       else