[libcxx-commits] [PATCH] D144734: [libcxx] Enable support for static and debug Windows runtimes

[Martin Storsjö via Phabricator via libcxx-commits]

mstorsjo added a comment.

This looks like a rather large change, it's hard to get a proper overview of everything that is going on here, so I can't really give an overall comment on it as such. I would very much prefer if this can be split up into a series of commits (I see at least 3-5 different individual atomic changes here). E.g. first, allow linking against different C runtime configurations only, then adjusting how libc++experimental is handled.

Note that we normally only build a static c++experimental, since it's not ABI frozen; starting to provide a shared linked libc++experimental for this configuration, since it doesn't work without that, is a rather significant policy change, which definitely needs to be split out into a separate commit, clearly labelled as such, and with pros/cons of the change enumerated.

Also, note that https://cmake.org/cmake/help/latest/policy/CMP0091.html changes how the MSVC runtimes are selected - previously, as this patch shows, they were visible via e.g. `CMAKE_CXX_FLAGS_RELEASE`, but after that policy change, AFAIK they wouldn't be visible there. As the current CMake baseline version is < 3.15, we're still on the old behaviour, but we'll soon switch to requiring 3.20, which I believe would trigger the new behaviour. So for this change, maybe it's simplest to just hold off of doing it until we've switched to requiring 3.20, and then do it according to the new policy only - instead of requiring handling two different ways of signaling the same?



================
Comment at: libcxx/CMakeLists.txt:489
+if (LIBCXX_TARGETING_MSVC)
+  string(REGEX MATCH "(^| )/MTd?( |$)" result ${CMAKE_CXX_FLAGS_${uppercase_CMAKE_BUILD_TYPE}})
+  if ("${result}" STREQUAL "")
----------------
You'd want to match for `-` in addition to `/` for the character starting the option; `-MD` works just as well as `/MD`.



================
Comment at: libcxx/test/configs/llvm-libc++-static-clangcl.cfg.in:25
 config.substitutions.append(('%{link_flags}',
-    '-nostdlib -L %{lib} -llibc++ -lmsvcrt -lmsvcprt -loldnames'
+    '-nostartfiles -nostdlib++ -rtlib=compiler-rt -L %{lib} -llibc++ -l' + runtime_lib
 ))
----------------
What does the newly added `-rtlib=compiler-rt` have to do with the rest of the changes here?


================
Comment at: libcxx/test/std/experimental/memory/memory.resource.global/new_delete_resource.pass.cpp:94
     assert(ret);
-    assert(globalMemCounter.checkOutstandingNewEq(1));
-    assert(globalMemCounter.checkLastNewSizeEq(50));
+    ASSERT_WITH_LIBRARY_INTERNAL_ALLOCATIONS(globalMemCounter.checkOutstandingNewEq(1));
+    ASSERT_WITH_LIBRARY_INTERNAL_ALLOCATIONS(globalMemCounter.checkLastNewSizeEq(50));
----------------
This looks like a change that probably could be split out and landed separately, before the rest?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144734