[PATCH] D58598: [CMake] Support alternative C++ ABI library

[Shoaib Meenai via Phabricator via llvm-commits]

smeenai added inline comments.


================
Comment at: CMakeLists.txt:162
+set(CXXABIS none default libstdc++ libsupc++ libc++ libc++abi)
+set_property(CACHE TEST_SUITE_CXX_ABI PROPERTY STRINGS ;${CXXABIS})
+if (TEST_SUITE_CXX_ABI STREQUAL "default")
----------------
phosek wrote:
> smeenai wrote:
> > What's the purpose of the leading semicolon?
> TBH I don't really know, this is being used in compiler-rt CMake. AFAIK this is only needed for CMake GUI which I don't have any experience with, I'm fine omitting it.
I would think it's allowing an empty string as one of the values, but I'm not sure why you'd want that? It's a GUI-only thing though and CMake doesn't enforce it, so I don't think it matters either way.


================
Comment at: CMakeLists.txt:165
+  if (APPLE OR CMAKE_SYSTEM_NAME MATCHES "FreeBSD|Fuchsia")
+    set(TEST_SUITE_CXX_ABI_LIBNAME "c++")
+  else()
----------------
phosek wrote:
> smeenai wrote:
> > Why c++ instead of c++abi?
> Not every platform ships separate C++ ABI library, e.g. in our toolchain we combine libc++abi and libc++ into a single library.
Fair enough. Last I checked you'd run into issues if libc++ and libc++abi were separate static libraries, since you wouldn't get the linker script that links libc++abi as well as libc++ in that case, but I don't think many people use that setup.


================
Comment at: CMakeLists.txt:167
+  else()
+    set(TEST_SUITE_CXX_ABI_LIBNAME "stdc++")
+  endif()
----------------
phosek wrote:
> smeenai wrote:
> > Why stdc++ instead of supc++?
> The same as above, I'm fine keeping it as supc++ to match the current behavior, although stdc++ seems safer to me.
Yeah, I think I'd prefer to keep it as supc++ to match the existing behavior. I'm worried about the case where stdc++ is a static library and you need to link against supc++ separately, for example. (I'm not actually sure if that's a concern with libstdc++ or if they use a linker script even for their static libraries, but I know it's been an issue with libc++ in the past.)


Repository:
  rT test-suite

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58598/new/

https://reviews.llvm.org/D58598