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

fix: allow dynamic owned resources to be used as borrowed parameters #7783

Merged

Conversation

rvolosatovs
Copy link
Member

@rvolosatovs rvolosatovs commented Jan 17, 2024

This enables using owned resources in places where borrowed resources of the same type are expected (e.g. dynamically-typed functions/methods defined in Linker::func_new)
Note, that this is already possible in e.g.

// If this resource is not currently in a table then it
// needs to move into a table to participate in state
// related to borrow tracking. Execute the
// `host_resource_lower_own` operation here and update our
// state.
//
// Afterwards this is the same as the `idx` case below.
NOT_IN_TABLE => {
let idx = cx.host_resource_lower_own(self.rep);
let prev = self.state.swap(idx, Relaxed);
assert_eq!(prev, NOT_IN_TABLE);
cx.host_resource_lift_borrow(idx)?
}

cc @alexcrichton , we discussed this on a call, I believe this is the changeset we agreed upon

@rvolosatovs rvolosatovs marked this pull request as ready for review January 17, 2024 20:38
@rvolosatovs rvolosatovs requested a review from a team as a code owner January 17, 2024 20:38
@rvolosatovs rvolosatovs requested review from pchickey and removed request for a team January 17, 2024 20:38
@pchickey pchickey requested review from alexcrichton and removed request for pchickey January 17, 2024 21:22
@github-actions github-actions bot added the wasmtime:api Related to the API of the `wasmtime` crate itself label Jan 17, 2024
Copy link

Subscribe to Label Action

cc @peterhuene

This issue or pull request has been labeled: "wasmtime:api"

Thus the following users have been cc'd because of the following labels:

  • peterhuene: wasmtime:api

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Looks great to me, thanks! Mind adding a test as well for this?

Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
@rvolosatovs
Copy link
Member Author

Added a test case, but also uncovered a bug while doing that #7793

Honestly, I'm not sure what's the use case for using the resource borrows pointing to non-existent resources and it appears to me that neither of Resources::new_borrow cases should actually work (without panic though), but perhaps I don't understand the semantics here fully.

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Thanks for the test! I believe that issue is orthogonal to this, so I'm going to mark this for merge while that other issue is sorted out.

@alexcrichton alexcrichton added this pull request to the merge queue Jan 18, 2024
Merged via the queue into bytecodealliance:main with commit 2c86e26 Jan 18, 2024
18 checks passed
@rvolosatovs rvolosatovs deleted the fix/dyn-owned-resource-borrow branch January 18, 2024 18:08
vigoo pushed a commit to golemcloud/wasmtime that referenced this pull request Feb 23, 2024
…ytecodealliance#7783)

* fix: allow dynamic owned resources to be used as borrowed parameters

Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>

* tests: add `can_use_own_for_borrow` test

Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>

---------

Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
(cherry picked from commit 2c86e26)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasmtime:api Related to the API of the `wasmtime` crate itself
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants