[libcxx-commits] [PATCH] D148753: [libcxx] move abi symbol checker logic to generic location

Tue May 2 13:53:35 PDT 2023

ldionne added a comment.

> 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!

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148753