[libc++][CI] Tests the no RTTI configuration. #65518

mordante · 2023-09-06T19:07:32Z

There are a few drive-by fixes:

Since the combination RTTI disabled and exceptions enabled do not work, this combination is prohibited.
A small NFC in any fixing clang-tidy.

The code in the Buildkite configuration is prepared for using the std module. There are more fixes needed for that configuration which will be done in a separate commit.

philnik777

LGTM assuming the CI passes.

libcxx/CMakeLists.txt

EricWF

I still have remaining questions.

libcxx/CMakeLists.txt

libcxx/utils/ci/buildkite-pipeline.yml

EricWF · 2023-09-12T16:37:53Z

Sorry, still getting used to github PR's. I thought I had submitted these comments last week.

ldionne

LGTM, this adds important missing coverage to our CI.

I would not make the changes in libcxx/include/any in this PR though, it should be a separate NFC IIUC.

libcxx/include/any

llvmbot · 2023-11-24T18:50:11Z

@llvm/pr-subscribers-github-workflow

Author: Mark de Wever (mordante)

Changes

There are a few drive-by fixes:

Since the combination RTTI disabled and exceptions enabled do not work, this combination is prohibited.
A small NFC in any fixing clang-tidy.

The code in the Buildkite configuration is prepared for using the std module. There are more fixes needed for that configuration which will be done in a separate commit.

Full diff: https://github.com/llvm/llvm-project/pull/65518.diff

6 Files Affected:

(modified) .github/workflows/libcxx-build-and-test.yaml (+1)
(modified) libcxx/CMakeLists.txt (+8-1)
(added) libcxx/cmake/caches/Generic-no-rtti.cmake (+4)
(modified) libcxx/docs/BuildingLibcxx.rst (+1)
(modified) libcxx/include/any (+1-3)
(modified) libcxx/utils/ci/run-buildbot (+5)

diff --git a/.github/workflows/libcxx-build-and-test.yaml b/.github/workflows/libcxx-build-and-test.yaml
index dccfe54d4646767..2f7458fe9db4338 100644
--- a/.github/workflows/libcxx-build-and-test.yaml
+++ b/.github/workflows/libcxx-build-and-test.yaml
@@ -156,6 +156,7 @@ jobs:
           'generic-no-tzdb',
           'generic-no-unicode',
           'generic-no-wide-characters',
+          'generic-no-rtti',
           'generic-static',
           'generic-with_llvm_unwinder'
         ]
diff --git a/libcxx/CMakeLists.txt b/libcxx/CMakeLists.txt
index 843ccbd0ed92edb..77384f526b22282 100644
--- a/libcxx/CMakeLists.txt
+++ b/libcxx/CMakeLists.txt
@@ -284,7 +284,9 @@ endif()
 
 # Feature options -------------------------------------------------------------
 option(LIBCXX_ENABLE_EXCEPTIONS "Use exceptions." ON)
-option(LIBCXX_ENABLE_RTTI "Use run time type information." ON)
+option(LIBCXX_ENABLE_RTTI
+  "Use run time type information.
+   This option may only be set to OFF when LIBCXX_ENABLE_EXCEPTIONS=OFF." ON)
 option(LIBCXX_ENABLE_THREADS "Build libc++ with support for threads." ON)
 option(LIBCXX_ENABLE_MONOTONIC_CLOCK
   "Build libc++ with support for a monotonic clock.
@@ -374,6 +376,11 @@ if (LIBCXX_HAS_PTHREAD_API)
   endif()
 endif()
 
+if (NOT LIBCXX_ENABLE_RTTI AND LIBCXX_ENABLE_EXCEPTIONS)
+  message(FATAL_ERROR "The option LIBCXX_ENABLE_RTTI can not be turned off"
+	                  " when LIBCXX_ENABLE_EXCEPTIONS is turned on.")
+endif()
+
 # Ensure LLVM_USE_SANITIZER is not specified when LIBCXX_GENERATE_COVERAGE
 # is ON.
 if (LLVM_USE_SANITIZER AND LIBCXX_GENERATE_COVERAGE)
diff --git a/libcxx/cmake/caches/Generic-no-rtti.cmake b/libcxx/cmake/caches/Generic-no-rtti.cmake
new file mode 100644
index 000000000000000..c62ddcec90149b4
--- /dev/null
+++ b/libcxx/cmake/caches/Generic-no-rtti.cmake
@@ -0,0 +1,4 @@
+set(LIBCXX_ENABLE_RTTI OFF CACHE BOOL "")
+set(LIBCXX_ENABLE_EXCEPTIONS OFF CACHE BOOL "")
+set(LIBCXXABI_ENABLE_RTTI OFF CACHE BOOL "")
+set(LIBCXXABI_ENABLE_EXCEPTIONS OFF CACHE BOOL "")
diff --git a/libcxx/docs/BuildingLibcxx.rst b/libcxx/docs/BuildingLibcxx.rst
index 2cee97c03ced089..0b4d42052865121 100644
--- a/libcxx/docs/BuildingLibcxx.rst
+++ b/libcxx/docs/BuildingLibcxx.rst
@@ -369,6 +369,7 @@ libc++ Feature Options
   **Default**: ``ON``
 
   Build libc++ with run time type information.
+  This option may only be set to OFF when LIBCXX_ENABLE_EXCEPTIONS=OFF.
 
 .. option:: LIBCXX_INCLUDE_TESTS:BOOL
 
diff --git a/libcxx/include/any b/libcxx/include/any
index 86dd43cf80764ab..c35353906a4091c 100644
--- a/libcxx/include/any
+++ b/libcxx/include/any
@@ -194,9 +194,7 @@ namespace __any_imp
       if (__id && *__id == typeid(_Tp))
           return true;
 #endif
-      if (!__id && __fallback_id == __any_imp::__get_fallback_typeid<_Tp>())
-          return true;
-      return false;
+      return !__id && __fallback_id == __any_imp::__get_fallback_typeid<_Tp>();
   }
 
   template <class _Tp>
diff --git a/libcxx/utils/ci/run-buildbot b/libcxx/utils/ci/run-buildbot
index 58c4c2a1021b600..864eb84d1492905 100755
--- a/libcxx/utils/ci/run-buildbot
+++ b/libcxx/utils/ci/run-buildbot
@@ -472,6 +472,11 @@ generic-no-exceptions)
     check-runtimes
     check-abi-list
 ;;
+generic-no-rtti)
+    clean
+    generate-cmake -C "${MONOREPO_ROOT}/libcxx/cmake/caches/Generic-no-rtti.cmake"
+    check-runtimes
+;;
 #
 # Other miscellaneous jobs
 #

mordante · 2023-11-25T13:00:19Z

@EricWF are you happy with the changes?

ldionne

LGTM with small comments.

libcxx/CMakeLists.txt

EricWF · 2023-11-28T18:27:31Z

I would much rather we do what our users tend to do here, and that's build with rtti enabled, and then disable rtti only during the test suite run.

ldionne · 2023-11-28T18:31:24Z

I would much rather we do what our users tend to do here, and that's build with rtti enabled, and then disable rtti only during the test suite run.

In reality, we have instances of both things happening. Most embedded configurations will actually build the library without RTTI. I wonder if we really want to test both combinations -- we generally don't.

I do agree that most users on e.g. desktops will disable RTTI yet their dylib will have been built with RTTI enabled. I think there's a question of how we want to approach this configuration explosion. FWIW I could even be talked into dropping support for building the library without RTTI -- I wonder how much impact this actually has on the size of the library but I suspect not that much.

mordante · 2023-11-28T18:43:45Z

The reason for adding the configuration is that users can do this and it was not tested.

Maybe we should brainstorm how we can avoid running all configurations for all patches, but still allow to run them manually. For example, format related patches really should test no-wide-characters, mdspan patches are very unlikely to find an issue in that configuration.

This fixes a clang-tidy diagnostic when building libc++ with RTTI disabled. This was originally part of #65518.

ldionne

LGTM -- this adds coverage for something we claim to support right now but didn't test. I'd be in favour of investigating whether we actually want to support building with RTTI disabled at all (perhaps not), but that would be a separate effort.

There are a few drive-by fixes: - Since the combination RTTI disabled and exceptions enabled do not work, this combination is prohibited. - A small NFC in any fixing clang-tidy. The code in the Buildkite configuration is prepared for using the std module. There are more fixes needed for that configuration which will be done in a separate commit.

* main: (5908 commits) [readtapi] Cleanup printing command line options (llvm#75106) [flang] Fix compilation error due to variable no being used (llvm#75210) [C API] Add getters and setters for fast-math flags on relevant instructions (llvm#75123) [libc++][CI] Tests the no RTTI configuration. (llvm#65518) [RemoveDIs] Fold variable into assert, it's only used once. NFC [RemoveDI] Handle DPValues in SROA (llvm#74089) [AArch64][GlobalISel] Test Pre-Commit for Look into array's element [mlir][tensor] Fix bug in `tensor.extract(tensor.from_elements)` folder (llvm#75109) [analyzer] Move alpha checker EnumCastOutOfRange to optin (llvm#67157) [RemoveDIs] Handle DPValues in replaceDbgDeclare (llvm#73507) [SHT_LLVM_BB_ADDR_MAP] Implements PGOAnalysisMap in Object and ObjectYAML with tests. [X86][GlobalISel] Add instruction selection for G_SELECT (llvm#70753) [AMDGPU] Remove unused function splitScalar64BitAddSub [LLVM][DWARF] Add compilation directory and dwo name to TU in dwo section (llvm#74909) [clang][Interp] Implement __builtin_ffs (llvm#72988) [RemoveDIs] Update ConvertDebugDeclareToDebugValue after llvm#72276 (llvm#73508) [libc][NFC] Reuse `FloatProperties` constant instead of creating new ones (llvm#75187) [RemoveDIs] Fix removeRedundantDdbgInstrs utils for dbg.declares (llvm#74102) Reapply "[RemoveDIs][NFC] Find DPValues using findDbgDeclares (llvm#73500)" [GitHub] Remove author association print from new-prs workflow ...

github-actions bot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Sep 6, 2023

mordante marked this pull request as ready for review September 6, 2023 19:09

mordante requested a review from a team as a code owner September 6, 2023 19:09

mordante force-pushed the test_no_RTTI_in_CI branch 3 times, most recently from e77cc43 to 264236c Compare September 10, 2023 16:57

philnik777 approved these changes Sep 10, 2023

View reviewed changes

libcxx/CMakeLists.txt Show resolved Hide resolved

EricWF requested changes Sep 12, 2023

View reviewed changes

libcxx/CMakeLists.txt Outdated Show resolved Hide resolved

libcxx/utils/ci/buildkite-pipeline.yml Outdated Show resolved Hide resolved

ldionne self-assigned this Sep 13, 2023

ldionne approved these changes Sep 13, 2023

View reviewed changes

libcxx/include/any Outdated Show resolved Hide resolved

mordante force-pushed the test_no_RTTI_in_CI branch from 264236c to 1be6dfe Compare November 24, 2023 18:49

mordante requested a review from a team as a code owner November 24, 2023 18:49

llvmbot added the github:workflow label Nov 24, 2023

ldionne approved these changes Nov 27, 2023

View reviewed changes

libcxx/CMakeLists.txt Outdated Show resolved Hide resolved

libcxx/CMakeLists.txt Outdated Show resolved Hide resolved

mordante added a commit that referenced this pull request Dec 9, 2023

[NFC][libc++] Rewrites a return statement.

b5a6e8a

This fixes a clang-tidy diagnostic when building libc++ with RTTI disabled. This was originally part of #65518.

mordante force-pushed the test_no_RTTI_in_CI branch 2 times, most recently from 2da0130 to bf7483b Compare December 10, 2023 15:03

ldionne approved these changes Dec 11, 2023

View reviewed changes

mordante force-pushed the test_no_RTTI_in_CI branch from bf7483b to 457c487 Compare December 12, 2023 16:11

mordante merged commit ed210f9 into llvm:main Dec 12, 2023

mordante deleted the test_no_RTTI_in_CI branch December 12, 2023 16:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[libc++][CI] Tests the no RTTI configuration. #65518

[libc++][CI] Tests the no RTTI configuration. #65518

mordante commented Sep 6, 2023

philnik777 left a comment

EricWF left a comment

EricWF commented Sep 12, 2023

ldionne left a comment

llvmbot commented Nov 24, 2023

mordante commented Nov 25, 2023

ldionne left a comment

EricWF commented Nov 28, 2023

ldionne commented Nov 28, 2023

mordante commented Nov 28, 2023

ldionne left a comment

[libc++][CI] Tests the no RTTI configuration. #65518

[libc++][CI] Tests the no RTTI configuration. #65518

Conversation

mordante commented Sep 6, 2023

philnik777 left a comment

Choose a reason for hiding this comment

EricWF left a comment

Choose a reason for hiding this comment

EricWF commented Sep 12, 2023

ldionne left a comment

Choose a reason for hiding this comment

llvmbot commented Nov 24, 2023

mordante commented Nov 25, 2023

ldionne left a comment

Choose a reason for hiding this comment

EricWF commented Nov 28, 2023

ldionne commented Nov 28, 2023

mordante commented Nov 28, 2023

ldionne left a comment

Choose a reason for hiding this comment