Do not pass an explicit reexported symbol list when building libc++ dylib if also defining new/delete

[mehdi_amini]

Summary

The linker would fail because the list of reexported symbols contains new/delete operators.

[EricWF]

I'm a bit confused by the description of this change. Libc++ has been enabling the new/delete definitions in its dylib since forever and I've never experienced a link error. Did you mean to say the link error occurs only when libc++abi doesn't define them?

[mehdi_amini]

In D31272#711804, @EricWF wrote:

I'm a bit confused by the description of this change. Libc++ has been enabling the new/delete definitions in its dylib since forever and I've never experienced a link error. Did you mean to say the link error occurs only when libc++abi doesn't define them?

Most people have the "command line tools" package installed on macOS, and it means that you're taking this condition:

if (DEFINED CMAKE_OSX_SYSROOT AND NOT CMAKE_OSX_SYSROOT STREQUAL "")

which leads you to:

-Wl,-reexport_library,${CMAKE_OSX_SYSROOT}/usr/lib/libc++abi.dylib

While the GreenDragon bots don't have the command line tools, so CMAKE_OSX_SYSROOT is defined by cmake and then this is used instead:

set(OSX_RE_EXPORT_LINE "/usr/lib/libc++abi.dylib -Wl,-reexported_symbols_list,${CMAKE_CURRENT_SOURCE_DIR}/libc++abi${LIBCXX_LIBCPPABI_VERSION}.exp")

[mehdi_amini]

Not that I'm claiming to understand why we're changing strategy when we have the command line tools installed or not in this case ;)

(CC @beanz if he know by any chance!)

[EricWF]

In D31272#711860, @mehdi_amini wrote:

In D31272#711804, @EricWF wrote:

I'm a bit confused by the description of this change. Libc++ has been enabling the new/delete definitions in its dylib since forever and I've never experienced a link error. Did you mean to say the link error occurs only when libc++abi doesn't define them?

Most people have the "command line tools" package installed on macOS, and it means that you're taking this condition:

if (DEFINED CMAKE_OSX_SYSROOT AND NOT CMAKE_OSX_SYSROOT STREQUAL "")

which leads you to:

-Wl,-reexport_library,${CMAKE_OSX_SYSROOT}/usr/lib/libc++abi.dylib

While the GreenDragon bots don't have the command line tools, so CMAKE_OSX_SYSROOT is defined by cmake and then this is used instead:

set(OSX_RE_EXPORT_LINE "/usr/lib/libc++abi.dylib -Wl,-reexported_symbols_list,${CMAKE_CURRENT_SOURCE_DIR}/libc++abi${LIBCXX_LIBCPPABI_VERSION}.exp")

I think you have that reversed. I have command line tools installed and CMAKE_OSX_SYSROOT isn't defined. Therefore the else() branch is taken and libc++abi is exported via the ABI list.

[mehdi_amini]

Strange. So installing the command line tools is not enough. It has to be that CMAKE_OSX_SYSROOT is only defined on Apple internal install maybe?

Anyway, if you're exporting through the ABI list and have LIBCXX_ENABLE_NEW_DELETE_DEFINITIONS I expect the link to fail. You're not observing this?

I have this error:

ld: requested re-export symbol operator new(unsigned long) is not from a dylib, but from lib/CMakeFiles/cxx_objects.dir/__/src/new.cpp.o
 file 'lib/CMakeFiles/cxx_objects.dir/__/src/new.cpp.o' for architecture x86_64

I can reproduce with: cmake -GNinja -DCMAKE_OSX_SYSROOT="" path/to/libcxx

[EricWF]

In D31272#711876, @mehdi_amini wrote:

Strange. So installing the command line tools is not enough. It has to be that CMAKE_OSX_SYSROOT is only defined on Apple internal install maybe?

Are you sure you're starting with a clean CMake build directory?

Anyway, if you're exporting through the ABI list and have LIBCXX_ENABLE_NEW_DELETE_DEFINITIONS I expect the link to fail. You're not observing this?

I am not observing this, and LIBCXX_ENABLE_NEW_DELETE_DEFINITIONS is ON. I also added -DCMAKE_OSX_SYSROOT="" but that had no effect as I would expect.

I have this error:

ld: requested re-export symbol operator new(unsigned long) is not from a dylib, but from lib/CMakeFiles/cxx_objects.dir/__/src/new.cpp.o
 file 'lib/CMakeFiles/cxx_objects.dir/__/src/new.cpp.o' for architecture x86_64

Interesting. That seems to suggest that libc++abi.dylib doesn't provide the definition at all. Not necessarily that
the duplicate definitions are causing this.

I can reproduce with: cmake -GNinja -DCMAKE_OSX_SYSROOT="" path/to/libcxx

Here is the script I use to build libc++ on https://gist.github.com/EricWF/0bc0ba09b8844d0201b18a01f8f7ffb9.

[mehdi_amini]

In D31272#711877, @EricWF wrote:

In D31272#711876, @mehdi_amini wrote:

Strange. So installing the command line tools is not enough. It has to be that CMAKE_OSX_SYSROOT is only defined on Apple internal install maybe?

Are you sure you're starting with a clean CMake build directory?

Yes. But now that I think about it, I'm using an internal branch, the issue has to come from that. I'll look deeper tomorrow!

I have this error:

ld: requested re-export symbol operator new(unsigned long) is not from a dylib, but from lib/CMakeFiles/cxx_objects.dir/__/src/new.cpp.o
 file 'lib/CMakeFiles/cxx_objects.dir/__/src/new.cpp.o' for architecture x86_64

Interesting. That seems to suggest that libc++abi.dylib doesn't provide the definition at all. Not necessarily that
the duplicate definitions are causing this.

I think it says that the list of reexport explicitly mentions a symbol that comes from an object (new.cpp.o) and not another dylib. This error will happens whether the symbol is also in libc++abi or not.

[mehdi_amini]

OK, I figured, it is because I have this revision locally on top of this one: https://reviews.llvm.org/D30765 ; and I can't submit the latter without the change here.

[EricWF]

lib/CMakeLists.txt

155

Please explain that the linker will fail because libc++abi also provides those definitions.

[mehdi_amini]

lib/CMakeLists.txt

155

Mmmm, actually no, the linker fails because libc++ provide those definition.
The linker complains because we can't ask to export the definitions from libc++abi while we already have some definitions!

[mehdi_amini]

r299052 ; let me know if you want to improve the comment in any way.

lib/CMakeLists.txt

       "-Wl,-unexported_symbols_list,${CMAKE_CURRENT_SOURCE_DIR}/libc++unexp.exp"
       "/usr/lib/libSystem.B.dylib")
   else()
-    if (DEFINED CMAKE_OSX_SYSROOT AND NOT CMAKE_OSX_SYSROOT STREQUAL "")
+    if (LIBCXX_ENABLE_NEW_DELETE_DEFINITIONS)
+        # We can't use the "-reexported_symbols_list" when we build the
+        # new/delete operators as part of the dylib: the linker would fail.
+        set(OSX_RE_EXPORT_LINE "-Wl,-reexport_library,${CMAKE_OSX_SYSROOT}/usr/lib/libc++abi.dylib")
+  elseif (DEFINED CMAKE_OSX_SYSROOT AND NOT CMAKE_OSX_SYSROOT STREQUAL "")
       list(FIND CMAKE_OSX_ARCHITECTURES "armv7" OSX_HAS_ARMV7)
       if (NOT OSX_HAS_ARMV7 EQUAL -1)
         set(OSX_RE_EXPORT_LINE