[libc++] Reject abilist if it contains an ABI tag #139030
Conversation
|
@llvm/pr-subscribers-libcxx Author: Nikolas Klauser (philnik777) ChangesWe currently don't have any ABI tags in our dylib symbols, and this is unlikely to change in the future. By diagnosing this we avoid accidentally adding one through e.g. having Full diff: https://github.com/llvm/llvm-project/pull/139030.diff 1 Files Affected:
diff --git a/libcxx/utils/sym_diff.py b/libcxx/utils/sym_diff.py
index 8eaf8b7a57591..91af4a0bddaa7 100755
--- a/libcxx/utils/sym_diff.py
+++ b/libcxx/utils/sym_diff.py
@@ -80,6 +80,11 @@ def main():
old_syms_list, _ = util.filter_stdlib_symbols(old_syms_list)
new_syms_list, _ = util.filter_stdlib_symbols(new_syms_list)
+ for symbol in new_syms_list:
+ if 'B' in symbol["name"]:
+ print(f"Symbol {symbol["name"]} contains an ABI tag!")
+ sys.exit(1)
+
added, removed, changed = diff.diff(old_syms_list, new_syms_list)
if args.removed_only:
added = {}
|
libcxx/utils/sym_diff.py
Outdated
| for symbol in new_syms_list: | ||
| if 'B' in symbol["name"]: | ||
| print(f"Symbol {symbol["name"]} contains an ABI tag!") | ||
| sys.exit(1) |
There was a problem hiding this comment.
Would it be possible to add this as a lit test instead? One thing we could potentially do is nm -g %{lib-dir}/libc++.dylib | grep -v 'B' or something along those lines?
There was a problem hiding this comment.
Since the name of the library changes depending on the platform this is non-trivial I think. Do you have any idea how to get that except passing it through as another lit substitution?
There was a problem hiding this comment.
That's right, this is kinda tricky. I had plans to migrate the ABI list tests to Lit at some point, but this is one of the difficulties I encountered too. I think this is good enough for now. For anyone curious: ldionne@060c7d6.
libcxx/utils/sym_diff.py
Outdated
| for symbol in new_syms_list: | ||
| if 'B' in symbol["name"]: | ||
| print(f"Symbol {symbol["name"]} contains an ABI tag!") | ||
| sys.exit(1) |
There was a problem hiding this comment.
That's right, this is kinda tricky. I had plans to migrate the ABI list tests to Lit at some point, but this is one of the difficulties I encountered too. I think this is good enough for now. For anyone curious: ldionne@060c7d6.
|
The CI is failing for real though. |
You can test this locally with the following command:darker --check --diff -r HEAD~1...HEAD libcxx/utils/sym_diff.pyView the diff from darker here.--- sym_diff.py 2025-05-15 11:28:16.000000 +0000
+++ sym_diff.py 2025-05-15 11:31:10.689848 +0000
@@ -79,11 +79,11 @@
if args.only_stdlib:
old_syms_list, _ = util.filter_stdlib_symbols(old_syms_list)
new_syms_list, _ = util.filter_stdlib_symbols(new_syms_list)
for symbol in new_syms_list:
- if symbol["is_defined"] and 'B' in symbol["name"]:
+ if symbol["is_defined"] and "B" in symbol["name"]:
print(f"Symbol {symbol['name']} contains an ABI tag!")
sys.exit(1)
added, removed, changed = diff.diff(old_syms_list, new_syms_list)
if args.removed_only:
|
Update libcxx/utils/sym_diff.py
af05bf8 to
6f7128c
Compare
We currently don't have any ABI tags in our dylib symbols, and this is unlikely to change in the future. By diagnosing this we avoid accidentally adding one through e.g. having
_LIBCPP_HIDE_FROM_ABIon an exported symbol.