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

[Joel Brobecker <brobecker@adacore.com>]

[-- 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