Add evmc::loader library to support dynamic loading #40

chfast · 2018-07-05T09:36:29Z

Added evmc::loader library that have to code for dynamically loading the EVM implementations. It's going to be used by vmtester (already migrated), aleth and go/evmc package. Warning! A lot of C language used.

chfast · 2018-07-09T09:45:53Z

Can you review this?

axic · 2018-07-09T10:40:57Z

include/evmc/loader.h

+    EVMC_ERRC_INVALID_ARGUMENT,
+};
+
+struct evmc_instance* evmc_load(const char* filename, enum evmc_loader_error_code* error_code);


Should have documentation here. Also mention that error_code can be NULL and in that case it won't be set.

Okay this was added with a subsequent commit.

axic · 2018-07-09T10:41:17Z

include/evmc/loader.h

+
+enum evmc_loader_error_code
+{
+    EVMC_ERRC_SUCCESS = 0,


Why not EVMC_LOADER_SUCCESS?

@chfast this is still outstanding.

At first I wanted to use a subset of error codes from C++ errc: https://en.cppreference.com/w/cpp/error/errc, but in the end they didn't match.
We can either make it evmc-generic: evmc_loader_error_code -> evmc_error_code, or use EVMC_LOADER_ prefix as you proposed.

I'd vote for EVMC_LOADER_

axic · 2018-07-09T10:46:28Z

include/evmc/loader.h

+ * - the filename is taken from the path,
+ * - the "lib" prefix and file extension are stripped from the name,
+ * - all "-" are replaced with "_" to construct _full name_,
+ * - the last stem from the _full name_ separated with "_" is taken to construct the _short name_,


Can you explain this?

How about?

The full name is split by "_" char and the last item is taken to form the short name.

Can you give an example of a filename having both short and full names?

Yeah, example in the comment would be helpful, like for "libfull-evm-name.so" the full name "evmc_create_full_evm_name" and "evmc_create_name" are checked in the library.

Also. why do we need these complicated rules, can't we just have one hard-coded name for create function in any evmc library?

Want to support both static and dynamic linking of the same source.

On POSIX we could use symbol alias in the .so library. On Windows it is much harder. In the end I had to just define evmc_create() function that executes evmc_create_interpreter() and disable the evmc_create() when building a static library. This generally gets messy and requires toolchain specific build flags. And I expect that to be even harder for rust or nim. So I decided to provide this naming convention and keep the implementation of it in this single place.

axic · 2018-07-09T10:47:52Z

lib/loader/loader.c

+
+    // Create name buffer with the prefix.
+    const char prefix[] = "evmc_create_";
+    const size_t prefix_length = strlen(prefix);


Wouldn't sizeof(prefix) work here?

Then I would need sizeof(prefix) - 1.

The compiler is replacing strlen(prefix) with a constant.

Are you sure it replaces it with a constant?

https://godbolt.org/g/1u39cy

Nice. Damn compilers, they're becoming way too smart 😉

That's not super smart. They have hardcoded knowledge of some C standard library functions.

gumb0 · 2018-07-09T14:50:26Z

include/evmc/loader.h

+};
+
+/**
+ * Dynamically loads the shared object and with an EVM implementation.


Some verb missing after "and"?

gumb0 · 2018-07-09T14:51:30Z

include/evmc/loader.h

+ * This function tries to open a DLL at the given `filename`. On UNIX-like systems dlopen() function
+ * is used. On Windows LoadLibrary() function is used.
+ *
+ * If the file does not exists or is not a valid shared library the ::EVMC_ERRC_CANNOT_OPEN error


does not exist

gumb0 · 2018-07-09T15:43:40Z

lib/loader/loader.c

+#define HAVE_STRCPY_S 0
+#endif
+
+#define PATH_MAX_LENGHT 4096


gumb0 · 2018-07-09T16:19:20Z

lib/loader/loader.c

+    strcpy_s(name, sizeof(name), prefix);
+
+    // Find filename in the path.
+    const char* sep_pos = strrchr(filename, '/');


will not work when windows path provided

True. I have to add a test for it. What do you think about doing a normalization on Windows before this by replacing \ with /. I believe on Windows you can have your separators mixed.

I think it should work

gumb0 · 2018-07-09T16:29:23Z

lib/loader/loader.c

+exit:
+    if (error_code)
+        *error_code = ec;
+    return create_fn;


So handle stays open forever after sucessful evmc_load, is this ok?

I don't think you can close it if there's a pointer into it?

On Windows there is FreeLibrary, and GetProcAddress doesn't influence it

Documentation is not clear, but I would assume that if the flow is

lib = dlopen() fn = dlsym(lib, "hello") dlclose(lib) fn()

Then it should crash. man page for dlclose states it uses reference counting and if it hits zero it will run all the deallocation code (such as atexit if a similar thing exists for libraries) and remove the shared library from memory. Not sure if that reference counting also considers anything returned by dlsym, since that would be really hard to track. Instead I would assume it just counts how many time dlopen was called for the same thing.

The dlclose() will unload the library from the memory so you cannot use functions from it (similarly on Windows). We are doing so only when we could not find the create function because then the library is not needed any more. In case we use the library, it is never unloaded.

gumb0 · 2018-07-09T16:30:37Z

lib/loader/loader.c

+        if (short_name_pos)
+        {
+            short_name_pos += 1;
+            memmove(name + prefix_length, short_name_pos, strlen(short_name_pos) + 1);


Does this still work fine when file name ends with _? Add a test for it

It works. For lib_.so the full name is "_", and short name is "". I'm adding unit tests for it.

chfast · 2018-07-10T09:38:38Z

Ready. Rebase is needed before merge.

chfast force-pushed the loader branch 3 times, most recently from 575eb79 to 9f82448 Compare July 5, 2018 12:20

chfast requested review from axic and gumb0 July 5, 2018 13:11

axic reviewed Jul 9, 2018

View reviewed changes

gumb0 reviewed Jul 9, 2018

View reviewed changes

lib/loader/loader.c Outdated

#define HAVE_STRCPY_S 0

#endif

#define PATH_MAX_LENGHT 4096

Copy link

Member

gumb0 Jul 9, 2018

Choose a reason for hiding this comment

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

length

gumb0 reviewed Jul 9, 2018

View reviewed changes

chfast force-pushed the loader branch from 50485c4 to 894c2f1 Compare July 10, 2018 09:25

gumb0 approved these changes Jul 10, 2018

View reviewed changes

chfast added 7 commits July 10, 2018 12:57

Add evmc::loader library to support dynamic loading

02b52e2

loader: Add Windows support

49e26b1

loader: Add documentation

4a929e2

loader: Return function pointer instead of EVM instance

721f2b3

vmtester: Use loader library

8331ca1

vmtester: Use CLI11 instead of boost::program_options

18a96c8

loader: Fix support for Windows paths

6b0ab05

chfast force-pushed the loader branch from 894c2f1 to 6b0ab05 Compare July 10, 2018 11:07

chfast merged commit 4ae7660 into master Jul 10, 2018

chfast deleted the loader branch July 10, 2018 11:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add evmc::loader library to support dynamic loading #40

Add evmc::loader library to support dynamic loading #40

chfast commented Jul 5, 2018 •

edited

Loading

chfast commented Jul 9, 2018

axic Jul 9, 2018

axic Jul 9, 2018

axic Jul 9, 2018

axic Jul 9, 2018

chfast Jul 9, 2018

axic Jul 9, 2018

axic Jul 9, 2018

chfast Jul 9, 2018

axic Jul 9, 2018

gumb0 Jul 9, 2018

gumb0 Jul 9, 2018

axic Jul 9, 2018

chfast Jul 10, 2018

axic Jul 9, 2018

chfast Jul 9, 2018

axic Jul 9, 2018

chfast Jul 9, 2018

axic Jul 9, 2018

chfast Jul 9, 2018

gumb0 Jul 9, 2018

gumb0 Jul 9, 2018

gumb0 Jul 9, 2018

gumb0 Jul 9, 2018

chfast Jul 10, 2018

gumb0 Jul 10, 2018

gumb0 Jul 9, 2018

axic Jul 9, 2018

gumb0 Jul 9, 2018 •

edited

Loading

axic Jul 9, 2018

chfast Jul 10, 2018

gumb0 Jul 9, 2018

chfast Jul 10, 2018

chfast commented Jul 10, 2018

Add evmc::loader library to support dynamic loading #40

Add evmc::loader library to support dynamic loading #40

Conversation

chfast commented Jul 5, 2018 • edited Loading

chfast commented Jul 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gumb0 Jul 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chfast commented Jul 10, 2018

chfast commented Jul 5, 2018 •

edited

Loading

gumb0 Jul 9, 2018 •

edited

Loading