This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] convert symbol checker from CMake target to lit test
Needs ReviewPublic

Authored by francii on Apr 19 2023, 4:03 PM.

Details

Reviewers
ldionne
daltenty
Jake-Egan
Group Reviewers
Restricted Project
Restricted Project
Restricted Project
Summary

Replaces the symbol checker CMake target with a lit test (per the TODO: https://github.com/llvm/llvm-project/blob/main/libcxx/utils/ci/run-buildbot#L157).

The changes can be broken down as:

  1. Moved the existing libcxx logic to runtimes/utils/, while making it more generic to support additional runtimes.
  2. Create a lit test to check the libc++ symbols, removing the CMake target that runs in the builds.

(Original description for reference. Will be removed before patch lands)
Ultimately, there are 2 goals that begin with this patch:

  1. Allow the abi symbol checker support additional runtimes (libunwind, libc++abi).
  1. Replace the symbol checker CMake target with a lit test (per the TODO: https://github.com/llvm/llvm-project/blob/main/libcxx/utils/ci/run-buildbot#L157).

The first step to this transition is to take the existing libcxx logic and make it more generic. This patch moves the files out of the libcxx/utils directory and into the utils directory, while making the necessary changes to allow the files to continue to run as expected.

There will be a follow-up patch that makes generate_abi_list.py only filter the stdlib symbols if an argument is passed. This way, the file can be used on the other libraries.

Finally, there will be a patch that creates a lit test to check the libc++ symbols. This lit test will also serve as the template for the libunwind and libc++abi symbol checking lit tests.

Diff Detail

Event Timeline

francii created this revision.Apr 19 2023, 4:03 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 19 2023, 4:03 PM
Herald added a subscriber: arichardson. · View Herald Transcript
francii requested review of this revision.Apr 19 2023, 4:03 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 19 2023, 4:03 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
francii updated this revision to Diff 515118.Apr 19 2023, 4:05 PM

Update CMakeLists.txt

francii removed 1 blocking reviewer(s): ldionne.
francii edited the summary of this revision. (Show Details)Apr 20 2023, 8:31 AM
francii edited the summary of this revision. (Show Details)Apr 20 2023, 11:04 AM
francii edited the summary of this revision. (Show Details)Apr 21 2023, 3:27 PM
francii edited the summary of this revision. (Show Details)
francii edited the summary of this revision. (Show Details)

@ldionne please give your thoughts on everything laid out in the summary of this patch and let me know your thoughts

Ultimately, there are 2 goals that begin with this patch:

  1. Allow the abi symbol checker support additional runtimes (libunwind, libc++abi).
  2. Replace the symbol checker CMake target with a lit test (per the TODO: https://github.com/llvm/llvm-project/blob/main/libcxx/utils/ci/run-buildbot#L157).

That makes sense to me.

The first step to this transition is to take the existing libcxx logic and make it more generic. This patch moves the files out of the libcxx/utils directory and into the utils directory, while making the necessary changes to allow the files to continue to run as expected.

I don't think we want to use the top-level utils/ directory for this, this is closely related to the runtimes. Maybe we could put it under runtimes/utils?

There will be a follow-up patch that makes generate_abi_list.py only filter the stdlib symbols if an argument is passed. This way, the file can be used on the other libraries.

I actually don't understand why we even need to filter stdlib symbols. We should be running this on libc++.dylib and observing all the symbols regardless, I think?

Finally, there will be a patch that creates a lit test to check the libc++ symbols. This lit test will also serve as the template for the libunwind and libc++abi symbol checking lit tests.

Sounds good! There will be a bit of a challenge to determine what the UNSUPPORTED: XXXXXXXX for the Lit test is going to be -- basically it will have to be marked as unsupported whenever we don't have an ABI list for the configuration. We'll also need to pass the ABI list path to the test somehow, I guess. Anyway, we can cross that bridge when we get to it.

I'd suggest you make your follow-up reviews as a stack on top of this one, that way we can look at what things look like before merging this. Thanks a lot for your interest in improving this, it's been on my TODO list for a long long time!

I don't think we want to use the top-level utils/ directory for this, this is closely related to the runtimes. Maybe we could put it under runtimes/utils?

Sounds good! I'll update the patch

I actually don't understand why we even need to filter stdlib symbols. We should be running this on libc++.dylib and observing all the symbols regardless, I think?

I believe so. But currently, generate_abi_list.py always removes any non-stdlib symbols for any library that is passed to it (on line 34).

symbols, _ = libcxx.sym_check.util.filter_stdlib_symbols(symbols)

I need to remove (or at least guard) this line so that we can generate the symbols for the other runtimes as well. I added an argument because as to not break expectations of what this file can do, but I'm also okay with just removing the filter entirely.

We'll also need to pass the ABI list path to the test somehow

I've noticed that even the name of the ABI list file is dependent on variables defined in CMake during the build/testing phase. My plan is to export the ABI list path to the appropriate lit configs (i.e. libcxx/test/configs/cmake-bridge.cfg.in for libc++) so that they can be used as variables for the lit tests.

Thank you for looking at this! I have a (rough) prototype for everything up to the libc++ lit test, so I will include all those changes now and we can go through what works and what doesn't :)

francii updated this revision to Diff 518877.May 2 2023, 3:11 PM

Move files to runtimes/utils/

Herald added a project: Restricted Project. · View Herald TranscriptMay 2 2023, 3:11 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
francii updated this revision to Diff 518880.May 2 2023, 3:16 PM

Add --only-stdlib-symbols option to generate_abi_list.py

francii updated this revision to Diff 518885.May 2 2023, 3:25 PM

Create libc++ lit test

@ldionne I'd also like your input on how to handle the current generate-cxx-abilist target.

The current abi check works by calling this function:

function check-abi-list() {
    echo "+++ Running the libc++ ABI list test"
    ${NINJA} -vC "${BUILD_DIR}" check-cxx-abilist || (
        echo "+++ Generating the libc++ ABI list after failed check"
        ${NINJA} -vC "${BUILD_DIR}" generate-cxx-abilist
        false
    )
}

My understanding is, currently, we only generate the abi list after a failed abi check. Since we're removing the CMake target, we'll be removing this call as well, but as a result we cannot conditionally generate the abi list during the build (as we do currently). My ideas for alternatives are:

  1. Alter the function to just call generate-cxx-abilist. The CMake target will be updated to first check if there is an existing .abilist file before generating a new one.
  2. Remove this function entirely. If the lit test fails, the user can run the generate_abi_list.py file directly if they fail the test and want to break the ABI.
  3. In the lit test, call generate_abi_list.py if the abi check fails. We would need to consider how to mark the lit test as failure if this is the case.
francii added inline comments.May 2 2023, 3:45 PM
libcxx/test/libcxx/symbols/check_cxx_abilist.sh.cpp
1 ↗(On Diff #518885)

Is there a separate macro to refer to llvm home or something higher up? I wonder if there's a better way than
%S/../../../../runtimes/utils/
I believe I can get away with %{lib}/../runtimes/utils

francii retitled this revision from [libcxx] move abi symbol checker logic to generic location to [libcxx] convert symbol checker from CMake target to tit test.May 2 2023, 3:51 PM
francii edited the summary of this revision. (Show Details)
francii edited the summary of this revision. (Show Details)
francii updated this revision to Diff 518899.May 2 2023, 4:11 PM

Update CMakeLists.txt

francii added inline comments.May 2 2023, 4:16 PM
libcxx/lib/abi/CMakeLists.txt
79

If we go this route, the call to the check-cxx-abilist target will need to be removed from run-buildbot

s/tit/lit in title

francii retitled this revision from [libcxx] convert symbol checker from CMake target to tit test to [libcxx] convert symbol checker from CMake target to lit test.May 2 2023, 4:20 PM

s/tit/lit in title

Updated, thanks!

francii updated this revision to Diff 519085.May 3 2023, 8:27 AM

Update lit test

My understanding is, currently, we only generate the abi list after a failed abi check. Since we're removing the CMake target, we'll be removing this call as well, but as a result we cannot conditionally generate the abi list during the build (as we do currently). My ideas for alternatives are:

For context, we started generating the abi list after a failed check to make it easier for people to fix the test or update the ABI list on platforms they don't have access to.

  1. Remove this function entirely. If the lit test fails, the user can run the generate_abi_list.py file directly if they fail the test and want to break the ABI.

That's what I would do, but ideally we could actually diff the expected ABI list with the existing ABI list, and have the test print that diff. That way, if the test fails, one could simply apply the diff to the appropriate ABI lists.

libcxx/lib/abi/CMakeLists.txt
79

I think we should unconditionally have a generate-cxx-abilist target available. I'm not sure why we'd want to disable it sometimes?

libcxx/test/libcxx/symbols/check_cxx_abilist.sh.cpp
1 ↗(On Diff #518885)

You could use %{libcxx}/../runtimes/utils. Not perfect but a bit better. Don't use %{lib} though, it could be something way out of the source directory.

libcxx/utils/generate_abi_list.py
31–33

I just tried removing the filter_stdlib_symbols call below unconditionally, and here's what I found. This makes me realize that I think our current check is wrong currently. We disregard some symbols like ___muloti4 that *we* provide in our dylib because they don't match the regex in util.py, which is bad. I guess what I'm wondering here is why don't we just have a list of *all* the symbols that we export, end of the story. And do we really need to track the symbols that we refer to but don't export? Let's discuss, but I think we might want to refactor this before we make this move.

Here's the diff after re-generating my local ABI list:

diff --git a/libcxx/lib/abi/arm64-apple-darwin.libcxxabi.v1.stable.exceptions.nonew.abilist b/libcxx/lib/abi/arm64-apple-darwin.libcxxabi.v1.stable.exceptions.nonew.abilist
index 1c416134cf1f..15f8b24ba675 100644
--- a/libcxx/lib/abi/arm64-apple-darwin.libcxxabi.v1.stable.exceptions.nonew.abilist
+++ b/libcxx/lib/abi/arm64-apple-darwin.libcxxabi.v1.stable.exceptions.nonew.abilist
@@ -1,3 +1,5 @@
+{'is_defined': False, 'name': '__DefaultRuneLocale', 'type': 'U'}
+{'is_defined': False, 'name': '__Unwind_Resume', 'type': 'U'}
 {'is_defined': False, 'name': '__ZNKSt10bad_typeid4whatEv', 'type': 'U'}
 {'is_defined': False, 'name': '__ZNKSt11logic_error4whatEv', 'type': 'U'}
 {'is_defined': False, 'name': '__ZNKSt13bad_exception4whatEv', 'type': 'U'}
@@ -278,6 +280,9 @@
 {'is_defined': False, 'name': '__ZnwmRKSt9nothrow_t', 'type': 'U'}
 {'is_defined': False, 'name': '__ZnwmSt11align_val_t', 'type': 'U'}
 {'is_defined': False, 'name': '__ZnwmSt11align_val_tRKSt9nothrow_t', 'type': 'U'}
+{'is_defined': False, 'name': '____mb_cur_max_l', 'type': 'U'}
+{'is_defined': False, 'name': '____tolower_l', 'type': 'U'}
+{'is_defined': False, 'name': '____toupper_l', 'type': 'U'}
 {'is_defined': False, 'name': '___cxa_allocate_dependent_exception', 'type': 'U'}
 {'is_defined': False, 'name': '___cxa_allocate_exception', 'type': 'U'}
 {'is_defined': False, 'name': '___cxa_atexit', 'type': 'U'}
@@ -316,8 +321,150 @@
 {'is_defined': False, 'name': '___cxa_vec_new', 'type': 'U'}
 {'is_defined': False, 'name': '___cxa_vec_new2', 'type': 'U'}
 {'is_defined': False, 'name': '___cxa_vec_new3', 'type': 'U'}
+{'is_defined': False, 'name': '___divti3', 'type': 'U'}
 {'is_defined': False, 'name': '___dynamic_cast', 'type': 'U'}
+{'is_defined': False, 'name': '___error', 'type': 'U'}
 {'is_defined': False, 'name': '___gxx_personality_v0', 'type': 'U'}
+{'is_defined': False, 'name': '___maskrune_l', 'type': 'U'}
+{'is_defined': False, 'name': '___stack_chk_fail', 'type': 'U'}
+{'is_defined': False, 'name': '___stack_chk_guard', 'type': 'U'}
+{'is_defined': False, 'name': '___stderrp', 'type': 'U'}
+{'is_defined': False, 'name': '___stdinp', 'type': 'U'}
+{'is_defined': False, 'name': '___stdoutp', 'type': 'U'}
+{'is_defined': False, 'name': '___tolower', 'type': 'U'}
+{'is_defined': False, 'name': '___toupper', 'type': 'U'}
+{'is_defined': False, 'name': '___udivti3', 'type': 'U'}
+{'is_defined': False, 'name': '_abort', 'type': 'U'}
+{'is_defined': False, 'name': '_arc4random', 'type': 'U'}
+{'is_defined': False, 'name': '_asprintf_l', 'type': 'U'}
+{'is_defined': False, 'name': '_btowc_l', 'type': 'U'}
+{'is_defined': False, 'name': '_bzero', 'type': 'U'}
+{'is_defined': False, 'name': '_catclose', 'type': 'U'}
+{'is_defined': False, 'name': '_catgets', 'type': 'U'}
+{'is_defined': False, 'name': '_catopen', 'type': 'U'}
+{'is_defined': False, 'name': '_chdir', 'type': 'U'}
+{'is_defined': False, 'name': '_clock_gettime', 'type': 'U'}
+{'is_defined': False, 'name': '_close', 'type': 'U'}
+{'is_defined': False, 'name': '_closedir', 'type': 'U'}
+{'is_defined': False, 'name': '_copyfile_state_alloc', 'type': 'U'}
+{'is_defined': False, 'name': '_copyfile_state_free', 'type': 'U'}
+{'is_defined': False, 'name': '_dlopen', 'type': 'U'}
+{'is_defined': False, 'name': '_dlsym', 'type': 'U'}
+{'is_defined': False, 'name': '_fchmod', 'type': 'U'}
+{'is_defined': False, 'name': '_fchmodat', 'type': 'U'}
+{'is_defined': False, 'name': '_fclose', 'type': 'U'}
+{'is_defined': False, 'name': '_fcopyfile', 'type': 'U'}
+{'is_defined': False, 'name': '_fdopendir', 'type': 'U'}
+{'is_defined': False, 'name': '_fflush', 'type': 'U'}
+{'is_defined': False, 'name': '_fopen', 'type': 'U'}
+{'is_defined': False, 'name': '_fread', 'type': 'U'}
+{'is_defined': False, 'name': '_free', 'type': 'U'}
+{'is_defined': False, 'name': '_freelocale', 'type': 'U'}
+{'is_defined': False, 'name': '_fseek', 'type': 'U'}
+{'is_defined': False, 'name': '_fseeko', 'type': 'U'}
+{'is_defined': False, 'name': '_fstat', 'type': 'U'}
+{'is_defined': False, 'name': '_ftello', 'type': 'U'}
+{'is_defined': False, 'name': '_ftruncate', 'type': 'U'}
+{'is_defined': False, 'name': '_fwrite', 'type': 'U'}
+{'is_defined': False, 'name': '_getc', 'type': 'U'}
+{'is_defined': False, 'name': '_getcwd', 'type': 'U'}
+{'is_defined': False, 'name': '_getenv', 'type': 'U'}
+{'is_defined': False, 'name': '_gettimeofday', 'type': 'U'}
+{'is_defined': False, 'name': '_link', 'type': 'U'}
+{'is_defined': False, 'name': '_localeconv_l', 'type': 'U'}
+{'is_defined': False, 'name': '_lstat', 'type': 'U'}
+{'is_defined': False, 'name': '_malloc', 'type': 'U'}
+{'is_defined': False, 'name': '_mbrlen_l', 'type': 'U'}
+{'is_defined': False, 'name': '_mbrtowc_l', 'type': 'U'}
+{'is_defined': False, 'name': '_mbsnrtowcs_l', 'type': 'U'}
+{'is_defined': False, 'name': '_mbsrtowcs_l', 'type': 'U'}
+{'is_defined': False, 'name': '_mbtowc_l', 'type': 'U'}
+{'is_defined': False, 'name': '_memchr', 'type': 'U'}
+{'is_defined': False, 'name': '_memcmp', 'type': 'U'}
+{'is_defined': False, 'name': '_memcpy', 'type': 'U'}
+{'is_defined': False, 'name': '_memmove', 'type': 'U'}
+{'is_defined': False, 'name': '_memset', 'type': 'U'}
+{'is_defined': False, 'name': '_mkdir', 'type': 'U'}
+{'is_defined': False, 'name': '_nanosleep', 'type': 'U'}
+{'is_defined': False, 'name': '_newlocale', 'type': 'U'}
+{'is_defined': False, 'name': '_open', 'type': 'U'}
+{'is_defined': False, 'name': '_openat', 'type': 'U'}
+{'is_defined': False, 'name': '_opendir', 'type': 'U'}
+{'is_defined': False, 'name': '_pthread_cond_broadcast', 'type': 'U'}
+{'is_defined': False, 'name': '_pthread_cond_destroy', 'type': 'U'}
+{'is_defined': False, 'name': '_pthread_cond_signal', 'type': 'U'}
+{'is_defined': False, 'name': '_pthread_cond_timedwait', 'type': 'U'}
+{'is_defined': False, 'name': '_pthread_cond_wait', 'type': 'U'}
+{'is_defined': False, 'name': '_pthread_detach', 'type': 'U'}
+{'is_defined': False, 'name': '_pthread_getspecific', 'type': 'U'}
+{'is_defined': False, 'name': '_pthread_join', 'type': 'U'}
+{'is_defined': False, 'name': '_pthread_key_create', 'type': 'U'}
+{'is_defined': False, 'name': '_pthread_mutex_destroy', 'type': 'U'}
+{'is_defined': False, 'name': '_pthread_mutex_init', 'type': 'U'}
+{'is_defined': False, 'name': '_pthread_mutex_lock', 'type': 'U'}
+{'is_defined': False, 'name': '_pthread_mutex_trylock', 'type': 'U'}
+{'is_defined': False, 'name': '_pthread_mutex_unlock', 'type': 'U'}
+{'is_defined': False, 'name': '_pthread_mutexattr_destroy', 'type': 'U'}
+{'is_defined': False, 'name': '_pthread_mutexattr_init', 'type': 'U'}
+{'is_defined': False, 'name': '_pthread_mutexattr_settype', 'type': 'U'}
+{'is_defined': False, 'name': '_pthread_self', 'type': 'U'}
+{'is_defined': False, 'name': '_pthread_setspecific', 'type': 'U'}
+{'is_defined': False, 'name': '_readdir', 'type': 'U'}
+{'is_defined': False, 'name': '_readlink', 'type': 'U'}
+{'is_defined': False, 'name': '_realloc', 'type': 'U'}
+{'is_defined': False, 'name': '_realpath$DARWIN_EXTSN', 'type': 'U'}
+{'is_defined': False, 'name': '_remove', 'type': 'U'}
+{'is_defined': False, 'name': '_rename', 'type': 'U'}
+{'is_defined': False, 'name': '_sched_yield', 'type': 'U'}
+{'is_defined': False, 'name': '_setlocale', 'type': 'U'}
+{'is_defined': False, 'name': '_snprintf', 'type': 'U'}
+{'is_defined': False, 'name': '_snprintf_l', 'type': 'U'}
+{'is_defined': False, 'name': '_sscanf', 'type': 'U'}
+{'is_defined': False, 'name': '_sscanf_l', 'type': 'U'}
+{'is_defined': False, 'name': '_stat', 'type': 'U'}
+{'is_defined': False, 'name': '_statvfs', 'type': 'U'}
+{'is_defined': False, 'name': '_strcmp', 'type': 'U'}
+{'is_defined': False, 'name': '_strcoll_l', 'type': 'U'}
+{'is_defined': False, 'name': '_strerror_r', 'type': 'U'}
+{'is_defined': False, 'name': '_strftime_l', 'type': 'U'}
+{'is_defined': False, 'name': '_strlen', 'type': 'U'}
+{'is_defined': False, 'name': '_strtod', 'type': 'U'}
+{'is_defined': False, 'name': '_strtod_l', 'type': 'U'}
+{'is_defined': False, 'name': '_strtof', 'type': 'U'}
+{'is_defined': False, 'name': '_strtof_l', 'type': 'U'}
+{'is_defined': False, 'name': '_strtol', 'type': 'U'}
+{'is_defined': False, 'name': '_strtold', 'type': 'U'}
+{'is_defined': False, 'name': '_strtold_l', 'type': 'U'}
+{'is_defined': False, 'name': '_strtoll', 'type': 'U'}
+{'is_defined': False, 'name': '_strtoll_l', 'type': 'U'}
+{'is_defined': False, 'name': '_strtoul', 'type': 'U'}
+{'is_defined': False, 'name': '_strtoull', 'type': 'U'}
+{'is_defined': False, 'name': '_strtoull_l', 'type': 'U'}
+{'is_defined': False, 'name': '_strxfrm_l', 'type': 'U'}
+{'is_defined': False, 'name': '_swprintf', 'type': 'U'}
+{'is_defined': False, 'name': '_symlink', 'type': 'U'}
+{'is_defined': False, 'name': '_sysconf', 'type': 'U'}
+{'is_defined': False, 'name': '_truncate', 'type': 'U'}
+{'is_defined': False, 'name': '_ungetc', 'type': 'U'}
+{'is_defined': False, 'name': '_unlinkat', 'type': 'U'}
+{'is_defined': False, 'name': '_utimensat', 'type': 'U'}
+{'is_defined': False, 'name': '_vfprintf', 'type': 'U'}
+{'is_defined': False, 'name': '_vsnprintf', 'type': 'U'}
+{'is_defined': False, 'name': '_wcrtomb_l', 'type': 'U'}
+{'is_defined': False, 'name': '_wcscoll_l', 'type': 'U'}
+{'is_defined': False, 'name': '_wcslen', 'type': 'U'}
+{'is_defined': False, 'name': '_wcsnrtombs_l', 'type': 'U'}
+{'is_defined': False, 'name': '_wcstod', 'type': 'U'}
+{'is_defined': False, 'name': '_wcstof', 'type': 'U'}
+{'is_defined': False, 'name': '_wcstol', 'type': 'U'}
+{'is_defined': False, 'name': '_wcstold', 'type': 'U'}
+{'is_defined': False, 'name': '_wcstoll', 'type': 'U'}
+{'is_defined': False, 'name': '_wcstoul', 'type': 'U'}
+{'is_defined': False, 'name': '_wcstoull', 'type': 'U'}
+{'is_defined': False, 'name': '_wcsxfrm_l', 'type': 'U'}
+{'is_defined': False, 'name': '_wctob_l', 'type': 'U'}
+{'is_defined': False, 'name': '_wmemchr', 'type': 'U'}
+{'is_defined': False, 'name': '_wmemcmp', 'type': 'U'}
 {'is_defined': True, 'name': '__ZNKSt10bad_typeid4whatEv', 'type': 'I'}
 {'is_defined': True, 'name': '__ZNKSt11logic_error4whatEv', 'type': 'I'}
 {'is_defined': True, 'name': '__ZNKSt12bad_any_cast4whatEv', 'type': 'FUNC'}
@@ -2526,4 +2673,5 @@
 {'is_defined': True, 'name': '___cxa_vec_new2', 'type': 'I'}
 {'is_defined': True, 'name': '___cxa_vec_new3', 'type': 'I'}
 {'is_defined': True, 'name': '___dynamic_cast', 'type': 'I'}
-{'is_defined': True, 'name': '___gxx_personality_v0', 'type': 'I'}
\ No newline at end of file
+{'is_defined': True, 'name': '___gxx_personality_v0', 'type': 'I'}
+{'is_defined': True, 'name': '___muloti4', 'type': 'FUNC'}
\ No newline at end of file
Herald added a project: Restricted Project. · View Herald TranscriptMay 3 2023, 12:51 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
francii added inline comments.May 3 2023, 1:49 PM
libcxx/lib/abi/CMakeLists.txt
79

Based on my understanding, does it make sense to move the condition inside the target instead of removing it?:

Let's say after a build, you always generate the .abilist file (overwriting the existing one). When you run the lit test, you will be comparing the newly-built library against the .abilist file that was generated from the same library. Wouldn't this test always pass?

Hence why I kept the condition from the original file. This way, the lit test will compare the new library against the existing abi list file, and if the developer wants to update the abi list file they can run the script manually. Alternatively, we can update the lit test to do the generation if the ABI has changed.

libcxx/utils/generate_abi_list.py
31–33

Great catch!
I'm going to throw the ball to @daltenty to help reach a solid answer on this question:

And do we really need to track the symbols that we refer to but don't export?

But I'm inclined to say that we don't need to track them.

francii updated this revision to Diff 519243.May 3 2023, 1:58 PM

Update CMake target and run-buildbot function

ldionne added inline comments.May 4 2023, 2:02 PM
libcxx/lib/abi/CMakeLists.txt
79

We always want the custom target to exist, but we don't always want to run it. We should indeed only run it manually, but it should always be defined.

I agree that it doesn't make sense to run the generate-cxx-abilist target before running the test, but we're not doing that right now (nor in this patch AFAICT).

francii updated this revision to Diff 519724.May 4 2023, 7:35 PM

Update sym_diff.py path in lit test

francii marked 2 inline comments as done.May 5 2023, 3:33 PM
francii marked an inline comment as done.May 10 2023, 7:33 AM

And do we really need to track the symbols that we refer to but don't export?

I'm going to throw the ball to @daltenty to help reach a solid answer on this question. But I'm inclined to say that we don't need to track them.

I was asking myself the same thing when we originally added the ABI list support for AIX. I agree, I don't think we really care too much about imports which aren't exports, they don't really affect the ABI surface of the library.

Not to sure about the history of this filtering business either. Wouldn't removing one of these filtered symbols still constitute an ABI break, so you'd want to know about it regardless?

Not to sure about the history of this filtering business either. Wouldn't removing one of these filtered symbols still constitute an ABI break, so you'd want to know about it regardless?

I mean, technically if we start depending on a symbol that we didn't depend on before, we could impact our ability to back-deploy to previous targets. However, I don't think that's really the intent of theses tests. What we're really trying to test is what the library is exporting.

And yeah, I don't think we should filter out any of the symbols we export, since they are all part of our ABI.

Stepping back, I think adding ABI testing to libc++abi and libunwind should hence not reuse the logic for libc++. I think we should do it "the right way" using llvm-ifs or similar, and then we can actually refactor the libc++ tests to use the same mechanism instead of the other way around. That's just a suggestion on how to proceed, however it doesn't seem logical to me to do work towards sharing the libc++ test mechanism with libc++abi when we know the libc++ mechanism is wrong.

francii added inline comments.May 18 2023, 9:39 PM
libcxx/utils/generate_abi_list.py
31–33

I've posted a separate patch to refactor the scripts. Let's try to land that refactor before we continue with this patch.

https://reviews.llvm.org/D150691

Please give me your thoughts and let me know what functionality you'd like to see added/removed.

francii updated this revision to Diff 531007.Jun 13 2023, 11:31 AM

Rebase onto patch for symbol checking refactor.

francii updated this revision to Diff 544465.Jul 26 2023, 11:54 AM

Rebase onto main

utils/sym_check/diff.py