LogExecution: optionally take a module name for the logger function

[yamt]

also, fix a crash on modules w/o imports.

[kripken]

I agree that the automatic behavior here can be hard to use. But we do need a solution for the case where "env" was minified, which that PR solved. How about making the automatic behavior customizable? We could perhaps allow not just --log-execution but also --log-execution=PREFIX, and in the second case that exact prefix would be used and nothing else (no automatic behavior). Would that work for your use case @yamt ?

If so then to implement it see e.g. ExtractFunction.cpp for an example of a pass that receives a parameter like that.

[yamt]

sure, making it an option works for me.
however, i don't understand why the automatic behavior is necessary in the first place.
probably because i'm not familiar with how minimization works.
can't you simply minimize after applying log-execute instrumentation?

[kripken]

You're right that a different order might work. But sometimes you want to add the logging after all optimizations have been done, maybe because the bug only shows itself then (maybe it's a bug in an optimization pass).

In general I've personally found this pass useful on various wasm files, optimized and not, and I'm not sure how other people are using the pass, so I'd rather not remove functionality.

[yamt]

You're right that a different order might work. But sometimes you want to add the logging after all optimizations have been done, maybe because the bug only shows itself then (maybe it's a bug in an optimization pass).

In general I've personally found this pass useful on various wasm files, optimized and not, and I'm not sure how other people are using the pass, so I'd rather not remove functionality.

ok. it makes sense.

[yamt]

i added the suggested option.

[kripken]

Please add a test for this. It could be similar to this test file that I just opened a PR for,

https://github.com/WebAssembly/binaryen/pull/6634/files#diff-26883f6202548dd332237e9002be15d8e33b958bdbbb52eef03e0999e147dd6e

Maybe call it test/lit/passes/log-execution_arg.wast (adding _arg), and put a comment in it saying

;; Test the option to provide the module name as an argument.

Then you can just modify the wasm-opt command there. To update the expected output there use

./scripts/update_lit_checks.py test/lit/passes/log-execution_arg.wast

[kripken]

Suggested change

	IString logger_module;
	// The module name the logger function is imported from.
	Name loggerModule;

[yamt]

done

[kripken]

Aside from the comment, the diff also renamed the variable. loggerModule is more consistent with our naming conventions (compared to logger_module).

[yamt]

oops, i haven't noticed it. done.

[yamt]

Please add a test for this. It could be similar to this test file that I just opened a PR for,

https://github.com/WebAssembly/binaryen/pull/6634/files#diff-26883f6202548dd332237e9002be15d8e33b958bdbbb52eef03e0999e147dd6e

Maybe call it test/lit/passes/log-execution_arg.wast (adding _arg), and put a comment in it saying

;; Test the option to provide the module name as an argument.

Then you can just modify the wasm-opt command there. To update the expected output there use

./scripts/update_lit_checks.py test/lit/passes/log-execution_arg.wast

done.
i needed --all-items to make it check the import.

[kripken]

I think it's ok to stop this test here. The first 25 lines show that it uses "foo" and that it emits a valid call to log_execution.

[kripken]

(That is, it doesn't need to be as comprehensive as the other test, as the focus here is just on the argument to the pass, which lines 1-25 test.)

[yamt]

are you suggesting to minimize the test?

[yamt]

done

[kripken]

Yes, exactly.

[kripken]

Looks good aside from those last comments.