From 98ed1f9ca97882c2566646c67cdfabaa71d7ac5c Mon Sep 17 00:00:00 2001 From: kschimpf Date: Fri, 24 Mar 2017 18:54:09 -0700 Subject: [PATCH] Hide WasmModule.origin field behind readable accessors. Besides adding accessors get_origin() and set_origin(), it creates easier test accessors is_wasm() and is_asm_js(). This allows the possibility of caching boolean flags for is_wasm() and is_asm_js() without having to change any code except for the files containing the class definition for WasmModule. BUG= v8:6152 R=bbudge@chromium.org,mtrofin@chromium.org Review-Url: https://codereview.chromium.org/2771803005 Cr-Commit-Position: refs/heads/master@{#44130} --- src/compiler/wasm-compiler.cc | 4 +-- src/wasm/function-body-decoder.cc | 20 ++++++------- src/wasm/module-decoder.cc | 2 +- src/wasm/wasm-module.cc | 30 +++++++++---------- src/wasm/wasm-module.h | 12 ++++++-- src/wasm/wasm-objects.cc | 9 +++--- test/cctest/wasm/wasm-run-utils.h | 8 ++--- .../wasm/function-body-decoder-unittest.cc | 2 +- 8 files changed, 47 insertions(+), 40 deletions(-) diff --git a/src/compiler/wasm-compiler.cc b/src/compiler/wasm-compiler.cc index 34d700ce68..790e1d5cad 100644 --- a/src/compiler/wasm-compiler.cc +++ b/src/compiler/wasm-compiler.cc @@ -2856,7 +2856,7 @@ Node* WasmGraphBuilder::MemBuffer(uint32_t offset) { Node* WasmGraphBuilder::CurrentMemoryPages() { // CurrentMemoryPages will not be called from asm.js, hence we cannot be in // lazy-compilation mode, hence the instance will be set. - DCHECK_EQ(wasm::kWasmOrigin, module_->module->origin); + DCHECK_EQ(wasm::kWasmOrigin, module_->module->get_origin()); DCHECK_NOT_NULL(module_); DCHECK_NOT_NULL(module_->instance); @@ -3967,7 +3967,7 @@ void WasmCompilationUnit::ExecuteCompilation() { } job_.reset(Pipeline::NewWasmCompilationJob( &info_, jsgraph_, descriptor, source_positions, &protected_instructions_, - module_env_->module->origin != wasm::kWasmOrigin)); + !module_env_->module->is_wasm())); ok_ = job_->ExecuteJob() == CompilationJob::SUCCEEDED; // TODO(bradnelson): Improve histogram handling of size_t. // TODO(ahaas): The counters are not thread-safe at the moment. diff --git a/src/wasm/function-body-decoder.cc b/src/wasm/function-body-decoder.cc index 71605d0d78..ba86133155 100644 --- a/src/wasm/function-body-decoder.cc +++ b/src/wasm/function-body-decoder.cc @@ -35,13 +35,13 @@ namespace wasm { #define TRACE(...) #endif -#define CHECK_PROTOTYPE_OPCODE(flag) \ - if (module_ != nullptr && module_->origin == kAsmJsOrigin) { \ - error("Opcode not supported for asmjs modules"); \ - } \ - if (!FLAG_##flag) { \ - error("Invalid opcode (enable with --" #flag ")"); \ - break; \ +#define CHECK_PROTOTYPE_OPCODE(flag) \ + if (module_ != nullptr && module_->is_asm_js()) { \ + error("Opcode not supported for asmjs modules"); \ + } \ + if (!FLAG_##flag) { \ + error("Invalid opcode (enable with --" #flag ")"); \ + break; \ } // An SsaEnv environment carries the current local variable renaming @@ -1194,7 +1194,7 @@ class WasmFullDecoder : public WasmDecoder { if (!CheckHasMemory()) break; MemoryIndexOperand operand(this, pc_); DCHECK_NOT_NULL(module_); - if (module_->origin != kAsmJsOrigin) { + if (module_->is_wasm()) { Value val = Pop(0, kWasmI32); Push(kWasmI32, BUILD(GrowMemory, val.node)); } else { @@ -1245,7 +1245,7 @@ class WasmFullDecoder : public WasmDecoder { break; } case kAtomicPrefix: { - if (module_ == nullptr || module_->origin != kAsmJsOrigin) { + if (module_ == nullptr || !module_->is_asm_js()) { error("Atomics are allowed only in AsmJs modules"); break; } @@ -1264,7 +1264,7 @@ class WasmFullDecoder : public WasmDecoder { } default: { // Deal with special asmjs opcodes. - if (module_ != nullptr && module_->origin == kAsmJsOrigin) { + if (module_ != nullptr && module_->is_asm_js()) { sig = WasmOpcodes::AsmjsSignature(opcode); if (sig) { BuildSimpleOperator(opcode, sig); diff --git a/src/wasm/module-decoder.cc b/src/wasm/module-decoder.cc index ada0c60e4f..d6f40ccd9e 100644 --- a/src/wasm/module-decoder.cc +++ b/src/wasm/module-decoder.cc @@ -259,7 +259,7 @@ class ModuleDecoder : public Decoder { module->min_mem_pages = 0; module->max_mem_pages = 0; module->mem_export = false; - module->origin = origin_; + module->set_origin(origin_); const byte* pos = pc_; uint32_t magic_word = consume_u32("wasm magic"); diff --git a/src/wasm/wasm-module.cc b/src/wasm/wasm-module.cc index ef789fbbb8..d1f0760825 100644 --- a/src/wasm/wasm-module.cc +++ b/src/wasm/wasm-module.cc @@ -290,8 +290,8 @@ Handle EnsureTableExportLazyDeoptData( } bool compile_lazy(const WasmModule* module) { - return FLAG_wasm_lazy_compilation || (FLAG_asm_wasm_lazy_compilation && - module->origin == wasm::kAsmJsOrigin); + return FLAG_wasm_lazy_compilation || + (FLAG_asm_wasm_lazy_compilation && module->is_asm_js()); } // A helper for compiling an entire module. @@ -514,7 +514,7 @@ class CompilationHelper { } HistogramTimerScope wasm_compile_module_time_scope( - module_->origin == ModuleOrigin::kWasmOrigin + module_->is_wasm() ? isolate_->counters()->wasm_compile_wasm_module_time() : isolate_->counters()->wasm_compile_asm_module_time()); @@ -1157,7 +1157,7 @@ class InstantiationHelper { // Record build time into correct bucket, then build instance. HistogramTimerScope wasm_instantiate_module_time_scope( - module_->origin == ModuleOrigin::kWasmOrigin + module_->is_wasm() ? isolate_->counters()->wasm_instantiate_wasm_module_time() : isolate_->counters()->wasm_instantiate_asm_module_time()); Factory* factory = isolate_->factory(); @@ -1319,8 +1319,8 @@ class InstantiationHelper { // Set externally passed ArrayBuffer non neuterable. memory_->set_is_neuterable(false); - DCHECK_IMPLIES(EnableGuardRegions(), module_->origin == kAsmJsOrigin || - memory_->has_guard_region()); + DCHECK_IMPLIES(EnableGuardRegions(), + module_->is_asm_js() || memory_->has_guard_region()); } else if (min_mem_pages > 0) { memory_ = AllocateMemory(min_mem_pages); if (memory_.is_null()) return {}; // failed to allocate memory @@ -1706,7 +1706,7 @@ class InstantiationHelper { Handle import_wrapper = CompileImportWrapper( isolate_, index, module_->functions[import.index].sig, Handle::cast(value), module_name, import_name, - module_->origin); + module_->get_origin()); if (import_wrapper.is_null()) { ReportLinkError( "imported function does not match the expected type", index, @@ -1940,10 +1940,10 @@ class InstantiationHelper { } Handle exports_object; - if (module_->origin == kWasmOrigin) { + if (module_->is_wasm()) { // Create the "exports" object. exports_object = isolate_->factory()->NewJSObjectWithNullProto(); - } else if (module_->origin == kAsmJsOrigin) { + } else if (module_->is_asm_js()) { Handle object_function = Handle( isolate_->native_context()->object_function(), isolate_); exports_object = isolate_->factory()->NewJSObject(object_function); @@ -1962,7 +1962,7 @@ class InstantiationHelper { wasm::AsmWasmBuilder::single_function_name); PropertyDescriptor desc; - desc.set_writable(module_->origin == kAsmJsOrigin); + desc.set_writable(module_->is_asm_js()); desc.set_enumerable(true); // Count up export indexes. @@ -1992,7 +1992,7 @@ class InstantiationHelper { isolate_, compiled_module_, exp.name_offset, exp.name_length) .ToHandleChecked(); Handle export_to; - if (module_->origin == kAsmJsOrigin && exp.kind == kExternalFunction && + if (module_->is_asm_js() && exp.kind == kExternalFunction && (String::Equals(name, foreign_init_name) || String::Equals(name, single_function_name))) { export_to = instance; @@ -2012,7 +2012,7 @@ class InstantiationHelper { Handle export_code = code_table->GetValueChecked(isolate_, func_index); MaybeHandle func_name; - if (module_->origin == kAsmJsOrigin) { + if (module_->is_asm_js()) { // For modules arising from asm.js, honor the names section. func_name = WasmCompiledModule::ExtractUtf8StringFromModuleBytes( isolate_, compiled_module_, function.name_offset, @@ -2095,7 +2095,7 @@ class InstantiationHelper { } // Skip duplicates for asm.js. - if (module_->origin == kAsmJsOrigin) { + if (module_->is_asm_js()) { v8::Maybe status = JSReceiver::HasOwnProperty(export_to, name); if (status.FromMaybe(false)) { continue; @@ -2110,7 +2110,7 @@ class InstantiationHelper { } } - if (module_->origin == kWasmOrigin) { + if (module_->is_wasm()) { v8::Maybe success = JSReceiver::SetIntegrityLevel( exports_object, FROZEN, Object::DONT_THROW); DCHECK(success.FromMaybe(false)); @@ -2245,7 +2245,7 @@ class InstantiationHelper { js_to_wasm_cache_.CloneOrCompileJSToWasmWrapper( isolate_, module_, wasm_code, func_index); MaybeHandle func_name; - if (module_->origin == kAsmJsOrigin) { + if (module_->is_asm_js()) { // For modules arising from asm.js, honor the names section. func_name = WasmCompiledModule::ExtractUtf8StringFromModuleBytes( diff --git a/src/wasm/wasm-module.h b/src/wasm/wasm-module.h index bfedbff61f..ca0be798cc 100644 --- a/src/wasm/wasm-module.h +++ b/src/wasm/wasm-module.h @@ -154,7 +154,6 @@ struct V8_EXPORT_PRIVATE WasmModule { // the fact that we index on uint32_t, so we may technically not be // able to represent some start_function_index -es. int start_function_index = -1; // start function, if any - ModuleOrigin origin = kWasmOrigin; // origin of the module std::vector globals; // globals in this module. uint32_t globals_size = 0; // size of globals table. @@ -182,6 +181,15 @@ struct V8_EXPORT_PRIVATE WasmModule { ~WasmModule() { if (owned_zone) delete owned_zone; } + + ModuleOrigin get_origin() const { return origin_; } + void set_origin(ModuleOrigin new_value) { origin_ = new_value; } + bool is_wasm() const { return origin_ == kWasmOrigin; } + bool is_asm_js() const { return origin_ == kAsmJsOrigin; } + + private: + // TODO(kschimpf) - Encapsulate more fields. + ModuleOrigin origin_ = kWasmOrigin; // origin of the module }; typedef Managed WasmModuleWrapper; @@ -318,7 +326,7 @@ struct V8_EXPORT_PRIVATE ModuleEnv { return &module->function_tables[index]; } - bool asm_js() { return module->origin == kAsmJsOrigin; } + bool asm_js() { return module->is_asm_js(); } // Only used for testing. Handle GetFunctionCode(uint32_t index) { diff --git a/src/wasm/wasm-objects.cc b/src/wasm/wasm-objects.cc index 619c0a0845..8e67d10290 100644 --- a/src/wasm/wasm-objects.cc +++ b/src/wasm/wasm-objects.cc @@ -221,10 +221,9 @@ bool IsBreakablePosition(Handle compiled_module, Handle WasmModuleObject::New( Isolate* isolate, Handle compiled_module) { - ModuleOrigin origin = compiled_module->module()->origin; - + WasmModule* module = compiled_module->module(); Handle module_object; - if (origin == ModuleOrigin::kWasmOrigin) { + if (module->is_wasm()) { Handle module_cons( isolate->native_context()->wasm_module_constructor()); module_object = isolate->factory()->NewJSObject(module_cons); @@ -232,7 +231,7 @@ Handle WasmModuleObject::New( Object::SetProperty(module_object, module_sym, module_object, STRICT) .Check(); } else { - DCHECK(origin == ModuleOrigin::kAsmJsOrigin); + DCHECK(module->is_asm_js()); Handle map = isolate->factory()->NewMap( JS_OBJECT_TYPE, JSObject::kHeaderSize + WasmModuleObject::kFieldCount * kPointerSize); @@ -584,7 +583,7 @@ Handle WasmSharedModuleData::New( } bool WasmSharedModuleData::is_asm_js() { - bool asm_js = module()->origin == wasm::ModuleOrigin::kAsmJsOrigin; + bool asm_js = module()->is_asm_js(); DCHECK_EQ(asm_js, script()->IsUserJavaScript()); DCHECK_EQ(asm_js, has_asm_js_offset_table()); return asm_js; diff --git a/test/cctest/wasm/wasm-run-utils.h b/test/cctest/wasm/wasm-run-utils.h index 8bc28c23f1..8b678efffe 100644 --- a/test/cctest/wasm/wasm-run-utils.h +++ b/test/cctest/wasm/wasm-run-utils.h @@ -98,7 +98,7 @@ class TestingModule : public ModuleEnv { ~TestingModule() { if (instance->mem_start) { - if (EnableGuardRegions() && module_.origin == kWasmOrigin) { + if (EnableGuardRegions() && module_.is_wasm()) { // See the corresponding code in AddMemory. We use a different // allocation path when guard regions are enabled, which means we have // to free it differently too. @@ -112,14 +112,14 @@ class TestingModule : public ModuleEnv { if (interpreter_) delete interpreter_; } - void ChangeOriginToAsmjs() { module_.origin = kAsmJsOrigin; } + void ChangeOriginToAsmjs() { module_.set_origin(kAsmJsOrigin); } byte* AddMemory(uint32_t size) { CHECK(!module_.has_memory); CHECK_NULL(instance->mem_start); CHECK_EQ(0, instance->mem_size); module_.has_memory = true; - if (EnableGuardRegions() && module_.origin == kWasmOrigin) { + if (EnableGuardRegions() && module_.is_wasm()) { const size_t alloc_size = RoundUp(kWasmMaxHeapOffset, v8::base::OS::CommitPageSize()); instance->mem_start = reinterpret_cast( @@ -235,7 +235,7 @@ class TestingModule : public ModuleEnv { uint32_t index = AddFunction(sig, Handle::null(), nullptr); Handle code = CompileWasmToJSWrapper( isolate_, jsfunc, sig, index, Handle::null(), - Handle::null(), module->origin); + Handle::null(), module->get_origin()); instance->function_code[index] = code; return index; } diff --git a/test/unittests/wasm/function-body-decoder-unittest.cc b/test/unittests/wasm/function-body-decoder-unittest.cc index 5e382f92de..0b54175fef 100644 --- a/test/unittests/wasm/function-body-decoder-unittest.cc +++ b/test/unittests/wasm/function-body-decoder-unittest.cc @@ -211,7 +211,7 @@ class TestModuleEnv : public ModuleEnv { public: explicit TestModuleEnv(ModuleOrigin origin = kWasmOrigin) : ModuleEnv(&mod, nullptr) { - mod.origin = origin; + mod.set_origin(origin); } byte AddGlobal(ValueType type, bool mutability = true) { mod.globals.push_back({type, mutability, WasmInitExpr(), 0, false, false});