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

[francii]

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).

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).

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.

[francii]

Update CMakeLists.txt

[francii]

Rebase

[francii]

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

[ldionne]

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!

[francii]

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]

Move files to runtimes/utils/

[francii]

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

[francii]

Create libc++ lit test

[francii]

@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]

libcxx/test/libcxx/symbols/check_cxx_abilist.sh.cpp

2

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]

Update CMakeLists.txt

[francii]

libcxx/lib/abi/CMakeLists.txt

58–68

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

[paulkirth]

s/tit/lit in title

[francii]

In D148753#4314067, @paulkirth wrote:

s/tit/lit in title

Updated, thanks!

[francii]

Update lit test

[ldionne]

In D148753#4313917, @francii wrote:

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.

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.

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

58–68

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

2

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.

runtimes/utils/generate_abi_list.py

31–33 ↗

(On Diff #519085)

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

[francii]

libcxx/lib/abi/CMakeLists.txt

58–68

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.

runtimes/utils/generate_abi_list.py

31–33 ↗

(On Diff #519085)

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]

Update CMake target and run-buildbot function

[ldionne]

libcxx/lib/abi/CMakeLists.txt

58–68

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]

Update sym_diff.py path in lit test

[daltenty]

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?

[ldionne]

In D148753#4332277, @daltenty wrote:

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]

runtimes/utils/generate_abi_list.py

31–33 ↗

(On Diff #519085)

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]

Rebase onto patch for symbol checking refactor.

[francii]

Rebase onto main

libcxx/CMakeLists.txt

if (LIBCXX_ENABLE_STD_MODULES)
   list(APPEND LIBCXX_TEST_DEPS generate-cxx-modules generate-test-module-std)
 endif()
 
 if (LIBCXX_INCLUDE_BENCHMARKS)
   add_subdirectory(benchmarks)
 endif()
 
 if (LIBCXX_INCLUDE_TESTS)
-  add_subdirectory(test)
   add_subdirectory(lib/abi)
+  add_subdirectory(test)
 endif()
 
 if (LIBCXX_INCLUDE_DOCS)
   add_subdirectory(docs)
 endif()

libcxx/lib/abi/CMakeLists.txt

cxx_abi_list_identifier(abi_list_identifier
   "${LIBCXX_CXX_ABI}"
   "${LIBCXX_ABI_VERSION}"
   "${LIBCXX_ABI_UNSTABLE}"
   "${LIBCXX_ENABLE_EXCEPTIONS}"
   "${LIBCXX_ENABLE_NEW_DELETE_DEFINITIONS}"
 )
 
 if (TARGET cxx_shared)
-  set(abi_list_file "${CMAKE_CURRENT_SOURCE_DIR}/${abi_list_identifier}.abilist")
-
-  if (EXISTS "${abi_list_file}")
-    add_custom_target(check-cxx-abilist
-      "${Python3_EXECUTABLE}" "${LIBCXX_SOURCE_DIR}/utils/sym_diff.py"
-          --only-stdlib-symbols
-          --strict "${abi_list_file}"
-          $<TARGET_FILE:cxx_shared>
-      DEPENDS cxx_shared
-      COMMENT "Testing libc++'s exported symbols against the ABI list")
-  else()
-    message(STATUS "ABI list file not generated for configuration ${abi_list_identifier}, `check-cxx-abilist` will not be available.")
-  endif()
-
+ set(abi_list_file "${CMAKE_CURRENT_SOURCE_DIR}/${abi_list_identifier}.abilist" PARENT_SCOPE)
    add_custom_target(generate-cxx-abilist
-    COMMAND "${Python3_EXECUTABLE}" "${LIBCXX_SOURCE_DIR}/utils/generate_abi_list.py"
+     if (NOT EXISTS "${abi_list_file}")
+       COMMAND "${Python3_EXECUTABLE}" "${LIBCXX_SOURCE_DIR}/../runtimes/utils/generate_abi_list.py"
                 --output "${abi_list_file}"
                 "$<TARGET_FILE:cxx_shared>"
        DEPENDS cxx_shared
        COMMENT "Generating the ABI list file for configuration ${abi_list_identifier}")
      else()
-  message(STATUS "Not building a shared library for libc++ -- the ABI list targets will not be available.")
+       message(STATUS "Not building the ABI list targets - file already exists.")
+     endif()
 endif()

libcxx/test/configs/cmake-bridge.cfg.in

 config.substitutions.append(('%{cxx}', shlex.quote('@CMAKE_CXX_COMPILER@')))
 config.substitutions.append(('%{libcxx}', '@LIBCXX_SOURCE_DIR@'))
 config.substitutions.append(('%{include}', '@LIBCXX_GENERATED_INCLUDE_DIR@'))
 config.substitutions.append(('%{target-include}', '@LIBCXX_GENERATED_INCLUDE_TARGET_DIR@'))
 config.substitutions.append(('%{lib}', '@LIBCXX_LIBRARY_DIR@'))
 config.substitutions.append(('%{module}', '@LIBCXX_GENERATED_MODULE_DIR@'))
 config.substitutions.append(('%{executor}', '@LIBCXX_EXECUTOR@'))
 config.substitutions.append(('%{test-tools}', '@LIBCXX_TEST_TOOLS_PATH@'))
+config.substitutions.append(('%{abi-list-file}', '@abi_list_file@'))
+config.substitutions.append(('%{shared-lib-name}', 'lib@LIBCXX_SHARED_OUTPUT_NAME@@CMAKE_SHARED_LIBRARY_SUFFIX@'))
 
 # The test needs to manually rebuild the module. The compiler flags used in the
 # test need to be the same as the compiler flags used to generate the module.
 # In the future, when CMake can generated modules this may no longer be
 # necessary.
 # TODO MODULES whether it's possible to remove this substitution.
 config.substitutions.append(('%{cmake}', '@CMAKE_COMMAND@'))

libcxx/test/libcxx/symbols/check_cxx_abilist.sh.cpp

+// RUN: %{python} %{libcxx}/../runtimes/utils/sym_diff.py \
+// RUN: --only-stdlib-symbols \
+// RUN: --strict \
+// RUN: %{abi-list-file} \
+// RUN: %{lib}/%{shared-lib-name} \
+// RUN: 2>&1

libcxx/utils/ci/run-buildbot

function check-runtimes() {
 
     echo "+++ Running the libc++abi tests"
     ${NINJA} -vC "${BUILD_DIR}" check-cxxabi
 
     echo "+++ Running the libunwind tests"
     ${NINJA} -vC "${BUILD_DIR}" check-unwind
 }
 
-# TODO: The goal is to test this against all configurations. We should also move
-#       this to the Lit test suite instead of being a separate CMake target.
-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
-    )
+function generate-abi-list() {
+    echo "+++ Generating the libc++ ABI list"
 }
 
 function check-cxx-benchmarks() {
     echo "--- Running the benchmarks"
     ${NINJA} -vC "${BUILD_DIR}" check-cxx-benchmarks
 }
 
 # Print the version of a few tools to aid diagnostics in some cases

 ;;
 #
 # Various Standard modes
 #
 generic-cxx03)
     clean
     generate-cmake -C "${MONOREPO_ROOT}/libcxx/cmake/caches/Generic-cxx03.cmake"
     check-runtimes
-    check-abi-list
+    generate-abi-list
 ;;
 generic-cxx11)
     clean
     generate-cmake -C "${MONOREPO_ROOT}/libcxx/cmake/caches/Generic-cxx11.cmake"
     check-runtimes
-    check-abi-list
+    generate-abi-list
 ;;
 generic-cxx14)
     clean
     generate-cmake -C "${MONOREPO_ROOT}/libcxx/cmake/caches/Generic-cxx14.cmake"
     check-runtimes
-    check-abi-list
+    generate-abi-list
 ;;
 generic-cxx17)
     clean
     generate-cmake -C "${MONOREPO_ROOT}/libcxx/cmake/caches/Generic-cxx17.cmake"
     check-runtimes
-    check-abi-list
+    generate-abi-list
 ;;
 generic-cxx20)
     clean
     generate-cmake -C "${MONOREPO_ROOT}/libcxx/cmake/caches/Generic-cxx20.cmake"
     check-runtimes
-    check-abi-list
+    generate-abi-list
 ;;
 generic-cxx23)
     clean
     generate-cmake -C "${MONOREPO_ROOT}/libcxx/cmake/caches/Generic-cxx23.cmake"
     check-runtimes
-    check-abi-list
+    generate-abi-list
 ;;
 generic-cxx26)
     clean
     generate-cmake -C "${MONOREPO_ROOT}/libcxx/cmake/caches/Generic-cxx26.cmake"
     check-runtimes
-    check-abi-list
+    generate-abi-list
 ;;
 #
 # Other compiler support
 #
 generic-gcc)
     clean
     generate-cmake -DLIBCXX_ENABLE_WERROR=NO \
                    -DLIBCXXABI_ENABLE_WERROR=NO \
generate-cmake -C "${MONOREPO_ROOT}/libcxx/cmake/caches/Generic-merged.cmake" \
                    -DLIBCXXABI_TEST_CONFIG="llvm-libc++abi-merged.cfg.in" \
                    -DLIBUNWIND_TEST_CONFIG="llvm-libunwind-merged.cfg.in"
     check-runtimes
 ;;
 generic-hardened-mode)
     clean
     generate-cmake -C "${MONOREPO_ROOT}/libcxx/cmake/caches/Generic-hardened-mode.cmake"
     check-runtimes
-    check-abi-list
+    generate-abi-list
 ;;
 generic-debug-mode)
     clean
     generate-cmake -C "${MONOREPO_ROOT}/libcxx/cmake/caches/Generic-debug-mode.cmake"
     check-runtimes
     check-abi-list
 ;;
 generic-with_llvm_unwinder)
     clean
     generate-cmake -DLIBCXXABI_USE_LLVM_UNWINDER=ON
     check-runtimes
 ;;
 #
 # Module builds
 #
 generic-modules)
     clean
     generate-cmake -C "${MONOREPO_ROOT}/libcxx/cmake/caches/Generic-modules.cmake"
     check-runtimes
-    check-abi-list
+    generate-abi-list
 ;;
 generic-modules-lsv)
     clean
     generate-cmake -C "${MONOREPO_ROOT}/libcxx/cmake/caches/Generic-modules-lsv.cmake"
     check-runtimes
-    check-abi-list
+    generate-abi-list
 ;;
 generic-module-std-cxx23)
     clean
     generate-cmake -C "${MONOREPO_ROOT}/libcxx/cmake/caches/Generic-module-std-cxx23.cmake"
     check-runtimes
     check-abi-list
 ;;
 #
generic-no-wide-characters)
     clean
     generate-cmake -C "${MONOREPO_ROOT}/libcxx/cmake/caches/Generic-no-wide-characters.cmake"
     check-runtimes
 ;;
 generic-no-experimental)
     clean
     generate-cmake -C "${MONOREPO_ROOT}/libcxx/cmake/caches/Generic-no-experimental.cmake"
     check-runtimes
-    check-abi-list
+    generate-abi-list
 ;;
 generic-no-exceptions)
     clean
     generate-cmake -C "${MONOREPO_ROOT}/libcxx/cmake/caches/Generic-no-exceptions.cmake"
     check-runtimes
-    check-abi-list
+    generate-abi-list
 ;;
 #
 # Other miscellaneous jobs
 #
 generic-abi-unstable)
     clean
     generate-cmake -C "${MONOREPO_ROOT}/libcxx/cmake/caches/Generic-abi-unstable.cmake"
     check-runtimes
mingw-dll-i686)
     check-runtimes
 ;;
 aix)
     clean
     generate-cmake -C "${MONOREPO_ROOT}/libcxx/cmake/caches/AIX.cmake" \
                    -DLIBCXX_TEST_CONFIG="ibm-libc++-shared.cfg.in" \
                    -DLIBCXXABI_TEST_CONFIG="ibm-libc++abi-shared.cfg.in" \
                    -DLIBUNWIND_TEST_CONFIG="ibm-libunwind-shared.cfg.in"
-    check-abi-list
+    generate-abi-list
     check-runtimes
 ;;
 #################################################################
 # Insert vendor-specific internal configurations below.
 #
 # This allows vendors to extend this file with their own internal
 # configurations without running into merge conflicts with upstream.
 #################################################################
 
 #################################################################
 *)
     echo "${BUILDER} is not a known configuration"
     exit 1
 ;;
 esac