[PATCH] Define GNULIB_NAMESPACE in unittests/string_view-selftests.c

[PATCH] Define GNULIB_NAMESPACE in unittests/string_view-selftests.c

[Simon Marchi]

When building with x86_64-w64-mingw32-g++ (to test cross-compiling for
Windows), I get this error:

unittests/string_view-selftests.o: In function `selftests::string_view::inserters_2::test05(unsigned long long)':
/home/emaisin/src/binutils-gdb/gdb/unittests/basic_string_view/inserters/char/2.cc:60: undefined reference to `std::basic_ofstream<char, std::char_traits<char> >::rpl_close()'

This is caused by gnulib redefining "close" as "rpl_close", and
therefore messing up the declaration of basic_ofstream in the libstdc++
header.  The solution would be to use gnulib namespaces [1].  Until we
use them across GDB, we can use them locally in files that are
problematic, like this one.

gdb/ChangeLog:

	* unittests/string_view-selftests.c: Define GNULIB_NAMESPACE.
---
 gdb/unittests/string_view-selftests.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/gdb/unittests/string_view-selftests.c b/gdb/unittests/string_view-selftests.c
index 182a5df..55ffe64 100644
--- a/gdb/unittests/string_view-selftests.c
+++ b/gdb/unittests/string_view-selftests.c
@@ -21,6 +21,8 @@
    the "real" version.  */
 #if __cplusplus < 201703L
 
+#define GNULIB_NAMESPACE gnulib
+
 #include "defs.h"
 #include "selftest.h"
 #include "common/gdb_string_view.h"
-- 
2.7.4

Re: [PATCH] Define GNULIB_NAMESPACE in unittests/string_view-selftests.c

[Joel Brobecker]

> When building with x86_64-w64-mingw32-g++ (to test cross-compiling for
> Windows), I get this error:
> 
> unittests/string_view-selftests.o: In function `selftests::string_view::inserters_2::test05(unsigned long long)':
> /home/emaisin/src/binutils-gdb/gdb/unittests/basic_string_view/inserters/char/2.cc:60: undefined reference to `std::basic_ofstream<char, std::char_traits<char> >::rpl_close()'
> 
> This is caused by gnulib redefining "close" as "rpl_close", and
> therefore messing up the declaration of basic_ofstream in the libstdc++
> header.  The solution would be to use gnulib namespaces [1].  Until we
> use them across GDB, we can use them locally in files that are
> problematic, like this one.
> 
> gdb/ChangeLog:
> 
> 	* unittests/string_view-selftests.c: Define GNULIB_NAMESPACE.

I noticed the exact same problem, and came up with the exact same
solution. Sorry for not reporting right away, I have been swamped
:-( and also wanted to spend some more time investigating the bigger
picture; it seems to me like we may be having a huge time bomb
in our hands?!?

In other words, either we don't define GNULIB_NAMESPACE, but then
we run into unwanted renames of class method names; or we defined
GNULIB_NAMESPACE, but that means we must be prefixing all the symbols
we call that are covered by the imported part of GNULIB with that
namespace.

What worries me is that I don't see what's preventing us from hitting
that issue outside of the unittests code? We know we can adjust our
own classes, but this problem occured with a system class, so we had
no choice but to use GNULIB_NAMESPACE. I worry that the transition
from no GNULIB_NAMESPACE to using GNULIB_NAMESPACE in a given unit
will leave some C system calls that should normally be covered by
gnulib silently now reverting to the system (buggy) version.

-- 
Joel

Re: [PATCH] Define GNULIB_NAMESPACE in unittests/string_view-selftests.c

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 1477 bytes --]

> > gdb/ChangeLog:
> > 
> > 	* unittests/string_view-selftests.c: Define GNULIB_NAMESPACE.
> 
> I noticed the exact same problem, and came up with the exact same
> solution. Sorry for not reporting right away, I have been swamped
> :-( and also wanted to spend some more time investigating the bigger
> picture; it seems to me like we may be having a huge time bomb
> in our hands?!?
> 
> In other words, either we don't define GNULIB_NAMESPACE, but then
> we run into unwanted renames of class method names; or we defined
> GNULIB_NAMESPACE, but that means we must be prefixing all the symbols
> we call that are covered by the imported part of GNULIB with that
> namespace.
> 
> What worries me is that I don't see what's preventing us from hitting
> that issue outside of the unittests code? We know we can adjust our
> own classes, but this problem occured with a system class, so we had
> no choice but to use GNULIB_NAMESPACE. I worry that the transition
> from no GNULIB_NAMESPACE to using GNULIB_NAMESPACE in a given unit
> will leave some C system calls that should normally be covered by
> gnulib silently now reverting to the system (buggy) version.

Actually, for the record, here is the patch I came up with.
Similar in nature, but additional comments and also only defining
the GNULIB_NAMESPACE macro on Windows hosts -- as a strategy to
minimize impact.

I think your approach is best, as we'll at least get the same
conditions regardless of the host.

-- 
Joel

[-- Attachment #2: 0001-avoid-undefined-reference-to-basic_ofstream-rpl_clos.patch --]
[-- Type: text/x-diff, Size: 2620 bytes --]

From 711720d2b969d679880383cafe48fb09a1f21262 Mon Sep 17 00:00:00 2001
From: Joel Brobecker <brobecker@adacore.com>
Date: Tue, 24 Apr 2018 13:07:19 -0400
Subject: [PATCH] avoid undefined reference to basic_ofstream::rpl_close durin
 GDB link

On Windows hosts, GDB currently fails to link with the following error:

    |  CXXLD  gdb.exe
    |unittests/string_view-selftests.o: In function `ZN9selftests11string_view11inserters_26test05Ej':
    |/[...]/gdb/unittests/basic_string_view/inserters/char/2.cc:60: undefined reference to `std::basic_ofstream<char, std::char_traits<char> >::rpl_close()'
    |collect2.exe: error: ld returned 1 exit status

This is due to gnulib's unistd.h defining "close" as "rpl_close",
which interferes with our use of ofstream::close.

This patch works around the issue by defining GNULIB_NAMESPACE
inside the unit test itself, when building on Windows hosts.

gdb/ChangeLog:

	* unittests/string_view-selftests.c [_WIN32]: Define GNULIB_NAMESPACE.

TN: R424-035
---
 gdb/unittests/string_view-selftests.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/gdb/unittests/string_view-selftests.c b/gdb/unittests/string_view-selftests.c
index 182a5df9e4d..dd8bf8b8c52 100644
--- a/gdb/unittests/string_view-selftests.c
+++ b/gdb/unittests/string_view-selftests.c
@@ -21,6 +21,33 @@
    the "real" version.  */
 #if __cplusplus < 201703L
 
+#if defined (_WIN32)
+/* On Windows hosts, gnulib's unistd.h uses a macro to redefine "close"
+   into "rpl_close", because of:
+
+        #  if !(defined __cplusplus && defined GNULIB_NAMESPACE)
+        #   undef close
+        #   define close rpl_close
+        #  endif
+
+   This interferes with one of the tests below that calls class
+   std::ofstream's "close" method, which gets renamed into a call
+   to non-existant "rpl_close" method instead, thus leading to
+   an undefined symbol error during the link:
+
+        unittests/basic_string_view/inserters/char/2.cc:60:
+	undefined reference toi
+	`std::basic_ofstream<char, std::char_traits<char> >::rpl_close()'
+
+   The gnulib documentation discusses this exact issue and says
+   we can define GNULIB_NAMESPACE to put all gnulib symbols into
+   the given namespace, but this is at the cost of gnulib no longer
+   replacing possibly faulty system functions. We have no real choice
+   here, and this is about unittest, so define GNULIB_NAMESPACE,
+   solely for this unit (the gnulib manual confirms this is OK).  */
+#define GNULIB_NAMESPACE gnulib
+#endif
+
 #include "defs.h"
 #include "selftest.h"
 #include "common/gdb_string_view.h"
-- 
2.11.0

Re: [PATCH] Define GNULIB_NAMESPACE in unittests/string_view-selftests.c

[Pedro Alves]

On 05/04/2018 05:55 PM, Joel Brobecker wrote:

> What worries me is that I don't see what's preventing us from hitting
> that issue outside of the unittests code? We know we can adjust our
> own classes, but this problem occured with a system class, so we had
> no choice but to use GNULIB_NAMESPACE. I worry that the transition
> from no GNULIB_NAMESPACE to using GNULIB_NAMESPACE in a given unit
> will leave some C system calls that should normally be covered by
> gnulib silently now reverting to the system (buggy) version.

Note that this is what led me to work on the whole wild/full matching
for C++, the TAB improvements, etc.  See:

 https://sourceware.org/ml/gdb-patches/2017-06/msg00012.html

I haven't rebased/updated the cxx-gdb-namespace branch since, as
I assumed it prudent to wait some time after a gdb is released
with support for wildmatching.  That has happened, so maybe we can
consider going through with wrapping all of gdb in a namespace?
Or is it still too soon?

Thanks,
Pedro Alves

Re: [PATCH] Define GNULIB_NAMESPACE in unittests/string_view-selftests.c

[Joel Brobecker]

> I haven't rebased/updated the cxx-gdb-namespace branch since, as
> I assumed it prudent to wait some time after a gdb is released
> with support for wildmatching.  That has happened, so maybe we can
> consider going through with wrapping all of gdb in a namespace?
> Or is it still too soon?

Not sure myself whether it's too soon or not. But if we were to
switch sooner rather than later, I would say now is a pretty good
time. Or perhaps right after we release 18.2? It gives us a good
3 months of lead time before we create the next release branch,
while the delay up to now has probably helped keep the master
and gdb-8.1-branch close enough to facilitate backporting...

-- 
Joel

Re: [PATCH] Define GNULIB_NAMESPACE in unittests/string_view-selftests.c

[Simon Marchi]

On 2018-05-04 13:10, Pedro Alves wrote:
> On 05/04/2018 05:55 PM, Joel Brobecker wrote:
> 
>> What worries me is that I don't see what's preventing us from hitting
>> that issue outside of the unittests code? We know we can adjust our
>> own classes, but this problem occured with a system class, so we had
>> no choice but to use GNULIB_NAMESPACE. I worry that the transition
>> from no GNULIB_NAMESPACE to using GNULIB_NAMESPACE in a given unit
>> will leave some C system calls that should normally be covered by
>> gnulib silently now reverting to the system (buggy) version.
> 
> Note that this is what led me to work on the whole wild/full matching
> for C++, the TAB improvements, etc.  See:
> 
>  https://sourceware.org/ml/gdb-patches/2017-06/msg00012.html
> 
> I haven't rebased/updated the cxx-gdb-namespace branch since, as
> I assumed it prudent to wait some time after a gdb is released
> with support for wildmatching.  That has happened, so maybe we can
> consider going through with wrapping all of gdb in a namespace?
> Or is it still too soon?
> 
> Thanks,
> Pedro Alves

I pushed the patch to un-break the mingw build.

Could you remind me what's the link between putting gdb in its own 
namespace and putting gnulib in its own namespace?  How are they 
related?

Simon

Re: [PATCH] Define GNULIB_NAMESPACE in unittests/string_view-selftests.c

[Joel Brobecker]

> I pushed the patch to un-break the mingw build.

Thank you!

> Could you remind me what's the link between putting gdb in its own
> namespace and putting gnulib in its own namespace?  How are they
> related?

I think it's about making it easier to debug code that's declared
inside a namespace. In other words, it allows us to say "break
run_command", rather than having to type "break gdb::run_command".

-- 
Joel

Re: [PATCH] Define GNULIB_NAMESPACE in unittests/string_view-selftests.c

[Pedro Alves]

On 05/08/2018 09:47 PM, Simon Marchi wrote:

> Could you remind me what's the link between putting gdb in its own namespace and putting gnulib in its own namespace?  How are they related?

You can see the whole story in more detail in my C++ dogfooding talk
in last year's Caldron (around 11mins in), but the gist of it is

So late 2016 / early 2017 I looked at fixing this whole gnulib
issue with macros for good, by using gnulib's namespace support,
by putting:

 #define GNULIB_NAMESPACE gnulib

in common/common-defs.h, and then fix up the whole tree.

The problem with that is you have to adjust _all_  calls to
functions that gnulib replaces, like e.g.:

  - "printf" => "gnulib::printf"
  - "fwrite" => "gnulib::fwrite"

And it's not a small number.  After a lot of tedious hacking, I got
GDB to build on x86 GNU/Linux, see here:

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

Specifically:

  https://github.com/palves/gdb/commit/bad0ab49ccc3c0de131bc6788da3703924d0903e

Note that patch has a detailed commit log as well, as I almost
posted it at the time.  But then the ugliness of having to write
(and read!) "gnulib::" all over the place put me off more and more
and eventually I caved in and looked for a better idea -- some way
to avoid those explicit "gnulib::"s.

The alternative idea was to put _all_ of gdb in a namespace
instead.

I'd argue that that's a good thing on its own, but the
connection with gnulib is that while you can't do

  using gnulib::printf;

at top level to avoid having to explicitly write the "gnulib::"
namespace throughout all the calls, because that would conflict with
the system's "printf" (it wouldn't compile), you can do that in
a namespace, like:

  namespace gdb {
    using gnulib::printf;
    ...
  }

or simpler, just do:

 #define GNULIB_NAMESPACE gdb

And with that, if all of gdb including the gnulib replacements are in
the same namespace then the calls to printf etc no longer need any
explicit "gnulib::" namespace qualifier, a plain "printf" call
calls "gdb::printf", etc.

That is the approach taken in the palves/cxx-gdb-namespace branch:

  https://github.com/palves/gdb/compare/palves/cxx-gdb-namespace

The downside of putting all of gdb in a namespace is that
gdb becomes a little harder to debug, as you now must
prefix breakpoint locations with "gdb::", like:

  (gdb) b gdb::internal_error
  (gdb) b gdb::foo_bar

... and that's what led to last year's C++ wildmatching support,
"b -qualified", tab completion improvements, etc.

So I was waiting for gdb 8.1 (at least) to be out before proposing
moving all of gdb to a namespace, in order to give folks a little
time to get comfortable with the new features, and make it
reasonable to suggest that folks upgrade their top gdb to 8.1 (a
released version) instead of to git/trunk gdb if they want to gdb
a bit more conveniently.  That's why I was wondering whether
now would be good time to propose moving forward with the
"namespace gdb" approach, since 8.1 has been out for a bit.

Thanks,
Pedro Alves

Re: [PATCH] Define GNULIB_NAMESPACE in unittests/string_view-selftests.c

[Joel Brobecker]

[...]
> ... and that's what led to last year's C++ wildmatching support,
> "b -qualified", tab completion improvements, etc.

Very interesting, erm, story. Thanks for taking the time to share
it again.

> So I was waiting for gdb 8.1 (at least) to be out before proposing
> moving all of gdb to a namespace, in order to give folks a little
> time to get comfortable with the new features, and make it
> reasonable to suggest that folks upgrade their top gdb to 8.1 (a
> released version) instead of to git/trunk gdb if they want to gdb
> a bit more conveniently.  That's why I was wondering whether
> now would be good time to propose moving forward with the
> "namespace gdb" approach, since 8.1 has been out for a bit.

Personally, I always use the latest and greatest, as a way of
giving it more actual-use testing. So switching now wouldn't be
a problem.

Since 8.1 is out, and we had very very very very very few bug
reports so far, I would vote for going ahead now.

-- 
Joel

Re: [PATCH] Define GNULIB_NAMESPACE in unittests/string_view-selftests.c

[Simon Marchi]

On 2018-05-09 09:49, Pedro Alves wrote:
> On 05/08/2018 09:47 PM, Simon Marchi wrote:
> 
>> Could you remind me what's the link between putting gdb in its own 
>> namespace and putting gnulib in its own namespace?  How are they 
>> related?
> 
> You can see the whole story in more detail in my C++ dogfooding talk
> in last year's Caldron (around 11mins in), but the gist of it is
> 
> So late 2016 / early 2017 I looked at fixing this whole gnulib
> issue with macros for good, by using gnulib's namespace support,
> by putting:
> 
>  #define GNULIB_NAMESPACE gnulib
> 
> in common/common-defs.h, and then fix up the whole tree.
> 
> The problem with that is you have to adjust _all_  calls to
> functions that gnulib replaces, like e.g.:
> 
>   - "printf" => "gnulib::printf"
>   - "fwrite" => "gnulib::fwrite"
> 
> And it's not a small number.  After a lot of tedious hacking, I got
> GDB to build on x86 GNU/Linux, see here:
> 
>   https://github.com/palves/gdb/commits/palves/cxx-gnulib-namespace
> 
> Specifically:
> 
>   
> https://github.com/palves/gdb/commit/bad0ab49ccc3c0de131bc6788da3703924d0903e
> 
> Note that patch has a detailed commit log as well, as I almost
> posted it at the time.  But then the ugliness of having to write
> (and read!) "gnulib::" all over the place put me off more and more
> and eventually I caved in and looked for a better idea -- some way
> to avoid those explicit "gnulib::"s.
> 
> The alternative idea was to put _all_ of gdb in a namespace
> instead.
> 
> I'd argue that that's a good thing on its own, but the
> connection with gnulib is that while you can't do
> 
>   using gnulib::printf;
> 
> at top level to avoid having to explicitly write the "gnulib::"
> namespace throughout all the calls, because that would conflict with
> the system's "printf" (it wouldn't compile), you can do that in
> a namespace, like:
> 
>   namespace gdb {
>     using gnulib::printf;
>     ...
>   }
> 
> or simpler, just do:
> 
>  #define GNULIB_NAMESPACE gdb
> 
> And with that, if all of gdb including the gnulib replacements are in
> the same namespace then the calls to printf etc no longer need any
> explicit "gnulib::" namespace qualifier, a plain "printf" call
> calls "gdb::printf", etc.
> 
> That is the approach taken in the palves/cxx-gdb-namespace branch:
> 
>   https://github.com/palves/gdb/compare/palves/cxx-gdb-namespace
> 
> The downside of putting all of gdb in a namespace is that
> gdb becomes a little harder to debug, as you now must
> prefix breakpoint locations with "gdb::", like:
> 
>   (gdb) b gdb::internal_error
>   (gdb) b gdb::foo_bar
> 
> ... and that's what led to last year's C++ wildmatching support,
> "b -qualified", tab completion improvements, etc.
> 
> So I was waiting for gdb 8.1 (at least) to be out before proposing
> moving all of gdb to a namespace, in order to give folks a little
> time to get comfortable with the new features, and make it
> reasonable to suggest that folks upgrade their top gdb to 8.1 (a
> released version) instead of to git/trunk gdb if they want to gdb
> a bit more conveniently.  That's why I was wondering whether
> now would be good time to propose moving forward with the
> "namespace gdb" approach, since 8.1 has been out for a bit.

Ok, it's the part about wanting to use "using gnulib::..." that makes 
the gdb namespace necessary that I had forgotten about.  Either way 
would be ok with me ("using gnulib::..." or using the namespace 
explicitly at every call.  They both have their advantages.

Like Joel, I'm fine with going ahead now.

Thanks,

Simon

Re: [PATCH] Define GNULIB_NAMESPACE in unittests/string_view-selftests.c

[Simon Marchi]

On 2018-05-04 12:55 PM, Joel Brobecker wrote:
>> When building with x86_64-w64-mingw32-g++ (to test cross-compiling for
>> Windows), I get this error:
>>
>> unittests/string_view-selftests.o: In function `selftests::string_view::inserters_2::test05(unsigned long long)':
>> /home/emaisin/src/binutils-gdb/gdb/unittests/basic_string_view/inserters/char/2.cc:60: undefined reference to `std::basic_ofstream<char, std::char_traits<char> >::rpl_close()'
>>
>> This is caused by gnulib redefining "close" as "rpl_close", and
>> therefore messing up the declaration of basic_ofstream in the libstdc++
>> header.  The solution would be to use gnulib namespaces [1].  Until we
>> use them across GDB, we can use them locally in files that are
>> problematic, like this one.
>>
>> gdb/ChangeLog:
>>
>> 	* unittests/string_view-selftests.c: Define GNULIB_NAMESPACE.
> 
> I noticed the exact same problem, and came up with the exact same
> solution. Sorry for not reporting right away, I have been swamped
> :-( and also wanted to spend some more time investigating the bigger
> picture; it seems to me like we may be having a huge time bomb
> in our hands?!?

I am not sure if it can get any worse than that, just a compile or link
failure that is relatively easy to fix.

> In other words, either we don't define GNULIB_NAMESPACE, but then
> we run into unwanted renames of class method names; or we defined
> GNULIB_NAMESPACE, but that means we must be prefixing all the symbols
> we call that are covered by the imported part of GNULIB with that
> namespace.

Exact.
> What worries me is that I don't see what's preventing us from hitting
> that issue outside of the unittests code? We know we can adjust our
> own classes, but this problem occured with a system class, so we had
> no choice but to use GNULIB_NAMESPACE. I worry that the transition
> from no GNULIB_NAMESPACE to using GNULIB_NAMESPACE in a given unit
> will leave some C system calls that should normally be covered by
> gnulib silently now reverting to the system (buggy) version.

Actually, gnulib seems to poison the function it replaces, so we won't be
able to use it by mistake.  Let's say I add a "close" call:

  CXX    unittests/string_view-selftests.o
/home/emaisin/src/binutils-gdb/gdb/unittests/string_view-selftests.c: In function ‘void selftests::string_view::run_tests()’:
/home/emaisin/src/binutils-gdb/gdb/unittests/string_view-selftests.c:169:12: error: call to ‘close’ declared with attribute warning: The symbol ::close refers to the system function. Use gnulib::close instead. [-Werror]
   close (0);
            ^

And since we can enable GNULIB_NAMESPACE per compilation unit, we can
do it progressively.  Once we are done, we can remove all those
GNULIB_NAMESPACE defines and have a single one in common-defs.h, just
before including gnulib.

Pedro already started playing with that a while ago, I am not sure
what was the conclusion:

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

I'll wait for him to give his feedback on this patch.

Simon

Re: [PATCH] Define GNULIB_NAMESPACE in unittests/string_view-selftests.c

[Joel Brobecker]

> > What worries me is that I don't see what's preventing us from hitting
> > that issue outside of the unittests code? We know we can adjust our
> > own classes, but this problem occured with a system class, so we had
> > no choice but to use GNULIB_NAMESPACE. I worry that the transition
> > from no GNULIB_NAMESPACE to using GNULIB_NAMESPACE in a given unit
> > will leave some C system calls that should normally be covered by
> > gnulib silently now reverting to the system (buggy) version.
> 
> Actually, gnulib seems to poison the function it replaces, so we won't be
> able to use it by mistake.  Let's say I add a "close" call:
> 
>   CXX    unittests/string_view-selftests.o
> /home/emaisin/src/binutils-gdb/gdb/unittests/string_view-selftests.c: In function ‘void selftests::string_view::run_tests()’:
> /home/emaisin/src/binutils-gdb/gdb/unittests/string_view-selftests.c:169:12: error: call to ‘close’ declared with attribute warning: The symbol ::close refers to the system function. Use gnulib::close instead. [-Werror]
>    close (0);
>             ^
> 
> And since we can enable GNULIB_NAMESPACE per compilation unit, we can
> do it progressively.  Once we are done, we can remove all those
> GNULIB_NAMESPACE defines and have a single one in common-defs.h, just
> before including gnulib.

Aha! This completely reassures me. Thanks!

-- 
Joel