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(wasip2): export _initialize #4490

Closed

Conversation

rvolosatovs
Copy link
Contributor

@rvolosatovs rvolosatovs commented Oct 2, 2024

Closes #4488
Refs #4482

Turns out _initialize was not exported by wasip2 components and so init() was never called in reactors. This PR fixes that, by initializing the heap and calling all initializers, just like run() would normally do

Note: wasm-tools 1.217 or later are required for init() to work in reactors (since _initialize is ignored in earlier versions, see bytecodealliance/wasm-tools#1747)

Thanks @alexcrichton for assisting in debugging this in https://bytecodealliance.zulipchat.com/#narrow/stream/217126-wasmtime/topic/calling.20.60_initialize.60

@lxfontes
Copy link

lxfontes commented Oct 2, 2024

ran same scenario as in #4482

Before

varHolder: {str:top ptr:0x15b30 mapStr:map[] mapPtr:map[]}
ptrHolder: &{str:top ptr:0x15b40 mapStr:map[] mapPtr:map[]}
callHolder: {str:top ptr:0x15df0 mapStr:map[a:b] mapPtr:map[c:0x16140]}

With this PR

varHolder: {str:top ptr:0x15b38 mapStr:map[a:b] mapPtr:map[c:0x15b40]}
ptrHolder: &{str:top ptr:0x15b48 mapStr:map[a:b] mapPtr:map[c:0x15b50]}
callHolder: {str:top ptr:0x164c0 mapStr:map[a:b] mapPtr:map[c:0x16720]}

🎉

Copy link
Member

@aykevl aykevl left a comment

Choose a reason for hiding this comment

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

With this change, if wasi:cli/run@0.2.0#run is called, the package initializers will be called twice. I don't think that's the intention?

@rvolosatovs
Copy link
Contributor Author

With this change, if wasi:cli/run@0.2.0#run is called, the package initializers will be called twice. I don't think that's the intention?

how about 03d535d ?

@dgryski
Copy link
Member

dgryski commented Oct 2, 2024

/cc @ydnar

@rvolosatovs
Copy link
Contributor Author

Looking at tests, it does not look like _initialize is actually called for run implementations?
Perhaps we need to call _initialize from within run and guard by sync.Once or such?

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

rvolosatovs commented Oct 2, 2024

Reworked the PR a bit - initialization is now done always, but guarded by sync.OnceValue - since the spec is not completely clear on the semantics here, this ensures that initialization is done for both reactors and command components.

For reactors, runtime must always call _initialize and for commands it may not

@aykevl
Copy link
Member

aykevl commented Oct 2, 2024

Looking at tests, it does not look like _initialize is actually called for run implementations?

That's not how I understand the wasip2 spec, my understanding is that _initialize needs to be called first in all cases but that it wasn't always done previously.

@ydnar?

@ydnar
Copy link
Contributor

ydnar commented Oct 2, 2024

What version of wasm-tools is used to create the component? It needs at least v1.217.0: bytecodealliance/wasm-tools@v1.216.0...v1.217.0

The relevant commit is: bytecodealliance/wasm-tools@108c37d

@ydnar
Copy link
Contributor

ydnar commented Oct 2, 2024

Once #3165 lands, the wasi:cli/command#run export should be wired up to call main.main and do nothing else.

@ydnar
Copy link
Contributor

ydnar commented Oct 3, 2024

Fixed in #4496.

@ydnar
Copy link
Contributor

ydnar commented Oct 5, 2024

#4496 merged. Want to try dev branch?

@ydnar
Copy link
Contributor

ydnar commented Oct 11, 2024

@rvolosatovs is still necessary? Can you verify against dev branch?

@rvolosatovs
Copy link
Contributor Author

@rvolosatovs is still necessary? Can you verify against dev branch?

latest dev branch seems to work

@rvolosatovs rvolosatovs deleted the fix/wasip2-initialize branch October 12, 2024 00:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants