search

bytecodealliance / wasm-micro-runtime

WebAssembly Micro Runtime (WAMR)
Apache License 2.0
4.66k stars 576 forks source link

wasm-mutator-fuzz: disable SIMD by default #3592

Open yamt opened 5 days ago

yamt commented 5 days ago

because our interpreter's SIMD support is incomplete.

cf. https://github.com/bytecodealliance/wasm-micro-runtime/issues/3580

lum1n0us commented 5 days ago

In wasm_mutator_fuzz.cc, the loader is actually the target.

https://github.com/bytecodealliance/wasm-micro-runtime/blob/main/tests/fuzz/wasm-mutator-fuzz/wasm_mutator_fuzz.cc#L33

    /* init runtime environment */
    wasm_runtime_init();
    wasm_module_t module =
        wasm_runtime_load((uint8_t *)myData.data(), Size, nullptr, 0);  // <- load only
    if (module) {
        wasm_runtime_unload(module);
    }
    /* destroy runtime environment */
    wasm_runtime_destroy();

It doesn't involve execution. So it is better to keep SIMD enabled.

yamt commented 5 days ago

In wasm_mutator_fuzz.cc, the loader is actually the target.

https://github.com/bytecodealliance/wasm-micro-runtime/blob/main/tests/fuzz/wasm-mutator-fuzz/wasm_mutator_fuzz.cc#L33

    /* init runtime environment */
    wasm_runtime_init();
    wasm_module_t module =
        wasm_runtime_load((uint8_t *)myData.data(), Size, nullptr, 0);  // <- load only
    if (module) {
        wasm_runtime_unload(module);
    }
    /* destroy runtime environment */
    wasm_runtime_destroy();

It doesn't involve execution. So it is better to keep SIMD enabled.

do you mean the loader supports SIMD well? i tend to disagree.

if you want to test the loader for JIT/AOT, i guess it's better to actually configure the loader that way.

lum1n0us commented 5 days ago

do you mean the loader supports SIMD well? i tend to disagree.

IUC, both loader needs to support SIMD well. If there is any issue, we shall resolve them.

yamt commented 5 days ago

do you mean the loader supports SIMD well? i tend to disagree.

IUC, both loader needs to support SIMD well. If there is any issue, we shall resolve them.

can you explain "both"?

and why does it need to support SIMD? are you just saying everything ideally should support SIMD?

lum1n0us commented 5 days ago

I mean wasm_loader and aot_loader.

For now, classic-interp, fast-interp and fast-jit don't support SIMD. llvm-jit and aot support SIMD. And it only affects execution. Loader should be able to work well with SIMD opcodes.

yamt commented 5 days ago

ok. is it acceptable for you to disable fast interpreter instead then?

lum1n0us commented 5 days ago

YES. It is OK.

wenyongh commented 5 days ago

ok. is it acceptable for you to disable fast interpreter instead then?

Is it better to report error in a both SIMD+fast-interp enabled wasm loader when an SIMD type or opcode is found? Disabling fast interpreter will disable the pre-compilation testing for fast interpreter. Or is it configurable to test multiple running modes in Oss-fuzz? e.g. pass different cmake flags to wasm-mutator-fuzz CMakeLists.txt?

yamt commented 4 days ago

ok. is it acceptable for you to disable fast interpreter instead then?

Is it better to report error in a both SIMD+fast-interp enabled wasm loader when an SIMD type or opcode is found? Disabling fast interpreter will disable the pre-compilation testing for fast interpreter. Or is it configurable to test multiple running modes in Oss-fuzz? e.g. pass different cmake flags to wasm-mutator-fuzz CMakeLists.txt?

i suppose it's possible to have multiple targets in oss-fuzz. but i dunno.

wenyongh commented 4 days ago

ok. is it acceptable for you to disable fast interpreter instead then?

Is it better to report error in a both SIMD+fast-interp enabled wasm loader when an SIMD type or opcode is found? Disabling fast interpreter will disable the pre-compilation testing for fast interpreter. Or is it configurable to test multiple running modes in Oss-fuzz? e.g. pass different cmake flags to wasm-mutator-fuzz CMakeLists.txt?

i suppose it's possible to have multiple targets in oss-fuzz. but i dunno.

Since we can also configure the running mode and features in oss-fuzz by passing different cmake flags, how about not to change the current CMakeLists.txt?

yamt commented 4 days ago

ok. is it acceptable for you to disable fast interpreter instead then?

Is it better to report error in a both SIMD+fast-interp enabled wasm loader when an SIMD type or opcode is found? Disabling fast interpreter will disable the pre-compilation testing for fast interpreter. Or is it configurable to test multiple running modes in Oss-fuzz? e.g. pass different cmake flags to wasm-mutator-fuzz CMakeLists.txt?

i suppose it's possible to have multiple targets in oss-fuzz. but i dunno.

Since we can also configure the running mode and features in oss-fuzz by passing different cmake flags, how about not to change the current CMakeLists.txt?

what's the point? if you prefer the current default for some reasons, can you explain a bit?

wenyongh commented 4 days ago

ok. is it acceptable for you to disable fast interpreter instead then?

Is it better to report error in a both SIMD+fast-interp enabled wasm loader when an SIMD type or opcode is found? Disabling fast interpreter will disable the pre-compilation testing for fast interpreter. Or is it configurable to test multiple running modes in Oss-fuzz? e.g. pass different cmake flags to wasm-mutator-fuzz CMakeLists.txt?

i suppose it's possible to have multiple targets in oss-fuzz. but i dunno.

Since we can also configure the running mode and features in oss-fuzz by passing different cmake flags, how about not to change the current CMakeLists.txt?

what's the point? if you prefer the current default for some reasons, can you explain a bit?

The main goal of fuzz is to find issue, it would be good if enabling SIMD can help find issue in wasm loader, it seems a little tricky that since fuzz with SIMD+fast-interp will report issue, we disable one of them. Though SIMD isn't supported in interpreter, but definitely runtime can report error in the loader stage. How about fixing the issue when it is reported?

lum1n0us commented 4 days ago

there is a problem about SIMD. because of

#if WASM_ENABLE_SIMD != 0
#if (WASM_ENABLE_WAMR_COMPILER != 0) || (WASM_ENABLE_JIT != 0)
  case WASM_OP_SIMD_PREFIX:

current configuration actually doesn't let loader process SIMD opcodes.

IIUC, we shall add more running-model specific (fuzz) targets. like wasm_mutator_fuzz_classic_interp, wasm_mutator_fuzz_fast_interp, wasm_mutatar_fuzz_llvm_jit, wasm_muatator_fuzz_aot and so on. The original one should be wasm_mutator_fuzz_loader. Different targets focus on different purpose with different feature combinations.

yamt commented 4 days ago

ok. is it acceptable for you to disable fast interpreter instead then?

Is it better to report error in a both SIMD+fast-interp enabled wasm loader when an SIMD type or opcode is found? Disabling fast interpreter will disable the pre-compilation testing for fast interpreter. Or is it configurable to test multiple running modes in Oss-fuzz? e.g. pass different cmake flags to wasm-mutator-fuzz CMakeLists.txt?

i suppose it's possible to have multiple targets in oss-fuzz. but i dunno.

Since we can also configure the running mode and features in oss-fuzz by passing different cmake flags, how about not to change the current CMakeLists.txt?

what's the point? if you prefer the current default for some reasons, can you explain a bit?

The main goal of fuzz is to find issue, it would be good if enabling SIMD can help find issue in wasm loader, it seems a little tricky that since fuzz with SIMD+fast-interp will report issue, we disable one of them.

i want to disable one of them because we don't really support the combination. not because the fuzz found / will find issues. it makes more sense to test what we support by default.

Though SIMD isn't supported in interpreter, but definitely runtime can report error in the loader stage. How about fixing the issue when it is reported?

the simplest fix would be:

#if defined(fast interp) && defined(simd enabled)
#error not implemented yet
#endif

a runtime error is less user-friendly, IMO.

having said that, if you (or anyone) plan to make fast interpreter compatible with jit, and/or implement simd for fast interpreter, and want to use this fuzzer target to help the effort, i can understand. is it the case?

yamt commented 4 days ago

IIUC, we shall add more running-model specific (fuzz) targets. like wasm_mutator_fuzz_classic_interp, wasm_mutator_fuzz_fast_interp, wasm_mutatar_fuzz_llvm_jit, wasm_muatator_fuzz_aot and so on. The original one should be wasm_mutator_fuzz_loader. Different targets focus on different purpose with different feature combinations.

is it desirable to have oss-fuzz run many variations like them? certainly it's good for us. but it would consume a lot more resources.

lum1n0us commented 4 days ago

oss-fuzz doesn't limit the target amount. I guess it's fine.

Or else, just in case, I can manage variant fuzz targets in variant days to make sure a widely test.

wenyongh commented 4 days ago

Though SIMD isn't supported in interpreter, but definitely runtime can report error in the loader stage. How about fixing the issue when it is reported?

the simplest fix would be:

#if defined(fast interp) && defined(simd enabled)
#error not implemented yet
#endif

a runtime error is less user-friendly, IMO.

Unfortunately, the default iwasm building enables aot+simd+fast-interp, simd can be enabled when aot is enabled, adding such lines will lead to compilation error, so using runtime error should be necessary. And in fact, we already added some similar macro controls in wasm loader to make SIMD only available when JIT or AOT compiler is enabled: https://github.com/bytecodealliance/wasm-micro-runtime/blob/777121217ecf52fa2e73bb64a3ae0dbaf5950cc8/core/iwasm/interpreter/wasm_loader.c#L741-L744 and let loader report error in most cases. I think we can add more if needed.

My concern is that we should handle loader error when aot+simd+fast-interp are all enabled. But if you prefer to disable SIMD to better focus the testing on fast-interp itself, it is also good to me.

As adding the lines you mentioned, I think we had better change to be like:

#if WASM_ENABLE_WAMR_COMPILER == 0 && WASM_ENABLE_JIT == 0 && WASM_ENABLE_AOT == 0 \
    && WASM_ENABLE_SIMD != 0
#error "..."
#endif

And change the cmake files to disable SIMD when non of aot-compiler, jit or aot is enabled. The current default interpreter building enables SIMD and compilation error will be reported if the above lines are added into wasm_loader.c and cmake -DWAMR_BUILD_AOT=0 is used.

having said that, if you (or anyone) plan to make fast interpreter compatible with jit, and/or implement simd for fast interpreter, and want to use this fuzzer target to help the effort, i can understand. is it the case?

No plan to enable simd for fast interpreter yet. Similar to above, if to better focus testing on interpreter itself, we had better also disable aot mode or block the aot file in oss-fuzz when testing interpreter, currently oss-fuzz is very clever and will change the magic header of wasm file to ".aot" and eventually run into aot loader. But as you know, we suppose that aot file should not be modified after compiled, I am afraid that someday it will do fuzz in the aot code and run crashed, and we cannot fix that kind of issue.

yamt commented 4 days ago

My concern is that we should handle loader error when aot+simd+fast-interp are all enabled.

how about having a separate version of is_valid_value_type for interpreter, which rejects v128 unless (WASM_ENABLE_WAMR_COMPILER != 0) || (WASM_ENABLE_JIT != 0) ?

wenyongh commented 4 days ago

My concern is that we should handle loader error when aot+simd+fast-interp are all enabled.

how about having a separate version of is_valid_value_type for interpreter, which rejects v128 unless (WASM_ENABLE_WAMR_COMPILER != 0) || (WASM_ENABLE_JIT != 0) ?

Yes, it makes sense.