Page MenuHomePhabricator

Don't try to run MCJIT/OrcJIT EH tests when C++ library is statically linked
ClosedPublic

Authored by phosek on Dec 14 2017, 8:46 PM.

Details

Summary

These tests assume availability of external symbols provided by the
C++ library, but those won't be available in case when the C++ library
is statically linked because lli itself doesn't need these.

This uses llvm-readobj -needed-libs to check if C++ library is linked as
shared library and exposes that information as a feature to lit.

Diff Detail

Repository
rL LLVM

Event Timeline

phosek created this revision.Dec 14 2017, 8:46 PM
phosek updated this revision to Diff 127196.Dec 15 2017, 3:10 PM
phosek edited the summary of this revision. (Show Details)
phosek updated this revision to Diff 127421.Dec 18 2017, 3:19 PM
beanz added a comment.Dec 18 2017, 4:39 PM

A few inline comments. Overall I like the direction.

cmake/modules/CheckCXXSharedLibrary.cmake
60 ↗(On Diff #127421)

Based on our IRC conversation I ran off and read CMake's sources. Looks like CMAKE_OBJDUMP is unset on Windows, and only set elsewhere if objdump is found by CMake.

Might be worth adding an early out at the beginning of this function to mark the check as failed if CMAKE_OBJDUMP is unset. In that case you should also probably log that the check failed to the CMake status, and log why in the error log.

65 ↗(On Diff #127421)

CHECK_RESULT and CHECK_ERROR are unused here, as well as LINK_RESULT, LINK_OUTPUT and LINK_ERROR.

Probably makes sense to write the value of all those variables out to log files in the event the link or objdump commands fail.

phosek updated this revision to Diff 127457.Dec 18 2017, 6:36 PM
phosek marked 2 inline comments as done.
phosek updated this revision to Diff 127978.Dec 21 2017, 6:54 PM
phosek edited the summary of this revision. (Show Details)

I have reworked this patch to use llvm-readobj relying on the -needed-libs which will be supported in ELF, COFF and Mach-O after D41529 and D41527 lands.

beanz accepted this revision.Jan 5 2018, 1:39 PM

This looks good to me. Please correct me if I'm wrong, but it also looks like you've written this patch so that it will work correctly even if it is landed before the llvm-readobj changes are landed. If that is the case go ahead and land this.

This revision is now accepted and ready to land.Jan 5 2018, 1:39 PM
phosek added a comment.Jan 5 2018, 2:22 PM

This looks good to me. Please correct me if I'm wrong, but it also looks like you've written this patch so that it will work correctly even if it is landed before the llvm-readobj changes are landed. If that is the case go ahead and land this.

It'd disable the MCJIT tests though because the cxx-shared-library wouldn't be available which is probably not something we want. However, D41527 is ready to land unless you have some other comments and D41529 has already landed so this could land immediately after.

This revision was automatically updated to reflect the committed changes.