From 16aac493e59519377071e900d119ba2e7e5b525d Mon Sep 17 00:00:00 2001 From: Fangrui Song Date: Mon, 19 Jul 2021 17:09:01 -0700 Subject: [PATCH] Revert D105519 "[WebAssembly] Deduplicate imports of the same module name, field name, and type" and its followup This reverts commit 4ae575b9997e0903d1c2ec01a43e3f3f2db5df16 and 9b965b37c75d626c01951184088314590e38d299. There is an use-of-uninitialized-value bug in the `else` branch in ImportSection::addImport. --- lld/test/wasm/duplicate-function-imports.s | 57 ---------------- lld/test/wasm/duplicate-global-imports.s | 69 ------------------- lld/test/wasm/duplicate-table-imports.s | 75 --------------------- lld/wasm/Symbols.h | 29 ++++---- lld/wasm/SyntheticSections.cpp | 58 ++++++---------- lld/wasm/SyntheticSections.h | 65 ------------------ lld/wasm/Writer.cpp | 8 ++- llvm/include/llvm/BinaryFormat/Wasm.h | 9 --- llvm/include/llvm/BinaryFormat/WasmTraits.h | 41 ----------- 9 files changed, 41 insertions(+), 370 deletions(-) delete mode 100644 lld/test/wasm/duplicate-function-imports.s delete mode 100644 lld/test/wasm/duplicate-global-imports.s delete mode 100644 lld/test/wasm/duplicate-table-imports.s diff --git a/lld/test/wasm/duplicate-function-imports.s b/lld/test/wasm/duplicate-function-imports.s deleted file mode 100644 index e2c942f62408..000000000000 --- a/lld/test/wasm/duplicate-function-imports.s +++ /dev/null @@ -1,57 +0,0 @@ -# RUN: llvm-mc -filetype=obj -triple=wasm32-unknown-unknown %s -o %t.o -# RUN: wasm-ld -o %t1.wasm %t.o -# RUN: obj2yaml %t1.wasm | FileCheck %s - -.globl f1 -.import_module f1, env -.import_name f1, f -.functype f1 () -> (i32) - -# Same import module/name/type as `f1`, should be de-duped. -.globl f2 -.import_module f2, env -.import_name f2, f -.functype f2 () -> (i32) - -# Same import module/name, but different type. Should not be de-duped. -.globl f3 -.import_module f3, env -.import_name f3, f -.functype f3 () -> (f32) - -.globl _start -_start: - .functype _start () -> () - call f1 - drop - call f2 - drop - call f3 - drop - end_function - - -# CHECK: - Type: TYPE -# CHECK-NEXT: Signatures: -# CHECK-NEXT: - Index: 0 -# CHECK-NEXT: ParamTypes: [] -# CHECK-NEXT: ReturnTypes: -# CHECK-NEXT: - I32 -# CHECK-NEXT: - Index: 1 -# CHECK-NEXT: ParamTypes: [] -# CHECK-NEXT: ReturnTypes: -# CHECK-NEXT: - F32 -# CHECK-NEXT: - Index: 2 -# CHECK-NEXT: ParamTypes: [] -# CHECK-NEXT: ReturnTypes: [] -# CHECK-NEXT: - Type: IMPORT -# CHECK-NEXT: Imports: -# CHECK-NEXT: - Module: env -# CHECK-NEXT: Field: f -# CHECK-NEXT: Kind: FUNCTION -# CHECK-NEXT: SigIndex: 0 -# CHECK-NEXT: - Module: env -# CHECK-NEXT: Field: f -# CHECK-NEXT: Kind: FUNCTION -# CHECK-NEXT: SigIndex: 1 -# CHECK-NEXT: - Type: diff --git a/lld/test/wasm/duplicate-global-imports.s b/lld/test/wasm/duplicate-global-imports.s deleted file mode 100644 index 8f057cd4f19e..000000000000 --- a/lld/test/wasm/duplicate-global-imports.s +++ /dev/null @@ -1,69 +0,0 @@ -# RUN: llvm-mc -filetype=obj -triple=wasm32-unknown-unknown %s -o %t.o -# RUN: wasm-ld --no-check-features -o %t1.wasm %t.o -# RUN: obj2yaml %t1.wasm | FileCheck %s - -.global g1 -.import_module g1, env -.import_name g1, g -.globaltype g1, i64, immutable - -# Same import module/name/type as `g1`, should be de-duped. -.global g2 -.import_module g2, env -.import_name g2, g -.globaltype g2, i64, immutable - -# Imported as an i32 instead of i64, so should not be de-duped. -.global g3 -.import_module g3, env -.import_name g3, g -.globaltype g3, i32, immutable - -# Imported as mutable instead of immutable, so should not be de-duped. -.global g4 -.import_module g4, env -.import_name g4, g -.globaltype g4, i64 - -.globl _start -_start: - .functype _start () -> () - global.get g1 - drop - global.get g2 - drop - global.get g3 - drop - global.get g4 - drop - end_function - - -# CHECK: - Type: IMPORT -# CHECK-NEXT: Imports: -# CHECK-NEXT: - Module: env -# CHECK-NEXT: Field: g -# CHECK-NEXT: Kind: GLOBAL -# CHECK-NEXT: GlobalType: I64 -# CHECK-NEXT: GlobalMutable: false -# CHECK-NEXT: - Module: env -# CHECK-NEXT: Field: g -# CHECK-NEXT: Kind: GLOBAL -# CHECK-NEXT: GlobalType: I32 -# CHECK-NEXT: GlobalMutable: false -# CHECK-NEXT: - Module: env -# CHECK-NEXT: Field: g -# CHECK-NEXT: Kind: GLOBAL -# CHECK-NEXT: GlobalType: I64 -# CHECK-NEXT: GlobalMutable: true -# CHECK-NEXT: - Type: - -# CHECK: GlobalNames: -# CHECK-NEXT: - Index: 0 -# CHECK-NEXT: Name: g1 -# CHECK-NEXT: - Index: 1 -# CHECK-NEXT: Name: g3 -# CHECK-NEXT: - Index: 2 -# CHECK-NEXT: Name: g4 -# CHECK-NEXT: - Index: 3 -# CHECK-NEXT: Name: __stack_pointer diff --git a/lld/test/wasm/duplicate-table-imports.s b/lld/test/wasm/duplicate-table-imports.s deleted file mode 100644 index b56a319f376e..000000000000 --- a/lld/test/wasm/duplicate-table-imports.s +++ /dev/null @@ -1,75 +0,0 @@ -# RUN: llvm-mc -mattr=+reference-types -triple=wasm32-unknown-unknown -filetype=obj -o %t.o %s -# RUN: wasm-ld --allow-undefined -o %t1.wasm %t.o -# RUN: obj2yaml %t1.wasm | FileCheck %s - -.tabletype t1, funcref -.import_module t1, env -.import_name t1, t -.globl t1 - -# Same import module/name/type as `t1`, should be de-duped. -.tabletype t2, funcref -.import_module t2, env -.import_name t2, t -.globl t2 - -# Imported as an externref instead of funcref, so should not be de-duped. -.tabletype t3, externref -.import_module t3, env -.import_name t3, t -.globl t3 - -.globl _start -_start: - .functype _start () -> () - - # Read from `t1` - i32.const 0 - table.get t1 - drop - - # Read from `t2` - i32.const 0 - table.get t2 - drop - - # Read from `t3` - i32.const 0 - table.get t3 - drop - - end_function - -# XXX: the second imported table has index 1, not 0. I've verified by hand (with -# `wasm2wat`) that the resulting Wasm file is correct: `t3` does end up at index -# 1 and our `table.get` instructions are using the proper table index -# immediates. This is also asserted (less legibly) in the hexdump of the code -# body below. It looks like there's a bug in how `obj2yaml` disassembles -# multiple table imports. - -# CHECK: - Type: IMPORT -# CHECK-NEXT: Imports: -# CHECK-NEXT: - Module: env -# CHECK-NEXT: Field: t -# CHECK-NEXT: Kind: TABLE -# CHECK-NEXT: Table: -# CHECK-NEXT: Index: 0 -# CHECK-NEXT: ElemType: FUNCREF -# CHECK-NEXT: Limits: -# CHECK-NEXT: Minimum: 0x0 -# CHECK-NEXT: - Module: env -# CHECK-NEXT: Field: t -# CHECK-NEXT: Kind: TABLE -# CHECK-NEXT: Table: -# CHECK-NEXT: Index: 0 -# CHECK-NEXT: ElemType: EXTERNREF -# CHECK-NEXT: Limits: -# CHECK-NEXT: Minimum: 0x0 -# CHECK-NEXT: - Type: - -# CHECK: - Type: CODE -# CHECK-NEXT: Functions: -# CHECK-NEXT: - Index: 0 -# CHECK-NEXT: Locals: [] -# CHECK-NEXT: Body: 41002580808080001A41002580808080001A41002581808080001A0B -# CHECK-NEXT: - Type: diff --git a/lld/wasm/Symbols.h b/lld/wasm/Symbols.h index a883b8ac5865..243f194b7531 100644 --- a/lld/wasm/Symbols.h +++ b/lld/wasm/Symbols.h @@ -172,9 +172,6 @@ public: bool isStub : 1; uint32_t flags; - - llvm::Optional importName; - llvm::Optional importModule; }; class FunctionSymbol : public Symbol { @@ -225,15 +222,15 @@ public: const WasmSignature *type = nullptr, bool isCalledDirectly = true) : FunctionSymbol(name, UndefinedFunctionKind, flags, file, type), - isCalledDirectly(isCalledDirectly) { - this->importName = importName; - this->importModule = importModule; - } + importName(importName), importModule(importModule), + isCalledDirectly(isCalledDirectly) {} static bool classof(const Symbol *s) { return s->kind() == UndefinedFunctionKind; } + llvm::Optional importName; + llvm::Optional importModule; DefinedFunction *stubFunction = nullptr; bool isCalledDirectly; }; @@ -357,14 +354,15 @@ public: llvm::Optional importModule, uint32_t flags, InputFile *file = nullptr, const WasmGlobalType *type = nullptr) - : GlobalSymbol(name, UndefinedGlobalKind, flags, file, type) { - this->importName = importName; - this->importModule = importModule; - } + : GlobalSymbol(name, UndefinedGlobalKind, flags, file, type), + importName(importName), importModule(importModule) {} static bool classof(const Symbol *s) { return s->kind() == UndefinedGlobalKind; } + + llvm::Optional importName; + llvm::Optional importModule; }; class TableSymbol : public Symbol { @@ -405,14 +403,15 @@ public: UndefinedTable(StringRef name, llvm::Optional importName, llvm::Optional importModule, uint32_t flags, InputFile *file, const WasmTableType *type) - : TableSymbol(name, UndefinedTableKind, flags, file, type) { - this->importName = importName; - this->importModule = importModule; - } + : TableSymbol(name, UndefinedTableKind, flags, file, type), + importName(importName), importModule(importModule) {} static bool classof(const Symbol *s) { return s->kind() == UndefinedTableKind; } + + llvm::Optional importName; + llvm::Optional importModule; }; // A tag is a general format to distinguish typed entities. Each tag has an diff --git a/lld/wasm/SyntheticSections.cpp b/lld/wasm/SyntheticSections.cpp index ceb196ae1a36..96f7f3055316 100644 --- a/lld/wasm/SyntheticSections.cpp +++ b/lld/wasm/SyntheticSections.cpp @@ -109,42 +109,15 @@ void ImportSection::addGOTEntry(Symbol *sym) { void ImportSection::addImport(Symbol *sym) { assert(!isSealed); - StringRef module = sym->importModule.getValueOr(defaultModule); - StringRef name = sym->importName.getValueOr(sym->getName()); - if (auto *f = dyn_cast(sym)) { - ImportKey key(*(f->getSignature()), module, name); - auto entry = importedFunctions.try_emplace(key, numImportedFunctions); - if (entry.second) { - importedSymbols.emplace_back(sym); - f->setFunctionIndex(numImportedFunctions++); - } else { - f->setFunctionIndex(entry.first->second); - } - } else if (auto *g = dyn_cast(sym)) { - ImportKey key(*(g->getGlobalType()), module, name); - auto entry = importedGlobals.try_emplace(key, numImportedGlobals); - if (entry.second) { - importedSymbols.emplace_back(sym); - g->setGlobalIndex(numImportedGlobals++); - } else { - g->setGlobalIndex(entry.first->second); - } - } else if (auto *t = dyn_cast(sym)) { - // NB: There's currently only one possible kind of tag, and no - // `UndefinedTag`, so we don't bother de-duplicating tag imports. - importedSymbols.emplace_back(sym); + importedSymbols.emplace_back(sym); + if (auto *f = dyn_cast(sym)) + f->setFunctionIndex(numImportedFunctions++); + else if (auto *g = dyn_cast(sym)) + g->setGlobalIndex(numImportedGlobals++); + else if (auto *t = dyn_cast(sym)) t->setTagIndex(numImportedTags++); - } else { - auto *table = cast(sym); - ImportKey key(*(table->getTableType()), module, name); - auto entry = importedTables.try_emplace(key, numImportedTables); - if (entry.second) { - importedSymbols.emplace_back(sym); - table->setTableNumber(numImportedTables++); - } else { - table->setTableNumber(entry.first->second); - } - } + else + cast(sym)->setTableNumber(numImportedTables++); } void ImportSection::writeBody() { @@ -174,8 +147,19 @@ void ImportSection::writeBody() { for (const Symbol *sym : importedSymbols) { WasmImport import; - import.Field = sym->importName.getValueOr(sym->getName()); - import.Module = sym->importModule.getValueOr(defaultModule); + if (auto *f = dyn_cast(sym)) { + import.Field = f->importName ? *f->importName : sym->getName(); + import.Module = f->importModule ? *f->importModule : defaultModule; + } else if (auto *g = dyn_cast(sym)) { + import.Field = g->importName ? *g->importName : sym->getName(); + import.Module = g->importModule ? *g->importModule : defaultModule; + } else if (auto *t = dyn_cast(sym)) { + import.Field = t->importName ? *t->importName : sym->getName(); + import.Module = t->importModule ? *t->importModule : defaultModule; + } else { + import.Field = sym->getName(); + import.Module = defaultModule; + } if (auto *functionSym = dyn_cast(sym)) { import.Kind = WASM_EXTERNAL_FUNCTION; diff --git a/lld/wasm/SyntheticSections.h b/lld/wasm/SyntheticSections.h index 8c678fa0853f..f57fc8832dd2 100644 --- a/lld/wasm/SyntheticSections.h +++ b/lld/wasm/SyntheticSections.h @@ -96,68 +96,6 @@ protected: llvm::DenseMap typeIndices; }; -/** - * A key for some kind of imported entity of type `T`. - * - * Used when de-duplicating imports. - */ -template struct ImportKey { -public: - enum class State { Plain, Empty, Tombstone }; - -public: - T type; - llvm::Optional importModule; - llvm::Optional importName; - State state; - -public: - ImportKey(T type) : type(type), state(State::Plain) {} - ImportKey(T type, State state) : type(type), state(state) {} - ImportKey(T type, llvm::Optional importModule, - llvm::Optional importName) - : type(type), importModule(importModule), importName(importName), - state(State::Plain) {} -}; - -template -inline bool operator==(const ImportKey &lhs, const ImportKey &rhs) { - return lhs.state == rhs.state && lhs.importModule == rhs.importModule && - lhs.importName == rhs.importName && lhs.type == rhs.type; -} - -} // namespace wasm -} // namespace lld - -// `ImportKey` can be used as a key in a `DenseMap` if `T` can be used as a -// key in a `DenseMap`. -template struct llvm::DenseMapInfo> { - static lld::wasm::ImportKey getEmptyKey() { - typename lld::wasm::ImportKey key(llvm::DenseMapInfo::getEmptyKey()); - key.state = lld::wasm::ImportKey::State::Empty; - return key; - } - static lld::wasm::ImportKey getTombstoneKey() { - typename lld::wasm::ImportKey key(llvm::DenseMapInfo::getEmptyKey()); - key.state = lld::wasm::ImportKey::State::Tombstone; - return key; - } - static unsigned getHashValue(const lld::wasm::ImportKey &key) { - uintptr_t hash = hash_value(key.importModule); - hash = hash_combine(hash, key.importName); - hash = hash_combine(hash, llvm::DenseMapInfo::getHashValue(key.type)); - hash = hash_combine(hash, key.state); - return hash; - } - static bool isEqual(const lld::wasm::ImportKey &lhs, - const lld::wasm::ImportKey &rhs) { - return lhs == rhs; - } -}; - -namespace lld { -namespace wasm { - class ImportSection : public SyntheticSection { public: ImportSection() : SyntheticSection(llvm::wasm::WASM_SEC_IMPORT) {} @@ -193,9 +131,6 @@ protected: unsigned numImportedFunctions = 0; unsigned numImportedTags = 0; unsigned numImportedTables = 0; - llvm::DenseMap, uint32_t> importedGlobals; - llvm::DenseMap, uint32_t> importedFunctions; - llvm::DenseMap, uint32_t> importedTables; }; class FunctionSection : public SyntheticSection { diff --git a/lld/wasm/Writer.cpp b/lld/wasm/Writer.cpp index 53fe6b62608c..42dbc27261f2 100644 --- a/lld/wasm/Writer.cpp +++ b/lld/wasm/Writer.cpp @@ -562,8 +562,12 @@ static bool shouldImport(Symbol *sym) { return true; if (config->allowUndefinedSymbols.count(sym->getName()) != 0) return true; - if (sym->isUndefined()) - return sym->importName.hasValue(); + if (auto *g = dyn_cast(sym)) + return g->importName.hasValue(); + if (auto *f = dyn_cast(sym)) + return f->importName.hasValue(); + if (auto *t = dyn_cast(sym)) + return t->importName.hasValue(); return false; } diff --git a/llvm/include/llvm/BinaryFormat/Wasm.h b/llvm/include/llvm/BinaryFormat/Wasm.h index 54c34844c5c8..61544c364a3b 100644 --- a/llvm/include/llvm/BinaryFormat/Wasm.h +++ b/llvm/include/llvm/BinaryFormat/Wasm.h @@ -67,20 +67,11 @@ struct WasmLimits { uint64_t Maximum; }; -inline bool operator==(const WasmLimits &LHS, const WasmLimits &RHS) { - return LHS.Flags == RHS.Flags && LHS.Minimum == RHS.Minimum && - LHS.Maximum == RHS.Maximum; -} - struct WasmTableType { uint8_t ElemType; WasmLimits Limits; }; -inline bool operator==(const WasmTableType &LHS, const WasmTableType &RHS) { - return LHS.ElemType == RHS.ElemType && LHS.Limits == RHS.Limits; -} - struct WasmTable { uint32_t Index; WasmTableType Type; diff --git a/llvm/include/llvm/BinaryFormat/WasmTraits.h b/llvm/include/llvm/BinaryFormat/WasmTraits.h index e61ab8088496..e34182499187 100644 --- a/llvm/include/llvm/BinaryFormat/WasmTraits.h +++ b/llvm/include/llvm/BinaryFormat/WasmTraits.h @@ -63,47 +63,6 @@ template <> struct DenseMapInfo { } }; -// Traits for using WasmLimits in a DenseMap -template <> struct DenseMapInfo { - static wasm::WasmLimits getEmptyKey() { - return wasm::WasmLimits{0xff, 0xff, 0xff}; - } - static wasm::WasmLimits getTombstoneKey() { - return wasm::WasmLimits{0xee, 0xee, 0xee}; - } - static unsigned getHashValue(const wasm::WasmLimits &Limits) { - unsigned Hash = hash_value(Limits.Flags); - Hash = hash_combine(Hash, Limits.Minimum); - Hash = hash_combine(Hash, Limits.Maximum); - return Hash; - } - static bool isEqual(const wasm::WasmLimits &LHS, - const wasm::WasmLimits &RHS) { - return LHS == RHS; - } -}; - -// Traits for using WasmTableType in a DenseMap -template <> struct DenseMapInfo { - static wasm::WasmTableType getEmptyKey() { - return wasm::WasmTableType{0, - DenseMapInfo::getEmptyKey()}; - } - static wasm::WasmTableType getTombstoneKey() { - return wasm::WasmTableType{ - 1, DenseMapInfo::getTombstoneKey()}; - } - static unsigned getHashValue(const wasm::WasmTableType &TableType) { - return hash_combine( - TableType.ElemType, - DenseMapInfo::getHashValue(TableType.Limits)); - } - static bool isEqual(const wasm::WasmTableType &LHS, - const wasm::WasmTableType &RHS) { - return LHS == RHS; - } -}; - } // end namespace llvm #endif // LLVM_BINARYFORMAT_WASMTRAITS_H