Discussion:
[PATCH] Avoid having GNULIB_NAMESPACE::func always inject references to rpl_func
Pedro Alves
2016-11-12 16:22:54 UTC
Permalink
Currently, with C++ namespace support enabled, by defining
GNULIB_NAMESPACE, the replacement symbols put in the GNULIB_NAMESPACE
namespace are function pointers that point to the rpl_func functions.
This means that the linker must pull in the rpl_func functions, even
when "GNULIB_NAMESPACE::func(....)" is not called anywhere in the
program.

Fix that by making the GNULIB_NAMESPACE::func symbols be C++ (empty)
objects with inline conversion operators instead of function pointers
that point to the replacement.

This ends up addressing the exiting comment about optimization as
well:

~~~~~~
/* If we were to write
rettype (*const func) parameters = ::func;
like above in _GL_CXXALIAS_RPL_1, the compiler could optimize calls
better (remove an indirection through a 'static' pointer variable),
~~~~~~

Original motivation:

Now that GDB is a C++ program, we stumble on the fact that gnulib
defines its replacements as '#define func rpl_func' macros, which get
in the way of making use of "func" as name of unrelated symbols, in
classes, namespaces, etc.

Gnulib's C++ namespace support was designed to solve that problem, and
I tried enabling it in GDB. After tweaking all replacement function
calls to use the appropriate namespace [e.g., printf(....); ->
gnulib::printf(....);], I stumbled on another problem: gdbserver's
in-process agent (IPA) shared library fails to link at -O0, with:

tracepoint-ipa.o:(.data.rel.ro+0x0): undefined reference to `rpl_mbrtowc'
format-ipa.o:(.data.rel.ro+0x0): undefined reference to `rpl_mbrtowc'
utils-ipa.o:(.data.rel.ro+0x0): undefined reference to `rpl_mbrtowc'
regcache-ipa.o:(.data.rel.ro+0x0): undefined reference to `rpl_mbrtowc'
remote-utils-ipa.o:(.data.rel.ro+0x0): more undefined references to `rpl_mbrtowc' follow
collect2: error: ld returned 1 exit status
Makefile:327: recipe for target 'libinproctrace.so' failed
make: *** [libinproctrace.so] Error 1

This is even though gdbserver's IPA DSO doesn't call mbrtowc anywhere.

Now, the IPA does not link with gnulib purposedly -- it's designed to
be injected in the inferior and be a bridge for running async-signal
safe code that GDB JITs and injects in the inferior's address space.
I.e., it doesn't require much out of the inferior's runtime. Still,
it's very convenient that it shares a lot of code with gdb and
gdbserver, and that in turn means using gnulib headers and some gnulib
header-only features.

Making the IPA link with gnulib would fix this, of course, but that'd
require building gnulib as -fPIC, which we don't do currently, all for
satisfying a link dependency on a function that we don't actually want
to use.

In any case, ending up with strong references to replacement functions
even if they're not used means that binaries end up containing gnulib
code they don't really use. So this seems like a good change even not
considering the GDB's IPA.

ChangeLog:
2016-11-12 Pedro Alves <***@redhat.com>

Avoid having GNULIB_NAMESPACE::func always inject references to
rpl_func.
* build-aux/snippet/c++defs.h [__cplusplus && GNULIB_NAMESPACE]
(_GL_CXXALIAS_RPL_1, _GL_CXXALIAS_RPL_CAST_1, _GL_CXXALIAS_SYS)
(_GL_CXXALIAS_SYS_CAST, _GL_CXXALIAS_SYS_CAST2): Define a wrapper
struct instead of a function pointer.
---
build-aux/snippet/c++defs.h | 66 +++++++++++++++++++++++++++++++--------------
1 file changed, 46 insertions(+), 20 deletions(-)

diff --git a/build-aux/snippet/c++defs.h b/build-aux/snippet/c++defs.h
index 5f19630..8fc95e3 100644
--- a/build-aux/snippet/c++defs.h
+++ b/build-aux/snippet/c++defs.h
@@ -111,14 +111,22 @@
that redirects to rpl_func, if GNULIB_NAMESPACE is defined.
Example:
_GL_CXXALIAS_RPL (open, int, (const char *filename, int flags, ...));
- */
+
+ Wrapping rpl_func in an object with an inline conversion operator
+ avoids a reference to rpl_func unless GNULIB_NAMESPACE::func is
+ actually used in the program. */
#define _GL_CXXALIAS_RPL(func,rettype,parameters) \
_GL_CXXALIAS_RPL_1 (func, rpl_##func, rettype, parameters)
#if defined __cplusplus && defined GNULIB_NAMESPACE
# define _GL_CXXALIAS_RPL_1(func,rpl_func,rettype,parameters) \
namespace GNULIB_NAMESPACE \
{ \
- rettype (*const func) parameters = ::rpl_func; \
+ static const struct _gl_ ## func ## _wrapper \
+ { \
+ typedef rettype (*type) parameters; \
+ inline type rpl () const { return ::rpl_func; } \
+ inline operator type () const { return rpl (); } \
+ } func = {}; \
} \
_GL_EXTERN_C int _gl_cxxalias_dummy
#else
@@ -135,8 +143,13 @@
# define _GL_CXXALIAS_RPL_CAST_1(func,rpl_func,rettype,parameters) \
namespace GNULIB_NAMESPACE \
{ \
- rettype (*const func) parameters = \
- reinterpret_cast<rettype(*)parameters>(::rpl_func); \
+ static const struct _gl_ ## func ## _wrapper \
+ { \
+ typedef rettype (*type) parameters; \
+ inline type rpl () const \
+ { return reinterpret_cast<type>(::rpl_func); } \
+ inline operator type () const { return rpl (); } \
+ } func = {}; \
} \
_GL_EXTERN_C int _gl_cxxalias_dummy
#else
@@ -150,18 +163,20 @@
is defined.
Example:
_GL_CXXALIAS_SYS (open, int, (const char *filename, int flags, ...));
- */
+
+ Wrapping func in an object with an inline conversion operator
+ avoids a reference to func unless GNULIB_NAMESPACE::func is
+ actually used in the program. */
#if defined __cplusplus && defined GNULIB_NAMESPACE
- /* If we were to write
- rettype (*const func) parameters = ::func;
- like above in _GL_CXXALIAS_RPL_1, the compiler could optimize calls
- better (remove an indirection through a 'static' pointer variable),
- but then the _GL_CXXALIASWARN macro below would cause a warning not only
- for uses of ::func but also for uses of GNULIB_NAMESPACE::func. */
-# define _GL_CXXALIAS_SYS(func,rettype,parameters) \
- namespace GNULIB_NAMESPACE \
- { \
- static rettype (*func) parameters = ::func; \
+# define _GL_CXXALIAS_SYS(func,rettype,parameters) \
+ namespace GNULIB_NAMESPACE \
+ { \
+ static const struct _gl_ ## func ## _wrapper \
+ { \
+ typedef rettype (*type) parameters; \
+ inline type rpl () const { return ::func; } \
+ inline operator type () const { return rpl (); } \
+ } func = {}; \
} \
_GL_EXTERN_C int _gl_cxxalias_dummy
#else
@@ -178,8 +193,13 @@
# define _GL_CXXALIAS_SYS_CAST(func,rettype,parameters) \
namespace GNULIB_NAMESPACE \
{ \
- static rettype (*func) parameters = \
- reinterpret_cast<rettype(*)parameters>(::func); \
+ static const struct _gl_ ## func ## _wrapper \
+ { \
+ typedef rettype (*type) parameters; \
+ inline type rpl () const \
+ { return reinterpret_cast<type>(::func); } \
+ inline operator type () const { return rpl (); }\
+ } func = {}; \
} \
_GL_EXTERN_C int _gl_cxxalias_dummy
#else
@@ -202,9 +222,15 @@
# define _GL_CXXALIAS_SYS_CAST2(func,rettype,parameters,rettype2,parameters2) \
namespace GNULIB_NAMESPACE \
{ \
- static rettype (*func) parameters = \
- reinterpret_cast<rettype(*)parameters>( \
- (rettype2(*)parameters2)(::func)); \
+ static const struct _gl_ ## func ## _wrapper \
+ { \
+ typedef rettype (*type) parameters; \
+ \
+ inline type rpl () const \
+ { return reinterpret_cast<type>((rettype2 (*) parameters2)(::func)); }\
+ \
+ inline operator type () const { return rpl (); } \
+ } func = {}; \
} \
_GL_EXTERN_C int _gl_cxxalias_dummy
#else
--
2.5.5
Paul Eggert
2016-11-12 17:30:38 UTC
Permalink
Thanks, I installed your two recent patches. I don't know C++ well; perhaps
Bruno or another reviewer can double-check if they have the time.

I noticed that with the patches installed, the command './gnulib-tool --test
--with-c++-tests frexp frexpl frexpf frexp-nolibm' fails with diagnostics like
the following (Fedora 24, GCC 6.2.1 20160916)

make[3]: Entering directory '/home/eggert/src/gnu/gnulib/testdir4584/build/gltests'
g++ -DHAVE_CONFIG_H -I. -I../../gltests -DGNULIB_STRICT_CHECKING=1 -I.
-I../../gltests -I.. -I../../gltests/.. -I../gllib -I../../gltests/../gllib
-MT test-math-c++.o -MD -MP -MF .deps/test-math-c++.Tpo -c -o test-math-c++.o
../../gltests/test-math-c++.cc
../../gltests/test-math-c++.cc:37:45: error: invalid static_cast from type
‘<unresolved overloaded function type>’ to type ‘int (*)(float)’
= static_cast<rettype(*)parameters>(func)
^
../../gltests/test-math-c++.cc:32:3: note: in expansion of macro ‘OVERLOADED_CHECK’
OVERLOADED_CHECK (func, rettype1, parameters1, _1); \
^~~~~~~~~~~~~~~~


However, similar failures occur even without the patches (the patches fix one of
the failures, actually) so this is not a regression.
Pedro Alves
2016-11-14 18:16:07 UTC
Permalink
Post by Paul Eggert
Thanks, I installed your two recent patches.
Thanks!
Post by Paul Eggert
I don't know C++ well;
perhaps Bruno or another reviewer can double-check if they have the time.
I noticed that with the patches installed, the command './gnulib-tool
--test --with-c++-tests frexp frexpl frexpf frexp-nolibm' fails with
diagnostics like the following (Fedora 24, GCC 6.2.1 20160916)
make[3]: Entering directory
'/home/eggert/src/gnu/gnulib/testdir4584/build/gltests'
g++ -DHAVE_CONFIG_H -I. -I../../gltests -DGNULIB_STRICT_CHECKING=1 -I.
-I../../gltests -I.. -I../../gltests/.. -I../gllib
-I../../gltests/../gllib -MT test-math-c++.o -MD -MP -MF
.deps/test-math-c++.Tpo -c -o test-math-c++.o
../../gltests/test-math-c++.cc
../../gltests/test-math-c++.cc:37:45: error: invalid static_cast from
type ‘<unresolved overloaded function type>’ to type ‘int (*)(float)’
= static_cast<rettype(*)parameters>(func)
^
../../gltests/test-math-c++.cc:32:3: note: in expansion of macro ‘OVERLOADED_CHECK’
OVERLOADED_CHECK (func, rettype1, parameters1, _1); \
^~~~~~~~~~~~~~~~
However, similar failures occur even without the patches (the patches
fix one of the failures, actually) so this is not a regression.
I looked at this today.

The problem here is that these functions (signbit, isfinite,
isnan, etc.) return bool in a conforming C++11 implementation:

http://en.cppreference.com/w/cpp/numeric/math/signbit

Tweaking the test the obvious way to expect bool return types
makes the test pass with gcc >= 6.

However, if we _just_ do that, the test starts failing with gcc < 6,
which didn't have the new libstdc++ math.h wrapper that makes gcc
fully C++11 conforming. We end up with gnulib providing the overloads as
returning int. If we tweak the gnulib replacements to define the
functions as returning bool (with the obvious return type
change to _GL_MATH_CXX_REAL_FLOATING_DECL_1/_GL_MATH_CXX_REAL_FLOATING_DECL_2)),
we stumble on this on Fedora 23, at least:

make[3]: Entering directory '/home/pedro/brno/pedro/src/gnulib/src/testdir11559/build/gltests'
Making all in .
make[4]: Entering directory '/home/pedro/brno/pedro/src/gnulib/src/testdir11559/build/gltests'
g++ -std=gnu++11 -Wall -Wno-unused-variable -DHAVE_CONFIG_H -I. -I../../gltests -DGNULIB_STRICT_CHECKING=1 -I. -I../../gltests -I.. -I../../gltests/.. -I../gllib -I../../gltests/../gllib -MT test-math-c++.o -MD -MP -MF .deps/test-math-c++.Tpo -c -o test-math-c++.o ../../gltests/test-math-c++.cc
In file included from ../../gltests/test-math-c++.cc:22:0:
../gllib/math.h: In function ‘bool isinf(double)’:
../gllib/math.h:2422:1: error: ambiguating new declaration of ‘bool isinf(double)’
_GL_MATH_CXX_REAL_FLOATING_DECL_2 (isinf)
^
In file included from /usr/include/features.h:365:0,
from /usr/include/math.h:26,
from ../gllib/math.h:27,
from ../../gltests/test-math-c++.cc:22:
/usr/include/bits/mathcalls.h:201:1: note: old declaration ‘int isinf(double)’
__MATHDECL_1 (int,isinf,, (_Mdouble_ __value)) __attribute__ ((__const__));
^
In file included from ../../gltests/test-math-c++.cc:22:0:
../gllib/math.h: In function ‘bool isnan(double)’:
../gllib/math.h:2540:1: error: ambiguating new declaration of ‘bool isnan(double)’
_GL_MATH_CXX_REAL_FLOATING_DECL_2 (isnan)
^

See <https://sourceware.org/bugzilla/show_bug.cgi?id=19439> for
background.

It seems to me that the spirit of gnulib should be to
define C/POSIX replacements, while largely moving out of the
way of the C++ runtime. I don't think providing replacements
for C++ runtime holes (which is much larger than what
it inherits from C) should be in scope for gnulib.

So I think this should be addressed by defining the replacements
for these functions in the GNULIB_NAMESPACE namespace instead of
in the global namespace, like all other function replacements.
And given gnulib aims at C/POSIX, I think it's better/simpler if
the replacements follow the C interface, and thus return int, not
bool.

So, here's a patch that works for me on Fedora 23, tested with
gcc/g++ 4.7, 4.8, 4.9, 5.3.1 (system compiler) and 7 (trunk),
all in both c++03 and c++11 modes, using this script:

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
#!/bin/bash

set -e

test="./gnulib-tool --test --with-c++-tests frexp frexpl frexpf frexp-nolibm isnan isnanf isnand isnanl isfinite isinf"

WARN_CFLAGS="-Wall -Wno-unused-variable"

PREFIX="/opt/gcc-4.7/bin" CC="$PREFIX/gcc -Wall" CXX="$PREFIX/g++ -std=gnu++03 $WARN_CFLAGS" $test
PREFIX="/opt/gcc-4.7/bin" CC="$PREFIX/gcc -Wall" CXX="$PREFIX/g++ -std=gnu++0x $WARN_CFLAGS" $test

PREFIX="/opt/gcc-4.8/bin" CC="$PREFIX/gcc -Wall" CXX="$PREFIX/g++ -std=gnu++03 $WARN_CFLAGS" $test
PREFIX="/opt/gcc-4.8/bin" CC="$PREFIX/gcc -Wall" CXX="$PREFIX/g++ -std=gnu++11 $WARN_CFLAGS" $test

PREFIX="/opt/gcc-4.9/bin" CC="$PREFIX/gcc -Wall" CXX="$PREFIX/g++ -std=gnu++03 $WARN_CFLAGS" $test
PREFIX="/opt/gcc-4.9/bin" CC="$PREFIX/gcc -Wall" CXX="$PREFIX/g++ -std=gnu++11 $WARN_CFLAGS" $test

# gcc 5.3.1 on Fedora 23
CC="gcc -Wall" CXX="g++ -std=gnu++03 -Wall -Wno-unused-variable" $test
CC="gcc -Wall" CXX="g++ -std=gnu++11 -Wall -Wno-unused-variable" $test

# gcc trunk on Fedora 23
PREFIX="/opt/gcc/bin" CC="$PREFIX/gcc -Wall" CXX="$PREFIX/g++ -std=gnu++03 $WARN_CFLAGS" $test
PREFIX="/opt/gcc/bin" CC="$PREFIX/gcc -Wall" CXX="$PREFIX/g++ -std=gnu++11 $WARN_CFLAGS" $test
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

From 0163126ec110fba504995a2aabebf09610cad1c4 Mon Sep 17 00:00:00 2001
From: Pedro Alves <***@redhat.com>
Date: Mon, 14 Nov 2016 17:08:23 +0000
Subject: [PATCH] Fix real-floating argument functions in C++ mode

ChangeLog:
2016-11-14 Pedro Alves <***@redhat.com>

Fix real-floating argument functions in C++ mode. Define define
isfinite, isinf, isnan, signbit in the gnulib namespace instead of
in the global namespace.
* lib/math.in.h (_GL_MATH_CXX_REAL_FLOATING_DECL_NS): New macro.
(isfinite, isinf, isnan, signbit): Use it.
* tests/test-math-c++.cc: Test the isfinite, isinf, isnan, signbit
overloads in the GNULIB_NAMESPACE namespace, instead of the global
namespace.
---
lib/math.in.h | 34 ++++++++++++++++++++++++++++++----
tests/test-math-c++.cc | 5 +++--
2 files changed, 33 insertions(+), 6 deletions(-)

diff --git a/lib/math.in.h b/lib/math.in.h
index 9a194cb..8bbd25d 100644
--- a/lib/math.in.h
+++ b/lib/math.in.h
@@ -80,6 +80,16 @@ func (long double l) \
}
#endif

+/* Define the FUNC overloads in the GNULIB_NAMESPACE namespace. */
+#ifdef GNULIB_NAMESPACE
+# define _GL_MATH_CXX_REAL_FLOATING_DECL_NS(func) \
+namespace GNULIB_NAMESPACE { \
+_GL_MATH_CXX_REAL_FLOATING_DECL_2 (func) \
+}
+#else
+# define _GL_MATH_CXX_REAL_FLOATING_DECL_NS(func)
+#endif
+
/* Helper macros to define a portability warning for the
classification macro FUNC called with VALUE. POSIX declares the
classification macros with an argument of real-floating (that is,
@@ -2044,10 +2054,14 @@ _GL_EXTERN_C int gl_isfinitel (long double x);
gl_isfinitef (x))
# endif
# ifdef __cplusplus
-# ifdef isfinite
+# if defined isfinite || defined GNULIB_NAMESPACE
_GL_MATH_CXX_REAL_FLOATING_DECL_1 (isfinite)
# undef isfinite
+# ifdef GNULIB_NAMESPACE
+_GL_MATH_CXX_REAL_FLOATING_DECL_NS (isfinite)
+# else
_GL_MATH_CXX_REAL_FLOATING_DECL_2 (isfinite)
+# endif
# endif
# endif
#elif defined GNULIB_POSIXCHECK
@@ -2071,10 +2085,14 @@ _GL_EXTERN_C int gl_isinfl (long double x);
gl_isinff (x))
# endif
# ifdef __cplusplus
-# ifdef isinf
+# if defined isinf || defined GNULIB_NAMESPACE
_GL_MATH_CXX_REAL_FLOATING_DECL_1 (isinf)
# undef isinf
+# ifdef GNULIB_NAMESPACE
+_GL_MATH_CXX_REAL_FLOATING_DECL_NS (isinf)
+# else
_GL_MATH_CXX_REAL_FLOATING_DECL_2 (isinf)
+# endif
# endif
# endif
#elif defined GNULIB_POSIXCHECK
@@ -2189,10 +2207,14 @@ _GL_EXTERN_C int rpl_isnanl (long double x) _GL_ATTRIBUTE_CONST;
__builtin_isnanf ((float)(x)))
# endif
# ifdef __cplusplus
-# ifdef isnan
+# if defined isnan || defined GNULIB_NAMESPACE
_GL_MATH_CXX_REAL_FLOATING_DECL_1 (isnan)
# undef isnan
+# ifdef GNULIB_NAMESPACE
+_GL_MATH_CXX_REAL_FLOATING_DECL_NS (isnan)
+# else
_GL_MATH_CXX_REAL_FLOATING_DECL_2 (isnan)
+# endif
# endif
# else
/* Ensure isnan is a macro. */
@@ -2264,10 +2286,14 @@ _GL_EXTERN_C int gl_signbitl (long double arg);
gl_signbitf (x))
# endif
# ifdef __cplusplus
-# ifdef signbit
+# if defined signbit || defined GNULIB_NAMESPACE
_GL_MATH_CXX_REAL_FLOATING_DECL_1 (signbit)
# undef signbit
+# ifdef GNULIB_NAMESPACE
+_GL_MATH_CXX_REAL_FLOATING_DECL_NS (signbit)
+# else
_GL_MATH_CXX_REAL_FLOATING_DECL_2 (signbit)
+# endif
# endif
# endif
#elif defined GNULIB_POSIXCHECK
diff --git a/tests/test-math-c++.cc b/tests/test-math-c++.cc
index f0f1448..cc7378c 100644
--- a/tests/test-math-c++.cc
+++ b/tests/test-math-c++.cc
@@ -24,7 +24,8 @@
#include "signature.h"

/* Signature check for a function that takes a real-floating argument.
- Check that each overloaded function with the specified signature exists. */
+ Check that each overloaded function with the specified signature
+ exists in the GNULIB_NAMESPACE namespace. */
#define REAL_FLOATING_CHECK(func,\
rettype1, parameters1,\
rettype2, parameters2,\
@@ -34,7 +35,7 @@
OVERLOADED_CHECK (func, rettype3, parameters3, _3)
#define OVERLOADED_CHECK(func, rettype, parameters, suffix) \
static rettype (* _GL_UNUSED signature_check_ ## func ## suffix) parameters \
- = static_cast<rettype(*)parameters>(func)
+ = static_cast<rettype(*)parameters>(GNULIB_NAMESPACE::func)


/* Keep these checks in the same order as math.in.h! */
--
2.5.5
Pedro Alves
2016-11-14 21:19:37 UTC
Permalink
Post by Pedro Alves
Post by Paul Eggert
Thanks, I installed your two recent patches.
Thanks!
Post by Paul Eggert
I don't know C++ well;
perhaps Bruno or another reviewer can double-check if they have the time.
I noticed that with the patches installed, the command './gnulib-tool
--test --with-c++-tests frexp frexpl frexpf frexp-nolibm' fails with
diagnostics like the following (Fedora 24, GCC 6.2.1 20160916)
make[3]: Entering directory
'/home/eggert/src/gnu/gnulib/testdir4584/build/gltests'
g++ -DHAVE_CONFIG_H -I. -I../../gltests -DGNULIB_STRICT_CHECKING=1 -I.
-I../../gltests -I.. -I../../gltests/.. -I../gllib
-I../../gltests/../gllib -MT test-math-c++.o -MD -MP -MF
.deps/test-math-c++.Tpo -c -o test-math-c++.o
../../gltests/test-math-c++.cc
../../gltests/test-math-c++.cc:37:45: error: invalid static_cast from
type ‘<unresolved overloaded function type>’ to type ‘int (*)(float)’
= static_cast<rettype(*)parameters>(func)
^
../../gltests/test-math-c++.cc:32:3: note: in expansion of macro ‘OVERLOADED_CHECK’
OVERLOADED_CHECK (func, rettype1, parameters1, _1); \
^~~~~~~~~~~~~~~~
However, similar failures occur even without the patches (the patches
fix one of the failures, actually) so this is not a regression.
I looked at this today.
The problem here is that these functions (signbit, isfinite,
http://en.cppreference.com/w/cpp/numeric/math/signbit
Tweaking the test the obvious way to expect bool return types
makes the test pass with gcc >= 6.
However, if we _just_ do that, the test starts failing with gcc < 6,
which didn't have the new libstdc++ math.h wrapper that makes gcc
fully C++11 conforming. We end up with gnulib providing the overloads as
returning int. If we tweak the gnulib replacements to define the
functions as returning bool (with the obvious return type
change to _GL_MATH_CXX_REAL_FLOATING_DECL_1/_GL_MATH_CXX_REAL_FLOATING_DECL_2)),
make[3]: Entering directory '/home/pedro/brno/pedro/src/gnulib/src/testdir11559/build/gltests'
Making all in .
make[4]: Entering directory '/home/pedro/brno/pedro/src/gnulib/src/testdir11559/build/gltests'
g++ -std=gnu++11 -Wall -Wno-unused-variable -DHAVE_CONFIG_H -I. -I../../gltests -DGNULIB_STRICT_CHECKING=1 -I. -I../../gltests -I.. -I../../gltests/.. -I../gllib -I../../gltests/../gllib -MT test-math-c++.o -MD -MP -MF .deps/test-math-c++.Tpo -c -o test-math-c++.o ../../gltests/test-math-c++.cc
../gllib/math.h:2422:1: error: ambiguating new declaration of ‘bool isinf(double)’
_GL_MATH_CXX_REAL_FLOATING_DECL_2 (isinf)
^
In file included from /usr/include/features.h:365:0,
from /usr/include/math.h:26,
from ../gllib/math.h:27,
/usr/include/bits/mathcalls.h:201:1: note: old declaration ‘int isinf(double)’
__MATHDECL_1 (int,isinf,, (_Mdouble_ __value)) __attribute__ ((__const__));
^
../gllib/math.h:2540:1: error: ambiguating new declaration of ‘bool isnan(double)’
_GL_MATH_CXX_REAL_FLOATING_DECL_2 (isnan)
^
See <https://sourceware.org/bugzilla/show_bug.cgi?id=19439> for
background.
It seems to me that the spirit of gnulib should be to
define C/POSIX replacements, while largely moving out of the
way of the C++ runtime. I don't think providing replacements
for C++ runtime holes (which is much larger than what
it inherits from C) should be in scope for gnulib.
So I think this should be addressed by defining the replacements
for these functions in the GNULIB_NAMESPACE namespace instead of
in the global namespace, like all other function replacements.
And given gnulib aims at C/POSIX, I think it's better/simpler if
the replacements follow the C interface, and thus return int, not
bool.
So, here's a patch that works for me on Fedora 23, tested with
gcc/g++ 4.7, 4.8, 4.9, 5.3.1 (system compiler) and 7 (trunk),
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
#!/bin/bash
set -e
test="./gnulib-tool --test --with-c++-tests frexp frexpl frexpf frexp-nolibm isnan isnanf isnand isnanl isfinite isinf"
WARN_CFLAGS="-Wall -Wno-unused-variable"
PREFIX="/opt/gcc-4.7/bin" CC="$PREFIX/gcc -Wall" CXX="$PREFIX/g++ -std=gnu++03 $WARN_CFLAGS" $test
PREFIX="/opt/gcc-4.7/bin" CC="$PREFIX/gcc -Wall" CXX="$PREFIX/g++ -std=gnu++0x $WARN_CFLAGS" $test
PREFIX="/opt/gcc-4.8/bin" CC="$PREFIX/gcc -Wall" CXX="$PREFIX/g++ -std=gnu++03 $WARN_CFLAGS" $test
PREFIX="/opt/gcc-4.8/bin" CC="$PREFIX/gcc -Wall" CXX="$PREFIX/g++ -std=gnu++11 $WARN_CFLAGS" $test
PREFIX="/opt/gcc-4.9/bin" CC="$PREFIX/gcc -Wall" CXX="$PREFIX/g++ -std=gnu++03 $WARN_CFLAGS" $test
PREFIX="/opt/gcc-4.9/bin" CC="$PREFIX/gcc -Wall" CXX="$PREFIX/g++ -std=gnu++11 $WARN_CFLAGS" $test
# gcc 5.3.1 on Fedora 23
CC="gcc -Wall" CXX="g++ -std=gnu++03 -Wall -Wno-unused-variable" $test
CC="gcc -Wall" CXX="g++ -std=gnu++11 -Wall -Wno-unused-variable" $test
# gcc trunk on Fedora 23
PREFIX="/opt/gcc/bin" CC="$PREFIX/gcc -Wall" CXX="$PREFIX/g++ -std=gnu++03 $WARN_CFLAGS" $test
PREFIX="/opt/gcc/bin" CC="$PREFIX/gcc -Wall" CXX="$PREFIX/g++ -std=gnu++11 $WARN_CFLAGS" $test
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
From 0163126ec110fba504995a2aabebf09610cad1c4 Mon Sep 17 00:00:00 2001
Date: Mon, 14 Nov 2016 17:08:23 +0000
Subject: [PATCH] Fix real-floating argument functions in C++ mode
Fix real-floating argument functions in C++ mode. Define define
isfinite, isinf, isnan, signbit in the gnulib namespace instead of
in the global namespace.
* lib/math.in.h (_GL_MATH_CXX_REAL_FLOATING_DECL_NS): New macro.
(isfinite, isinf, isnan, signbit): Use it.
* tests/test-math-c++.cc: Test the isfinite, isinf, isnan, signbit
overloads in the GNULIB_NAMESPACE namespace, instead of the global
namespace.
---
lib/math.in.h | 34 ++++++++++++++++++++++++++++++----
tests/test-math-c++.cc | 5 +++--
2 files changed, 33 insertions(+), 6 deletions(-)
diff --git a/lib/math.in.h b/lib/math.in.h
index 9a194cb..8bbd25d 100644
--- a/lib/math.in.h
+++ b/lib/math.in.h
@@ -80,6 +80,16 @@ func (long double l) \
}
#endif
+/* Define the FUNC overloads in the GNULIB_NAMESPACE namespace. */
+#ifdef GNULIB_NAMESPACE
+# define _GL_MATH_CXX_REAL_FLOATING_DECL_NS(func) \
+namespace GNULIB_NAMESPACE { \
+_GL_MATH_CXX_REAL_FLOATING_DECL_2 (func) \
+}
+#else
+# define _GL_MATH_CXX_REAL_FLOATING_DECL_NS(func)
+#endif
Sorry, I wrote this this patch in a roundabout way, trying
different potential solutions, and the version I sent
somewhat in a hurry didn't simplify as much as possible
in the end...

If we rename the new macro to _GL_MATH_CXX_REAL_FLOATING_DECL_2
(and rename the old _GL_MATH_CXX_REAL_FLOATING_DECL_2 to
something else, making it a helper macro), then these ...
Post by Pedro Alves
+
/* Helper macros to define a portability warning for the
classification macro FUNC called with VALUE. POSIX declares the
classification macros with an argument of real-floating (that is,
@@ -2044,10 +2054,14 @@ _GL_EXTERN_C int gl_isfinitel (long double x);
gl_isfinitef (x))
# endif
# ifdef __cplusplus
-# ifdef isfinite
+# if defined isfinite || defined GNULIB_NAMESPACE
_GL_MATH_CXX_REAL_FLOATING_DECL_1 (isfinite)
# undef isfinite
+# ifdef GNULIB_NAMESPACE
+_GL_MATH_CXX_REAL_FLOATING_DECL_NS (isfinite)
+# else
_GL_MATH_CXX_REAL_FLOATING_DECL_2 (isfinite)
... changes here (and similar places) shouldn't be necessary.

Let me try that and send a new patch.
--
Thanks,
Pedro Alves
Pedro Alves
2016-11-14 23:08:21 UTC
Permalink
Post by Pedro Alves
Let me try that and send a new patch.
Here's an simpler implementation, IMO. I went ahead and added
the _GL_BEGIN_NAMESPACE/_GL_END_NAMESPACE macros to c++defs.h as
I'm fairly sure they'll end up being useful in more places.

Passes same testing as before, with all tests now green.

From cf4f33cb8f8185f1c7d7ee6dd6da45e7153ab7d2 Mon Sep 17 00:00:00 2001
From: Pedro Alves <***@redhat.com>
Date: Mon, 14 Nov 2016 22:49:33 +0000
Subject: [PATCH] Fix real-floating argument functions in C++ mode

ChangeLog:
2016-11-14 Pedro Alves <***@redhat.com>

Fix real-floating argument functions in C++ mode. Define
isfinite, isinf, isnan, signbit in the gnulib namespace instead of
in the global namespace.
* build-aux/snippet/c++defs.h (_GL_BEGIN_NAMESPACE)
(_GL_END_NAMESPACE): New.
* lib/math.in.h (_GL_MATH_CXX_REAL_FLOATING_DECL_2): Use them.
(isfinite, isinf, isnan, signbit) [__cplusplus &&
GNULIB_NAMESPACE]: Define them in the GNULIB_NAMESPACE namespace
instead of in the global namespace.
* tests/test-math-c++.cc: Check that the isfinite, isinf, isnan,
signbit overloads exist in the GNULIB_NAMESPACE namespace, instead
of in the global namespace.
---
build-aux/snippet/c++defs.h | 9 +++++++++
lib/math.in.h | 12 +++++++-----
tests/test-math-c++.cc | 5 +++--
3 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/build-aux/snippet/c++defs.h b/build-aux/snippet/c++defs.h
index 8fc95e3..b19a0be 100644
--- a/build-aux/snippet/c++defs.h
+++ b/build-aux/snippet/c++defs.h
@@ -17,6 +17,15 @@
#ifndef _GL_CXXDEFS_H
#define _GL_CXXDEFS_H

+/* Begin/end the GNULIB_NAMESPACE namespace. */
+#if defined __cplusplus && defined GNULIB_NAMESPACE
+# define _GL_BEGIN_NAMESPACE namespace GNULIB_NAMESPACE {
+# define _GL_END_NAMESPACE }
+#else
+# define _GL_BEGIN_NAMESPACE
+# define _GL_END_NAMESPACE
+#endif
+
/* The three most frequent use cases of these macros are:

* For providing a substitute for a function that is missing on some
diff --git a/lib/math.in.h b/lib/math.in.h
index 9a194cb..e1dc970 100644
--- a/lib/math.in.h
+++ b/lib/math.in.h
@@ -63,6 +63,7 @@ _gl_cxx_ ## func ## l (long double l) \
return func (l); \
}
# define _GL_MATH_CXX_REAL_FLOATING_DECL_2(func) \
+_GL_BEGIN_NAMESPACE \
inline int \
func (float f) \
{ \
@@ -77,7 +78,8 @@ inline int \
func (long double l) \
{ \
return _gl_cxx_ ## func ## l (l); \
-}
+} \
+_GL_END_NAMESPACE
#endif

/* Helper macros to define a portability warning for the
@@ -2044,7 +2046,7 @@ _GL_EXTERN_C int gl_isfinitel (long double x);
gl_isfinitef (x))
# endif
# ifdef __cplusplus
-# ifdef isfinite
+# if defined isfinite || defined GNULIB_NAMESPACE
_GL_MATH_CXX_REAL_FLOATING_DECL_1 (isfinite)
# undef isfinite
_GL_MATH_CXX_REAL_FLOATING_DECL_2 (isfinite)
@@ -2071,7 +2073,7 @@ _GL_EXTERN_C int gl_isinfl (long double x);
gl_isinff (x))
# endif
# ifdef __cplusplus
-# ifdef isinf
+# if defined isinf || defined GNULIB_NAMESPACE
_GL_MATH_CXX_REAL_FLOATING_DECL_1 (isinf)
# undef isinf
_GL_MATH_CXX_REAL_FLOATING_DECL_2 (isinf)
@@ -2189,7 +2191,7 @@ _GL_EXTERN_C int rpl_isnanl (long double x) _GL_ATTRIBUTE_CONST;
__builtin_isnanf ((float)(x)))
# endif
# ifdef __cplusplus
-# ifdef isnan
+# if defined isnan || defined GNULIB_NAMESPACE
_GL_MATH_CXX_REAL_FLOATING_DECL_1 (isnan)
# undef isnan
_GL_MATH_CXX_REAL_FLOATING_DECL_2 (isnan)
@@ -2264,7 +2266,7 @@ _GL_EXTERN_C int gl_signbitl (long double arg);
gl_signbitf (x))
# endif
# ifdef __cplusplus
-# ifdef signbit
+# if defined signbit || defined GNULIB_NAMESPACE
_GL_MATH_CXX_REAL_FLOATING_DECL_1 (signbit)
# undef signbit
_GL_MATH_CXX_REAL_FLOATING_DECL_2 (signbit)
diff --git a/tests/test-math-c++.cc b/tests/test-math-c++.cc
index f0f1448..cc7378c 100644
--- a/tests/test-math-c++.cc
+++ b/tests/test-math-c++.cc
@@ -24,7 +24,8 @@
#include "signature.h"

/* Signature check for a function that takes a real-floating argument.
- Check that each overloaded function with the specified signature exists. */
+ Check that each overloaded function with the specified signature
+ exists in the GNULIB_NAMESPACE namespace. */
#define REAL_FLOATING_CHECK(func,\
rettype1, parameters1,\
rettype2, parameters2,\
@@ -34,7 +35,7 @@
OVERLOADED_CHECK (func, rettype3, parameters3, _3)
#define OVERLOADED_CHECK(func, rettype, parameters, suffix) \
static rettype (* _GL_UNUSED signature_check_ ## func ## suffix) parameters \
- = static_cast<rettype(*)parameters>(func)
+ = static_cast<rettype(*)parameters>(GNULIB_NAMESPACE::func)


/* Keep these checks in the same order as math.in.h! */
--
2.5.5
Paul Eggert
2016-11-15 17:17:51 UTC
Permalink
Thanks, I installed the attached; it's what you sent, except in git
format-patch format, with the commit message equaling the ChangeLog
entry (with one empty line appended after the first line, and without
indentation), and with the first commit-message line trying to fit in 50
chars with a prefix indicating which module changed (admittedly a severe
budget). Gnulib has been conservative about keeping a ChangeLog file,
which puts a duplicate of the commit message into the patch, unfortunately.
Bruno Haible
2016-11-20 12:26:32 UTC
Permalink
Hi Pedro,
Post by Pedro Alves
Currently, with C++ namespace support enabled, by defining
GNULIB_NAMESPACE, the replacement symbols put in the GNULIB_NAMESPACE
namespace are function pointers that point to the rpl_func functions.
This means that the linker must pull in the rpl_func functions, even
when "GNULIB_NAMESPACE::func(....)" is not called anywhere in the
program.
Fix that by making the GNULIB_NAMESPACE::func symbols be C++ (empty)
objects with inline conversion operators instead of function pointers
that point to the replacement.
A really nice improvement. It works even with -O0.

I added ChangeLog entries for your two patches from 2016-11-12
(that Paul committed).

The member function 'rpl ()' is not used, other than in the 'operator type'.
How about inlining and eliminating it?


2016-11-19 Bruno Haible <***@clisp.org>

snippet/c++defs: Simplify _GL_CXXALIAS_* macros.
* build-aux/snippet/c++defs.h [__cplusplus && GNULIB_NAMESPACE]
(_GL_CXXALIAS_RPL_1, _GL_CXXALIAS_RPL_CAST_1,
_GL_CXXALIAS_SYS, _GL_CXXALIAS_SYS_CAST, _GL_CXXALIAS_SYS_CAST2):
Inline and remove member function 'rpl ()' of the wrapper struct.

diff --git a/build-aux/snippet/c++defs.h b/build-aux/snippet/c++defs.h
index b19a0be..c71475d 100644
--- a/build-aux/snippet/c++defs.h
+++ b/build-aux/snippet/c++defs.h
@@ -133,8 +133,7 @@
static const struct _gl_ ## func ## _wrapper \
{ \
typedef rettype (*type) parameters; \
- inline type rpl () const { return ::rpl_func; } \
- inline operator type () const { return rpl (); } \
+ inline operator type () const { return ::rpl_func; } \
} func = {}; \
} \
_GL_EXTERN_C int _gl_cxxalias_dummy
@@ -155,9 +154,7 @@
static const struct _gl_ ## func ## _wrapper \
{ \
typedef rettype (*type) parameters; \
- inline type rpl () const \
- { return reinterpret_cast<type>(::rpl_func); } \
- inline operator type () const { return rpl (); } \
+ inline operator type () const { return reinterpret_cast<type>(::rpl_func); } \
} func = {}; \
} \
_GL_EXTERN_C int _gl_cxxalias_dummy
@@ -183,10 +180,9 @@
static const struct _gl_ ## func ## _wrapper \
{ \
typedef rettype (*type) parameters; \
- inline type rpl () const { return ::func; } \
- inline operator type () const { return rpl (); } \
+ inline operator type () const { return ::func; } \
} func = {}; \
- } \
+ } \
_GL_EXTERN_C int _gl_cxxalias_dummy
#else
# define _GL_CXXALIAS_SYS(func,rettype,parameters) \
@@ -205,9 +201,7 @@
static const struct _gl_ ## func ## _wrapper \
{ \
typedef rettype (*type) parameters; \
- inline type rpl () const \
- { return reinterpret_cast<type>(::func); } \
- inline operator type () const { return rpl (); }\
+ inline operator type () const { return reinterpret_cast<type>(::func); } \
} func = {}; \
} \
_GL_EXTERN_C int _gl_cxxalias_dummy
@@ -234,11 +228,7 @@
static const struct _gl_ ## func ## _wrapper \
{ \
typedef rettype (*type) parameters; \
- \
- inline type rpl () const \
- { return reinterpret_cast<type>((rettype2 (*) parameters2)(::func)); }\
- \
- inline operator type () const { return rpl (); } \
+ inline operator type () const { return reinterpret_cast<type>((rettype2 (*) parameters2)(::func)); } \
} func = {}; \
} \
_GL_EXTERN_C int _gl_cxxalias_dummy
Pedro Alves
2016-11-21 20:24:43 UTC
Permalink
Post by Bruno Haible
Hi Pedro,
Hi!
Post by Bruno Haible
I added ChangeLog entries for your two patches from 2016-11-12
(that Paul committed).
Thanks! Should I assume you're all using git-merge-changelog and
include ChangeLog changes in the diff as well as in the
commit log?
Post by Bruno Haible
The member function 'rpl ()' is not used, other than in the 'operator type'.
How about inlining and eliminating it?
That's fine with me.
\
Post by Bruno Haible
_GL_EXTERN_C int _gl_cxxalias_dummy
@@ -234,11 +228,7 @@
static const struct _gl_ ## func ## _wrapper \
{ \
typedef rettype (*type) parameters; \
- \
- inline type rpl () const \
- { return reinterpret_cast<type>((rettype2 (*) parameters2)(::func)); }\
- \
- inline operator type () const { return rpl (); } \
+ inline operator type () const { return reinterpret_cast<type>((rettype2 (*) parameters2)(::func)); } \
How about breaking this line like the rpl method was breaking it, in order
to avoid overly-long lines?

inline operator type () const
{ return reinterpret_cast<type>((rettype2 (*) parameters2)(::func)); }

Likewise the other similar cases.

Thanks,
Pedro Alves
Bruno Haible
2016-11-21 23:27:11 UTC
Permalink
Post by Pedro Alves
How about breaking this line like the rpl method was breaking it, in order
to avoid overly-long lines?
inline operator type () const
{ return reinterpret_cast<type>((rettype2 (*) parameters2)(::func)); }
Likewise the other similar cases.
If line breaking it needed, I prefer the GNU style for functions,
and do it systematically. It takes more vertical space, but it
make is easier to compare the different variants of _gl_ ## func ## _wrapper.

--- a/build-aux/snippet/c++defs.h
+++ b/build-aux/snippet/c++defs.h
@@ -133,7 +133,11 @@
static const struct _gl_ ## func ## _wrapper \
{ \
typedef rettype (*type) parameters; \
- inline operator type () const { return ::rpl_func; } \
+ \
+ inline operator type () const \
+ { \
+ return ::rpl_func; \
+ } \
} func = {}; \
} \
_GL_EXTERN_C int _gl_cxxalias_dummy
@@ -154,7 +158,11 @@
static const struct _gl_ ## func ## _wrapper \
{ \
typedef rettype (*type) parameters; \
- inline operator type () const { return reinterpret_cast<type>(::rpl_func); } \
+ \
+ inline operator type () const \
+ { \
+ return reinterpret_cast<type>(::rpl_func); \
+ } \
} func = {}; \
} \
_GL_EXTERN_C int _gl_cxxalias_dummy
@@ -180,7 +188,11 @@
static const struct _gl_ ## func ## _wrapper \
{ \
typedef rettype (*type) parameters; \
- inline operator type () const { return ::func; } \
+ \
+ inline operator type () const \
+ { \
+ return ::func; \
+ } \
} func = {}; \
} \
_GL_EXTERN_C int _gl_cxxalias_dummy
@@ -201,7 +213,11 @@
static const struct _gl_ ## func ## _wrapper \
{ \
typedef rettype (*type) parameters; \
- inline operator type () const { return reinterpret_cast<type>(::func); } \
+ \
+ inline operator type () const \
+ { \
+ return reinterpret_cast<type>(::func); \
+ } \
} func = {}; \
} \
_GL_EXTERN_C int _gl_cxxalias_dummy
@@ -228,7 +244,11 @@
static const struct _gl_ ## func ## _wrapper \
{ \
typedef rettype (*type) parameters; \
- inline operator type () const { return reinterpret_cast<type>((rettype2 (*) parameters2)(::func)); } \
+ \
+ inline operator type () const \
+ { \
+ return reinterpret_cast<type>((rettype2 (*) parameters2)(::func)); \
+ } \
} func = {}; \
} \
_GL_EXTERN_C int _gl_cxxalias_dummy


Pushed with this style change.

Bruno
Pedro Alves
2016-11-21 23:33:03 UTC
Permalink
Post by Bruno Haible
Post by Pedro Alves
How about breaking this line like the rpl method was breaking it, in order
to avoid overly-long lines?
inline operator type () const
{ return reinterpret_cast<type>((rettype2 (*) parameters2)(::func)); }
Likewise the other similar cases.
If line breaking it needed, I prefer the GNU style for functions,
That's fine with me. (The single-line {...} style comes from libstdc++.)
Post by Bruno Haible
and do it systematically. It takes more vertical space, but it
make is easier to compare the different variants of _gl_ ## func ## _wrapper.
Agreed, that's what I was thinking when suggesting to give
the other similar cases the same treatment.

Thanks,
Pedro Alves
Bruno Haible
2016-11-21 23:34:09 UTC
Permalink
Post by Pedro Alves
Post by Bruno Haible
I added ChangeLog entries for your two patches from 2016-11-12
(that Paul committed).
Thanks! Should I assume you're all using git-merge-changelog and
Yes, we are all using the git-merge-changelog. It handles about 60% of
the ChangeLog entry conflicts correctly. (A couple of years ago, it
handled 95% of such conflicts correctly. I guess something in the git
internals has changed...)
Post by Pedro Alves
include ChangeLog changes in the diff as well as in the
commit log?
Yes, this is how we all do it when we commit a patch.

When a contributor sends in a patch, the person who pushes it on behalf
of the author takes care of doing the right thing.

Many of us also use vc-dwim: It makes it less tedious to create the detailed
ChangeLog entry.

Bruno

Loading...