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

Libsnark get_evaluation_domain works on macOS #1419

Merged
merged 5 commits into from Jan 10, 2019

Conversation

Projects
None yet
2 participants
@bkase
Copy link
Contributor

bkase commented Jan 10, 2019

Before this change on macOS systems, libsnark would crash on
get_root_of_unity and exception handlers would not work. The
hypothesis is that using exceptions for control flow in
get_evaluation_domain triggers some sort of UB (probably because this
runs statically before the main function). On Linux, the UB happens to
work properly, but on OSX it doesn't. As such, I rewrote all the
control-flow related exception handling in get_evaluation_domain to use
an error bool pointer.

I'll open a PR against libsnark directly when the review for this PR
finishes.

On macOS the Coda build succeeds only after these changes are made (as
we use libsnark at compile time). CI will tell us that the build still
works on Linux.

@@ -16,6 +16,11 @@
#include <libff/common/double.hpp>
#include <libff/common/utils.hpp>

#include <execinfo.h>

This comment has been minimized.

@cmr

cmr Jan 10, 2019

Contributor

unneeded

@cmr

This comment has been minimized.

Copy link
Contributor

cmr commented Jan 10, 2019

bool* should be bool&

bkase added some commits Jan 10, 2019

Libsnark get_evaluation_domain works on macOS
Before this change on macOS systems, libsnark would crash on
get_root_of_unity and exception handlers would not work. The
hypothesis is that using exceptions for control flow in
get_evaluation_domain triggers some sort of UB (probably because this
runs statically before the main function). On Linux, the UB happens to
work properly, but on OSX it doesn't. As such, I rewrote all the
control-flow related exception handling in get_evaluation_domain to use
an error bool pointer.

I'll open a PR against libsnark directly when the review for this PR
finishes.

@bkase bkase force-pushed the libsnark-macos-support branch from 52fc59f to 5b8b806 Jan 10, 2019

bkase added some commits Jan 10, 2019

@cmr

This comment has been minimized.

Copy link
Contributor

cmr commented Jan 10, 2019

There are ways to avoid initializing those fields on exit paths, but the code is already there, and it's the FieldTs that are expensive anyway. I'm fine with it. Thank you for not letting uninitialized data escape.

@cmr

cmr approved these changes Jan 10, 2019

@bkase bkase merged commit 750d8a3 into master Jan 10, 2019

9 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: build_public Your tests passed on CircleCI!
Details
ci/circleci: build_withsnark Your tests passed on CircleCI!
Details
ci/circleci: test-all_sig_integration_tests Your tests passed on CircleCI!
Details
ci/circleci: test-all_stake_integration_tests Your tests passed on CircleCI!
Details
ci/circleci: test-unit-test Your tests passed on CircleCI!
Details
ci/circleci: test-withsnark Your tests passed on CircleCI!
Details
ci/circleci: tracetool Your tests passed on CircleCI!
Details

@cmr cmr referenced this pull request Jan 10, 2019

Open

Run-time errors on OSX #13

@cmr cmr deleted the libsnark-macos-support branch Jan 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment