[lld] 16aac49 - Revert D105519 "[WebAssembly] Deduplicate imports of the same module name, field name, and type" and its followup
Fangrui Song via llvm-commits
llvm-commits at lists.llvm.org
Mon Jul 19 17:09:08 PDT 2021
Author: Fangrui Song
Date: 2021-07-19T17:09:01-07:00
New Revision: 16aac493e59519377071e900d119ba2e7e5b525d
URL: https://github.com/llvm/llvm-project/commit/16aac493e59519377071e900d119ba2e7e5b525d
DIFF: https://github.com/llvm/llvm-project/commit/16aac493e59519377071e900d119ba2e7e5b525d.diff
LOG: 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.
Added:
Modified:
lld/wasm/Symbols.h
lld/wasm/SyntheticSections.cpp
lld/wasm/SyntheticSections.h
lld/wasm/Writer.cpp
llvm/include/llvm/BinaryFormat/Wasm.h
llvm/include/llvm/BinaryFormat/WasmTraits.h
Removed:
lld/test/wasm/duplicate-function-imports.s
lld/test/wasm/duplicate-global-imports.s
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 e2c942f624088..0000000000000
--- 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
diff erent 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 8f057cd4f19e1..0000000000000
--- 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 b56a319f376e6..0000000000000
--- 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 a883b8ac58657..243f194b75310 100644
--- a/lld/wasm/Symbols.h
+++ b/lld/wasm/Symbols.h
@@ -172,9 +172,6 @@ class Symbol {
bool isStub : 1;
uint32_t flags;
-
- llvm::Optional<StringRef> importName;
- llvm::Optional<StringRef> importModule;
};
class FunctionSymbol : public Symbol {
@@ -225,15 +222,15 @@ class UndefinedFunction : public FunctionSymbol {
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<StringRef> importName;
+ llvm::Optional<StringRef> importModule;
DefinedFunction *stubFunction = nullptr;
bool isCalledDirectly;
};
@@ -357,14 +354,15 @@ class UndefinedGlobal : public GlobalSymbol {
llvm::Optional<StringRef> 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<StringRef> importName;
+ llvm::Optional<StringRef> importModule;
};
class TableSymbol : public Symbol {
@@ -405,14 +403,15 @@ class UndefinedTable : public TableSymbol {
UndefinedTable(StringRef name, llvm::Optional<StringRef> importName,
llvm::Optional<StringRef> 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<StringRef> importName;
+ llvm::Optional<StringRef> 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 ceb196ae1a366..96f7f30553161 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<FunctionSymbol>(sym)) {
- ImportKey<WasmSignature> 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<GlobalSymbol>(sym)) {
- ImportKey<WasmGlobalType> 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<TagSymbol>(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<FunctionSymbol>(sym))
+ f->setFunctionIndex(numImportedFunctions++);
+ else if (auto *g = dyn_cast<GlobalSymbol>(sym))
+ g->setGlobalIndex(numImportedGlobals++);
+ else if (auto *t = dyn_cast<TagSymbol>(sym))
t->setTagIndex(numImportedTags++);
- } else {
- auto *table = cast<TableSymbol>(sym);
- ImportKey<WasmTableType> 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<TableSymbol>(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<UndefinedFunction>(sym)) {
+ import.Field = f->importName ? *f->importName : sym->getName();
+ import.Module = f->importModule ? *f->importModule : defaultModule;
+ } else if (auto *g = dyn_cast<UndefinedGlobal>(sym)) {
+ import.Field = g->importName ? *g->importName : sym->getName();
+ import.Module = g->importModule ? *g->importModule : defaultModule;
+ } else if (auto *t = dyn_cast<UndefinedTable>(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<FunctionSymbol>(sym)) {
import.Kind = WASM_EXTERNAL_FUNCTION;
diff --git a/lld/wasm/SyntheticSections.h b/lld/wasm/SyntheticSections.h
index 8c678fa0853f3..f57fc8832dd28 100644
--- a/lld/wasm/SyntheticSections.h
+++ b/lld/wasm/SyntheticSections.h
@@ -96,68 +96,6 @@ class TypeSection : public SyntheticSection {
llvm::DenseMap<WasmSignature, int32_t> typeIndices;
};
-/**
- * A key for some kind of imported entity of type `T`.
- *
- * Used when de-duplicating imports.
- */
-template <typename T> struct ImportKey {
-public:
- enum class State { Plain, Empty, Tombstone };
-
-public:
- T type;
- llvm::Optional<StringRef> importModule;
- llvm::Optional<StringRef> 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<StringRef> importModule,
- llvm::Optional<StringRef> importName)
- : type(type), importModule(importModule), importName(importName),
- state(State::Plain) {}
-};
-
-template <typename T>
-inline bool operator==(const ImportKey<T> &lhs, const ImportKey<T> &rhs) {
- return lhs.state == rhs.state && lhs.importModule == rhs.importModule &&
- lhs.importName == rhs.importName && lhs.type == rhs.type;
-}
-
-} // namespace wasm
-} // namespace lld
-
-// `ImportKey<T>` can be used as a key in a `DenseMap` if `T` can be used as a
-// key in a `DenseMap`.
-template <typename T> struct llvm::DenseMapInfo<lld::wasm::ImportKey<T>> {
- static lld::wasm::ImportKey<T> getEmptyKey() {
- typename lld::wasm::ImportKey<T> key(llvm::DenseMapInfo<T>::getEmptyKey());
- key.state = lld::wasm::ImportKey<T>::State::Empty;
- return key;
- }
- static lld::wasm::ImportKey<T> getTombstoneKey() {
- typename lld::wasm::ImportKey<T> key(llvm::DenseMapInfo<T>::getEmptyKey());
- key.state = lld::wasm::ImportKey<T>::State::Tombstone;
- return key;
- }
- static unsigned getHashValue(const lld::wasm::ImportKey<T> &key) {
- uintptr_t hash = hash_value(key.importModule);
- hash = hash_combine(hash, key.importName);
- hash = hash_combine(hash, llvm::DenseMapInfo<T>::getHashValue(key.type));
- hash = hash_combine(hash, key.state);
- return hash;
- }
- static bool isEqual(const lld::wasm::ImportKey<T> &lhs,
- const lld::wasm::ImportKey<T> &rhs) {
- return lhs == rhs;
- }
-};
-
-namespace lld {
-namespace wasm {
-
class ImportSection : public SyntheticSection {
public:
ImportSection() : SyntheticSection(llvm::wasm::WASM_SEC_IMPORT) {}
@@ -193,9 +131,6 @@ class ImportSection : public SyntheticSection {
unsigned numImportedFunctions = 0;
unsigned numImportedTags = 0;
unsigned numImportedTables = 0;
- llvm::DenseMap<ImportKey<WasmGlobalType>, uint32_t> importedGlobals;
- llvm::DenseMap<ImportKey<WasmSignature>, uint32_t> importedFunctions;
- llvm::DenseMap<ImportKey<WasmTableType>, uint32_t> importedTables;
};
class FunctionSection : public SyntheticSection {
diff --git a/lld/wasm/Writer.cpp b/lld/wasm/Writer.cpp
index 53fe6b62608c8..42dbc27261f2b 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<UndefinedGlobal>(sym))
+ return g->importName.hasValue();
+ if (auto *f = dyn_cast<UndefinedFunction>(sym))
+ return f->importName.hasValue();
+ if (auto *t = dyn_cast<UndefinedTable>(sym))
+ return t->importName.hasValue();
return false;
}
diff --git a/llvm/include/llvm/BinaryFormat/Wasm.h b/llvm/include/llvm/BinaryFormat/Wasm.h
index 54c34844c5c8a..61544c364a3ba 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 e61ab80884969..e34182499187f 100644
--- a/llvm/include/llvm/BinaryFormat/WasmTraits.h
+++ b/llvm/include/llvm/BinaryFormat/WasmTraits.h
@@ -63,47 +63,6 @@ template <> struct DenseMapInfo<wasm::WasmGlobalType> {
}
};
-// Traits for using WasmLimits in a DenseMap
-template <> struct DenseMapInfo<wasm::WasmLimits> {
- 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<wasm::WasmTableType> {
- static wasm::WasmTableType getEmptyKey() {
- return wasm::WasmTableType{0,
- DenseMapInfo<wasm::WasmLimits>::getEmptyKey()};
- }
- static wasm::WasmTableType getTombstoneKey() {
- return wasm::WasmTableType{
- 1, DenseMapInfo<wasm::WasmLimits>::getTombstoneKey()};
- }
- static unsigned getHashValue(const wasm::WasmTableType &TableType) {
- return hash_combine(
- TableType.ElemType,
- DenseMapInfo<wasm::WasmLimits>::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
More information about the llvm-commits
mailing list