Martchus / cpp-utilities

Common C++ classes and routines used by my applications such as argument parser, IO and conversion utilities
GNU General Public License v2.0
52 stars 18 forks source link

catchiofailure.cpp: "_GLIBCXX_USE_CXX11_ABI" redefined #7

Closed ghost closed 7 years ago

ghost commented 7 years ago
/var/cache/glade/cpp-utilities/io/catchiofailure.cpp:9:0: warning:
"_GLIBCXX_USE_CXX11_ABI" redefined
 #define _GLIBCXX_USE_CXX11_ABI 0
 ^
In file included from
/usr/lib/gcc/x86_64-w64-mingw32/5.4.0/include/c++/cctype:41:0, from
/var/cache/glade/cpp-utilities/io/catchiofailure.cpp:3:
/usr/lib/gcc/x86_64-w64-mingw32/5.4.0/include/c++/x86_64-w64-mingw32/bits/
c++config.h:212:0:
note: this is the location of the previous definition
 # define _GLIBCXX_USE_CXX11_ABI 1
 ^
2625f82e49214bce656c8deb8cc40a913f39a3ae is the first bad commit
commit 2625f82e49214bce656c8deb8cc40a913f39a3ae
Author: Martchus <martchus@gmx.net>
Date:   Thu Jun 1 11:29:33 2017 +0200

    ios_base::failure workaround: Ensure c++config.h is included

2625f82 is the culprit

Martchus commented 7 years ago

This workaround for GCC bug https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66145 is getting really annoying. Unfortunately 2625f82 is required for detecting version of libstdc++ to apply the workaround correctly for versions >= 7.

Does it help to insert

#ifdef _GLIBCXX_USE_CXX11_ABI
#undef _GLIBCXX_USE_CXX11_ABI
#endif

before #if defined(_GLIBCXX_RELEASE) && _GLIBCXX_RELEASE >= 7? If yes, then this would be the fix.

ghost commented 7 years ago

@Martchus that fixes the reported issue. However I did not mention this error was occuring as well:

In file included from
/usr/lib/gcc/x86_64-w64-mingw32/5.4.0/include/c++/string:52:0,
from /var/cache/glade/cpp-utilities/io/catchiofailure.h:6,
from /var/cache/glade/cpp-utilities/io/catchiofailure.cpp:25:
/usr/lib/gcc/x86_64-w64-mingw32/5.4.0/include/c++/bits/basic_string.h:4782:5:
error: reference to ‘basic_string’ is ambiguous
     basic_string<_CharT, _Traits, _Alloc>
     ^
/usr/lib/gcc/x86_64-w64-mingw32/5.4.0/include/c++/bits/basic_string.h:2509:11:
note: candidates are: template<class _CharT, class _Traits, class _Alloc> class
std::basic_string
     class basic_string
           ^

which remains even after the fix.

Martchus commented 7 years ago

So #include <cctype> already pulls #include <string> in your GCC version. This is also the reason the macro has already been defined.

I suppose removing #include <string> in catchiofailure.h will not help because that header will be pulled anyways.

Not sure how to fix this. What happens if you replace #include <bits/c++config.h>?

ghost commented 7 years ago

Putting back bits/c++config.h doesnt change anything that I can see

Martchus commented 7 years ago

Sorry, I mean what happens if you replace #include <ctype> with #include <bits/c++config.h>?

ghost commented 7 years ago

@Martchus I think I got the problem. You are doing:

#include <cctype>
#define _GLIBCXX_USE_CXX11_ABI 0

You should be doing:

#define _GLIBCXX_USE_CXX11_ABI 0
#include <cctype>

You have to define the constant first, because cctype probably calls on it. Similar to:

#define __USE_MINGW_ANSI_STDIO 1
#include <stdio.h>

http://github.com/svnpenn/rosso/blob/66bbe01/rosso.c#L5

Martchus commented 7 years ago

because cctype probably calls on it.

But interestingly only in your case.

The tricky part is that _GLIBCXX_USE_CXX11_ABI needs to be 0 or 1 depending on the libstdc++ version. To find the libstdc++ version I include cctype to make _GLIBCXX_RELEASE available. But since cctype does not use _GLIBCXX_USE_CXX11_ABI in my case I guess there shouldn't be a problem by just setting it to 0 in any case.

BTW: I can not include bits/c++config.h directly because this would break builds using libc++. But it seems that even this header uses _GLIBCXX_USE_CXX11_ABI in your version so that wouldn't help anyways.

ghost commented 7 years ago

@Martchus what do you mean "only in my case"? It is defined by the standard:

http://gcc.gnu.org/onlinedocs/gcc-5.4.0/libstdc++/api/a01066_source.html

I think you should be careful to respect that not everyone is using GCC 6+, and certainly not GCC 7+.

Martchus commented 7 years ago

what do you mean "only in my case"? It is defined by the standard

I mean that in my case inclusion of cctype does not cause inclusion of basic_string or any other type using _GLIBCXX_USE_CXX11_ABI.

Using legacy GCC is not my use-case so I'm not taking any effort in testing it. Of course I'll try to fix reported bugs.

Note that I can no longer support GCC 4.8 because it lacks some required features.

ghost commented 7 years ago

@Martchus cant you just use __GLIBCXX__? It is defined by #include <ios>:

Martchus commented 7 years ago

It is defined by #include

Both, ios and cctype just include bits/c++config.h which then defines _GLIBCXX_RELEASE, __GLIBCXX__ and lots of other macros. The definition of __GLIBCXX__ actually comse just after the definition of _GLIBCXX_RELEASE. So it would make no difference.

Also it wouldn't make sense to use a macro defined in ios to make a decision which is required before including ios.

Hence my approach is to include cctype to have _GLIBCXX_RELEASE but not already _GLIBCXX_USE_CXX11_ABI. I'm still wondering why including cctype or even bits/c++config.h already pulls _GLIBCXX_USE_CXX11_ABI in your setup. Can you provide the g++ invocation? To get not only the error message, use make VERBOSE=1. I'm just curious why GCC 5.4 behaves different and maybe it is because of some flags you're using. If CMake is using @file-syntax it would be nice to have the specified file, too.

Anyways, I think I can fix the issue using this:

// fix compilation with Cygwin GCC 5.4 and should not be relevant somewhere else
#define _GLIBCXX_USE_CXX11_ABI 0
// include libstd++ specific header <bits/c++config.h> containing _GLIBCXX_RELEASE
// without including ios already (which must be included after setting _GLIBCXX_USE_CXX11_ABI)
#include <cctype>

// undefine _GLIBCXX_USE_CXX11_ABI again
#ifdef _GLIBCXX_USE_CXX11_ABI
#undef _GLIBCXX_USE_CXX11_ABI
#endif

// ensure the old ABI is used under libstd++ < 7 and the new ABI under libstd++ >= 7
#if defined(_GLIBCXX_RELEASE) && _GLIBCXX_RELEASE >= 7
#define _GLIBCXX_USE_CXX11_ABI 1
#else
#define _GLIBCXX_USE_CXX11_ABI 0
#endif

I think you should be careful to respect that not everyone is using GCC 6+, and certainly not GCC 7+.

Also note that the whole purpose of this file is to support legacy GCC versions beyond GCC 7.

ghost commented 7 years ago

My build files are here:

http://github.com/svnpenn/glade/tree/2284c08/cpp-utilities

Here is a simple demonstration of the problem and solution:

$ cat alfa.cpp
#include <ios>
int main() {
  printf("%d\n", __GLIBCXX__);
}

$ x86_64-w64-mingw32-g++ -o alfa alfa.cpp
$ ./alfa
20160603

$ cat bravo.cpp
#include <ios>
int main() {
  printf("%d\n", _GLIBCXX_RELEASE);
}

$ x86_64-w64-mingw32-g++ -o bravo bravo.cpp
bravo.cpp: In function ‘int main()’:
bravo.cpp:3:18: error: ‘_GLIBCXX_RELEASE’ was not declared in this scope
   printf("%d\n", _GLIBCXX_RELEASE);
                  ^
Martchus commented 7 years ago

of the problem

It is not the problem that _GLIBCXX_RELEASE is not defined. In this case libstdc++ version can be considered < 7 and _GLIBCXX_USE_CXX11_ABI must be set to zero.

and solution

As already explained, it is not a solution to include ios to get the macros as those macros are required before including it.

Your build files show the invocation of CMake (which looks fine) but not the invocation of the particular g++ line. However, only this line tells the concrete flags being passed to the compiler.

But does my proposed solution work for you?

BTW: You know that you can create a CMake toolchain file instead of specifying the same arguments again and again?

ghost commented 7 years ago

Here is what you should do:

#include <ios>
#if __GLIBCXX__ >= 20170502
#define _GLIBCXX_USE_CXX11_ABI 1
#else
#define _GLIBCXX_USE_CXX11_ABI 0
#endif

http://gcc.gnu.org/develop.html#timeline

Martchus commented 7 years ago

As already explained twice, it does not work to inlcude ios before setting _GLIBCXX_USE_CXX11_ABI. Stuff pulled by ios uses different ABI depending on that switch and the workaround requires the new ABI under libstdc++ >= 7 so _GLIBCXX_USE_CXX11_ABI must be set to 1 and otherwise to 0 before including ios. That means your code will not work. Just execute the IO tests of the testsuite if you don't believe me. Ok, the tests might pass if the default of _GLIBCXX_USE_CXX11_ABI in your configuration matches the required value by accident.

Does my proposed solution work for you?

ghost commented 7 years ago

I think I see the problem here:

BTW: I can not include bits/c++config.h directly because this would break builds using libc++.

I seems you are putting yourself in a box by doing this. bits/c++config.h is the way to do this when using libstdc++:

http://gcc.gnu.org/onlinedocs/libstdc++/manual/abi.html

So if you are trying to account for MacOS, then you should just be checking that:

#if LIBSTDC
#include <bits/c++config.h> 
#else
#include <MacOS.h>
#endif

I am willing to try your workaround, but it really seems like a bad hack.

Martchus commented 7 years ago

The macro LIBSTDC exists in your environment? Here it is not defined and I also didn't find a similar macro when searching before. Hence I came up with just including cctype. You're right that this is a bad hack and I'm aware of that - actually the whole workaround itself is a bad hack.

Note that I'm not taking particularly MacOS into account. Clang and sometimes even libc++ are also used on other platforms.

ghost commented 7 years ago

@Martchus I dont know if I have LIBSTDC - that block was just pseudocode. Have you seen this?

http://stackoverflow.com/a/31658120

Looks like you can do something like this:

#ifdef _LIBCPP_VERSION
#include <MacOS.h>
#else
#include <bits/c++config.h>
#endif
Martchus commented 7 years ago

Yes, and the answer proposes the same approach of my hack: Including a header which does not pull things I don't want at this point just to get the macros.

The answer porposes use of ciso646 but since this wouldn't work under libstdc++ <= 5 (according to the answer itself) I decided to use cctype instead. This header does not pull anything where the ABI would make a difference. This should be also true for GCC 5. Hence I'd like to see your g++ invocation, just to outrule the change of a bad flag here.

Looks like you can do something like this

Of course I could also just check for libc++ where including ciso646 always works and treat everything else as libstdc++. But I think that is even worse than the current approach since there are more std libs than libc++ and libstdc++ which would then just be treated as libstdc++.

ghost commented 7 years ago

Here is my make VERBOSE=1:

/usr/bin/x86_64-w64-mingw32-g++.exe -DCPP_UTILITIES_STATIC
-I/var/cache/glade/cpp-utilities -std=gnu++14 -o
CMakeFiles/c++utilities_static.dir/io/catchiofailure.cpp.o -c
/var/cache/glade/cpp-utilities/io/catchiofailure.cpp

Also look at this to see how they are determining the STD Libary:

http://github.com/seqan/seqan/blob/master/include/seqan/platform.h

Martchus commented 7 years ago

The g++ command looks fine.

Thanks for the link - that is what I was looking for earlier. But it is actually very similar to what I already have. So I'll just replace cctype with cstddef which seems more appropriate indeed. I can continue using _GLIBCXX_RELEASE because if it is not defined I I also know libstdc++ is older than 7.

So

// fix compilation with Cygwin GCC 5.4, should not be relevant somewhere else
#define _GLIBCXX_USE_CXX11_ABI 0

// include libstdc++ specific header <bits/c++config.h> containing _GLIBCXX_RELEASE when libstdc++ is >= 7
// * can not include header directly because it would break build with libc++, so include it indirectly via cstddef
// * this should not include ios or anything using _GLIBCXX_USE_CXX11_ABI already
#include <cstddef>

// undefine _GLIBCXX_USE_CXX11_ABI again
#ifdef _GLIBCXX_USE_CXX11_ABI
#undef _GLIBCXX_USE_CXX11_ABI
#endif

// ensure the old ABI is used under libstd++ < 7 and the new ABI under libstd++ >= 7
#if defined(_GLIBCXX_RELEASE) && _GLIBCXX_RELEASE >= 7
#define _GLIBCXX_USE_CXX11_ABI 1
#else
#define _GLIBCXX_USE_CXX11_ABI 0
#endif

would work for you then? If yes, I'll change it that way. Maybe it also works when leaving out defining #define _GLIBCXX_USE_CXX11_ABI 0 in the first place?

ghost commented 7 years ago

@Martchus here is the important part of my bits/c++config.h:

#ifndef _GLIBCXX_USE_CXX11_ABI
# define _GLIBCXX_USE_CXX11_ABI 1
#endif

which makes this the solution:

#define _GLIBCXX_USE_CXX11_ABI 0
#include <cstddef>
#if __GLIBCXX__ >= 20170502
#undef _GLIBCXX_USE_CXX11_ABI
#define _GLIBCXX_USE_CXX11_ABI 1
#endif
Martchus commented 7 years ago

That should work. Thanks for making a PR.