-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Remove FUNCTION_POINTER_ALIGNMENT setting #6091
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
Conversation
| function removeFunction(index) { | ||
| #if EMULATED_FUNCTION_POINTERS == 0 | ||
| functionPointers[(index-{{{ FUNCTION_POINTER_ALIGNMENT }}})/{{{ FUNCTION_POINTER_ALIGNMENT }}}] = null; | ||
| functionPointers[index-1] = null; |
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.
hmm, this line should just be removed, I think - it was only valid when FUNCTION_POINTER_ALIGNMENT was 2. When it's 1 it will null out something in a dangerous way, unless I'm missing something.
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.
(it wasn't even valid before, if I understand it correctly - it just happened to not cause any harm...)
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.
Why? addFunction fills up the functionPointers array from 0 but it returns the actual index + 1, and when the given index is 1, removeFunction would just delete the function added in functionPointers[0]. I think the reason addFunction returns index + 1 is in asm.js every function table (each with different signature) has a null function at its index 0.
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.
Yes, I think you're right.
But that means that a few lines down, the tables[sig][index] = null; is wrong, it should have been index - 1 all along?
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.
Oh, I guess not, the indexing is just different there.
|
Thanks, landing. I don't think we need to tag a new version for this fastcomp change as I don't see breakage locally with new emscripten but old fastcomp, but we should probably tag just to be safe. There is some current unrelated breakage though which I'd rather not tag because of (18af894 and 99e4fc3 ) |
FUNCTION_POINTER_ALIGNMENT was removes from emscripten in January 2018: emscripten-core/emscripten#6091
FUNCTION_POINTER_ALIGNMENT was removes from emscripten in January 2018: emscripten-core/emscripten#6091
Use -fstack-protector-strong instead of -fstack-protector when available.
Move the SGX checks
Check if SGX is enabled before testing for any headers (like pthread).
Check the right headers when in SGX mode.
Neither signal.h nor sys/mman.h are present in the SGX version of libc.
Add some SGX-specific code for thread-safety.
Use SGX multi-threading code for the critical sections guards.
Fix the RNG for SGX
Use SGX API (sgx_read_rand) to generate random bytes.
We still need to figure something out for the SGX simulation mode (the current code is insecure in this mode).
Remove some function checks for SGX builds.
We know some of them won't be available, and for a reason I do not understand, they still might me detected by autoconf.
Fix the Salsa20 RNG for SGX.
Use sgx_read_rand to see the stream cipher generating the random bytes.
Embed bitcode in iOS builds
Disastrous typo
Beginning to develop SGX unit tests
Do not put .so files in the source tree.
Improve the autoconf and automate files.
What we modify with SGX when compiling the library is the include path, so let's restrict ourselves to only change CPPFLAGS during the configuration.
A specific SGX_CPPFLAGS variable is created and used during the compilation of the library. Note that it is empty by default.
A first functional test (aead_aes256_gcm).
The other tests should be easy to construct from this base.
Unifying the test Makefile
Try to have a Makefile that differs only a little between the SGX and non-SGX mode.
Fix an issue with the include path.
The code determining wether AES-NI is supported or not was bugged.
Tests compile for both the SGX and regular target
Correctly build test apps for SGX
Add explicit rules to build the enclaves.
It happens that make rules with pattern do not work with automake (they are not portable).
Having explicit rules seems to work, but is ugly. It would be cool to automatically generate them using autoconf.
All tests compile, many (but not all) pass.
Fix the rand() function for SGX
Fix the sodium_core and sodium_utils{2,3} test.
All the tests now pass on an SGX build. Though it is not clear that sodium_utils3 actually tests anything on SGX.
Fix autoconf
Fix the rand() placeholder for SGX
rand() has to return a non negative value.
Increase the maximum heap size of the test enclaves.
Test programs used to allocate more memory than allowed by the enclave, and thus miserably failed.
Remove obsolete setting FUNCTION_POINTER_ALIGNMENT
FUNCTION_POINTER_ALIGNMENT was removes from emscripten in January 2018:
emscripten-core/emscripten#6091
Clean up the automake file
Use a suffix rule to compile the enclaves.
Automatically generate the enclaves filenames
Distribute the new files required by an SGX build
Properly clean enclaves
Fixed build command pretty-printing
Remove commented code
Link against the right SGX runtime libraries when in hardware mode
Add a configure option and fix the automake file
Automate does not support ifeq statements: they are not copied to the the generated makefile(s). Instead, use regular if statement with conditions defined in configure.ac.
As a consequence, add new a configuration option build the test programs in hardware mode when SGX is enabled.
Fix the distcheck target
enclave_u.h and enclave_u.c are intermediates that should
not be included in the distribution.
Also, because I fixed a typo in the test/default/Makefile.am file
(*_SOURCE -> *_SOURCES for the test targets), I had to define quirks.h source
dependencies as nodist_EXTRA. Note that quirks/quirks.h is already included in
the distribution by test/Makefile.am, so this is not a problem. Yet, it could
simplify things if we put quirks.h in the same directory as the other files
used by the test.
Stupid typo...
It is sgx_hw_tests (with an 's') and not sgx_hw_test (without the 's').
Change the enclave sources from the nodist_EXTRA_ to the nodist_ section.
Using EXTRA apparently prevents make from compiling the intermediates.
Make test_app.c explicitely depend on enclave_u.h
Otherwise, make might not create enclave_u.h on time,
before compiling test_app.c, causing a compilation error.
Typo
Add logging to enclave tests.
This should allow for better debugging of the enclave tests.
Immediately flush the log messages to sdtout.
Just in case we have an error killing the program before the
sdout buffer is flushed ...
Replace sodium_malloc by a memory pool for enclave tests.
sodium_malloc (and most of the memory management functions of utils.h)
use syscalls, are not supported inside an enclave (it raises a
SIGILL), which means that, to run the tests inside an enclave, we must allocate
the memory otherwise. I just reused the code of cmptest.h in benchmark
mode.
Also, the sodium_utils3 test is now disabled during an SGX build.
Closes #1692 and fixes #6068.
Should land with emscripten-core/emscripten-fastcomp#212.