From 2ccc3e076162ec5dd63d21841cd8a6c51000084d Mon Sep 17 00:00:00 2001 From: Thomas Lively Date: Tue, 28 Jun 2022 17:27:54 -0700 Subject: [PATCH 1/2] Disallow --nominal with GC Nominal types don't make much sense without GC, and in particular trying to emit them with typed function references but not GC enabled can result in invalid binaries because nominal types do not respect the type ordering constraints required by the typed function references proposal. Making this change was mostly straightforward, but required fixing the fuzzer to use --nominal only when GC is enabled and required exiting early from nominal-only optimizations when GC was not enabled. Fixes #4756. --- scripts/fuzz_opt.py | 23 +++++++++++------------ src/passes/ConstantFieldPropagation.cpp | 3 +++ src/passes/GlobalRefining.cpp | 3 +++ src/passes/GlobalStructInference.cpp | 3 +++ src/passes/GlobalTypeOptimization.cpp | 3 +++ src/passes/SignaturePruning.cpp | 3 +++ src/passes/SignatureRefining.cpp | 3 +++ src/passes/TypeRefining.cpp | 3 +++ src/tools/tool-options.h | 6 ++++++ test/lit/nominal-no-gc.wast | 21 +++------------------ 10 files changed, 41 insertions(+), 30 deletions(-) diff --git a/scripts/fuzz_opt.py b/scripts/fuzz_opt.py index 7450657327d..1ff5377c3cc 100755 --- a/scripts/fuzz_opt.py +++ b/scripts/fuzz_opt.py @@ -39,7 +39,6 @@ # feature options that are always passed to the tools. CONSTANT_FEATURE_OPTS = ['--all-features'] -CONSTANT_FEATURE_OPTS.append(TYPE_SYSTEM_FLAG) INPUT_SIZE_MIN = 1024 INPUT_SIZE_MEAN = 40 * 1024 @@ -120,6 +119,9 @@ def randomize_feature_opts(): if possible in IMPLIED_FEATURE_OPTS: FEATURE_OPTS.extend(IMPLIED_FEATURE_OPTS[possible]) print('randomized feature opts:', '\n ' + '\n '.join(FEATURE_OPTS)) + # Type system flags only make sense when GC is enabled + if "--disable-gc" not in FEATURE_OPTS: + FEATURE_OPTS.append(TYPE_SYSTEM_FLAG) ALL_FEATURE_OPTS = ['--all-features', '-all', '--mvp-features', '-mvp'] @@ -819,8 +821,8 @@ def handle_pair(self, input, before_wasm, after_wasm, opts): b1 = open('b1.wasm', 'rb').read() b2 = open('b2.wasm', 'rb').read() if (b1 != b2): - run([in_bin('wasm-dis'), 'b1.wasm', '-o', 'b1.wat', TYPE_SYSTEM_FLAG]) - run([in_bin('wasm-dis'), 'b2.wasm', '-o', 'b2.wat', TYPE_SYSTEM_FLAG]) + run([in_bin('wasm-dis'), 'b1.wasm', '-o', 'b1.wat', FEATURE_OPTS]) + run([in_bin('wasm-dis'), 'b2.wasm', '-o', 'b2.wat', FEATURE_OPTS]) t1 = open('b1.wat', 'r').read() t2 = open('b2.wat', 'r').read() compare(t1, t2, 'Output must be deterministic.', verbose=False) @@ -1379,10 +1381,8 @@ def randomize_opt_flags(): with open('reduce.sh', 'w') as reduce_sh: reduce_sh.write('''\ # check the input is even a valid wasm file -echo "At least one of the next two values should be 0:" -%(wasm_opt)s %(typesystem)s --detect-features %(temp_wasm)s -echo " " $? -%(wasm_opt)s %(typesystem)s --all-features %(temp_wasm)s +echo "The following value should be 0:" +%(wasm_opt)s %(features)s %(temp_wasm)s echo " " $? # run the command @@ -1419,7 +1419,7 @@ def randomize_opt_flags(): 'auto_init': auto_init, 'original_wasm': original_wasm, 'temp_wasm': os.path.abspath('t.wasm'), - 'typesystem': TYPE_SYSTEM_FLAG, + 'features': ' '.join(FEATURE_OPTS), 'reduce_sh': os.path.abspath('reduce.sh')}) print('''\ @@ -1441,7 +1441,7 @@ def randomize_opt_flags(): vvvv -%(wasm_reduce)s %(type_system_flag)s %(original_wasm)s '--command=bash %(reduce_sh)s' -t %(temp_wasm)s -w %(working_wasm)s +%(wasm_reduce)s %(features)s %(original_wasm)s '--command=bash %(reduce_sh)s' -t %(temp_wasm)s -w %(working_wasm)s ^^^^ @@ -1449,9 +1449,8 @@ def randomize_opt_flags(): Make sure to verify by eye that the output says something like this: -At least one of the next two values should be 0: +The following value should be 0: 0 - 1 The following value should be 1: 1 @@ -1471,7 +1470,7 @@ def randomize_opt_flags(): 'working_wasm': os.path.abspath('w.wasm'), 'wasm_reduce': in_bin('wasm-reduce'), 'reduce_sh': os.path.abspath('reduce.sh'), - 'type_system_flag': TYPE_SYSTEM_FLAG}) + 'features': ' '.join(FEATURE_OPTS)}) break if given_seed is not None: break diff --git a/src/passes/ConstantFieldPropagation.cpp b/src/passes/ConstantFieldPropagation.cpp index 2b314eecc66..d2b08ea30f6 100644 --- a/src/passes/ConstantFieldPropagation.cpp +++ b/src/passes/ConstantFieldPropagation.cpp @@ -176,6 +176,9 @@ struct PCVScanner struct ConstantFieldPropagation : public Pass { void run(PassRunner* runner, Module* module) override { + if (!module->features.hasGC()) { + return; + } if (getTypeSystem() != TypeSystem::Nominal) { Fatal() << "ConstantFieldPropagation requires nominal typing"; } diff --git a/src/passes/GlobalRefining.cpp b/src/passes/GlobalRefining.cpp index e43f8ef5949..6f2f198429d 100644 --- a/src/passes/GlobalRefining.cpp +++ b/src/passes/GlobalRefining.cpp @@ -32,6 +32,9 @@ namespace { struct GlobalRefining : public Pass { void run(PassRunner* runner, Module* module) override { + if (!module->features.hasGC()) { + return; + } if (getTypeSystem() != TypeSystem::Nominal) { Fatal() << "GlobalRefining requires nominal typing"; } diff --git a/src/passes/GlobalStructInference.cpp b/src/passes/GlobalStructInference.cpp index c7eec2b94e1..b2717b850eb 100644 --- a/src/passes/GlobalStructInference.cpp +++ b/src/passes/GlobalStructInference.cpp @@ -62,6 +62,9 @@ struct GlobalStructInference : public Pass { std::unordered_map> typeGlobals; void run(PassRunner* runner, Module* module) override { + if (!module->features.hasGC()) { + return; + } if (getTypeSystem() != TypeSystem::Nominal) { Fatal() << "GlobalStructInference requires nominal typing"; } diff --git a/src/passes/GlobalTypeOptimization.cpp b/src/passes/GlobalTypeOptimization.cpp index 9b2a1f269a0..865fe8f6970 100644 --- a/src/passes/GlobalTypeOptimization.cpp +++ b/src/passes/GlobalTypeOptimization.cpp @@ -112,6 +112,9 @@ struct GlobalTypeOptimization : public Pass { std::unordered_map> indexesAfterRemovals; void run(PassRunner* runner, Module* module) override { + if (!module->features.hasGC()) { + return; + } if (getTypeSystem() != TypeSystem::Nominal) { Fatal() << "GlobalTypeOptimization requires nominal typing"; } diff --git a/src/passes/SignaturePruning.cpp b/src/passes/SignaturePruning.cpp index 4f142eb9740..ecf6d7e21d4 100644 --- a/src/passes/SignaturePruning.cpp +++ b/src/passes/SignaturePruning.cpp @@ -48,6 +48,9 @@ struct SignaturePruning : public Pass { std::unordered_map newSignatures; void run(PassRunner* runner, Module* module) override { + if (!module->features.hasGC()) { + return; + } if (getTypeSystem() != TypeSystem::Nominal) { Fatal() << "SignaturePruning requires nominal typing"; } diff --git a/src/passes/SignatureRefining.cpp b/src/passes/SignatureRefining.cpp index 0574df43486..8a8c4587277 100644 --- a/src/passes/SignatureRefining.cpp +++ b/src/passes/SignatureRefining.cpp @@ -48,6 +48,9 @@ struct SignatureRefining : public Pass { std::unordered_map newSignatures; void run(PassRunner* runner, Module* module) override { + if (!module->features.hasGC()) { + return; + } if (getTypeSystem() != TypeSystem::Nominal) { Fatal() << "SignatureRefining requires nominal typing"; } diff --git a/src/passes/TypeRefining.cpp b/src/passes/TypeRefining.cpp index 0f5169f8f2b..c755c1af6db 100644 --- a/src/passes/TypeRefining.cpp +++ b/src/passes/TypeRefining.cpp @@ -78,6 +78,9 @@ struct TypeRefining : public Pass { StructUtils::StructValuesMap finalInfos; void run(PassRunner* runner, Module* module) override { + if (!module->features.hasGC()) { + return; + } if (getTypeSystem() != TypeSystem::Nominal) { Fatal() << "TypeRefining requires nominal typing"; } diff --git a/src/tools/tool-options.h b/src/tools/tool-options.h index bc948fc97c7..c15291c8ddd 100644 --- a/src/tools/tool-options.h +++ b/src/tools/tool-options.h @@ -176,6 +176,12 @@ struct ToolOptions : public Options { void applyFeatures(Module& module) const { module.features.enable(enabledFeatures); module.features.disable(disabledFeatures); + // Non-default type systems only make sense with GC enabled. TODO: Error on + // non-GC equirecursive types as well once we make isorecursive the default + // if we don't remove equirecursive types entirely. + if (!module.features.hasGC() && getTypeSystem() == TypeSystem::Nominal) { + Fatal() << "Nominal typing is only allowed when GC is enabled"; + } } private: diff --git a/test/lit/nominal-no-gc.wast b/test/lit/nominal-no-gc.wast index 7247caa5a04..82f2698d9da 100644 --- a/test/lit/nominal-no-gc.wast +++ b/test/lit/nominal-no-gc.wast @@ -1,20 +1,5 @@ -;; Write the module with --nominal but without GC -;; RUN: wasm-opt %s --nominal --disable-gc -g -o %t.wasm +;; Using --nominal without GC is not allowed. -;; We should not get any recursion groups even though we used --nominal. We use -;; --hybrid -all here to make sure that any rec groups from the binary will -;; actually show up in the output and cause the test to fail. -;; RUN: wasm-opt %t.wasm --hybrid -all -S -o - | filecheck %s +;; RUN: not wasm-opt %s --nominal --disable-gc -g -o %t.wasm 2>&1 | filecheck %s -;; Also check that we don't get a failure with the default configuration. -;; RUN: wasm-opt %t.wasm - -;; CHECK-NOT: rec - -(module - (type $foo (func)) - (type $bar (func)) - - (func $f1 (type $foo)) - (func $f2 (type $bar)) -) +;; CHECK: Nominal typing is only allowed when GC is enabled From 504a5410c7d365a9698b13046cb39035d867c0d4 Mon Sep 17 00:00:00 2001 From: Thomas Lively Date: Tue, 28 Jun 2022 18:21:43 -0700 Subject: [PATCH 2/2] Single quotes --- scripts/fuzz_opt.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/fuzz_opt.py b/scripts/fuzz_opt.py index 1ff5377c3cc..01895c585f1 100755 --- a/scripts/fuzz_opt.py +++ b/scripts/fuzz_opt.py @@ -120,7 +120,7 @@ def randomize_feature_opts(): FEATURE_OPTS.extend(IMPLIED_FEATURE_OPTS[possible]) print('randomized feature opts:', '\n ' + '\n '.join(FEATURE_OPTS)) # Type system flags only make sense when GC is enabled - if "--disable-gc" not in FEATURE_OPTS: + if '--disable-gc' not in FEATURE_OPTS: FEATURE_OPTS.append(TYPE_SYSTEM_FLAG)