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

Implement compatibility checks between munlib and runtime #188

Closed
8 tasks done
baszalmstra opened this issue May 16, 2020 · 5 comments · Fixed by #226
Closed
8 tasks done

Implement compatibility checks between munlib and runtime #188

baszalmstra opened this issue May 16, 2020 · 5 comments · Fixed by #226
Assignees
Labels
exp: low Achievable with little prior knowledge and guidance good first issue Good for newcomers pri: intermediate An issue resulting in non-critical functionality loss and no significant effect on usability type: feat New feature or request

Comments

@baszalmstra
Copy link
Collaborator

baszalmstra commented May 16, 2020

Problem

Currently, the runtime tries to load a *.munlib with the assumption that the ABI memory layout of the *.munlib matches its own ABI version. This can cause undefined behaviour when it tries to load a *.munlib with a different ABI version.

Solution

By including an ABI version in the *.munlib, the runtime can gracefully fail - reporting an error to the end user.

In the future - once we support backwards compatibility, we can even support loading of several different ABI versions using this functionality.

Tasks

  • add an integer ABI_VERSION constant to the mun_abi crate.
  • generate a get_version IR function
  • export the get_version function
  • in the runtime, call the get_version function and verify that the runtime's mun_abi::ABI_VERSION is compatible.
  • (bonus) add a constant with the get_version function name (and other API functions) to the mun_abi crate and use that constant in both the mun_codegen and mun_runtime crates - to avoid discrepancies.

Good first issue process:

If this is your first PR, welcome 🎉 😄

@baszalmstra baszalmstra added the good first issue Good for newcomers label May 16, 2020
@Wodann Wodann added pri: intermediate An issue resulting in non-critical functionality loss and no significant effect on usability type: perf Changes that improve performance exp: low Achievable with little prior knowledge and guidance labels May 22, 2020
@MineBill
Copy link

I would like to tackle this issue and implement the feature. This will be my first ever PR.

@Wodann
Copy link
Collaborator

Wodann commented Jun 14, 2020

Welcome and thank you for working on this PR. If you have any questions, please let us know.

@MineBill
Copy link

After implementing this, the following tests fail:

tests::abi_is_fresh
tests::runtime_capi_is_fresh

They require me to update the abi and the runtime-capi but doing so requires me to download both the ffi and the abi-c submodules. What should i do?

@Wodann
Copy link
Collaborator

Wodann commented Jun 15, 2020

You should always download the submodules, otherwise the tests will fail. Our install instructions describe how to do it, but it's just a single command:

git submodule update --init --recursive

@Wodann
Copy link
Collaborator

Wodann commented Jun 16, 2020

Exemplary work! Really quick workflow from claiming the issue, to submitting a PR, and incorporating recommended changes. Well done 👍

We look forward to your next contribution

@Wodann Wodann added type: fix Bug fix or report type: feat New feature or request and removed type: perf Changes that improve performance type: fix Bug fix or report labels Dec 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
exp: low Achievable with little prior knowledge and guidance good first issue Good for newcomers pri: intermediate An issue resulting in non-critical functionality loss and no significant effect on usability type: feat New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants