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

Joel Brobecker
2018-05-04T16:59:52Z

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