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.

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

Generalize loader searching algorithm #119

Merged
merged 2 commits into from
Aug 31, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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 CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@

## [6.0.0-dev] - unreleased

- Improved: [[#119](https://github.com/ethereum/evmc/pull/119)]
EVMC loader symbol searching has been generalized.

## [5.2.0] - 2018-08-28

- Feature: [[#81](https://github.com/ethereum/evmc/pull/81)]
Expand Down
15 changes: 8 additions & 7 deletions include/evmc/loader.h
Original file line number Diff line number Diff line change
Expand Up @@ -60,15 +60,16 @@ enum evmc_loader_error_code
* "libexample-interpreter.so",
* - the "lib" prefix and file extension are stripped from the name:
* "example-interpreter"
* - all "-" are replaced with "_" to construct _full name_:
* - all "-" are replaced with "_" to construct _base name_:
* "example_interpreter",
* - the _full name_ is split by "_" char and the last item is taken to form the _short name_:
* "interpreter",
* - the name "evmc_create_" + _full name_ is checked in the library:
* - the function name "evmc_create_" + _base name_ is searched in the library:
* "evmc_create_example_interpreter",
* - then, the name "evmc_create_" + _short name_ is checked in the library:
* "evmc_create_interpreter".
* - lastly, the name "evmc_create" is checked in the library
* - if function not found, the _base name_ is shorten by skipping the first word separated by "_":
* "interpreter",
* - then, the function of the shorter name "evmc_create_" + _base name_ is searched in the library:
* "evmc_create_interpreter",
Copy link
Member

Choose a reason for hiding this comment

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

So what exactly is the logic here? It removes the prefixes? example off example_interpreter

Copy link
Member Author

Choose a reason for hiding this comment

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

yes

Copy link
Member Author

Choose a reason for hiding this comment

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

But also a_b_c_d -> b_c_d -> c_d -> d.

Copy link
Member

Choose a reason for hiding this comment

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

What is the reasoning for that?

Copy link
Member Author

Choose a reason for hiding this comment

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

evmc-example-vm :) I want it to come up with function name evmc_create_example_vm() while previously it tried only evmc_create_evmc_example_vm() and evmc_create_vm().

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 that may not be the most sound reasoning :)

Why not just name the file example-vm (and the creator appropriately) and be done with it?

Copy link
Member Author

Choose a reason for hiding this comment

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

For 3 reasons:

  • I don't want to name the library example-vm in case it is going to be installed.
  • This code looks better than before.
  • Even the author of loader (me) was no able to remember how exactly it was working before so Host example #116 does not work.

Copy link
Member

Choose a reason for hiding this comment

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

Didn't notice the old code was doing the very same weird logic.

Copy link
Member Author

Choose a reason for hiding this comment

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

It was, but not fully. It was skipping all leading words except the last one.

* - the name shortening continues until a function is found or the name cannot be shorten more,
* - lastly, when no function found, the function name "evmc_create" is searched in the library.
*
* If the create function is found in the library, the pointer to the function is returned.
* Otherwise, the ::EVMC_LOADER_SYMBOL_NOT_FOUND error code is signaled and NULL is returned.
Expand Down
30 changes: 14 additions & 16 deletions lib/loader/loader.c
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,8 @@ evmc_create_fn evmc_load(const char* filename, enum evmc_loader_error_code* erro
// Create name buffer with the prefix.
const char prefix[] = "evmc_create_";
const size_t prefix_length = strlen(prefix);
char name[sizeof(prefix) + PATH_MAX_LENGTH];
strcpy_s(name, sizeof(name), prefix);
char prefixed_name[sizeof(prefix) + PATH_MAX_LENGTH];
strcpy_s(prefixed_name, sizeof(prefixed_name), prefix);

// Find filename in the path.
const char* sep_pos = strrchr(filename, '/');
Expand All @@ -87,30 +87,28 @@ evmc_create_fn evmc_load(const char* filename, enum evmc_loader_error_code* erro
if (strncmp(name_pos, lib_prefix, lib_prefix_length) == 0)
name_pos += lib_prefix_length;

strcpy_s(name + prefix_length, PATH_MAX_LENGTH, name_pos);
char* base_name = prefixed_name + prefix_length;
strcpy_s(base_name, PATH_MAX_LENGTH, name_pos);

// Trim the file extension.
char* ext_pos = strrchr(name, '.');
char* ext_pos = strrchr(prefixed_name, '.');
if (ext_pos)
*ext_pos = 0;

// Replace all "-" with "_".
char* dash_pos = name;
char* dash_pos = base_name;
while ((dash_pos = strchr(dash_pos, '-')) != NULL)
*dash_pos++ = '_';

// Search for the "full name" based function name.
create_fn = DLL_GET_CREATE_FN(handle, name);
if (!create_fn)
// Search for the built function name.
while ((create_fn = DLL_GET_CREATE_FN(handle, prefixed_name)) == NULL)
{
// Try the "short name" based function name.
const char* short_name_pos = strrchr(name, '_');
if (short_name_pos)
{
short_name_pos += 1;
memmove(name + prefix_length, short_name_pos, strlen(short_name_pos) + 1);
create_fn = DLL_GET_CREATE_FN(handle, name);
}
// Shorten the base name by skipping the `word_` segment.
const char* shorter_name_pos = strchr(base_name, '_');
if (!shorter_name_pos)
break;

memmove(base_name, shorter_name_pos + 1, strlen(shorter_name_pos) + 1);
}

if (!create_fn)
Copy link
Member

Choose a reason for hiding this comment

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

Practically this part wouldn't be needed if in the above loop on !short_name_pos base_name is set to \0.

Copy link
Member Author

Choose a reason for hiding this comment

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

True. I will check it out.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it will get messy. I would need to add another loop terminating condition (like strlen == 0) and we don't want to check for "evmc_create_" but "evmc_create".

Also, the naming convention will change soon and we might want to try "evmc_create" first.

Expand Down