-
Notifications
You must be signed in to change notification settings - Fork 14.3k
[clang][python][test] Check if libclang.so is loadable #142353
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
When building a 32-bit `clang` on a 64-bit system (like `i686-pc-linux-gnu` on a Linux/x86_64 system), `ninja check-all` fails: ``` FAILED: tools/clang/bindings/python/tests/CMakeFiles/check-clang-python tools/clang/bindings/python/tests/CMakeFiles/check-clang-python cd clang/bindings/python && /usr/bin/cmake -E env CLANG_NO_DEFAULT_CONFIG=1 CLANG_LIBRARY_PATH=lib /usr/bin/python3.11 -m unittest discover EEEEEEEE ``` and stops with `exit 1`. Further investigation shows that, `python3.11`, a 64-bit binary, tries to load the freshly build 32-bit `libclang.so`, which cannot work, thus breaking the build. Rather than trying to second-guess this situation, which seems very fragile, it's better to actually handle this situation when trying the load, which is what this patch does. The exact error message from `cdll.LoadLibrary` differs between systems: - On Linux, you get ``` clang.cindex.LibclangError: lib/libclang.so: wrong ELF class: ELFCLASS32. ``` while - on Solaris, there's ``` clang.cindex.LibclangError: ld.so.1: python3.11: lib/libclang.so: wrong ELF class: ELFCLASS32. ``` To allow for both cases, this patch just looks for the common `"wrong ELF class: ELFCLASS32"`. Tested on `amd64-pc-solaris2.11`, `i386-pc-solaris2.11`, `sparcv9-sun-solaris2.11`, `sparc-sun-solaris2.11`, `x86_64-pc-linux-gnu`, and `i686-pc-linux-gnu`.
@llvm/pr-subscribers-clang Author: Rainer Orth (rorth) ChangesWhen building a 32-bit
and stops with Further investigation shows that, Rather than trying to second-guess this situation, which seems very fragile, it's better to actually handle this situation when trying the load, which is what this patch does. The exact error message from
To allow for both cases, this patch just looks for the common Tested on Full diff: https://github.com/llvm/llvm-project/pull/142353.diff 1 Files Affected:
diff --git a/clang/bindings/python/clang/cindex.py b/clang/bindings/python/clang/cindex.py
index 6f7243cdf80ac..b4e5898223f0f 100644
--- a/clang/bindings/python/clang/cindex.py
+++ b/clang/bindings/python/clang/cindex.py
@@ -4370,12 +4370,17 @@ def get_cindex_library(self) -> CDLL:
try:
library = cdll.LoadLibrary(self.get_filename())
except OSError as e:
- msg = (
- str(e) + ". To provide a path to libclang use "
- "Config.set_library_path() or "
- "Config.set_library_file()."
- )
- raise LibclangError(msg)
+ if "wrong ELF class: ELFCLASS32" in str(e):
+ print("warning: skipping check-clang-python"
+ " since libclang cannot be loaded", file=sys.stderr)
+ os._exit(0)
+ else:
+ msg = (
+ str(e) + ". To provide a path to libclang use "
+ "Config.set_library_path() or "
+ "Config.set_library_file()."
+ )
+ raise LibclangError(msg)
return library
|
You can test this locally with the following command:darker --check --diff -r HEAD~1...HEAD clang/bindings/python/clang/cindex.py View the diff from darker here.--- cindex.py 2025-06-02 09:45:33.000000 +0000
+++ cindex.py 2025-06-02 09:52:37.367072 +0000
@@ -4369,12 +4369,15 @@
def get_cindex_library(self) -> CDLL:
try:
library = cdll.LoadLibrary(self.get_filename())
except OSError as e:
if "wrong ELF class: ELFCLASS32" in str(e):
- print("warning: skipping check-clang-python"
- " since libclang cannot be loaded", file=sys.stderr)
+ print(
+ "warning: skipping check-clang-python"
+ " since libclang cannot be loaded",
+ file=sys.stderr,
+ )
os._exit(0)
else:
msg = (
str(e) + ". To provide a path to libclang use "
"Config.set_library_path() or "
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR. Several points:
- What happens on Windows?
check-clang-python
is just one of the ways to run into this.cindex.py
is a user-facing interface, so we shouldn't emit messages that make sense only during development.- Given the fact that
OSError
already contains the message that indicates the problem, I think we should explore why it's not properly emitted in the first place, before introducing bespoke ways of delivering error messages. @DeinAlptraum any thoughts here?
I have no idea, knowing nothing at all about Windows. I cannot even tell if a similar situation can arise there, i.e. if you can execute 32-bit programs on a 64-bit Windows and, if you can, what the message would be if you tried. However, fixing an issue on one set of targets shouldn't force me to fix it everywhere: if/when such a situation arises elsewhere, any interested party can augment the code as necessary. Btw., Darwin could be similarly affected, but the last Darwin versions that could build and execute 32-bit programs are so old that I have no idea if they are still supported in LLVM.
I tried several different routes:
Besides, one could just change the wording of the warning message: trying to load a 32-bit
Even if the error message were emitted, this doesn't help because IMO the testsuite integration of
|
This has been bothering me for a while but I didn't think too much about it. Took this opportunity to finally take a look and opened a fix PR #142371 . |
That's why you shouldn't return an exit code of 0 when this error occurs.
Wouldn't it be a better solution to wrap the Probably not relevant then, but is there a reason you called |
That might work, though I'm not sure if this also works on Windows or non-Unix systems in general. Alternatively, as I suggested, one could wrap the actual
initially until I realized that doing this at One problem with all these approaches, as I'd already mentioned, is that the test outcome isn't properly reported, nor can one use the usual machinery to
I tried that at first, but got the |
I am unfortunately not at all familiar with how exactly the other tests are integrated and produce the outputs you described:
and couldn't find much on it either, but that sounds like
is pretty much the way to go here: wrap the You mentioned the problem of checking loadability at cmake time, but is this really necessary? Such a wrapper script could still parse the failure exceptions and emit an |
This PR changed the output from completely silent to excessively verbose: I now get
158 times total. While one can see what's going on, there's no way to avoid the issue. |
Indeed: one should move the test directory to (say)
Not at all: I just mentioned that attempt to describe why something along the lines of |
FWIW I now have a prototype of the python tests within the
|
We generally avoid XFAIL tests, instead we do normal tests where we expect the wrong behavior and leave a TODO. |
As discussed in PR llvm#142353, the current testsuite of the `clang` Python bindings has several issues: - It `libclang.so` cannot be loaded into `python` to run the testsuite, the whole `ninja check-all` aborts. - The result of running the testsuite isn't report like the `lit`-based tests, rendering them almost invisible. - The testsuite is disabled in a non-obvious way (`RUN_PYTHON_TESTS`) in `tests/CMakeLists.txt`, which again doesn't show up in the test results. All these issues can be avoided by integrating the Python bindings tests with `lit`, which is what this patch does: - The actual test lives in `clang/test/bindings/python/bindings.sh` and is run by `lit`. - The current `clang/bindings/python/tests` directory (minus the now-subperfluous `CMakeLists.txt`) is moved into the same directory. - The check if `libclang` is loadable (originally from PR llvm#142353) is now handled via a new `lit` feature, `libclang-loadable`. - The various ways to disable the tests have been turned into `XFAIL`s as appropriate. This isn't complete and not completely tested yet. Tested on `sparc-sun-solaris2.11`, `sparcv9-sun-solaris2.11`, `i386-pc-solaris2.11`, `amd64-pc-solaris2.11`, `i686-pc-linux-gnu`, and `x86_64-pc-linux-gnu`.
I've now created PR #142948 with an initial implementation of |
Superceded by PR #142948. |
As discussed in PR llvm#142353, the current testsuite of the `clang` Python bindings has several issues: - It `libclang.so` cannot be loaded into `python` to run the testsuite, the whole `ninja check-all` aborts. - The result of running the testsuite isn't report like the `lit`-based tests, rendering them almost invisible. - The testsuite is disabled in a non-obvious way (`RUN_PYTHON_TESTS`) in `tests/CMakeLists.txt`, which again doesn't show up in the test results. All these issues can be avoided by integrating the Python bindings tests with `lit`, which is what this patch does: - The actual test lives in `clang/test/bindings/python/bindings.sh` and is run by `lit`. - The current `clang/bindings/python/tests` directory (minus the now-subperfluous `CMakeLists.txt`) is moved into the same directory. - The check if `libclang` is loadable (originally from PR llvm#142353) is now handled via a new `lit` feature, `libclang-loadable`. - The various ways to disable the tests have been turned into `XFAIL`s as appropriate. This isn't complete and not completely tested yet. Tested on `sparc-sun-solaris2.11`, `sparcv9-sun-solaris2.11`, `i386-pc-solaris2.11`, `amd64-pc-solaris2.11`, `i686-pc-linux-gnu`, and `x86_64-pc-linux-gnu`.
As discussed in PR llvm#142353, the current testsuite of the `clang` Python bindings has several issues: - It `libclang.so` cannot be loaded into `python` to run the testsuite, the whole `ninja check-all` aborts. - The result of running the testsuite isn't report like the `lit`-based tests, rendering them almost invisible. - The testsuite is disabled in a non-obvious way (`RUN_PYTHON_TESTS`) in `tests/CMakeLists.txt`, which again doesn't show up in the test results. All these issues can be avoided by integrating the Python bindings tests with `lit`, which is what this patch does: - The actual test lives in `clang/test/bindings/python/bindings.sh` and is run by `lit`. - The current `clang/bindings/python/tests` directory (minus the now-subperfluous `CMakeLists.txt`) is moved into the same directory. - The check if `libclang` is loadable (originally from PR llvm#142353) is now handled via a new `lit` feature, `libclang-loadable`. - The various ways to disable the tests have been turned into `XFAIL`s as appropriate. This isn't complete and not completely tested yet. Tested on `sparc-sun-solaris2.11`, `sparcv9-sun-solaris2.11`, `i386-pc-solaris2.11`, `amd64-pc-solaris2.11`, `i686-pc-linux-gnu`, and `x86_64-pc-linux-gnu`.
…2948) As discussed in PR #142353, the current testsuite of the `clang` Python bindings has several issues: - If `libclang.so` cannot be loaded into `python` to run the testsuite, the whole `ninja check-all` aborts. - The result of running the testsuite isn't report like the `lit`-based tests, rendering them almost invisible. - The testsuite is disabled in a non-obvious way (`RUN_PYTHON_TESTS`) in `tests/CMakeLists.txt`, which again doesn't show up in the test results. All these issues can be avoided by integrating the Python bindings tests with `lit`, which is what this patch does: - The actual test lives in `clang/test/bindings/python/bindings.sh` and is run by `lit`. - The current `clang/bindings/python/tests` directory (minus the now-superfluous `CMakeLists.txt`) is moved into the same directory. - The check if `libclang` is loadable (originally from PR #142353) is now handled via a new `lit` feature, `libclang-loadable`. - The various ways to disable the tests have been turned into `XFAIL`s as appropriate. This isn't complete and not completely tested yet. - It keeps the `check-clang-python` target for use by the Clang Python CI. Tested on `sparc-sun-solaris2.11`, `sparcv9-sun-solaris2.11`, `i386-pc-solaris2.11`, `amd64-pc-solaris2.11`, `i686-pc-linux-gnu`, and `x86_64-pc-linux-gnu`.
When building a 32-bit
clang
on a 64-bit system (likei686-pc-linux-gnu
on a Linux/x86_64 system),ninja check-all
fails:and stops with
exit 1
.Further investigation shows that,
python3.11
, a 64-bit binary, tries to load the freshly build 32-bitlibclang.so
, which cannot work, thus breaking the build.Rather than trying to second-guess this situation, which seems very fragile, it's better to actually handle this situation when trying the load, which is what this patch does. The exact error message from
cdll.LoadLibrary
differs between systems:clang.cindex.LibclangError: lib/libclang.so: wrong ELF class: ELFCLASS32.
whileclang.cindex.LibclangError: ld.so.1: python3.11: lib/libclang.so: wrong ELF class: ELFCLASS32.
To allow for both cases, this patch just looks for the common
"wrong ELF class: ELFCLASS32"
.Tested on
amd64-pc-solaris2.11
,i386-pc-solaris2.11
,sparcv9-sun-solaris2.11
,sparc-sun-solaris2.11
,x86_64-pc-linux-gnu
, andi686-pc-linux-gnu
.