wasm-merge emits non-imported global initializer expression

[doehyunbaek]

index.wat

(module
  (import "env" "DYNAMICTOP_PTR" (global i32))
  (global (mut i32) (global.get 0))
  (func (export "_start") (;3480;)
    global.get 1
    drop
  )
)

(module
    (global (export "DYNAMICTOP_PTR") i32 (i32.const 9568))
)

~/binaryen-report/global_get_import wat2wasm index.wat && wat2wasm env.wat  && wasm-merge index.wasm index env.wasm env -o merged.wasm && wasm2wat merged.wasm
merged.wasm:0000019: error: global variable out of range: 1 (max 1)

It seems like in the resulting binary merged.wasm, global's initialization expression is still global.get 0, which is out of range. Reversing the two global declarations is also problematic with the error initializer expression can only reference an imported global.

  (global (;0;) (mut i32) (global.get 1))
  (global (;1;) i32 (i32.const 9568))

Interestingly, if you remove the usage of global 1 at global.get 1, wasm-merge successfuly merges the globals, as below

(module
  (type (;0;) (func))
  (func (;0;) (type 0))
  (global (;0;) i32 (i32.const 9568))
  (export "_start" (func 0))
  (export "DYNAMICTOP_PTR" (global 0)))

Maybe it's not a bug and out of scope for wasm-merge, but as I thought wasm-merge should give at least a warning on this case , I am reporting now.

Code is also at this repo.

If this is a duplicate issue or non-issue, sorry in advance. Feel free to close the issue in that case.

I opened a separate issue with #6219 as there seems to be separate root cause, but if you feel like they should be consolidated please close this issue.

[kripken]

Thanks @doehyunbaek , this is indeed a bug. Fix in #6221

[doehyunbaek]

Hi @kripken. It seems that running the test case I mentioned, it still emits error merged.wasm:0000020: error: initializer expression can only reference an imported global, when run with wasm-validate is this intended?

Originally, emitted globals are like this

  (global (;0;) (mut i32) (global.get 1))
  (global (;1;) i32 (i32.const 9568))

After the fix, emitted globals are like this

  (global (;0;) i32 (i32.const 9568))
  (global (;1;) (mut i32) (global.get 0))

I am a little bit weird that this might be some quirkiness on wasm-validate's part, as I could run some other binary with v8 successfully even though wasm-validate errors with the above message. I found one github issue with regards to this in the wabt repo. Maybe this is something disallowed in spec but allowed in practice in engines?

I'm opening this again for this comment. You can close again if this is intended behavior.

[kripken]

That is expected. It will also not validate in wasm-opt by itself, but it will with -all. Likewise --enable-all should fix things in wabt, if it is up to date with the latest specs.

The reason is that

  (global (;0;) i32 (i32.const 9568))
  (global (;1;) (mut i32) (global.get 0))

is only valid if the GC extension is enabled. That's a little weird, but before that proposal a global initializer could only read from an imported global. With GC, that is relaxed so that reading a non-imported global works as well. (This was useful for GC so it was bundled into that proposal, but it by itself is not really related to GC.)

Here is the relevant validation code in Binaryen for reference:

binaryen/src/ir/properties.cpp

Lines 65 to 67 in b4dee3d

	// Only imported globals are available in constant expressions unless GC is
	// enabled.
	return g->imported() || wasm.features.hasGC();

You can work around this by running wasm-opt -O or such. It should apply the first global's value to the second global, removing the global.get in the process. That may be necessary if you want to run in older VMs that do not support GC.

[doehyunbaek]

Oh thanks for sharing your knowledge. I think wabt is not really up-to-date with the latest spec as it didn't fix with --enable-all.

Also huge thanks to pointing out the workaround, I am really impressed by your knowledge and willingness to share in such detail. Thank you.

[doehyunbaek]

Hi @kripken. Kinda worried that I keep bothering you with the expected semantics of binaryen. But I have one more question. Please forgive me if this is also an expected behavior.

You can work around this by running wasm-opt -O or such. It should apply the first global's value to the second global, removing the global.get in the process.

This optimization, presumably from here:

binaryen/src/passes/SimplifyGlobals.cpp

Lines 668 to 684 in 45f5bd2

	// Constant propagation part 2: apply the values of immutable globals
	// with constant values to global.gets in the code.
	void propagateConstantsToCode() {
	NameSet constantGlobals;
	for (auto& global : module->globals) {
	if (!global->mutable_ && !global->imported() &&
	Properties::isConstantExpression(global->init)) {
	constantGlobals.insert(global->name);
	}
	}
	ConstantGlobalApplier(&constantGlobals, optimize)
	.run(getPassRunner(), module);
	// Note that we don't need to run on module code here, since we already
	// handle applying constants in globals in propagateConstantsToGlobals (and
	// in a more sophisticated manner, which takes into account that no sets of
	// globals are possible during global instantiation).
	}

Doesn't apply to initializer expressions in element segment. For example,

// index.wat
(module
  (import "env" "DYNAMICTOP_PTR" (global i32))
  (table 1 funcref)
  (elem (;0;) (global.get 0) func 0)
  (func (export "entry") (param i32)
    (table.get 0 (i32.const 0))
    return
  )
)

// env.wat
(module
    (global (export "DYNAMICTOP_PTR") i32 (i32.const 0))
)

After wasm-merge and wasm-opt, it errors with initializer expression can only reference an imported global. I also have a code repo with this example here.

Is this an expected behavior?

If not, I tried a fix with this commit 01e2691, and it at least happened to solve my use cases, including the example given above.

If this fix is appropriate, I would love to contribute a PR with a test case accompanied.

[kripken]

Thanks @doehyunbaek , good find! Yes, this was a missing optimization.

Your proposed fix is almost correct, but things are a little more complex I'm afraid. I almost wrote up a comment with a suggestion for a PR but I realized the comment would be as long as the code in the PR, so I just opened that instead here: #6226 . Thanks for offering to do the work, though!

(The specific reason I prefer the code in the PR is that the comment in the quoted code here is right that propagateConstantsToGlobals is the right place to do this optimization, since it can reason about the order of execution, specifically that even a mutable global can be applied in a segment offset, as mutation isn't possible there. The wasm spec actually doesn't allow mutable globals in offsets atm, but I suspect it may be extended in the future as it has already been in the GC proposal (mentioned above), so the PR does things in a way that will optimize that case as well, if/when it arrives.)

[doehyunbaek]

Good that I found a missing optimization! Thanks for the kind comments also.

I think it would be best if #6226 got merged.