[lldb] Skip libc++ category tests on Darwin when no in-tree libc++ is built#199262
[lldb] Skip libc++ category tests on Darwin when no in-tree libc++ is built#199262medismailben wants to merge 1 commit into
Conversation
a82f963 to
737030c
Compare
737030c to
0aa1f8b
Compare
|
@llvm/pr-subscribers-lldb Author: Med Ismail Bennani (medismailben) ChangesThe cmake check that fails when Dotest also has a category-based skip path: Hard-failing the cmake configure means downstreams that intentionally opt out of building libc++ in-tree can't keep Downgrade Full diff: https://github.com/llvm/llvm-project/pull/199262.diff 1 Files Affected:
diff --git a/lldb/test/CMakeLists.txt b/lldb/test/CMakeLists.txt
index ed9dcd11cc6cd..fdfa1fa83efef 100644
--- a/lldb/test/CMakeLists.txt
+++ b/lldb/test/CMakeLists.txt
@@ -205,13 +205,16 @@ if(TARGET clang)
"LLDB_TEST_LIBCXX_ROOT_DIR to it.")
endif()
else()
- # We require libcxx for the test suite, so if we aren't building it,
- # provide a helpful error about how to resolve the situation.
+ # The LLDB test suite uses libc++ for many tests. If it isn't being
+ # built in-tree, warn and let the suite fall back to whatever libc++
+ # the compiler picks up by default (the system one on Darwin); the
+ # libc++-specific tests will be skipped or run against system libc++.
if(NOT LLDB_HAS_LIBCXX)
- message(SEND_ERROR
- "LLDB test suite requires libc++, but it is currently disabled. "
- "Please add `libcxx` to `LLVM_ENABLE_RUNTIMES` or disable tests via "
- "`LLDB_INCLUDE_TESTS=OFF`.")
+ message(WARNING
+ "LLDB test suite normally uses an in-tree libc++; it is currently "
+ "disabled. libc++-specific tests will be skipped or fall back to the "
+ "system libc++. Add `libcxx` to `LLVM_ENABLE_RUNTIMES` to enable "
+ "the full test surface.")
endif()
endif()
endif()
|
| "Please add `libcxx` to `LLVM_ENABLE_RUNTIMES` or disable tests via " | ||
| "`LLDB_INCLUDE_TESTS=OFF`.") | ||
| message(WARNING | ||
| "LLDB test suite normally uses an in-tree libc++; it is currently " |
There was a problem hiding this comment.
I'd just keep the old phrasing tbh. I'm not sure users that aren't deeply familiar with the API test suite will understand the skip/fall back comment.
Do we skip all the data formatter tests if no local libc++ is available? If we're downgrading this to a warning, I think we should make sure that they are being skipped. Otherwise it just gives a false sense of security to someone working on the data formatters
There was a problem hiding this comment.
They should be skipped if they're all annotated as libc++ tests (based on what Ismail wrote in the description). @medismailben Can you verify that?
There was a problem hiding this comment.
Yeah, I need to test that on linux since macOS will always fallback to the system libcxx.
There was a problem hiding this comment.
I built lldb on linux without LLVM_ENABLE_RUNTIMES, so it linked against the system libstdc++:
$ ldd bin/lldb | grep c++
libstdc++.so.6 => /usr/lib/libstdc++.so.6 (0x00007fe93f000000)
All the tests that have @add_test_categories(["libc++"]) are skipped.
There was a problem hiding this comment.
@Michael137 I also went back to the old phrasing with some minor adjustments.
There was a problem hiding this comment.
macOS will always fallback to the system libcxx
This was what i'm worried about. We would still run the tests in the libcxx category even without a locally built libc++?
There was a problem hiding this comment.
True, on macOS we were still running libc++ category tests using the system libcxx when we couldn't find an in-tree one. I didn't notice previously because on my machine all the tests were passing so I thought that actually the libc++ were skipped correctly.
There was a problem hiding this comment.
There should still be a way to silence this warning. Right now the only options are to enable libc++ (which may not be possible/desirable) or to disable all the tests. What if I just want to run the non-libc++ specific tests? Something like LLDB_(DIS/EN)ABLE_LIBCXX_TESTS. Then maybe you can make that the canonical variable to key off elsewhere too?
There was a problem hiding this comment.
Proposed wording:
LLDB's libc++ specific tests will be skipped. Add
libcxxtoLLVM_ENABLE_RUNTIMESto get full coverage, passLLDB_ENABLE_LIBCXX_TESTS=OFFto disable the libc++ test and this warning, or disable all tests withLLDB_INCLUDE_TESTS=OFF.
There was a problem hiding this comment.
What if I just want to run the non-libc++ specific tests?
I don't really see the point of adding LLDB_ENABLE_LIBCXX_TESTS. Instead of setting that flag, the user should just not build libcxx (i.e. remove it from LLVM_ENABLE_RUNTIMES). To me it makes things more confusing.
9f5825d to
68f611f
Compare
68f611f to
97b8909
Compare
97b8909 to
9bd9ce3
Compare
… built canRunLibcxxTests() previously short-circuited with "libc++ always present" for all Darwin targets, meaning the "libc++" test category was never skipped on macOS — even when LLDB_HAS_LIBCXX is OFF and no --libcxx-include-dir / --libcxx-library-dir are passed to dotest. The tests would silently run against the system libc++ instead of an in-tree build, producing results inconsistent with what the suite is designed to validate. Fix canRunLibcxxTests() to apply the same libcxx_include_dir / libcxx_library_dir guard on Darwin that Linux already uses. When those dirs are absent (i.e. no in-tree libc++ was built), the function returns False and checkLibcxxSupport() appends "libc++" to skip_categories — skipping those tests exactly as Linux does. Also downgrade the CMakeLists.txt SEND_ERROR to a WARNING for LLDB_HAS_LIBCXX=OFF, so downstreams that intentionally skip the runtimes build can still keep LLDB_INCLUDE_TESTS=ON for the tests they actually want to run (Shell tests, non-libc++ API tests, etc.), and update the message to accurately say the libc++ tests will be skipped. Signed-off-by: Med Ismail Bennani <ismail@bennani.ma>
9bd9ce3 to
9066943
Compare
| "Please add `libcxx` to `LLVM_ENABLE_RUNTIMES` or disable tests via " | ||
| "`LLDB_INCLUDE_TESTS=OFF`.") | ||
| message(WARNING | ||
| "LLDB test suite normally uses an in-tree libc++; it is currently " |
There was a problem hiding this comment.
There should still be a way to silence this warning. Right now the only options are to enable libc++ (which may not be possible/desirable) or to disable all the tests. What if I just want to run the non-libc++ specific tests? Something like LLDB_(DIS/EN)ABLE_LIBCXX_TESTS. Then maybe you can make that the canonical variable to key off elsewhere too?
| "Please add `libcxx` to `LLVM_ENABLE_RUNTIMES` or disable tests via " | ||
| "`LLDB_INCLUDE_TESTS=OFF`.") | ||
| message(WARNING | ||
| "LLDB test suite normally uses an in-tree libc++; it is currently " |
There was a problem hiding this comment.
Proposed wording:
LLDB's libc++ specific tests will be skipped. Add
libcxxtoLLVM_ENABLE_RUNTIMESto get full coverage, passLLDB_ENABLE_LIBCXX_TESTS=OFFto disable the libc++ test and this warning, or disable all tests withLLDB_INCLUDE_TESTS=OFF.
canRunLibcxxTests()previously short-circuited with "libc++ always present" for all Darwin targets, meaning the "libc++" test category was never skipped on macOS — even whenLLDB_HAS_LIBCXXisOFFand no--libcxx-include-dir/--libcxx-library-dirare passed to dotest. The tests would silently run against the system libc++ instead of an in-tree build, producing results inconsistent with what the suite is designed to validate.This fixes
canRunLibcxxTests()to apply the samelibcxx_include_dir/libcxx_library_dirguard onDarwinthatLinuxalready uses. When those dirs are absent (i.e. no in-tree libc++ was built), the function returnsFalseandcheckLibcxxSupport()appendslibc++toskip_categories— skipping those tests exactly as Linux does.Also downgrade the CMakeLists.txt
SEND_ERRORto aWARNINGforLLDB_HAS_LIBCXX=OFF, so downstreams that intentionally skip the runtimes build can still keepLLDB_INCLUDE_TESTS=ONfor the tests they actually want to run (Shell tests, non-libc++ API tests, etc.), and update the message to accurately say the libc++ tests will be skipped.