Skip to content

[libcxxabi] Add test to assert that ItaniumDemangle.h is the same #140323

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

boomanaiden154
Copy link
Contributor

ItaniumDemangle.h exists in both llvm/ and libcxxabi/. These files are supposed to be copies of each other (minus the top two lines). This patch adds a test to assert that this is the case to enable tooling to automatically detect this as an issue, like in #139825. This makes it easier for contributors unfamiliar with the duplication to make changes/get appropriate reviews.

Ideally we would share the file and copy it from one place to the other but the ideal way to do this (based on previous discussion with libc++ maintainers) would be a new runtime library that clearly outlines requirements, so that is left for later with the test being used as a stopgap. This is a relatively common approach for structures shared between compiler-rt and LLVM.

This patch does make the test reference the LLVM source directory, but that should be fine given building libcxxabi is only supported through the runtimes build in the monorepo meaning it should always be available.

ItaniumDemangle.h exists in both llvm/ and libcxxabi/. These files are
supposed to be copies of each other (minus the top two lines). This
patch adds a test to assert that this is the case to enable tooling to
automatically detect this as an issue, like in llvm#139825. This makes it
easier for contributors unfamiliar with the duplication to make
changes/get appropriate reviews.

Ideally we would share the file and copy it from one place to the other
but the ideal way to do this (based on previous discussion with libc++
maintainers) would be a new runtime library that clearly outlines
requirements, so that is left for later with the test being used as a
stopgap. This is a relatively common approach for structures shared
between compiler-rt and LLVM.

This patch does make the test reference the LLVM source directory, but
that should be fine given building libcxxabi is only supported through
the runtimes build in the monorepo meaning it should always be
available.
@boomanaiden154 boomanaiden154 requested a review from a team as a code owner May 17, 2025 00:32
@llvmbot llvmbot added the libc++abi libc++abi C++ Runtime Library. Not libc++. label May 17, 2025
@llvmbot
Copy link
Member

llvmbot commented May 17, 2025

@llvm/pr-subscribers-libcxxabi

Author: Aiden Grossman (boomanaiden154)

Changes

ItaniumDemangle.h exists in both llvm/ and libcxxabi/. These files are supposed to be copies of each other (minus the top two lines). This patch adds a test to assert that this is the case to enable tooling to automatically detect this as an issue, like in #139825. This makes it easier for contributors unfamiliar with the duplication to make changes/get appropriate reviews.

Ideally we would share the file and copy it from one place to the other but the ideal way to do this (based on previous discussion with libc++ maintainers) would be a new runtime library that clearly outlines requirements, so that is left for later with the test being used as a stopgap. This is a relatively common approach for structures shared between compiler-rt and LLVM.

This patch does make the test reference the LLVM source directory, but that should be fine given building libcxxabi is only supported through the runtimes build in the monorepo meaning it should always be available.


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

2 Files Affected:

  • (modified) libcxxabi/test/configs/cmake-bridge.cfg.in (+2)
  • (added) libcxxabi/test/itanium_demangle_matches_llvm.sh.test (+6)
diff --git a/libcxxabi/test/configs/cmake-bridge.cfg.in b/libcxxabi/test/configs/cmake-bridge.cfg.in
index edf80c8f2a732..1bdba32405ddd 100644
--- a/libcxxabi/test/configs/cmake-bridge.cfg.in
+++ b/libcxxabi/test/configs/cmake-bridge.cfg.in
@@ -27,6 +27,8 @@ config.test_exec_root = os.path.join('@LIBCXXABI_BINARY_DIR@', 'test')
 config.host_triple = '@LLVM_HOST_TRIPLE@'
 
 config.substitutions.append(('%{libcxx}', '@LIBCXXABI_LIBCXX_PATH@'))
+config.substitutions.append(('%{libcxxabi}', '@LIBCXXABI_SOURCE_DIR@'))
+config.substitutions.append(('%{llvm}', '@LLVM_MAIN_SRC_DIR@'))
 config.substitutions.append(('%{install-prefix}', '@LIBCXXABI_TESTING_INSTALL_PREFIX@'))
 config.substitutions.append(('%{include}', '@LIBCXXABI_TESTING_INSTALL_PREFIX@/include'))
 config.substitutions.append(('%{cxx-include}', '@LIBCXXABI_TESTING_INSTALL_PREFIX@/@LIBCXXABI_INSTALL_INCLUDE_DIR@'))
diff --git a/libcxxabi/test/itanium_demangle_matches_llvm.sh.test b/libcxxabi/test/itanium_demangle_matches_llvm.sh.test
new file mode 100644
index 0000000000000..31371d121fbdd
--- /dev/null
+++ b/libcxxabi/test/itanium_demangle_matches_llvm.sh.test
@@ -0,0 +1,6 @@
+# This test diffs the ItaniumDemangle.h header in libcxxabi and LLVM to ensure
+# that they are the same.
+
+# RUN: tail -n +3 %{libcxxabi}/src/demangle/ItaniumDemangle.h > %t.libcxxabi_demangle
+# RUN: tail -n +3 %{llvm}/include/llvm/Demangle/ItaniumDemangle.h > %t.llvm_demangle
+# RUN: diff %t.libcxxabi_demangle %t.llvm_demangle

@boomanaiden154 boomanaiden154 requested a review from MaskRay May 19, 2025 16:58
@boomanaiden154 boomanaiden154 merged commit c1970c7 into llvm:main May 27, 2025
201 of 216 checks passed
@boomanaiden154 boomanaiden154 deleted the libcxx-abi-assert-llvm-libcxxabi-same-demangle branch May 27, 2025 23:40
sivan-shani pushed a commit to sivan-shani/llvm-project that referenced this pull request Jun 3, 2025
…vm#140323)

ItaniumDemangle.h exists in both llvm/ and libcxxabi/. These files are
supposed to be copies of each other (minus the top two lines). This
patch adds a test to assert that this is the case to enable tooling to
automatically detect this as an issue, like in llvm#139825. This makes it
easier for contributors unfamiliar with the duplication to make
changes/get appropriate reviews.

Ideally we would share the file and copy it from one place to the other
but the ideal way to do this (based on previous discussion with libc++
maintainers) would be a new runtime library that clearly outlines
requirements, so that is left for later with the test being used as a
stopgap. This is a relatively common approach for structures shared
between compiler-rt and LLVM.

This patch does make the test reference the LLVM source directory, but
that should be fine given building libcxxabi is only supported through
the runtimes build in the monorepo meaning it should always be
available.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++abi libc++abi C++ Runtime Library. Not libc++.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants