MinGW build failure for GDB 8.2.90 with source-highlight

MinGW build failure for GDB 8.2.90 with source-highlight

[Eli Zaretskii]

I've rebuilt the 8.2.90 pretest today on MinGW with source-highlight,
and the link step failed with this error message:

       CXXLD  gdb.exe
     d:/usr/bin/../lib/gcc/mingw32/6.3.0/../../../../mingw32/bin/ld.exe: source-cache.o: in function `ZNSt13basic_filebufIcSt11char_traitsIcEED4Ev':
     d:/usr/lib/gcc/mingw32/6.3.0/include/c++/fstream:239: undefined reference to `std::basic_filebuf<char, std::char_traits<char> >::rpl_close()'
     d:/usr/bin/../lib/gcc/mingw32/6.3.0/../../../../mingw32/bin/ld.exe: source-cache.o: in function `ZNSt14basic_ifstreamIcSt11char_traitsIcEE8rpl_openEPKcSt13_Ios_Openmode':
     d:/usr/lib/gcc/mingw32/6.3.0/include/c++/fstream:597: undefined reference to `std::basic_filebuf<char, std::char_traits<char> >::rpl_open(char const*, std::_Ios_Openmode)'
     d:/usr/bin/../lib/gcc/mingw32/6.3.0/../../../../mingw32/bin/ld.exe: source-cache.o: in function `ZNSt13basic_filebufIcSt11char_traitsIcEED4Ev':
     d:/usr/lib/gcc/mingw32/6.3.0/include/c++/fstream:239: undefined reference to `std::basic_filebuf<char, std::char_traits<char> >::rpl_close()'
     collect2.exe: error: ld returned 1 exit status
     Makefile:1888: recipe for target `gdb.exe' failed
     make[2]: *** [gdb.exe] Error 1
     make[2]: Leaving directory `/d/usr/eli/utils/gdb-8.2.90/gdb'
     Makefile:8831: recipe for target `all-gdb' failed
     make[1]: *** [all-gdb] Error 2
     make[1]: Leaving directory `/d/usr/eli/utils/gdb-8.2.90'
     Makefile:856: recipe for target `all' failed
     make: *** [all] Error 2

The rpl_open and rpl_close thingies come from Gnulib: in the MinGW
build it replaces 'open' and 'close' with its versions by using
#define.  Then when we include <fstream> in source-cache.c, the C++
compiler barfs.

Is this a Gnulib bug that I should report to its developers, or is
this our problem?  Perhaps we should simply #undef these two symbols
before including <fstream>?

Re: MinGW build failure for GDB 8.2.90 with source-highlight

[Simon Marchi]

On 2019-03-07 09:35, Eli Zaretskii wrote:
> I've rebuilt the 8.2.90 pretest today on MinGW with source-highlight,
> and the link step failed with this error message:
> 
>        CXXLD  gdb.exe
> 
> d:/usr/bin/../lib/gcc/mingw32/6.3.0/../../../../mingw32/bin/ld.exe:
> source-cache.o: in function
> `ZNSt13basic_filebufIcSt11char_traitsIcEED4Ev':
>      d:/usr/lib/gcc/mingw32/6.3.0/include/c++/fstream:239: undefined
> reference to `std::basic_filebuf<char, std::char_traits<char>
> >::rpl_close()'
> 
> d:/usr/bin/../lib/gcc/mingw32/6.3.0/../../../../mingw32/bin/ld.exe:
> source-cache.o: in function
> `ZNSt14basic_ifstreamIcSt11char_traitsIcEE8rpl_openEPKcSt13_Ios_Openmode':
>      d:/usr/lib/gcc/mingw32/6.3.0/include/c++/fstream:597: undefined
> reference to `std::basic_filebuf<char, std::char_traits<char>
> >::rpl_open(char const*, std::_Ios_Openmode)'
> 
> d:/usr/bin/../lib/gcc/mingw32/6.3.0/../../../../mingw32/bin/ld.exe:
> source-cache.o: in function
> `ZNSt13basic_filebufIcSt11char_traitsIcEED4Ev':
>      d:/usr/lib/gcc/mingw32/6.3.0/include/c++/fstream:239: undefined
> reference to `std::basic_filebuf<char, std::char_traits<char>
> >::rpl_close()'
>      collect2.exe: error: ld returned 1 exit status
>      Makefile:1888: recipe for target `gdb.exe' failed
>      make[2]: *** [gdb.exe] Error 1
>      make[2]: Leaving directory `/d/usr/eli/utils/gdb-8.2.90/gdb'
>      Makefile:8831: recipe for target `all-gdb' failed
>      make[1]: *** [all-gdb] Error 2
>      make[1]: Leaving directory `/d/usr/eli/utils/gdb-8.2.90'
>      Makefile:856: recipe for target `all' failed
>      make: *** [all] Error 2
> 
> The rpl_open and rpl_close thingies come from Gnulib: in the MinGW
> build it replaces 'open' and 'close' with its versions by using
> #define.  Then when we include <fstream> in source-cache.c, the C++
> compiler barfs.
> 
> Is this a Gnulib bug that I should report to its developers, or is
> this our problem?  Perhaps we should simply #undef these two symbols
> before including <fstream>?

Since there are problems to fix with open on mingw (according to [1]), I 
think it's fine that gnulib provides replacements.  And that problem of 
the macros replacing things we don't want them to replace (such as 
std::basic_filebuf::open has occured a few times in the past.

I don't recall what's the long term solution for this.  We could use 
gnulib's namespace support [2], but the disadvantage is that we would 
need to use gnulib::some_function (assuming we name the namespace 
"gnulib") instead of just some_function to use the "fixed" version.  If 
we use some_function directly, it will use the buggy version on those 
systems where it is buggy.

Undefining the macros seems dangerous too, if you actually use the 
"open" function from the C standard library on a system where it is 
buggy, you want to end up with gnulib's fixed version...

Simon

[1] https://www.gnu.org/software/gnulib/manual/html_node/open.html
[2] 
https://www.gnu.org/software/gnulib/manual/html_node/A-C_002b_002b-namespace-for-gnulib.html

Re: MinGW build failure for GDB 8.2.90 with source-highlight

[Pedro Alves]

On 03/07/2019 04:59 PM, Simon Marchi wrote:

> I don't recall what's the long term solution for this.  We could use gnulib's namespace support [2], but the disadvantage is that we would need to use gnulib::some_function (assuming we name the namespace "gnulib") instead of just some_function to use the "fixed" version.  If we use some_function directly, it will use the buggy version on those systems where it is buggy.

Wrap all of gdb in a namespace.  Recall that this was what led to C++ wildmatching support.

Thanks,
Pedro Alves

Re: MinGW build failure for GDB 8.2.90 with source-highlight

[Simon Marchi]

On 2019-03-07 12:11, Pedro Alves wrote:
> On 03/07/2019 04:59 PM, Simon Marchi wrote:
> 
>> I don't recall what's the long term solution for this.  We could use 
>> gnulib's namespace support [2], but the disadvantage is that we would 
>> need to use gnulib::some_function (assuming we name the namespace 
>> "gnulib") instead of just some_function to use the "fixed" version.  
>> If we use some_function directly, it will use the buggy version on 
>> those systems where it is buggy.
> 
> Wrap all of gdb in a namespace.  Recall that this was what led to C++
> wildmatching support.

I don't understand how this will help.  If you have

#define open rpl_open

namespace gdb {
   struct target_ops {
     void open();
   }
}

The macro will still wrongfully replace open.

Simon

Re: MinGW build failure for GDB 8.2.90 with source-highlight

[Pedro Alves]

On 03/07/2019 06:09 PM, Simon Marchi wrote:
> On 2019-03-07 12:11, Pedro Alves wrote:
>> On 03/07/2019 04:59 PM, Simon Marchi wrote:
>>
>>> I don't recall what's the long term solution for this.  We could use gnulib's namespace support [2], but the disadvantage is that we would need to use gnulib::some_function (assuming we name the namespace "gnulib") instead of just some_function to use the "fixed" version.  If we use some_function directly, it will use the buggy version on those systems where it is buggy.
>>
>> Wrap all of gdb in a namespace.  Recall that this was what led to C++
>> wildmatching support.
> 
> I don't understand how this will help.

Sorry, I thought the reference would make you recall
the previous discussions.  See below.

> If you have
> 
> #define open rpl_open
> 
> namespace gdb {
>   struct target_ops {
>     void open();
>   }
> }
> 
> The macro will still wrongfully replace open.

See slides #16-#17 of my Cauldron 2017 presentation:

 https://gcc.gnu.org/wiki/cauldron2017?action=AttachFile&do=view&target=gdb+-+C%2B%2B+conversion+%26+dogfood.pdf

The idea is to put all of gdb under "namespace gdb", and also put
gnulib in the same namespace, using gnulib's namespace support.
That way, there are no macros, and, the code looks just like it
does today, except for the "namespace gdb" wrapping.

Prototyped here:
 https://github.com/palves/gdb/commits/palves/cxx-gdb-namespace

Thanks,
Pedro Alves

Re: MinGW build failure for GDB 8.2.90 with source-highlight

[Simon Marchi]

On 2019-03-07 13:20, Pedro Alves wrote:
> On 03/07/2019 06:09 PM, Simon Marchi wrote:
>> On 2019-03-07 12:11, Pedro Alves wrote:
>>> On 03/07/2019 04:59 PM, Simon Marchi wrote:
>>> 
>>>> I don't recall what's the long term solution for this.  We could use 
>>>> gnulib's namespace support [2], but the disadvantage is that we 
>>>> would need to use gnulib::some_function (assuming we name the 
>>>> namespace "gnulib") instead of just some_function to use the "fixed" 
>>>> version.  If we use some_function directly, it will use the buggy 
>>>> version on those systems where it is buggy.
>>> 
>>> Wrap all of gdb in a namespace.  Recall that this was what led to C++
>>> wildmatching support.
>> 
>> I don't understand how this will help.
> 
> Sorry, I thought the reference would make you recall
> the previous discussions.  See below.

Yeah, I recalled these discussions, but I couldn't make the pieces fit 
in my head...

>> If you have
>> 
>> #define open rpl_open
>> 
>> namespace gdb {
>>   struct target_ops {
>>     void open();
>>   }
>> }
>> 
>> The macro will still wrongfully replace open.
> 
> See slides #16-#17 of my Cauldron 2017 presentation:
> 
> 
> https://gcc.gnu.org/wiki/cauldron2017?action=AttachFile&do=view&target=gdb+-+C%2B%2B+conversion+%26+dogfood.pdf
> 
> The idea is to put all of gdb under "namespace gdb", and also put
> gnulib in the same namespace, using gnulib's namespace support.
> That way, there are no macros, and, the code looks just like it
> does today, except for the "namespace gdb" wrapping.
> 
> Prototyped here:
>  https://github.com/palves/gdb/commits/palves/cxx-gdb-namespace

Ok, the key I was missing is that gdb and gnulib are in the same 
namespace, so if you call "close", for example, it uses gnulib's close.

Thanks!

Simon

Re: MinGW build failure for GDB 8.2.90 with source-highlight

[Eli Zaretskii]

> Cc: Eli Zaretskii <eliz@gnu.org>, gdb-patches@sourceware.org
> From: Pedro Alves <palves@redhat.com>
> Date: Thu, 7 Mar 2019 18:20:11 +0000
> 
> See slides #16-#17 of my Cauldron 2017 presentation:
> 
>  https://gcc.gnu.org/wiki/cauldron2017?action=AttachFile&do=view&target=gdb+-+C%2B%2B+conversion+%26+dogfood.pdf
> 
> The idea is to put all of gdb under "namespace gdb", and also put
> gnulib in the same namespace, using gnulib's namespace support.
> That way, there are no macros, and, the code looks just like it
> does today, except for the "namespace gdb" wrapping.
> 
> Prototyped here:
>  https://github.com/palves/gdb/commits/palves/cxx-gdb-namespace

Is this something that we'd like to do for GDB 8.3?  If not, then some
simpler/safer solution would be needed for the 8.3 branch.

Re: MinGW build failure for GDB 8.2.90 with source-highlight

[Eli Zaretskii]

> Date: Thu, 07 Mar 2019 11:59:47 -0500
> From: Simon Marchi <simon.marchi@polymtl.ca>
> Cc: gdb-patches@sourceware.org
> 
> Since there are problems to fix with open on mingw (according to [1]), I 
> think it's fine that gnulib provides replacements.

For calls into the C runtime, sure.  But I don't think they can
replace the methods reflected to C++, because libstdc++ will call the
C runtime against which it was linked, at least on MS-Windows.

> Undefining the macros seems dangerous too, if you actually use the 
> "open" function from the C standard library on a system where it is 
> buggy, you want to end up with gnulib's fixed version...

There's no such call in source-cache.c, AFAICS.

Re: MinGW build failure for GDB 8.2.90 with source-highlight

[Eli Zaretskii]

Ping!  This discussion seems to have stalled.  I'd like to have it
solved before GDB 8.3 is released.

If no better ideas come up, I'd like to commit the change described
here:

> Perhaps we should simply #undef these two symbols before including
> <fstream>?

TIA

Re: MinGW build failure for GDB 8.2.90 with source-highlight

[Simon Marchi]

On 2019-03-11 10:18, Eli Zaretskii wrote:
> Ping!  This discussion seems to have stalled.  I'd like to have it
> solved before GDB 8.3 is released.

Agreed, this is definitely a blocker for the release.  I recorded it 
here:

https://www.sourceware.org/gdb/wiki/GDB_8.3_Release

> If no better ideas come up, I'd like to commit the change described
> here:
> 
>> Perhaps we should simply #undef these two symbols before including
>> <fstream>?
> 
> TIA

If the change is localized in one or a handful of files, I think it 
would be acceptable for the 8.3 release, since the alternative solution 
would be (1) a lot of work and (2) risky.

Can you post a patch that fixes the build for you?

Pedro, did you have a branch where you put all of gdb in the gdb 
namespace?  I only found this branch, but it's using the gnulib:: 
namespace, which is not the same.

https://github.com/palves/gdb/commits/palves/cxx-gnulib-namespace

I just started to try to do it to get a feel of what's needed.  I guess 
we need to put pretty much everything except includes (and maybe some 
other rare exceptions) between namespace gdb { ... }?  And of course 
define GNULIB_NAMESPACE to gdb.

Simon

Re: MinGW build failure for GDB 8.2.90 with source-highlight

[Pedro Alves]

On 03/12/2019 02:31 AM, Simon Marchi wrote:
> On 2019-03-11 10:18, Eli Zaretskii wrote:
>> Ping!  This discussion seems to have stalled.  I'd like to have it
>> solved before GDB 8.3 is released.
> 
> Agreed, this is definitely a blocker for the release.  I recorded it here:
> 
> https://www.sourceware.org/gdb/wiki/GDB_8.3_Release
> 
>> If no better ideas come up, I'd like to commit the change described
>> here:
>>
>>> Perhaps we should simply #undef these two symbols before including
>>> <fstream>?
>>
>> TIA
> 
> If the change is localized in one or a handful of files, I think it would be acceptable for the 8.3 release, since the alternative solution would be (1) a lot of work and (2) risky.
> 
> Can you post a patch that fixes the build for you?
> 
> Pedro, did you have a branch where you put all of gdb in the gdb namespace?  I only found this branch, but it's using the gnulib:: namespace, which is not the same.
> 
> https://github.com/palves/gdb/commits/palves/cxx-gnulib-namespace
> 

Right, not that one.  That was the first attempt, and then I almost posted
that one upstream at the time.  I was about to press 'git send-email' but
got cold feet, thinking there must be a better way.  That's when
I tried the "namespace gdb" approach instead.  And that one is in
the branch I pointed at in my previous message:

> Prototyped here:
>  https://github.com/palves/gdb/commits/palves/cxx-gdb-namespace

Note: cxx-gdb-namespace != cxx-gnulib-namespace

> I just started to try to do it to get a feel of what's needed.  I guess we need to put pretty much everything except includes (and maybe some other rare exceptions) between namespace gdb { ... }?  And of course define GNULIB_NAMESPACE to gdb.
Yes, that's the gist of it.  

I've actually been working on that since Friday.  Spent a chunk of the weekend
on it, and some time yesterday.  Meant to reply back yesterday, but something
got in the way.

I have a script in the branch that does a good chunk of the work, but unfortunately,
it still requires a lot of manual touching up.  I thought the rebase would be
trivial, but with all the new files and churn in the codebase since 2016, it
still involved a lot of manual re-work.  I have it building on x86-64 now.
I've forced pushed what I have now.  The branch has >130 patches currently, most of
the them are small per-file patches.  I need to clean this up a bit, squash some
of the fixes-on-fixes patches.  I'm not exactly sure how to post this to the
list...  And also, of course this needs at least build-testing on a wider set
of hosts/cross compilers.  Maybe people could take a preliminary look at the
branch, see if they agree with direction?

But in any case, I think this would be too invasive for the 8.3
branch.  A smaller fix there would be safer.

Thanks,
Pedro Alves

Re: MinGW build failure for GDB 8.2.90 with source-highlight

[Simon Marchi]

On 2019-03-12 06:52, Pedro Alves wrote:
>> Pedro, did you have a branch where you put all of gdb in the gdb 
>> namespace?  I only found this branch, but it's using the gnulib:: 
>> namespace, which is not the same.
>> 
>> https://github.com/palves/gdb/commits/palves/cxx-gnulib-namespace
>> 
> 
> Right, not that one.  That was the first attempt, and then I almost 
> posted
> that one upstream at the time.  I was about to press 'git send-email' 
> but
> got cold feet, thinking there must be a better way.  That's when
> I tried the "namespace gdb" approach instead.  And that one is in
> the branch I pointed at in my previous message:
> 
>> Prototyped here:
>>  https://github.com/palves/gdb/commits/palves/cxx-gdb-namespace
> 
> Note: cxx-gdb-namespace != cxx-gnulib-namespace

Ah oops, sorry about that, it was late!

>> I just started to try to do it to get a feel of what's needed.  I 
>> guess we need to put pretty much everything except includes (and maybe 
>> some other rare exceptions) between namespace gdb { ... }?  And of 
>> course define GNULIB_NAMESPACE to gdb.
> Yes, that's the gist of it.
> 
> I've actually been working on that since Friday.  Spent a chunk of the 
> weekend
> on it, and some time yesterday.  Meant to reply back yesterday, but 
> something
> got in the way.
> 
> I have a script in the branch that does a good chunk of the work, but
> unfortunately,
> it still requires a lot of manual touching up.  I thought the rebase 
> would be
> trivial, but with all the new files and churn in the codebase since 
> 2016, it
> still involved a lot of manual re-work.

I can imagine.

> I have it building on x86-64 now.
> I've forced pushed what I have now.  The branch has >130 patches
> currently, most of
> the them are small per-file patches.  I need to clean this up a bit, 
> squash some
> of the fixes-on-fixes patches.  I'm not exactly sure how to post this 
> to the
> list...

I don't really see how this can be done incrementally, it would be 
really complicated to add the namespace to just some files, I think.  So 
I don't see any other choice than one big patch.  Plus maybe some others 
for preparation cleanups.  Ideally the big patch would be trivial and 
not move too much things around.

> And also, of course this needs at least build-testing on a wider set
> of hosts/cross compilers.  Maybe people could take a preliminary look 
> at the
> branch, see if they agree with direction?

I diffed the files that I modified manually when just trying it out 
yesterday with the same files in your branch, and there is no 
significant difference.  I think the direction you took is fine.

Simon

Re: MinGW build failure for GDB 8.2.90 with source-highlight

[Eli Zaretskii]

> X-Spam-Status: No, score=-1.5 required=5.0 tests=BAYES_50,RCVD_IN_DNSWL_MED,
> 	URIBL_BLOCKED autolearn=disabled version=3.3.2
> Date: Mon, 11 Mar 2019 22:31:17 -0400
> From: Simon Marchi <simon.marchi@polymtl.ca>
> Cc: gdb-patches@sourceware.org, Pedro Alves <palves@redhat.com>
> 
> >> Perhaps we should simply #undef these two symbols before including
> >> <fstream>?
> > 
> > TIA
> 
> If the change is localized in one or a handful of files, I think it 
> would be acceptable for the 8.3 release, since the alternative solution 
> would be (1) a lot of work and (2) risky.
> 
> Can you post a patch that fixes the build for you?

Sure, here it is:

diff --git a/gdb/source-cache.c b/gdb/source-cache.c
index 097c8a3..b98272c 100644
--- a/gdb/source-cache.c
+++ b/gdb/source-cache.c
@@ -23,6 +23,8 @@
 #include "cli/cli-style.h"
 
 #ifdef HAVE_SOURCE_HIGHLIGHT
+#undef open
+#undef close
 #include <fstream>
 #include <sstream>
 #include <srchilite/sourcehighlight.h>

Re: MinGW build failure for GDB 8.2.90 with source-highlight

[Simon Marchi]

On 2019-03-12 11:49, Eli Zaretskii wrote:
>> X-Spam-Status: No, score=-1.5 required=5.0 
>> tests=BAYES_50,RCVD_IN_DNSWL_MED,
>> 	URIBL_BLOCKED autolearn=disabled version=3.3.2
>> Date: Mon, 11 Mar 2019 22:31:17 -0400
>> From: Simon Marchi <simon.marchi@polymtl.ca>
>> Cc: gdb-patches@sourceware.org, Pedro Alves <palves@redhat.com>
>> 
>> >> Perhaps we should simply #undef these two symbols before including
>> >> <fstream>?
>> >
>> > TIA
>> 
>> If the change is localized in one or a handful of files, I think it
>> would be acceptable for the 8.3 release, since the alternative 
>> solution
>> would be (1) a lot of work and (2) risky.
>> 
>> Can you post a patch that fixes the build for you?
> 
> Sure, here it is:
> 
> diff --git a/gdb/source-cache.c b/gdb/source-cache.c
> index 097c8a3..b98272c 100644
> --- a/gdb/source-cache.c
> +++ b/gdb/source-cache.c
> @@ -23,6 +23,8 @@
>  #include "cli/cli-style.h"
> 
>  #ifdef HAVE_SOURCE_HIGHLIGHT
> +#undef open
> +#undef close
>  #include <fstream>
>  #include <sstream>
>  #include <srchilite/sourcehighlight.h>

Thanks, this is fine with me, maybe with a comment explaining why it's 
needed.  I also think you could push it to the master branch, so that it 
builds for mingw users.  We will try to get the proper solution merged 
in the next release cycle, at which point we can remove this "hack".

Simon

Re: MinGW build failure for GDB 8.2.90 with source-highlight

[Eli Zaretskii]

> Date: Tue, 12 Mar 2019 12:29:48 -0400
> From: Simon Marchi <simon.marchi@polymtl.ca>
> Cc: gdb-patches@sourceware.org, palves@redhat.com
> 
> > diff --git a/gdb/source-cache.c b/gdb/source-cache.c
> > index 097c8a3..b98272c 100644
> > --- a/gdb/source-cache.c
> > +++ b/gdb/source-cache.c
> > @@ -23,6 +23,8 @@
> >  #include "cli/cli-style.h"
> > 
> >  #ifdef HAVE_SOURCE_HIGHLIGHT
> > +#undef open
> > +#undef close
> >  #include <fstream>
> >  #include <sstream>
> >  #include <srchilite/sourcehighlight.h>
> 
> Thanks, this is fine with me, maybe with a comment explaining why it's 
> needed.  I also think you could push it to the master branch, so that it 
> builds for mingw users.

Done, on both counts.

Thanks for the review.