[libcxx-commits] [PATCH] D148753: [libcxx] convert symbol checker from CMake target to lit test

Louis Dionne via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Wed May 3 12:52:00 PDT 2023 

ldionne added a comment.
Herald added a project: libunwind.
Herald added a reviewer: libunwind.

In D148753#4313917 <https://reviews.llvm.org/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.



================
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
----------------
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?


================
Comment at: libcxx/test/libcxx/symbols/check_cxx_abilist.sh.cpp:1
+// RUN: %{python} %S/../../../../utils/sym_diff.py \
+// RUN: --only-stdlib-symbols \
----------------
francii wrote:
> 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`
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.


================
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)
----------------
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
```



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