New test threadcxx to test std::thread functionality in the enclave #612
Conversation
|
Anand/Mike/Simon: This is ready for your review. Thanks. |
tests/threadcxx/host/host.cpp
Outdated
| OE_TEST( | ||
| oe_call_enclave(enclave, "WaitForExclusiveAccessCxx", NULL) == | ||
| OE_OK); | ||
| // std::this_thread::sleep_for(std::chrono::microseconds(30 * 1000)); |
There was a problem hiding this comment.
Remove is not needed?
Does the test work if this is uncommented?
There was a problem hiding this comment.
Actually, the test works better without the extra sleep after WaitForExclusiveAccess and RelinquishExclusiveAccess. Will fix this.
CodeMonkeyLeet
added
the
testing
tests/threadcxx/enc/private.pem
Outdated
| @@ -0,0 +1,39 @@ | |||
| -----BEGIN RSA PRIVATE KEY----- | |||
There was a problem hiding this comment.
Avoid checking in new test signing keys/sign.conf and use the OE_SET_ENCLAVE_SGX macro instead unless you're explicitly testing signing. See tests\getenclave and others like it.
There was a problem hiding this comment.
The OE_SET_ENCLAVE_SGX macro does not work with cpp files. All the tests that use these are .c files.
There was a problem hiding this comment.
The compiler error is:
'visibility' attribute ignored [-Werror=attributes].
With cc1plus: all warnings being treated as errors
There was a problem hiding this comment.
I suspect the error happens because in C++ const objects by default have internal linkage (as if the "static" keyword was added to the declaration automatically). Therefore GCC specific visibility attributes are ignored so as to be C++ conformant.
To fix the compile error, we just need to add the "export" keyword to the variable being declared via the macro:
OE_EXPORT extern const oe_sgx_enclave_properties_t oe_enclavePropertiesSGX = \
There was a problem hiding this comment.
OK, implemented this. Thanks.
| oe_call_enclave(enclave, "WaitForExclusiveAccessCxx", NULL) == | ||
| OE_OK); | ||
|
|
||
| OE_TEST( |
There was a problem hiding this comment.
I was comparing this with the tests/thread implementation and noticed you took out the thread sleep here. It looks like the sleep was introduced to force threads to wait (i.e. it increases the window where a thread is causing other threads to wait) Did you find that it was unnecessary?
There was a problem hiding this comment.
Did you find out why the test was failing if we add the wait?
There was a problem hiding this comment.
Will need to investigate some more.
There was a problem hiding this comment.
@CodeMonkeyLeet - In my case, adding the wait times causes a lib panic assert in libcxxrt_stubs.c. Debugging to see why this happens.
There was a problem hiding this comment.
With oe-gdb, the lib panic assert never occurred as the timing of the threads is considerably changed. But able to reproduce this relatively easily using ctest -V. Will add the oe_print_backtrace new feature to get a stack trace.
There was a problem hiding this comment.
I also tried a test of removing these wait times in the original thread test and noticed that it never failed (ran it in 100+ times).
There was a problem hiding this comment.
My CoffeeLake lab machine is down today and I am unable to reproduce the failure (when the sleep is added) on Azure VM CoffeeLake and KabyLake.
| oe_call_enclave(enclave, "WaitForExclusiveAccessCxx", NULL) == | ||
| OE_OK); | ||
|
|
||
| OE_TEST( |
There was a problem hiding this comment.
Did you find out why the test was failing if we add the wait?
|
Addressed another round of feedback from Anand. |
|
Anand: The code has been merged with master and ready for your review. The failing case when the sleep is added in ExclusiveAccessThread is not reproducible on other machines and the lab machines are down until EOW. So, great if you can review this PR. Thanks. |
|
Fixed the minor formatting issue and also tested in all 3 modes (Debug, Release and RelWithDebInfo) on a KabyLake VM as CI KabyLake machines are also down. |
…612) * New test cthreadcxx to verify std::thread functionality in enclave * Standardized enclave printf to std::cout * Changed mtx to recursive_mutex * Extended TestMutexCxx to use 2 recursive mutexes * Changed cond_variable_any to cond_variable * Incorporated review comments - following RA11 paradigm * Modified enc.cpp to use condition_variable with unique_lock/lock_guard * Using OE_SET_ENCLAVE_SGX macro
New test threadcxx to excercise C++11 synchronization primitives using std::thread in enclaves. This was necessary as the OE thread APIs are now private. (Issue #476 )
Test was enhanced to use C++ RAII paradigm to benefit from the exception-safe resource management and avoid deadlocks/starvation seen with multi-threading.