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

Thu May 18 21:39:57 PDT 2023

francii added inline comments.

================
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)
----------------
francii wrote:
> 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.
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.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148753/new/

https://reviews.llvm.org/D148753