Quuxplusone / LLVMBugzillaTest

0 stars 0 forks source link

Placement new mistakenly typedefs char16_t and char32_t when compiling using c++98 #21306

Closed Quuxplusone closed 9 years ago

Quuxplusone commented 9 years ago
Bugzilla Link PR21307
Status RESOLVED INVALID
Importance P normal
Reported by Ilya Kulakov (kulakov.ilya@gmail.com)
Reported on 2014-10-17 18:02:30 -0700
Last modified on 2014-10-21 18:32:16 -0700
Version unspecified
Hardware Macintosh MacOS X
CC danalbert@google.com, eric@efcs.ca, llvm-bugs@lists.llvm.org, mclow.lists@gmail.com, richard-llvm@metafoo.co.uk
Fixed by commit(s)
Attachments
Blocks
Blocked by
See also
The following piece of code:

    #include <new>
    #include <stdint.h>

    typedef uint32_t char32_t;
    typedef uint16_t char16_t;

    int main()
    {
    return 0;
    }

cannot be compiled with the following flags set: -stdlib=libc++ -std=c++98
The errors I get:
    main.cpp:4:18: error: typedef redefinition with different types ('uint32_t' (aka 'unsigned int') vs 'char32_t')
    typedef uint32_t char32_t;
                 ^
    /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/__config:227:20: note: previous definition is here
    typedef __char32_t char32_t;
                   ^
    main.cpp:5:18: error: typedef redefinition with different types ('uint16_t' (aka 'unsigned short') vs 'char16_t')
    typedef uint16_t char16_t;
                 ^
    /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/__config:226:20: note: previous definition is here
    typedef __char16_t char16_t;

Mac OS X: 10.9.5 with all updates installed
clang: AppleClang 6.0.0.6000051
libc++ as reported by tool -l: 120.0.0

If I switch -stdlib to libstdc++ it compiles just fine.

Looks like including the new header file causes to include some C++11 header
files which in turn defines char16_t and char32_t. My understanding is that
libc++ thinks that placement new is C++11 feature for some reason.

                   ^
Quuxplusone commented 9 years ago

libc++ is a C++11 standard library that has a C++98 compatibility mode, not a C++98 standard library. In its C++98 compatibility mode, it provides definitions of various C++11 features, including char16_t and char32_t. I believe this is all behaving as intended, but Marshall will be able to say for sure.

Quuxplusone commented 9 years ago
Hi Richard.

I believe that this is a bug in C++98 compatibility.
If it provides definitions for some C++11 features in C++98 mode it should
provide a compile-time constant to determine presence of this feature.
Quuxplusone commented 9 years ago

I have to agree with Richard. I looked into the possibility of not defining these types in C++98 mode at it is nowhere close to viable. It's also not a change I would want to make since it removes a fair amount of functionality. My vote is WONTFIX, but it's up to Marshall.

If it provides definitions for some C++11 features in C++98 mode it should provide a compile-time constant to determine presence of this feature.

Based on my reading of __config (where the definitions live) char16_t and char32_t are always defined when they are not provided by the compiler.

Quuxplusone commented 9 years ago
> Based on my reading of __config (where the definitions live) char16_t and
> char32_t are always defined when they are not provided by the compiler.
That's ok if there is a constant so one could opt out his own definitions in
case of using libc++...

I'm trying to compile host-only pieces of the Android and their Unicode class
provides definitions of these 2 constants. Without a flag I simply cannot make
it work both for libstdc++ and libc++.
Quuxplusone commented 9 years ago
If you include any c++ standard library header provided by libc++ it will
define the macro _LIBCPP_VERSION. Using that you can determine when you
should/should not provide your own definitions.

Is there something I'm not understanding?

For example:

    #include <cstddef>
    #if __cplusplus < 201103L && !defined(_LIBCPP_VERSION)
      typedef ... char16_t;
      typedef ... char32_t;
    #endif
Quuxplusone commented 9 years ago
Hi Eric.

According to my knowledge __cplusplus doesn't actually work well with other
compilers...
As of _LIBCPP_VERSION: it doesn't seem to work:

    #if defined(_LIBCPP_VERSION)
    int x = 1;
    #else
    int x = 0;
    #endif

    #include <iostream>

    int main()
    {
        std::cout << x << std::endl;
        return 0;
    }

>> clang++ main.cpp -std=gnu++98 -stdlib=libc++
>> ./a.out
>> 0
Quuxplusone commented 9 years ago
Your not going to get any macros provided by libc++ until you include a
standard library header...

    #include <iostream>

    #if defined(_LIBCPP_VERSION)
    int x = 1;
    #else
    int x = 0;
    #endif

    int main()
    {
        std::cout << x << std::endl;
        return 0;
    }

That code works perfectly. I can't help you with how other compilers handle
__cplusplus, but you have more than enough to make things work with libc++.
Quuxplusone commented 9 years ago
Oh, didn't know that, thank you for clarification.
I was under impression that such flags are passed implicitly like -D
_LIBCPP_VERSION

But let's see what'll be your decision regarding not introducing those vars for
98 at all.
Quuxplusone commented 9 years ago
> According to my knowledge __cplusplus doesn't actually work well with other
> compilers...

If you're using a compiler that doesn't define __cplusplus in C++ mode, you're
going to have a lot more problems than unicode characters.

> I'm trying to compile host-only pieces of the Android and their Unicode class
> provides definitions of these 2 constants.

iirc (bringing libc++/c++11 to Android is my job), the definitions of
char(16|32)_t in system/core/include/utils/Unicode.h are going to cause a lot
more problems than just the definition from libc++.

libutils (Android's own, shitty, incomplete STL of sorts) defines a String16
and String32 type that are based on char16_t/char32_t respectively in both
C++98 and C++11 mode. The difference is that in C++11 mode, char16_t and
char32_t are builtin types, and otherwise they are typedefs from Unicode.h.
This means that libutils and all of its dependencies that use one of these
types must be compiled with the _same_ C++ standard version. Not doing so means
the linker cannot find the correct overloads:

    frameworks/native/libs/binder/Parcel.cpp:1109: error: undefined reference to 'android::String16::String16(char16_t const*, unsigned int)'

At some point in the near future, I'm probably just going to pull all of these
projects up to C++11 and delete the definitions in Unicode.h, at which point
your problem will go away.

> Without a flag I simply cannot make it work both for libstdc++ and libc++.

Another thing worth noting is that I'm probably (hopefully) flipping the switch
to move all of AOSP over to libc++ in the near future, which will also get you
around this problem.
Quuxplusone commented 9 years ago
(In reply to comment #9)
> If you're using a compiler that doesn't define __cplusplus in C++ mode,
> you're going to have a lot more problems than unicode characters.

Sorry, What I meant is that value of __cplusplus was rather ignored for many
years. E.g. http://stackoverflow.com/questions/14131454/visual-studio-2012-
cplusplus-and-c-11

> iirc (bringing libc++/c++11 to Android is my job), the definitions of
> char(16|32)_t in system/core/include/utils/Unicode.h are going to cause a
> lot more problems than just the definition from libc++.

Android is indeed full of repetitive code (we're working with 4.2.2 codebase
though).

> Another thing worth noting is that I'm probably (hopefully) flipping the
> switch to move all of AOSP over to libc++ in the near future, which will
> also get you around this problem.

I wrote you an email, probably we will be able to help each other.
Quuxplusone commented 9 years ago
(In reply to comment #9)
> If you're using a compiler that doesn't define __cplusplus in C++ mode,
> you're going to have a lot more problems than unicode characters.

Sorry, What I meant is that value of __cplusplus was rather ignored for many
years. E.g. http://stackoverflow.com/questions/14131454/visual-studio-2012-
cplusplus-and-c-11

> iirc (bringing libc++/c++11 to Android is my job), the definitions of
> char(16|32)_t in system/core/include/utils/Unicode.h are going to cause a
> lot more problems than just the definition from libc++.

Android is indeed full of repetitive code (we're working with 4.2.2 codebase
though).

> Another thing worth noting is that I'm probably (hopefully) flipping the
> switch to move all of AOSP over to libc++ in the near future, which will
> also get you around this problem.

I wrote you an email, probably we will be able to help each other.
Quuxplusone commented 9 years ago
(In reply to comment #6)
> Hi Eric.
>
> According to my knowledge __cplusplus doesn't actually work well with other
> compilers...
> As of _LIBCPP_VERSION: it doesn't seem to work:
>
>     #if defined(_LIBCPP_VERSION)
>     int x = 1;
>     #else
>     int x = 0;
>     #endif
>
>     #include <iostream>
>
>     int main()
>     {
>         std::cout << x << std::endl;
>         return 0;
>     }
>
> >> clang++ main.cpp -std=gnu++98 -stdlib=libc++
> >> ./a.out
> >> 0

_LIBCPP_VERSION is defined only after including at least one libc++ header file.
Quuxplusone commented 9 years ago
I agree with Richard, who wrote:
> libc++ is a C++11 standard library that has a C++98 compatibility mode, not a
C++98 standard library.

If other code is declaring char32_t and char16_t, then they're going to run
into problems with C++11 and later tools, and probably need to be
conditionalized anyway.

(and it sounds like Dan is working to fix the problem on the Android end)

So, I'm going to close this as INVALID, but if you disagree, please reopen and
state why.
Quuxplusone commented 9 years ago
I agree with that as well. But maybe there is a better way to "break" it?
clang is famous for its helpful error messages, but I spent a few hours to
figure out the problem.

Maybe libc++ should provide better checking for what is backward compatible and
what is not?