Skip to content

[SGX] Use Fortanix EDP - #2885

Closed
nhynes wants to merge 19 commits into
apache:masterfrom
nhynes:sgx-new
Closed

[SGX] Use Fortanix EDP#2885
nhynes wants to merge 19 commits into
apache:masterfrom
nhynes:sgx-new

Conversation

@nhynes

@nhynesnhynes commented Mar 24, 2019

Copy link
Copy Markdown
Member

Depends on #2616

TODO:

  • remove old sgx
  • add new sgx example
  • test new sgx example in hardware
  • update dockerfile
  • update docs

This currently breaks direct use of python, but greatly simplifies the use of TVM in SGX. Future improvements include:

  • enclave as TVM RPC server
  • calling enclaves from Python using custom enclave runner

Notes:

Thanks to @paddyhoran for adding docs fixes in #4114

@nhynesnhynes self-assigned this Mar 24, 2019
@nhynes
nhynes requested a review from tqchenMarch 24, 2019 08:03
@tqchen

tqchen commented Mar 24, 2019

Copy link
Copy Markdown
Member

This seems to be a significant change, perhaps it would be better to open a RFC to elaborate the reasons behind the change. Specifically:

  • What is the reason to move away from the original rust SGX SDK, benefits
  • Dependency and license issue(The Fortanix one is MPL while the original one is BSD)
    • This might affect whether a user will want to use the tool
  • Would the original C++ example makes it easier to port to other enclave based runtime?

I am not expert but my observation is that our current way of SGX is "closer to the metal", which brings a bit more boilerplate code. This may or may not be a bad thing depending on how stable the code is and how can we evolve.

Comment threadCMakeLists.txt Outdated
@tqchen

Copy link
Copy Markdown
Member

ping @nhynes see if you would like to push it through

@nhynes

Copy link
Copy Markdown
MemberAuthor

Yes, absolutely! I'll give it a go this weekend.

@nhynes
nhynesforce-pushed the sgx-new branch 2 times, most recently from fe30058 to cd95366CompareJuly 29, 2019 02:37
@kaimast

Copy link
Copy Markdown

Whats the status of this branch? Any chance it's being merged soon?

@nhynes

Copy link
Copy Markdown
MemberAuthor

Any chance it's being merged soon?

Sure! I didn't know if anyone but me was keen on using it, so I've been keeping it WIP as the EDP and Rust evolve. I'll start tidying it up now for a merge hopefully this week.

Mind if I ask what you're working on?

@nhynes
nhynes marked this pull request as ready for review September 23, 2019 04:05
@kaimast

Copy link
Copy Markdown

Thanks for updating the branch.

Are you able to build this with the most recent rust nightly? I'm still running into this strange compile error. https://discuss.tvm.ai/t/linker-errors-when-compiling-rust-bindings/3712

@nhynes

Copy link
Copy Markdown
MemberAuthor

strange compile error

For some reason the frontend was being compiled as a dylib. This PR fixes that.

Although, you shouldn't need the frontend if you're just trying to use the Rust runtime.

@tqchen

Copy link
Copy Markdown
Member

gentle ping :)

@nhynes

Copy link
Copy Markdown
MemberAuthor

this was ready two weeks ago but nobody reviewed it 🤷‍♂

@tqchen

Copy link
Copy Markdown
Member

Aaha, my bad, would be great if we could bring in more reviewers. @kaimast@jroesch@jethrogb can you review?

did we re-enabled the rust test in CI already?

@kaimast

Copy link
Copy Markdown

I still get the build error mentioned above if I compile on the latest rust nightly even if I use the default toolchain.

@nhynes

Copy link
Copy Markdown
MemberAuthor

build error mentioned above if I compile on the latest rust nightly

🤦‍♂ I had a3c7f9c in a local branch

Should be fixed now. Also CI is re-enabled (@tqchen)

@jethrogb

Copy link
Copy Markdown

Latest Rust nightly broke a couple of targets (including SGX), so you have to use 2019-10-11 instead until fixed. See rust-lang/rust#65335

@kaimast

Copy link
Copy Markdown

Thanks! I think my issue was related to the tvm_runtime being linked to CUDA. When I disable CUDA in the C library the rust bindings from this branch compile fine.

@kaimast

Copy link
Copy Markdown

Could we add a short readme for the rust bindings? In particular which configurations are compatible with the bindings. It seems like CUDA does not work right now but you need to enable LLVM and have the python bindings installed.

Also the tests currently fail for me. I get this error

 Running target/debug/deps/tvm_frontend-88fe1e18fefaf9ac
running 14 tests
test context::tests::context ... ok
test context::tests::sync ... ok
test ndarray::tests::basics ... ok
test ndarray::tests::copy ... ok
test function::tests::get_fn ... ok
test tests::print_version ... ok
test function::tests::provide_args ... ok
test value::tests::bytearray ... ok
test value::tests::ctx ... ok
test value::tests::ty ... ok
test ndarray::tests::rust_ndarray ... ok
test function::tests::list_global_func ... FAILED
test tests::set_error ... ok
test ndarray::tests::copy_wrong_dtype ... ok
failures:
---- function::tests::list_global_func stdout ----
thread 'function::tests::list_global_func' panicked at 'assertion failed: GLOBAL_FUNCTIONS.lock().unwrap().contains_key(CANARY)', frontend/src/function.rs:435:9
stack backtrace:
0: 0x5605c34fbbb4 - backtrace::backtrace::libunwind::trace::hd2a3ca53354879f0
at /cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.37/src/backtrace/libunwind.rs:88
1: 0x5605c34fbbb4 - backtrace::backtrace::trace_unsynchronized::h496e207d7166606f
at /cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.37/src/backtrace/mod.rs:66
2: 0x5605c34fbbb4 - std::sys_common::backtrace::_print_fmt::hd61d624f6bb2bb89
at src/libstd/sys_common/backtrace.rs:77
3: 0x5605c34fbbb4 - <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt::hb4ae8693e17fcf38
at src/libstd/sys_common/backtrace.rs:61
4: 0x5605c351f02c - core::fmt::write::hc4e24880b860fe1a
at src/libcore/fmt/mod.rs:1028
5: 0x5605c3460ec5 - std::io::Write::write_fmt::h4a7df9e887c709dc
at /rustc/6767d9b90b6b630ad8e2a9e5e02fd74c00c98759/src/libstd/io/mod.rs:1412
6: 0x5605c34f6a81 - std::io::impls::<impl std::io::Write for alloc::boxed::Box<W>>::write_fmt::h0530ccc4898179fc
at src/libstd/io/impls.rs:141
7: 0x5605c34fe20e - std::sys_common::backtrace::_print::h98e6de9f5de4128a
at src/libstd/sys_common/backtrace.rs:65
8: 0x5605c34fe20e - std::sys_common::backtrace::print::h1c1c91c190041b46
at src/libstd/sys_common/backtrace.rs:50
9: 0x5605c34fe20e - std::panicking::default_hook::{{closure}}::hd22864b49281a625
at src/libstd/panicking.rs:189
10: 0x5605c34fdeb8 - std::panicking::default_hook::h10e51c24f5aee33c
at src/libstd/panicking.rs:203
11: 0x5605c34fe905 - std::panicking::rust_panic_with_hook::h80d9b79c9fbb3dd8
at src/libstd/panicking.rs:469
12: 0x5605c34b2083 - std::panicking::begin_panic::h997488de7c947fc3
at /rustc/6767d9b90b6b630ad8e2a9e5e02fd74c00c98759/src/libstd/panicking.rs:403
13: 0x5605c3448def - tvm_frontend::function::tests::list_global_func::hfe81f970038bca3c
at frontend/src/function.rs:435
14: 0x5605c345d59a - tvm_frontend::function::tests::list_global_func::{{closure}}::h665b62f071bdbcbf
at frontend/src/function.rs:434
15: 0x5605c343f71e - core::ops::function::FnOnce::call_once::h9108880aa06b81d2
at /rustc/6767d9b90b6b630ad8e2a9e5e02fd74c00c98759/src/libcore/ops/function.rs:227
16: 0x5605c346bb2f - <alloc::boxed::Box<F> as core::ops::function::FnOnce<A>>::call_once::ha3f63bbdea82aa69
at /rustc/6767d9b90b6b630ad8e2a9e5e02fd74c00c98759/src/liballoc/boxed.rs:932
17: 0x5605c350533a - __rust_maybe_catch_panic
at src/libpanic_unwind/lib.rs:80
18: 0x5605c3486143 - std::panicking::try::h30bf0c0bf4c64eee
at /rustc/6767d9b90b6b630ad8e2a9e5e02fd74c00c98759/src/libstd/panicking.rs:267
19: 0x5605c3486143 - std::panic::catch_unwind::he3fd0643a539afe4
at /rustc/6767d9b90b6b630ad8e2a9e5e02fd74c00c98759/src/libstd/panic.rs:394
20: 0x5605c3486143 - test::run_test_in_process::h74528df2fe86d886
at src/libtest/lib.rs:1626
21: 0x5605c3486143 - test::run_test::run_test_inner::{{closure}}::hd7621b2ce3c9a746
at src/libtest/lib.rs:1504
22: 0x5605c3460389 - std::sys_common::backtrace::__rust_begin_short_backtrace::hb7dc479db53dc0fc
at /rustc/6767d9b90b6b630ad8e2a9e5e02fd74c00c98759/src/libstd/sys_common/backtrace.rs:129
23: 0x5605c34646f9 - std::thread::Builder::spawn_unchecked::{{closure}}::{{closure}}::ha4c6fa7b35628276
at /rustc/6767d9b90b6b630ad8e2a9e5e02fd74c00c98759/src/libstd/thread/mod.rs:469
24: 0x5605c34646f9 - <std::panic::AssertUnwindSafe<F> as core::ops::function::FnOnce<()>>::call_once::h4cf7f01511f160a2
at /rustc/6767d9b90b6b630ad8e2a9e5e02fd74c00c98759/src/libstd/panic.rs:315
25: 0x5605c34646f9 - std::panicking::try::do_call::ha00a7db6e089b078
at /rustc/6767d9b90b6b630ad8e2a9e5e02fd74c00c98759/src/libstd/panicking.rs:288
26: 0x5605c350533a - __rust_maybe_catch_panic
at src/libpanic_unwind/lib.rs:80
27: 0x5605c3465085 - std::panicking::try::h2533d60687a22eb8
at /rustc/6767d9b90b6b630ad8e2a9e5e02fd74c00c98759/src/libstd/panicking.rs:267
28: 0x5605c3465085 - std::panic::catch_unwind::h88221a6d383044a8
at /rustc/6767d9b90b6b630ad8e2a9e5e02fd74c00c98759/src/libstd/panic.rs:394
29: 0x5605c3465085 - std::thread::Builder::spawn_unchecked::{{closure}}::he4cccbef2ebdda0e
at /rustc/6767d9b90b6b630ad8e2a9e5e02fd74c00c98759/src/libstd/thread/mod.rs:468
30: 0x5605c3465085 - core::ops::function::FnOnce::call_once{{vtable.shim}}::h0f906181cf2d118a
at /rustc/6767d9b90b6b630ad8e2a9e5e02fd74c00c98759/src/libcore/ops/function.rs:227
31: 0x5605c34f2c3f - <alloc::boxed::Box<F> as core::ops::function::FnOnce<A>>::call_once::h18eab40ca66f6bb8
at /rustc/6767d9b90b6b630ad8e2a9e5e02fd74c00c98759/src/liballoc/boxed.rs:932
32: 0x5605c3504a90 - <alloc::boxed::Box<F> as core::ops::function::FnOnce<A>>::call_once::ha65343f0105edac8
at /rustc/6767d9b90b6b630ad8e2a9e5e02fd74c00c98759/src/liballoc/boxed.rs:932
33: 0x5605c3504a90 - std::sys_common::thread::start_thread::h13af62b537e51d94
at src/libstd/sys_common/thread.rs:13
34: 0x5605c3504a90 - std::sys::unix::thread::Thread::new::thread_start::hf6265eca56b829ab
at src/libstd/sys/unix/thread.rs:79
35: 0x7fab6969f669 - start_thread
36: 0x7fab695ab323 - clone
37: 0x0 - <unknown>
failures:
function::tests::list_global_func
test result: FAILED. 13 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out
error: test failed, to rerun pass '-p tvm-frontend --lib'

@tqchen

Copy link
Copy Markdown
Member

sorry that that i lost track of this thread, @nhynes can you rebase and let us merge it in :)?

@tqchentqchen added the status: need update need update based on feedbacks label Dec 18, 2019
@ehsanmok

ehsanmok commented Dec 19, 2019

Copy link
Copy Markdown
Contributor

@kaimast if you look at how tests are run, you should use limit to --test-threads=1 because the default registered global functions are not accessible in a multi-threaded way. There's no requirement of python binding installed for testing the rust frontend, only if you want to run the resnet example.

self.arg_buf.iter().map(|arg| arg.to_tvm_value()).unzip();

let mut ret_val = unsafe { std::mem::uninitialized::<TVMValue>() };
let mut ret_val = unsafe { MaybeUninit::uninit().assume_init() };

@ehsanmokehsanmokDec 19, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! you fixed everything :) I was about to send a PR just for that!

@ehsanmok

Copy link
Copy Markdown
Contributor

@nhynes perhaps we should update to rustc >= 1.40.0.

@tqchen

Copy link
Copy Markdown
Member

ping ping :)

@tqchen

tqchen commented Jan 6, 2020

Copy link
Copy Markdown
Member

ping ping @nhynes , let us get this in the new decade :)

@tqchen

Copy link
Copy Markdown
Member

ping @nhynes

@paddyhoran

Copy link
Copy Markdown
Contributor

I just thought about this yesterday. I'm not sure I have time to contribute much but development on Rust can't continue without this getting merged as it's such a large PR.

@jroeschjroesch mentioned this pull request Mar 2, 2020
@tqchen

tqchen commented Mar 10, 2020

Copy link
Copy Markdown
Member

superseded by #4976

@tqchentqchen closed this Mar 10, 2020
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status: need reviewstatus: need updateneed update based on feedbacks

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@nhynes@tqchen@kaimast@jethrogb@ehsanmok@paddyhoran