Page MenuHomePhabricator

Include Python binding tests in CMake rules
ClosedPublic

Authored by mgorny on Oct 3 2018, 11:43 AM.

Diff Detail

Repository
rC Clang

Event Timeline

steveire accepted this revision.Oct 3 2018, 2:03 PM
steveire added a subscriber: steveire.

I didn't run it, but CMake-wise this looks fine. (I trust you did)

I notice some tests use

from tests.cindex.util import foo

while others use

from .util import foo

You might want to make that consistent in a follow-up.

This revision is now accepted and ready to land.Oct 3 2018, 2:03 PM
mgorny added a comment.Oct 3 2018, 8:08 PM

Yes, I've tested it on Linux. I'll commit it once the dep is approved, and hopefully build bots will answer if they work across all platforms.

This revision was automatically updated to reflect the committed changes.

I'm afraid this may have broken many buildbots, I'm having issues building from scratch. Can you please take a look?

mgorny updated this revision to Diff 169218.Oct 11 2018, 8:23 AM

v2: fixed appending to check-all to make it compatible both with in-tree and standalone builds

mgorny updated this revision to Diff 169219.Oct 11 2018, 8:26 AM

v3: one more correction for stand-alone builds.

mgorny reopened this revision.Oct 11 2018, 8:26 AM
This revision is now accepted and ready to land.Oct 11 2018, 8:26 AM
mgorny requested review of this revision.Oct 11 2018, 8:27 AM
mgorny added a reviewer: aaron.ballman.

Please review the new version, addressing the issue caught by buildbots.

aaron.ballman accepted this revision.Oct 11 2018, 9:01 AM

I'm not a CMake expert, but when I apply this patch locally, I'm able to run cmake to completion without errors, so LGTM!

This revision is now accepted and ready to land.Oct 11 2018, 9:01 AM
This revision was automatically updated to reflect the committed changes.

This causes check-all to abort for me on SystemZ in Release mode (and never actually run the lit tests):

[40/365] cd /home/uweigand/llvm/llvm-head/tools/clang/bindings/python && /usr/bin/cmake -E...BRARY_PATH=/home/uweigand/llvm/build/llvm-head/lib /usr/bin/python2.7 -m unittest discover
FAILED: tools/clang/bindings/python/tests/CMakeFiles/check-clang-python 
cd /home/uweigand/llvm/llvm-head/tools/clang/bindings/python && /usr/bin/cmake -E env CLANG_LIBRARY_PATH=/home/uweigand/llvm/build/llvm-head/lib /usr/bin/python2.7 -m unittest discover
Invalid CXChildVisitResult!
UNREACHABLE executed at /home/uweigand/llvm/llvm-head/tools/clang/tools/libclang/CIndex.cpp:233!
Child aborted

When running in Debug mode I see instead:

[165/536] cd /home/uweigand/llvm/llvm-head/tools/clang/bindings/python && /usr/bin/cmake -...PATH=/home/uweigand/llvm/build/llvm-head-debug/lib /usr/bin/python2.7 -m unittest discover
..............................................................LIBCLANG TOOLING ERROR: fixed-compilation-database: Error while opening fixed database: No such file or directory
json-compilation-database: Error while opening JSON database: No such file or directory

..........................................................
----------------------------------------------------------------------
Ran 120 tests in 8.522s

OK

which apparently doesn't count as failure, but still doesn't look quite right to me.

Is this some environment setup problem, or does this expose a real bug? It's certainly not good that running the remaining tests is completely aborted in Release mode ...

The first one seems to indicate that your libclang.so is broken in release mode (optimization error?). The second one is correct (some of the tests test for errors, and apparently don't silence the messages).

The first one seems to indicate that your libclang.so is broken in release mode (optimization error?). The second one is correct (some of the tests test for errors, and apparently don't silence the messages).

Ok, thanks for the clarification on the second point.

As to the first issue, it turned out to be more complicated. The wrong value results from this call in CursorVisitor::Visit

switch (Visitor(Cursor, Parent, ClientData)) {

This is a callback routine passed in from the caller, which in the case of the Python bindings means that an FFI closure is being called.

Now, due to a historical quirk in the FFI ABI, return values of integral type smaller than the register word size must be passed as the special "ffi_arg" type when using FFI (either for calls or for closures). It seems the Python 2.7 code that interfaces with FFI does not correctly respect this. As a result, it seems that when the closure returns a 32-bit value, it is not properly extended and the high bits of the return register contain garbage.

But that violates our ABI, which requires extension to full register size. And it seems the clang code, when built with optimization, does indeed rely on that ABI guarantee.

So there's a bug in Python (or depending on how you want to look at it, in libffi -- that part of the interface has also long been under-documented unfortunately), which hits on our platform. I'll see if I can do anything about that, but for now we'll probably want to disable those tests on SystemZ.

Hexagon clang builder has been failing since around the time of this commit as well:
http://lab.llvm.org:8011/builders/clang-hexagon-elf/builds/20617/steps/ninja%20check%201/logs/stdio

Thanks for all your reports. I have filed https://reviews.llvm.org/D53326 to disable the tests on all three known-broken arches.