Essential fixes for EVMC Java bindings #532
Conversation
atoulme
force-pushed
the
evmc_java_fixes
branch
4 times, most recently
from
35150cb
to
0c9eb8e
Compare
bindings/java/java/src/test/java/org/ethereum/evmc/TestHostContext.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Nice job. A few comments:
-
you found that the call to
ReleaseByteArrayElements()inget_balance_fn()segfaults. I would think thatget_code_hash_fn()andcopy_code_fn()would have a similar problem. -
wdyt about opening an issue for better code coverage? if my initial unit tests were more complete, then 👆 would have been caught sooner, right?
-
small nit about removing DEBUG printouts (not really my call though)
|
you found that the call to ReleaseByteArrayElements() in get_balance_fn() segfaults. I would think that get_code_hash_fn() and copy_code_fn() would have a similar problem.
wdyt about opening an issue for better code coverage? if my initial unit tests were more complete, then 👆 would have been caught sooner, right?
small nit about removing DEBUG printouts (not really my call though)
|
|
debug prints are removed. In the next PR I run all VM tests and remove other |
|
I'm sorry folks it will take a bit longer for us to do a review, a bit busy on a different project. |
chfast
changed the title
axic
force-pushed
the
evmc_java_fixes
branch
from
af89a51
to
a5352ee
Compare
| @@ -77,7 +77,7 @@ static ByteBuffer copy_code(int context_index, byte[] address, int code_offset) | |||
| "HostContext does not exist for context_index: " + context_index); | |||
| byte[] code = context.getCode(address).array(); | |||
|
|
|||
| if (code != null && code_offset > 0 && code_offset < code.length) { | |||
There was a problem hiding this comment.
Is this change still relevant? It seems to have been missed in #535.
There was a problem hiding this comment.
Right - I can't remember if that change is required. I left it out. It doesn't hurt to check for those.
There was a problem hiding this comment.
OK I remember now. The code can never be null, since it's a byte array extracted from a ByteBuffer object. The code_offset > 0 should be removed imo since none of the other host impls check for that.
There was a problem hiding this comment.
Actually this entire copy_code_fn and it's Java counterpart is broken. It never copies anything back, that's also why the FIXME for buffer_size is there.
I suggest this is fixed independently.
There was a problem hiding this comment.
I suggest this is left to be done in #545.
| // load so containing the jni bindings to evmc | ||
| System.load(System.getProperty("user.dir") + "/../c/build/lib/libevmc-java.so"); | ||
| EvmcVm.isEvmcLibraryLoaded = true; | ||
| } catch (UnsatisfiedLinkError e) { |
There was a problem hiding this comment.
Couldn't System.load still emit this exception? Shouldn't create specify throws Exception?
There was a problem hiding this comment.
System.load does not throw this exception explicitly, so it doesn't have to be caught there. It can be caught by the code creating the EVM to try different paths in succession. The current code would run System.exit(1) if the lib was not found, which is a big problem if you're trying different paths.
Note also System.load is idempotent, so there's no need to check if it's loaded or not.
There was a problem hiding this comment.
It does seem to throw exceptions: https://docs.oracle.com/javase/10/docs/api/java/lang/System.html#load(java.lang.String)
There was a problem hiding this comment.
The exceptions are documented in the Javadoc but not in the signature of the method.
|
Ready for another look. |
axic
force-pushed
the
evmc_java_fixes
branch
2 times, most recently
from
b4b447b
to
a2bb261
Compare
| evmcVm = new EvmcVm(filename); | ||
| } | ||
| return evmcVm; | ||
| public static EvmcVm create(String evmcPath, String filename) { |
There was a problem hiding this comment.
Why aren't we following the old pattern of calling load once statically?
public final class EvmcVm {
static {
System.loadLibrary("libevmc.so");
}
There was a problem hiding this comment.
Say you do this - this means the static block runs at the time the class is loaded.
That means less control as to when you perform the load. If the static block throws, the class can never be loaded.
A behavior I have seen in Java is this:
https://www.adamh.cz/blog/2012/12/how-to-load-native-jni-library-from-jar/
Basically, your java jar ships with the so/dylib/dll file embedded. On startup, it copies the file out to a temp directory. Then it loads from it.
There was a problem hiding this comment.
Exactly that seems to be the best approach. libemvc-java.so is not something the user should supply or load multiple times, it is the binding.
| @@ -146,7 +146,8 @@ static synchronized int addContext(HostContext context) { | |||
| } | |||
|
|
|||
| static void removeContext(int index) { | |||
| contextList.remove(index); | |||
| HostContext context = contextList.remove(index); | |||
| context.close(); | |||
There was a problem hiding this comment.
Why not mark HostContext as AutoCloseable and then this is a given?
There was a problem hiding this comment.
AutoCloseable allows you to write syntactic sugar in java with the try-with-resources approach:
https://docs.oracle.com/javase/tutorial/essential/exceptions/tryResourceClose.html
Doesn't really apply here. Also that close method of AutoCloseable throws an Exception in its signature, so you'd have to deal with a try/catch.
There was a problem hiding this comment.
Actually after #548 this is not needed.
| } | ||
| return evmcVm; | ||
| public static EvmcVm create(String evmcPath, String filename) { | ||
| System.load(evmcPath); |
There was a problem hiding this comment.
I think it is a bad idea to keep reloading this. I'd imagine that messes up the bound addresses and may result in a crash.
A set of critical fixes for the Java bindings, removing the cruft of other fixes I added elsewhere.