Page MenuHomePhabricator

Partial Fix for PR#38964
ClosedPublic

Authored by mclow.lists on Sep 18 2018, 9:37 AM.

Details

Reviewers
ldionne
EricWF
Summary

In https://llvm.org/r336132, I broke <string> when compiled with GCC in -std=c++03

This fixes part of this, by wrapping the template constraints in an #ifndef _LIBCPP_HAS_NO_DEDUCTION_GUIDES block - since these ones are only necessary when using template deduction guides (implicit or explicit)

I'm a bit concerned about whether or not this moves stuff into/out of the dylib (since we instantiate basic_string there)

Diff Detail

Event Timeline

mclow.lists created this revision.Sep 18 2018, 9:37 AM

An alternate solution would be to define _LIBCPP_HAS_NO_DEFAULT_TEMPLATE_ARGS_IN_FUNCTIONS and use that.
(always true, for clang, and > C++03 for other compilers)

dim retitled this revision from Partial Fix for PR#39864 to Partial Fix for PR#38964.Sep 18 2018, 12:14 PM
dim added a subscriber: dim.

Built with mkdir build && cd build && cmake -G Ninja .. && ninja, the resulting libc++.so.1 has the following additional symbols:

std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >::basic_string(char const*)
std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >::basic_string(char const*, std::__1::allocator<char> const&)
std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >::basic_string(unsigned long, char, std::__1::allocator<char> const&)
std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >::basic_string(char const*)
std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >::basic_string(char const*, std::__1::allocator<char> const&)
std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >::basic_string(unsigned long, char, std::__1::allocator<char> const&)
std::__1::basic_string<wchar_t, std::__1::char_traits<wchar_t>, std::__1::allocator<wchar_t> >::basic_string(wchar_t const*)
std::__1::basic_string<wchar_t, std::__1::char_traits<wchar_t>, std::__1::allocator<wchar_t> >::basic_string(wchar_t const*, std::__1::allocator<wchar_t> const&)
std::__1::basic_string<wchar_t, std::__1::char_traits<wchar_t>, std::__1::allocator<wchar_t> >::basic_string(unsigned long, wchar_t, std::__1::allocator<wchar_t> const&)
std::__1::basic_string<wchar_t, std::__1::char_traits<wchar_t>, std::__1::allocator<wchar_t> >::basic_string(wchar_t const*)
std::__1::basic_string<wchar_t, std::__1::char_traits<wchar_t>, std::__1::allocator<wchar_t> >::basic_string(wchar_t const*, std::__1::allocator<wchar_t> const&)
std::__1::basic_string<wchar_t, std::__1::char_traits<wchar_t>, std::__1::allocator<wchar_t> >::basic_string(unsigned long, wchar_t, std::__1::allocator<wchar_t> const&)

So these are now instantiated in the .so file. This is on FreeBSD, but I assume it will be the same on macOS and Linux; I can't check at the moment.

I also still get some errors when compiling <string> with g++ in c++03 mode:

$ make
echo "#include <string>" | g++ -std=c++03 -nostdinc++ -isystem /share/dim/src/libcxx/trunk/include -x c++ -c - -o -
In file included from <stdin>:1:
/share/dim/src/libcxx/trunk/include/string:850:75: error: default template arguments may not be used in function templates without -std=c++11 or -std=gnu++11
                               const allocator_type& __a = allocator_type());
                                                                           ^
/share/dim/src/libcxx/trunk/include/string:854:45: error: default template arguments may not be used in function templates without -std=c++11 or -std=gnu++11
         explicit basic_string(const _Tp& __t);
                                             ^
/share/dim/src/libcxx/trunk/include/string:858:72: error: default template arguments may not be used in function templates without -std=c++11 or -std=gnu++11
         explicit basic_string(const _Tp& __t, const allocator_type& __a);
                                                                        ^
/share/dim/src/libcxx/trunk/include/string:881:43: error: default template arguments may not be used in function templates without -std=c++11 or -std=gnu++11
     basic_string& operator=(const _Tp& __t)
                                           ^
*** Error code 1

Stop.
make: stopped in /share/dim/src/llvm/bugs/pr38964
In D52240#1238688, @dim wrote:

I also still get some errors when compiling <string> with g++ in c++03 mode:

Right - that's why I said "partial fix".

Marshall, what's the error we get with GCC in C++03 mode that this patch is fixing? Is there a bug.llvm.org associated to that?

Marshall, what's the error we get with GCC in C++03 mode that this patch is fixing? Is there a bug.llvm.org associated to that?

PR#38964 (from the title) ;-)

https://llvm.org/PR38964

Marshall, what's the error we get with GCC in C++03 mode that this patch is fixing? Is there a bug.llvm.org associated to that?

PR#38964 (from the title) ;-)

https://llvm.org/PR38964

I'm sorry -- I didn't see it.

@dim:

So these are now instantiated in the .so file. This is on FreeBSD, but I assume it will be the same on macOS and Linux; I can't check at the moment.

I don't think that correct. All the functions are marked with _LIBCPP_INLINE_VISIBILITY, which means they are not exported from the dylib. At least that should be true on macOS.

ldionne accepted this revision.Sep 19 2018, 8:39 AM

Marshall, can you confirm that the change does not break check-cxx-abilist on OS X. I'm fine with the change if that's not broken.

This revision is now accepted and ready to land.Sep 19 2018, 8:39 AM
dim added a subscriber: emaste.Sep 21 2018, 11:53 AM

Marshall, can you confirm that the change does not break check-cxx-abilist on OS X. I'm fine with the change if that's not broken.

Hmm, I tried on macOS 10.13.6 with Xcode 9.4.1 and Apple LLVM version 9.1.0 (clang-902.0.39.2), using stock libc++ r342744, but that already failed:

ninja: Entering directory `/Users/dim/obj/llvm-342744-trunk-darwin17-x86_64-ninja-rel-1'
[1/1] Testing ABI compatibility...
FAILED: projects/libcxx/lib/abi/CMakeFiles/check-cxx-abilist
cd /Users/dim/obj/llvm-342744-trunk-darwin17-x86_64-ninja-rel-1/projects/libcxx/lib/abi && /Users/dim/tmp/llvm/projects/libcxx/utils/sym_diff.py --only-stdlib-symbols --strict /Users/dim/tmp/llvm/projects/libcxx/lib/abi/x86_64-apple-darwin.v1.abilist /Users/dim/obj/llvm-342744-trunk-darwin17-x86_64-ninja-rel-1/lib/libc++.1.dylib
Symbol added: __ZNSt3__16__itoa8__u32toaEjPc
    {'type': 'FUNC', 'is_defined': True, 'name': '__ZNSt3__16__itoa8__u32toaEjPc'}

Symbol added: __ZNSt3__16__itoa8__u64toaEyPc
    {'type': 'FUNC', 'is_defined': True, 'name': '__ZNSt3__16__itoa8__u64toaEyPc'}

Summary
    Added:   2
    Removed: 0
    Changed: 0
Symbols added.

So I assume the ABI is already broken in some way...

But on a more positive note, applying this review did not change the check-cxx-abilist output, so I guess for macOS it should be okay?

@emaste we should really try to also get the check-cxx-abilist target working for FreeBSD!

dim added a subscriber: lichray.Sep 21 2018, 1:53 PM
In D52240#1242341, @dim wrote:

Hmm, I tried on macOS 10.13.6 with Xcode 9.4.1 and Apple LLVM version 9.1.0 (clang-902.0.39.2), using stock libc++ r342744, but that already failed:

ninja: Entering directory `/Users/dim/obj/llvm-342744-trunk-darwin17-x86_64-ninja-rel-1'
[1/1] Testing ABI compatibility...
FAILED: projects/libcxx/lib/abi/CMakeFiles/check-cxx-abilist
cd /Users/dim/obj/llvm-342744-trunk-darwin17-x86_64-ninja-rel-1/projects/libcxx/lib/abi && /Users/dim/tmp/llvm/projects/libcxx/utils/sym_diff.py --only-stdlib-symbols --strict /Users/dim/tmp/llvm/projects/libcxx/lib/abi/x86_64-apple-darwin.v1.abilist /Users/dim/obj/llvm-342744-trunk-darwin17-x86_64-ninja-rel-1/lib/libc++.1.dylib
Symbol added: __ZNSt3__16__itoa8__u32toaEjPc
    {'type': 'FUNC', 'is_defined': True, 'name': '__ZNSt3__16__itoa8__u32toaEjPc'}

Symbol added: __ZNSt3__16__itoa8__u64toaEyPc
    {'type': 'FUNC', 'is_defined': True, 'name': '__ZNSt3__16__itoa8__u64toaEyPc'}

Summary
    Added:   2
    Removed: 0
    Changed: 0
Symbols added.

So I assume the ABI is already broken in some way...

This appears to be a consequence of rL338479 by @lichray , but since it only *adds* two functions, it might be harmless...

In D52240#1242536, @dim wrote:
Symbol added: __ZNSt3__16__itoa8__u32toaEjPc
    {'type': 'FUNC', 'is_defined': True, 'name': '__ZNSt3__16__itoa8__u32toaEjPc'}

Symbol added: __ZNSt3__16__itoa8__u64toaEyPc
    {'type': 'FUNC', 'is_defined': True, 'name': '__ZNSt3__16__itoa8__u64toaEyPc'}

So I assume the ABI is already broken in some way...

This appears to be a consequence of rL338479 by @lichray , but since it only *adds* two functions, it might be harmless...

Yes, I don't have a Mac to test so I'm reluctant to add those symbols when committing. Anyone who have a Mac can just add it.

This looks OK to me. But I'm not in love with using _LIBCPP_HAS_NO_DEDUCTION_GUIDES to check for the availability of default template arguments.

mclow.lists closed this revision.Oct 16 2018, 9:05 AM

Committed as revision 344616