Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Essential fixes for EVMC Java bindings #532

Closed
wants to merge 2 commits into from
+43 −37
Closed

Essential fixes for EVMC Java bindings #532

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a6e145c
Essential fixes for EVMc Java bindings
atoulme Aug 3, 2020
308354b
make address(ByteBuffer) public
atoulme Aug 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions bindings/java/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@ $(OUT_DIR)/lib/libevmc-java.so:
mkdir -p $(BUILD_DIR)
(cd $(BUILD_DIR) && cmake $(SOURCE_DIR) -DCMAKE_INSTALL_PREFIX=$(OUT_DIR) -DEVMC_JAVA=ON -DJAVA_HOME=$(JAVA_HOME) -DEVMC_EXAMPLES=ON)
cmake --build $(OUT_DIR)/_cmake_build --target install
mkdir -p $(CURDIR)/java/src/test/resources
cp $(OUT_DIR)/lib/libevmc-java.so $(CURDIR)/java/src/test/resources/libevmc.so
cp $(OUT_DIR)/lib/libexample-vm.so $(CURDIR)/java/src/test/resources/libexample-vm.so
atoulme marked this conversation as resolved.
Show resolved Hide resolved

clean:
rm -rf $(OUT_DIR)
Expand Down
9 changes: 9 additions & 0 deletions bindings/java/c/evmc-vm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,15 @@ JNIEXPORT jint JNICALL Java_org_ethereum_evmc_EvmcVm_set_1option(JNIEnv* jenv,
return (jint)option_result;
}

JNIEXPORT jlong JNICALL Java_org_ethereum_evmc_EvmcVm_address(JNIEnv* jenv,
atoulme marked this conversation as resolved.
Show resolved Hide resolved
jclass jcls,
jobject buf)
{
(void)jcls;
void* p = (*jenv)->GetDirectBufferAddress(jenv, buf);
return (jlong)p;
}

JNIEXPORT jint JNICALL Java_org_ethereum_evmc_EvmcVm_get_1result_1size(JNIEnv* jenv, jclass jcls)
{
(void)jenv;
Expand Down
36 changes: 12 additions & 24 deletions bindings/java/java/src/main/java/org/ethereum/evmc/EvmcVm.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,38 +6,25 @@
import org.ethereum.evmc.EvmcLoaderException;

import java.nio.ByteBuffer;
import java.util.Objects;

/**
* The Java interface to the evm instance.
*
* <p>Defines the Java methods capable of accessing the evm implementation.
*/
public final class EvmcVm implements AutoCloseable {
private static EvmcVm evmcVm;
private static boolean isEvmcLibraryLoaded = false;
private ByteBuffer nativeVm;

/**
* This method loads the specified evm shared library and loads/initializes the jni bindings.
*
* @param evmcPath the path to the evmc shared library
* @param filename /path/filename of the evm shared object
* @throws EvmcLoaderException
*/
public static EvmcVm create(String filename) throws EvmcLoaderException {
if (!EvmcVm.isEvmcLibraryLoaded) {
try {
// 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) {
Copy link
Member

Choose a reason for hiding this comment

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

Couldn't System.load still emit this exception? Shouldn't create specify throws Exception?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

It does seem to throw exceptions: https://docs.oracle.com/javase/10/docs/api/java/lang/System.html#load(java.lang.String)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The exceptions are documented in the Javadoc but not in the signature of the method.

System.err.println("Native code library failed to load.\n" + e);
System.exit(1);
}
}
if (Objects.isNull(evmcVm)) {
evmcVm = new EvmcVm(filename);
}
return evmcVm;
public static EvmcVm create(String evmcPath, String filename) {
Copy link
Member

Choose a reason for hiding this comment

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

Why aren't we following the old pattern of calling load once statically?

public final class EvmcVm {
   static {
      System.loadLibrary("libevmc.so");
   }

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

System.load(evmcPath);
Copy link
Member

Choose a reason for hiding this comment

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

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.

return new EvmcVm(filename);
atoulme marked this conversation as resolved.
Show resolved Hide resolved
}

private EvmcVm(String filename) throws EvmcLoaderException {
Expand Down Expand Up @@ -153,14 +140,15 @@ public int set_option(String name, String value) {
private static native int get_result_size();

/**
* This method cleans up resources
*
* @throws Exception
* Utility method to get a bytebuffer address
* @param buffer the byte buffer to consider
* @return the byte buffer address
*/
public native long address(ByteBuffer buffer);

/** This method cleans up resources */
@Override
public void close() throws Exception {
public void close() {
destroy(nativeVm);
isEvmcLibraryLoaded = false;
evmcVm = null;
}
}
5 changes: 5 additions & 0 deletions bindings/java/java/src/main/java/org/ethereum/evmc/HostContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,4 +145,9 @@ public interface HostContext {
* @param topicCount The number of the topics. Valid values are between 0 and 4 inclusively.
*/
void emitLog(byte[] address, byte[] data, int dataSize, byte[][] topics, int topicCount);

/**
* Closes the context and frees all underlying resources.
*/
default void close() {};
atoulme marked this conversation as resolved.
Show resolved Hide resolved
}
27 changes: 14 additions & 13 deletions bindings/java/java/src/test/java/org/ethereum/evmc/EvmcTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,28 +9,29 @@
import org.junit.jupiter.api.Test;

final class EvmcTest {
private static final String libEvmcPath = EvmcTest.class.getResource("/libevmc.so").getFile();
private static final String exampleVmPath =
System.getProperty("user.dir") + "/../c/build/lib/libexample-vm.so";
EvmcTest.class.getResource("/libexample-vm.so").getFile();
atoulme marked this conversation as resolved.
Show resolved Hide resolved

@Test
void testInitCloseDestroy() throws Exception {
Assertions.assertDoesNotThrow(
() -> {
try (EvmcVm vm = EvmcVm.create(exampleVmPath)) {}
try (EvmcVm vm = EvmcVm.create(libEvmcPath, exampleVmPath)) {}
});
}

@Test
void testAbiVersion() throws Exception {
try (EvmcVm vm = EvmcVm.create(exampleVmPath)) {
try (EvmcVm vm = EvmcVm.create(libEvmcPath, exampleVmPath)) {
int abiVersion = vm.abi_version();
assert (abiVersion > 0);
}
}

@Test
void testName() throws Exception {
try (EvmcVm vm = EvmcVm.create(exampleVmPath)) {
try (EvmcVm vm = EvmcVm.create(libEvmcPath, exampleVmPath)) {
String name = vm.name();
assert (name.length() > 0);
assert (name.equals("example_vm"));
Expand All @@ -39,15 +40,15 @@ void testName() throws Exception {

@Test
void testVersion() throws Exception {
try (EvmcVm vm = EvmcVm.create(exampleVmPath)) {
try (EvmcVm vm = EvmcVm.create(libEvmcPath, exampleVmPath)) {
String version = vm.version();
assert (version.length() >= 5);
}
}

@Test
void testExecute_returnAddress() throws Exception {
try (EvmcVm vm = EvmcVm.create(exampleVmPath)) {
try (EvmcVm vm = EvmcVm.create(libEvmcPath, exampleVmPath)) {
HostContext context = new TestHostContext();
int BYZANTIUM = 4;
int EVMC_CALL = 0;
Expand Down Expand Up @@ -77,7 +78,7 @@ void testExecute_returnAddress() throws Exception {
/** Tests callbacks: get_storage_fn & set_storage_fn */
@Test
void testExecute_counter() throws Exception {
try (EvmcVm vm = EvmcVm.create(exampleVmPath)) {
try (EvmcVm vm = EvmcVm.create(libEvmcPath, exampleVmPath)) {
HostContext context = new TestHostContext();
int BYZANTIUM = 4;
int EVMC_CALL = 0;
Expand Down Expand Up @@ -107,7 +108,7 @@ void testExecute_counter() throws Exception {
/** Tests callbacks: get_tx_context_fn */
@Test
void testExecute_returnBlockNumber() throws Exception {
try (EvmcVm vm = EvmcVm.create(exampleVmPath)) {
try (EvmcVm vm = EvmcVm.create(libEvmcPath, exampleVmPath)) {
HostContext context = new TestHostContext();
int BYZANTIUM = 4;
int EVMC_CALL = 0;
Expand Down Expand Up @@ -137,7 +138,7 @@ void testExecute_returnBlockNumber() throws Exception {
/** Tests callbacks: get_tx_context_fn & set_storage_fn */
@Test
void testExecute_saveReturnBlockNumber() throws Exception {
try (EvmcVm vm = EvmcVm.create(exampleVmPath)) {
try (EvmcVm vm = EvmcVm.create(libEvmcPath, exampleVmPath)) {
HostContext context = new TestHostContext();
int BYZANTIUM = 4;
int EVMC_CALL = 0;
Expand Down Expand Up @@ -169,7 +170,7 @@ void testExecute_saveReturnBlockNumber() throws Exception {
/** Tests callbacks: call_fn */
@Test
void testExecute_makeCall() throws Exception {
try (EvmcVm vm = EvmcVm.create(exampleVmPath)) {
try (EvmcVm vm = EvmcVm.create(libEvmcPath, exampleVmPath)) {
HostContext context = new TestHostContext();
int BYZANTIUM = 4;
int EVMC_CALL = 0;
Expand Down Expand Up @@ -207,7 +208,7 @@ void testExecute_makeCall() throws Exception {

@Test
void testExecute_EVMC_CREATE() throws Exception {
try (EvmcVm vm = EvmcVm.create(exampleVmPath)) {
try (EvmcVm vm = EvmcVm.create(libEvmcPath, exampleVmPath)) {
HostContext context = new TestHostContext();
int BYZANTIUM = 4;
int EVMC_CREATE = 3;
Expand Down Expand Up @@ -235,15 +236,15 @@ void testExecute_EVMC_CREATE() throws Exception {

@Test
void testGetCapabilities() throws Exception {
try (EvmcVm vm = EvmcVm.create(exampleVmPath)) {
try (EvmcVm vm = EvmcVm.create(libEvmcPath, exampleVmPath)) {
int capabilities = vm.get_capabilities();
assert (capabilities > 0);
}
}

@Test
void testSetOption() throws Exception {
try (EvmcVm vm = EvmcVm.create(exampleVmPath)) {
try (EvmcVm vm = EvmcVm.create(libEvmcPath, exampleVmPath)) {
int result = vm.set_option("verbose", "1");
assert (result == 0);
}
Expand Down