[libcxx-commits] [PATCH] D148753: [libcxx] convert symbol checker from CMake target to lit test
Michael Francis via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Wed May 3 13:49:22 PDT 2023
francii added inline comments.
================
Comment at: libcxx/lib/abi/CMakeLists.txt:60
- 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()
-
- add_custom_target(generate-cxx-abilist
- COMMAND "${Python3_EXECUTABLE}" "${LIBCXX_SOURCE_DIR}/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.")
+ if (NOT EXISTS "${abi_list_file}")
+ add_custom_target(generate-cxx-abilist
----------------
ldionne wrote:
> francii wrote:
> > If we go this route, the call to the `check-cxx-abilist` target will need to be removed from `run-buildbot`
> I think we should unconditionally have a `generate-cxx-abilist` target available. I'm not sure why we'd want to disable it sometimes?
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.
================
Comment at: runtimes/utils/generate_abi_list.py:31-33
+ parser.add_argument('--only-stdlib-symbols', dest='only_stdlib',
+ help='Filter all symbols not related to the stdlib',
+ action='store_true', default=False)
----------------
ldionne wrote:
> 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
> ```
>
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D148753/new/
https://reviews.llvm.org/D148753
More information about the libcxx-commits
mailing list