-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
Experimental mechanism for loading Z3 dynamically at runtime. #10483
Conversation
420f701
to
ecdb16a
Compare
libsmtutil/z3/wrapper.cpp
Outdated
sym(c, h); | ||
} | ||
|
||
ResultType<&Z3_algebraic_is_value> Z3_API Z3_algebraic_is_value(ArgType<&Z3_algebraic_is_value, 0> _0, ArgType<&Z3_algebraic_is_value, 1> _1) |
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.
By the way: IMHO these are just beautiful :-D... resp. C++17 is for allowing me to do this.
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.
The mechanism is by the way pretty fool-proof. If any of these entry points were to result in an incompatible signature: compile-time error in the reinterpret_cast
(ambiguous overloads in decltype
). If any of them are missing: link-time error: undefined reference. As weird as it may look, I can't imagine what could go wrong with it :-).
This is actually nice. |
The drawbacks I can think of are:
So there are downsides, but I'm not sure they are all that horrible. Am I missing any? |
I think those are fine, especially because we would see them before releasing (well, 1 we know statically, so this is more related to 2) |
The thing is rather that old historic binaries would stop working with 2 and we'd have to rebuild them. But I'd be surprised if 2 happened without any compatibility wrapper as long as x86_64 lives... |
The linked binary segfaults for me (when I use smt). Also I get
|
The CircleCI link seems to point somewhere weird - maybe better to fetch it from the |
Ah nice! That one seems to work with z3, but if I move libz3.so out of the way, I get the following
|
Oh that's weird, without z3 I get
|
With the latest commit, Z3 being absent should work as expected. |
Alpine, probably. Though I'm not sure - it has I tried it in Docker and I am getting errors. Here's how to reproduce. Get the container and executablesmkdir /tmp/alpine/
cd /tmp/alpine/
docker pull alpine
curl -OL https://github.com/ethereum/solidity/releases/download/v0.7.5/solc-static-linux
curl -L https://532552-40892817-gh.circle-artifacts.com/0/solc --output solc-dynamic
chmod +x solc-static-linux solc-dynamic Static build of 0.7.5docker run -it --rm --volume /tmp/alpine:/tmp/ alpine ldd /tmp/solc-static-linux
docker run -it --rm --volume /tmp/alpine:/tmp/ alpine /tmp/solc-static-linux --version Output:
Dynamic builds from this PRdocker run -it --rm --volume /tmp/alpine:/tmp/ alpine /tmp/solc-dynamic --version
docker run -it --rm --volume /tmp/alpine:/tmp/ alpine ldd /tmp/solc-dynamic Output:
|
Of that I'm aware, that's expected - alpine linux falls under "exotic embedded systems" in #10483 (comment) ;-). |
The one I downloaded from most recent run of
Works when Z3 is installed. |
Is it that exotic though? We've been still using it for building not that long ago. And I generally use it for containers if I can get away with it because it's very lightweight. |
Yes, it is :-). At least thinking of it as a distribution that you run, download software into and expect that to run. And it was also rather exotic that we built it in those before :-). And in general a container like that is expected to build it's own stuff and can't expect anything to just work in it. If we continue to build a docker image (do we still do that?), then that'll be a different build. So to me at least this seems to be of rather limited concern in general. Let me look at the segfaulting thing, I probably messed something up in the force pushes... |
Still need to bump cmake and docs? |
Curious - I was sure I bumped the docs as well earlier, but apparently I was imagining that or it was somehow lost again. Pushed it now. |
It would probably be good to have more approvals before merging |
@ekpyron Sorry for not getting back to you on this today. Too many meetings + stuff happening around external repos. It's a bit late now so I'll re-review it tomorrow. |
No worries ;-)! |
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.
A few more general things. I don't see any issues with the dynamic linking mechanism itself. If it works, it's fine and better than permanently not having Z3.
576bb02
to
6020b2f
Compare
Also needed to rebase. |
If you guys approve, give me a chance to squash before merging :-)! |
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.
LGTM. Please squash and ping to reapprove
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.
Looks fine to me now.
I'd still prefer to have a proper template rather than a Python-C++ hybrid (#10483 (comment)) but it's a very peripheral part of the PR so I approve anyway.
Alright - I also simultanously just squashed it (ping @leonardoalt). And yeah, I see the point about the template, but to me it still seems nicer to have everything in one file - it's not like it's exceedingly complex python logic there, and this way you don't need to switch files to get an idea of what's going to be generated there... but yeah, thanks for approving it like this contrary to your preference then ;-). |
Ok, so this is as close as we will get.
The "static" binary built by the
b_ubu_static
CI here now dynamically loads Z3 - it's available as CI artifact(https://531866-40892817-gh.circle-artifacts.com/0/solc).[Link doesn't seem to work]The resulting binary is in fact slightly (but insignificantly) smaller than the previous static build. However, it's not entirely static anymore:
I was strongly arguing for trying to achive entirely static binaries before today, but ;-)... all what remains here
libdl.so
,libm.so
andlibc.so
are all part ofglibc
. The binary should work as is against any glibc>=2.14 (released 2011). Some further minor trickery could even make it compatible down to glibc>=2.4 (released 2006) - beyond that it might be a hassle. Newer versions of glibc released in the future can reasonably be expected to stay compatible basically forever.So this is not entirely what we wanted, but I'm not sure it's all too bad.
The reason for entirely static linking not being possible is the fact that Z3 uses some thread-local storage variables and thread-local storage inherently depends on the dynamic linker and seems to work fundamentally different between static and dynamic linking - and to me it doesn't look like there is any workaround for that.
The PR is of course still draft, so not much sense to review the details, it's rather meant as proof-of-concept - e.g. we won't pull in the actual Z3 headers, but make them compile-time-only dependencies and also the CMake options need some refinement (e.g. errors for trying to do any of this on non-linux systems), etc.
On the other hand, note that there are hardly any changes to the existing code - the only real change is adding
Z3Interface::available()
, the rest is entirely transparent.Apart from all that, all tests were passed with this kind of linking and there is no noticable performance loss.
So yeah, opinions anyone :-)?
@leonardoalt @chriseth @axic @cameel ?