[PATCH] D30517: [libc++abi] Add option to enable definitions for the new/delete overloads.

[Eric Fiselier via Phabricator via cfe-commits]

EricWF added a comment.

In https://reviews.llvm.org/D30517#690328, @mehdi_amini wrote:

> LGTM.







Comment at: CMakeLists.txt:416
+set(LIBCXXABI_HAS_UNDEFINED_SYMBOLS ((NOT 
LIBCXXABI_ENABLE_NEW_DELETE_DEFINITIONS)
+OR (LIBCXXABI_BUILD_EXTERNAL_THREAD_LIBRARY AND LIBCXXABI_ENABLE_SHARED)))
+

mehdi_amini wrote:
> It is unrelated to this change, but I'm wondering about the 
> `LIBCXXABI_BUILD_EXTERNAL_THREAD_LIBRARY` condition. I see undefined 
> reference to libc symbols.
> That said we're linking to libSystem which provided everything, so I'm not 
> even sure why `dynamic_lookup` is needed at all?
The external thread library configuration is not something you have to worry 
about.  The issue this configuration handles is that libc++abi.so in unable to 
link to libc++external_threads.a, and instead expects the user to manually link 
libc++external_threads.a or it expects libc++ to handle the linking and 
undefined symbols later.

Once again you're never going to run into this since you don't use an 
externalized threading library. 


https://reviews.llvm.org/D30517



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30517: [libc++abi] Add option to enable definitions for the new/delete overloads.

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini accepted this revision.
mehdi_amini added a comment.
This revision is now accepted and ready to land.

LGTM.




Comment at: CMakeLists.txt:416
+set(LIBCXXABI_HAS_UNDEFINED_SYMBOLS ((NOT 
LIBCXXABI_ENABLE_NEW_DELETE_DEFINITIONS)
+OR (LIBCXXABI_BUILD_EXTERNAL_THREAD_LIBRARY AND LIBCXXABI_ENABLE_SHARED)))
+

It is unrelated to this change, but I'm wondering about the 
`LIBCXXABI_BUILD_EXTERNAL_THREAD_LIBRARY` condition. I see undefined reference 
to libc symbols.
That said we're linking to libSystem which provided everything, so I'm not even 
sure why `dynamic_lookup` is needed at all?


https://reviews.llvm.org/D30517



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30517: [libc++abi] Add option to enable definitions for the new/delete overloads.

[Eric Fiselier via Phabricator via cfe-commits]

EricWF created this revision.

Currently both libc++ and libc++abi provide definitions for operator 
new/delete. However I believe this is incorrect and that one or the other 
should offer them.

This patch adds the CMake option `-DLIBCXXABI_ENABLE_NEW_DELETE_DEFINITIONS` 
which defaults to `OFF` unless otherwise specified. This means that by default
only libc++ provides the new/delete definitions.


https://reviews.llvm.org/D30517

Files:
  CMakeLists.txt
  src/CMakeLists.txt


Index: src/CMakeLists.txt
===
--- src/CMakeLists.txt
+++ src/CMakeLists.txt
@@ -20,10 +20,9 @@
   private_typeinfo.cpp
 )
 
-# FIXME: This file should only be compiled in special configurations such
-#  as building the Apple system dylib where libc++abi is expected to provide
-#  the new/delete definitions instead of libc++.
-list(APPEND LIBCXXABI_SOURCES stdlib_new_delete.cpp)
+if (LIBCXXABI_ENABLE_NEW_DELETE_DEFINITIONS)
+  list(APPEND LIBCXXABI_SOURCES stdlib_new_delete.cpp)
+endif()
 
 if (LIBCXXABI_ENABLE_EXCEPTIONS)
   list(APPEND LIBCXXABI_SOURCES cxa_exception.cpp)
Index: CMakeLists.txt
===
--- CMakeLists.txt
+++ CMakeLists.txt
@@ -141,6 +141,9 @@
 option(LIBCXXABI_BUILD_EXTERNAL_THREAD_LIBRARY
   "Build libc++abi with an externalized threading library.
This option may only be set to ON when LIBCXXABI_ENABLE_THREADS=ON" OFF)
+option(LIBCXXABI_ENABLE_NEW_DELETE_DEFINITIONS
+"Build libc++abi with definitions for operator new/delete. Normally libc++
+provides these definitions" OFF)
 option(LIBCXXABI_BUILD_32_BITS "Build 32 bit libc++abi." ${LLVM_BUILD_32_BITS})
 option(LIBCXXABI_INCLUDE_TESTS "Generate build targets for the libc++abi unit 
tests." ${LLVM_INCLUDE_TESTS})
 set(LIBCXXABI_TARGET_TRIPLE "" CACHE STRING "Target triple for cross 
compiling.")
@@ -409,7 +412,10 @@
   endif()
 endif()
 
-if (LIBCXXABI_BUILD_EXTERNAL_THREAD_LIBRARY AND LIBCXXABI_ENABLE_SHARED)
+set(LIBCXXABI_HAS_UNDEFINED_SYMBOLS ((NOT 
LIBCXXABI_ENABLE_NEW_DELETE_DEFINITIONS)
+OR (LIBCXXABI_BUILD_EXTERNAL_THREAD_LIBRARY AND LIBCXXABI_ENABLE_SHARED)))
+
+if (LIBCXXABI_HAS_UNDEFINED_SYMBOLS)
   # Need to allow unresolved symbols if this is to work with shared library 
builds
   if (APPLE)
 add_link_flags("-undefined dynamic_lookup")


Index: src/CMakeLists.txt
===
--- src/CMakeLists.txt
+++ src/CMakeLists.txt
@@ -20,10 +20,9 @@
   private_typeinfo.cpp
 )
 
-# FIXME: This file should only be compiled in special configurations such
-#  as building the Apple system dylib where libc++abi is expected to provide
-#  the new/delete definitions instead of libc++.
-list(APPEND LIBCXXABI_SOURCES stdlib_new_delete.cpp)
+if (LIBCXXABI_ENABLE_NEW_DELETE_DEFINITIONS)
+  list(APPEND LIBCXXABI_SOURCES stdlib_new_delete.cpp)
+endif()
 
 if (LIBCXXABI_ENABLE_EXCEPTIONS)
   list(APPEND LIBCXXABI_SOURCES cxa_exception.cpp)
Index: CMakeLists.txt
===
--- CMakeLists.txt
+++ CMakeLists.txt
@@ -141,6 +141,9 @@
 option(LIBCXXABI_BUILD_EXTERNAL_THREAD_LIBRARY
   "Build libc++abi with an externalized threading library.
This option may only be set to ON when LIBCXXABI_ENABLE_THREADS=ON" OFF)
+option(LIBCXXABI_ENABLE_NEW_DELETE_DEFINITIONS
+"Build libc++abi with definitions for operator new/delete. Normally libc++
+provides these definitions" OFF)
 option(LIBCXXABI_BUILD_32_BITS "Build 32 bit libc++abi." ${LLVM_BUILD_32_BITS})
 option(LIBCXXABI_INCLUDE_TESTS "Generate build targets for the libc++abi unit tests." ${LLVM_INCLUDE_TESTS})
 set(LIBCXXABI_TARGET_TRIPLE "" CACHE STRING "Target triple for cross compiling.")
@@ -409,7 +412,10 @@
   endif()
 endif()
 
-if (LIBCXXABI_BUILD_EXTERNAL_THREAD_LIBRARY AND LIBCXXABI_ENABLE_SHARED)
+set(LIBCXXABI_HAS_UNDEFINED_SYMBOLS ((NOT LIBCXXABI_ENABLE_NEW_DELETE_DEFINITIONS)
+OR (LIBCXXABI_BUILD_EXTERNAL_THREAD_LIBRARY AND LIBCXXABI_ENABLE_SHARED)))
+
+if (LIBCXXABI_HAS_UNDEFINED_SYMBOLS)
   # Need to allow unresolved symbols if this is to work with shared library builds
   if (APPLE)
 add_link_flags("-undefined dynamic_lookup")
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits