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

[Simon Marchi <simon.marchi@ericsson.com>]

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