Bug #3787

gcc4.7 __cplusplus change incompatibility

Added by Alexander Pyhalov over 4 years ago. Updated about 4 years ago.

Status:ResolvedStart date:2013-05-25
Priority:NormalDue date:
Assignee:Alexander Pyhalov% Done:

100%

Category:lib - userland libraries
Target version:-
Difficulty:Medium Tags:c++, gcc

Description

In g++ 4.7 cplusplus macros was changed so it is no longer 1 ( http://gcc.gnu.org/bugzilla/show_bug.cgi?id=1773 ) . This change breaks g++ work on Solaris, because in many places there are assumptions that if _cplusplus >= 199711L, it is not GCC (e.g. /usr/include/stdlib.h) .
Possibly such conditions should be changed from "#if __cplusplus >= 199711L" to "#if __cplusplus >= 199711L && !defined(
_GNUG
)" (http://gcc.gnu.org/ml/libstdc++/2011-03/msg00032.html).

3787_il.patch Magnifier (6.77 KB) Igor Kozhukhov, 2013-05-26 02:58 PM

3787_il_studio.patch Magnifier (7.73 KB) Alexander Pyhalov, 2013-05-29 08:31 PM

3787_il_studio_20130531_1.patch Magnifier (33.3 KB) Alexander Pyhalov, 2013-05-31 06:33 PM

min-3787.patch Magnifier (6.14 KB) Alexander Pyhalov, 2013-06-05 12:13 PM

min-3787.patch Magnifier (2.78 KB) Alexander Pyhalov, 2013-06-12 06:50 AM

min-3787.patch Magnifier (2.37 KB) Alexander Pyhalov, 2013-06-12 07:13 AM


Related issues

Related to illumos gate - Bug #3823: wchar.h and wctype_iso.h incompatibility Resolved 2013-06-18

History

#1 Updated by Igor Kozhukhov over 4 years ago

i have tested uploaded patch on dilos-illumos-gate, but i have integrated LIBM.
illumos build fine, some c++ userland packages built well.

#2 Updated by Rich Lowe over 4 years ago

I suspect that the condition should probably be "if it is Studio" rather than "If it is not GCC", I would assume that any C++ standard library except Studio's would not know how to interact correctly when our headers attempt to "help" in this way.

#3 Updated by Alexander Pyhalov over 4 years ago

Igor, it seems that your patch is not enough. At least, it doesn't help to build aspell. I've attached modified patch.
1) I think it is still incomplete (not all headers are covered)
2) I verified that aspell at least builds successfully
3) All changes in patch shouldn't influence illumos compiling, because it doesn't change gcc 4.4 behavior.

In brief: it is useful in attached form, but still needs more work.
Rich, am I going in the right direction or should it involve more complex headers rewrite?

#4 Updated by Rich Lowe over 4 years ago

You're doing what I think is probably best, but I'm not certain.

What I understand from the discussion on the GCC list is that they expect a C library that does absolutely nothing to "help" with C++. That's sensible, they can never be sure how much a given standard library will help. Our standard library and headers do an amount of work for C++ which presumably the Sun Studio runtime expects, and we need to continue doing for folks using that compiler. Your patch covers both of those.

I suppose the big questions are:

  • What does libc++ expect? (I suspect it doesn't even work on illumos, so that's sort of an academic question, but it's the only other C++ runtime I know of that's potentially relevant)
  • Do we need to fix every use __cplusplus? (I don't know the answer)

#5 Updated by Alexander Pyhalov over 4 years ago

One more comment - about the following extract from Igor's patch:

--- a/usr/src/head/iso/string_iso.h Mon May 20 01:54:38 2013 +0400
+++ b/usr/src/head/iso/string_iso.h Sun May 26 04:28:54 2013 +0400
@ -98,6 +98,13 @@ * two declarations, both of which have the same behavior.
/
#if cplusplus >= 199711L
#ifdef __GNUG

#define _MEMCHR_INLINE
#define _STRCHR_INLINE
#define _STRPBRK_INLINE
#define _STRRCHR_INLINE
#define _STRSTR_INLINE
+#endif /
GNUG */
extern const void *memchr(const void *, int, size_t);
#ifndef _MEMCHR_INLINE
#define _MEMCHR_INLINE
@

This part leads to warnings (and while compiling aspell in userland environments - to errors) about casting char * to const char . If we comment out completly this section (as in uploaded patch) necessary functions will be defined for G++ later (strings 145-152 in string_iso.h):
@
#else /
__cplusplus >= 199711L /
extern void *memchr(const void *, int, size_t);
extern char *strchr(const char *, int);
extern char *strpbrk(const char *, const char *);
extern char *strrchr(const char *, int);
extern char *strstr(const char *, const char *);
#endif /
__cplusplus >= 199711L */

#else /* STDC */
@

One more question: does illumos library ever help G++ correctly? I mean correct constructions like this:

#if (__cplusplus < 199711L && defined(__GNUG__))

I haven't found any in /usr/include, so I suppose the answer is no.

#6 Updated by Alexander Pyhalov over 4 years ago

One new discovery. We can't just change #if _cplusplus >= 199711L to #if __cplusplus >= 199711L && defined(_SUNPRO_CC) in all possible places. At least it doesn't work for iso/stdio_iso.h (e.g. with typedef __FILE FILE; ) - g++ will also fail in some cases if we modify this section.
If it is not correct here, I don't know if it is safe in other places.

#7 Updated by Alexander Pyhalov over 4 years ago

Attaching last version of the patch without mentioned typedef. Maybe we should use changes to stdio_iso.h from first version of the patch.

#8 Updated by Alexander Pyhalov over 4 years ago

Some more tests. I've tried to execute g++ 4.7 tests from gcc/testsuit and received interesting result: lambda-pass.C which indirectly uses abs() fails in any case with error: call of overloaded 'abs(const double&)' is ambiguous. The only way to fix it is to set 109 line of iso/math_iso.h to "#if __cplusplus >= 199711L" (and not reference the type of compiler) AND include directly math.h or cmath in lambda-pass.C. It seems that compiler expects abs(double) to be in stdlib.h....

Overall test results:
with patched headers
  1. of expected passes 45403
  2. of unexpected failures 17
  3. of expected failures 286
  4. of unresolved testcases 10
  5. of unsupported tests 453
With original headers:
  1. of expected passes 44503
  2. of unexpected failures 586
  3. of expected failures 286
  4. of unresolved testcases 386
  5. of unsupported tests 453

The only visible error during tests with patched headers was related to double abs (double x) definition.
The main errors which showed up during tests with original headers were related to stdlib.h and stdio.h - getc, putc, std::bsearch and std::qsort definitions.
More details are available here: https://docs.google.com/document/d/1dYlt4TERoL0vws-M2mtKhLIuw0AgXC2vucR5vfAHO5c/edit?pli=1

#9 Updated by Alexander Pyhalov over 4 years ago

Attached patch allows GCC 4.7 to build several C++ programs. All but two g++ tests from gcc/testsuite succeed. These two failures are related with Sun ld linker. I've tried to keep only necessary changes (their absence lead to failures in several cases). Some cosmetic changes replace some GNUG checks with __SUNPRO_CC checks. One possible issue is wide character functions. I suspect that wchar.h, wctype.h and curses libraries need to be patched to work correctly with new C++ versions. However, I don't have a test base large enough to prove or debug these issues.

#10 Updated by Rich Lowe over 4 years ago

I don't really understand how the widechar stuff and C++ would interact either. Out of interest, which tests fail? I'm very interested in fixing such things, can you file separate bugs against ld?

The patch seems like it's relative to the previous ones, but if I'm adding it all together properly that seems like it's the right thing to do, to me -- I wish we had some more C++ aware people to think about it too.

For testing it, I would suggest you build, basically, "large complicated C++ things" and make sure they still build and pass their tests as well as they did before. When I've needed to do this previously I've used Boost, LLVM, and things like that.

#11 Updated by Alexander Pyhalov over 4 years ago

Ok, I'll test it. One related thing about gcc - https://www.illumos.org/issues/3801 . It seems that that one affected wide char stuff.

#12 Updated by Alexander Pyhalov over 4 years ago

I've built LLVM and boost successfully in a zone with applied patches (this one and https://www.illumos.org/attachments/917/feature_tests_1.patch).
LLVM test results:
Expected Passes : 6781
Expected Failures : 56
Unsupported Tests : 13

Boost tests were running for several days and managed to do a fork bomb...
From bjam log:

grep 'passed\\\\*' ~build/srcs/other/results/bjam.log |wc -l
2607


grep 'failed' ~build/srcs/other/results/bjam.log |grep -v 'failed as expected' | wc -l
1896

Most errors are related too
- 300 second time limit exceeded (526 errors)
- Some errors caused by redefining XOPEN_SOURCE to 500 (together with GCC 4.7 _STDC_C99 leads to "Compiler or options invalid for pre-UNIX 03 X/Open applications" (174 errors):

"g++" -ftemplate-depth-128 -O0 -fno-inline -Wall -g -pthreads -fPIC -DBOOST_ALL_NO_LIB=1 -DBOOST_CHRONO_DYN_LINK=1 -DBOOST_DATE_TIME_DYN_LINK=1 -DBOOST_SYSTEM_DYN_LINK=1 -DBOOST_SYSTEM_NO_DEPRECATED -DBOOST_TEST_NO_AUTO_LINK=1 -DBOOST_THREAD_BUILD_DLL=1 -DBOOST_THR
EAD_POSIX -DBOOST_THREAD_USE_DLL=1 -DDATE_TIME_INLINE -D_XOPEN_SOURCE=500 -D__EXTENSIONS__ -I".." -c -o /export/home/build/srcs/other/results/boost/bin.v2/libs/asio/test/basic_datagram_socket.test/gcc-4.7.3/debug/threading-multi/basic_datagram_socket.o" "../libs/asio/test/basic_datagram_socket.cpp"

- Some internal boost errors related to linking (Undefined symbol boost::locale::time_zone::global...) (9 errors)
- Linking errors related to not linking with libsocket (Undefined symbol __xnet_socket) (35 errors)
- Some namespace-related errors (it seems that instead of boost namespaces some tests try to use std namespace:

../libs/tr1/test/test_cmath_tricky.cpp:282:27: error: 'conf_hyperg' is not a member of 'std::tr1'

#13 Updated by Rich Lowe over 4 years ago

I think I recall the boost tests being flakey, and that matches my memory of how they're flakey (beyond a couple of gcc 4.7 specifics).

I think you should ask for review on the developer list, what you've done (as I understand it from this bug) looks reasonable to me, and seems to work as it should based on that last comment.

Thanks for doing this!

#14 Updated by Alexander Pyhalov over 4 years ago

The patch is incorrect. !defined(GNUG) != defined(__SUNPRO_CC) !!!! This breaks gcc programs. Should patch only stdlib_iso.h and stdio_iso.h . I'll attach the correct version of the patch later.

#15 Updated by Alexander Pyhalov over 4 years ago

#16 Updated by Alexander Pyhalov over 4 years ago

I think I shouldn't redefine GNUG => SUNPRO_CC macros for now. It will not clean up a code, but can create a mess. Just fixing stdio_iso.h and stdlib_iso.h, not touching anything else...

#17 Updated by Albert Lee over 4 years ago

  • Tags deleted (needs-triage)
  • Assignee set to Alexander Pyhalov
  • Status changed from New to In Progress

These are my review comments, for the record:
Our internal C headers (e.g. <iso/stdlib_iso.h>) still properly place the standard C functions in the std namespace when used with C++. These names should only be visible in the std namespace when e.g. <cstdlib> are used, and in both the global and std namespace when <stdlib.h> is used. This is the case for Sun Studio (where <cstdlib>, for example, is simply #include <iso/stdlib_iso.h>).

Interestingly, the GCC discussion revealed that the C++11 standard was recently relaxed to accommodate lazy implementations (almost everyone else) that just reused the regular C declarations in the global namespace, only importing them into the std namespace for the C++ version of the header. This change allowed <stdlib> to also have names in the global namespace, and <stdlib.h> to be missing names in the std namespace.

The changes proposed for the bug don't immediately relate to that, fortunately.

  • usr/src/head/iso/stdio_iso.h:
    Several blocks of C standard function declarations (in std namespace) require _REENTRANT or one of_REENTRANT or _LP64 to be defined. The reason is that further down there are inline definitions for these functions, and the extern declarations are only used when the inlines are unavailable.
    Right now the changes remove the restrictions on the declarations when the compiler is not Studio, so the inline definitions may be preceded by the extern declarations.

The reason the inline (not the extern) declarations for std::getc and std::putc are missing is that they are also guarded by !defined(_STRICT_STDC). So the when any compiler is in strict conformance mode we are missing getc and putc, despite these being required by both the inline getchar and putchar definitions, C99, and C++11's <cstdio> definition. I have not checked C11 but this is likely required there as well. This looks like an error.

I would replace:

#if    !defined(_REENTRANT) && !defined(_LP64) && !defined(_STRICT_STDC)

with:
#if    !defined(_REENTRANT) && !defined(_LP64)

and not touch anything else in sys/stdio_iso.h, other than updating the comments.

  • usr/src/head/iso/stdlib_iso.h:
    This declares additional (overloaded) variants of std::bsort and std::qsort with C++ linkage. The motivation is probably to allow C++ calling convention for the callback functions that these take. Both g++ and clang object to having the same function declared with different linkage. Dual linkage is illegal in C++11 7.5.5:

If two declarations declare functions with the same name and parameter-type-list (8.3.5) to be members of
the same namespace or declare objects with the same name to be members of the same namespace and the
declarations give the names different language linkages, the program is ill-formed; no diagnostic is required
if the declarations appear in different translation units.

These two declarations should definitely be made specific to Studio as they exist only to link the implementations in libCrun.

#18 Updated by Dan McDonald over 4 years ago

  • % Done changed from 0 to 100
  • Status changed from In Progress to Pending RTI

Integrated:

commit 693e4d84eb49b987c3d66cbcd4b13a5c6e9059bf
Author: Alexander Pyhalov <>
Date: Fri Jun 14 10:16:47 2013 +0400

3787 gcc4.7 __cplusplus change incompatibility
3823 wchar.h and wctype_iso.h incompatibility
Reviewed by: Albert Lee &lt;&gt;
Approved by: Dan McDonald &lt;&gt;

#19 Updated by Dan McDonald over 4 years ago

  • Status changed from Pending RTI to Resolved

#20 Updated by Albert Lee over 4 years ago

  • Tags set to c++, gcc

#21 Updated by Andrew Stormont about 4 years ago

I'm not 100% sure that's correct. Something like this is working for me:

--- /usr/include/iso/stdlib_iso.h.bak    2013-09-17 14:08:53.838818824 +0100
+++ /usr/include/iso/stdlib_iso.h    2013-09-17 13:46:36.048731566 +0100
@@ -126,13 +126,14 @@
 extern double atof(const char *);
 extern int atoi(const char *);
 extern long int atol(const char *);
-extern void *bsearch(const void *, const void *, size_t, size_t,
-    int (*)(const void *, const void *));
 #if __cplusplus >= 199711L
 extern "C++" {
     void *bsearch(const void *, const void *, size_t, size_t,
         int (*)(const void *, const void *));
 }
+#else
+extern void *bsearch(const void *, const void *, size_t, size_t,
+    int (*)(const void *, const void *));
 #endif /* __cplusplus >= 199711L */
 extern void *calloc(size_t, size_t);
 extern div_t div(int, int);
@@ -147,11 +148,12 @@
 extern size_t mbstowcs(wchar_t *_RESTRICT_KYWD, const char *_RESTRICT_KYWD,
     size_t);
 extern int mbtowc(wchar_t *_RESTRICT_KYWD, const char *_RESTRICT_KYWD, size_t);
-extern void qsort(void *, size_t, size_t, int (*)(const void *, const void *));
 #if __cplusplus >= 199711L
 extern "C++" {
     void qsort(void *, size_t, size_t, int (*)(const void *, const void *));
 }
+#else
+extern void qsort(void *, size_t, size_t, int (*)(const void *, const void *));
 #endif /* __cplusplus >= 199711L */
 extern int rand(void);
 extern void *realloc(void *, size_t);

At least for stdlib_iso.h.

#22 Updated by Igor Kozhukhov about 4 years ago

for my env all work without additional updates.
gcc-47 builds work well for userland components.
i can build illumos by gcc-47 with some additional changes, but build do not start by grub problem.

#23 Updated by Alexander Pyhalov about 4 years ago

If I understand correctly, the issue is that Studio allows function with C linkage to be redefined with C++ linkage later, but GCC doesn't. Currently committed patch resolves this issue by masking extern "C++" declaration from GCC.

Theoretically, it can cause an issue when qsort/bsearch receives C++ function as an argument. Andrew's fix declares these functions with C++ linkage. I don't have a final version of C++ standard, however the latest available C++ 11 draft states that:

Whether a name from the C standard library declared with external linkage has extern "C" or extern
"C++" linkage is implementation-defined. It is recommended that an implementation use extern "C++" 
linkage for this purpose.

So, I think that Andrew's notice is correct.

Also available in: Atom