[lld][WebAssembly] Reset context object after each link

[sbc100]

This mirrors how the ELF linker works. I wasn't able to find anywhere where this is currently tested.

Followup to #78640, which triggered a regression.

[llvmbot]

@llvm/pr-subscribers-lld-wasm
@llvm/pr-subscribers-lld-elf

@llvm/pr-subscribers-lld

Author: Sam Clegg (sbc100)

Changes

This mirrors how the ELF linker works. I wasn't able to find anywhere where this is currently tested.

Followup to #78640, which triggered a regression.

---

Full diff: https://github.com/llvm/llvm-project/pull/78770.diff

3 Files Affected:

• (modified) lld/ELF/Driver.cpp (+1-1)
• (modified) lld/wasm/Config.h (+2)
• (modified) lld/wasm/Driver.cpp (+13)

diff --git a/lld/ELF/Driver.cpp b/lld/ELF/Driver.cpp
index 5ccc65600dcb910..9c040de1e47e66b 100644
--- a/lld/ELF/Driver.cpp
+++ b/lld/ELF/Driver.cpp
@@ -130,7 +130,7 @@ bool link(ArrayRef<const char *> args, llvm::raw_ostream &stdoutOS,
 
   ctx->e.initialize(stdoutOS, stderrOS, exitEarly, disableOutput);
   ctx->e.cleanupCallback = []() {
-    elf::ctx.reset();
+    //elf::ctx.reset();
     symtab = SymbolTable();
 
     outputSections.clear();
diff --git a/lld/wasm/Config.h b/lld/wasm/Config.h
index dc7ca265e9a2cb1..308f8ab3eb0ebed 100644
--- a/lld/wasm/Config.h
+++ b/lld/wasm/Config.h
@@ -138,6 +138,8 @@ struct Ctx {
   llvm::SmallVector<std::tuple<std::string, const InputFile *, const Symbol &>,
                     0>
       whyExtractRecords;
+
+  void reset();
 };
 
 extern Ctx ctx;
diff --git a/lld/wasm/Driver.cpp b/lld/wasm/Driver.cpp
index 4a4f9a96227946d..96a214f76ebd70b 100644
--- a/lld/wasm/Driver.cpp
+++ b/lld/wasm/Driver.cpp
@@ -47,6 +47,16 @@ namespace lld::wasm {
 Configuration *config;
 Ctx ctx;
 
+void Ctx::reset() {
+  objectFiles.clear();
+  stubFiles.clear();
+  sharedFiles.clear();
+  bitcodeFiles.clear();
+  syntheticFunctions.clear();
+  syntheticGlobals.clear();
+  syntheticTables.clear();
+}
+
 namespace {
 
 // Create enum with OPT_xxx values for each option in Options.td
@@ -90,6 +100,9 @@ bool link(ArrayRef<const char *> args, llvm::raw_ostream &stdoutOS,
   auto *ctx = new CommonLinkerContext;
 
   ctx->e.initialize(stdoutOS, stderrOS, exitEarly, disableOutput);
+  ctx->e.cleanupCallback = []() {
+    wasm::ctx.reset();
+  };
   ctx->e.logName = args::getFilenameWithoutExe(args[0]);
   ctx->e.errorLimitExceededMsg = "too many errors emitted, stopping now (use "
                                  "-error-limit=0 to see all errors)";

[sbc100]

@steven-johnson

[sbc100]

I wonder if we can replace these elaborate reset() methods with a placement new in the static data location? Would seems less error prone.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[steven-johnson]

I wonder if we can replace these elaborate reset() methods with a placement new in the static data location? Would seems less error prone.

Or just ctx = Ctx(); ?

[sbc100]

I wonder if we can replace these elaborate reset() methods with a placement new in the static data location? Would seems less error prone.

Or just ctx = Ctx(); ?

Yeah, I'm guessing there was some reason the original author didn't take that much simpler approach. I'll go with the same method for now for consistency and we can perhaps followup with a simplification.

[sbc100]

I wonder if we can replace these elaborate reset() methods with a placement new in the static data location? Would seems less error prone.

Or just ctx = Ctx(); ?

Yeah, I'm guessing there was some reason the original author didn't take that much simpler approach. I'll go with the same method for now for consistency and we can perhaps followup with a simplification.

@MaskRay what would you think of such a simplification here? Maybe it would result in a little more allocator churn.. but that doesn't seem like a big concern if it only happens between lldMain operations (i.e. infrequently)

[steven-johnson]

I wonder if we can replace these elaborate reset() methods with a placement new in the static data location? Would seems less error prone.

Or just ctx = Ctx(); ?

Yeah, I'm guessing there was some reason the original author didn't take that much simpler approach. I'll go with the same method for now for consistency and we can perhaps followup with a simplification.

@MaskRay what would you think of such a simplification here? Maybe it would result in a little more allocator churn.. but that doesn't seem like a big concern if it only happens between lldMain operations (i.e. infrequently)

Hard to imagine that the single assignment would be anything more than noise in comparison to doing an object-linkage operation.

[steven-johnson]

Nit: shouldn't we reset all the fields, including the bool fields? (And maybe initialize isPic to some explicit value rather than leaving it as uninited?)

[MaskRay]

LGTM as this seems urgent.

wasm should have a unittest similar to unittests/AsLibELF/SomeDrivers.cpp for library users, which can be done in the future.

[sbc100]

Thanks for the pointer to unittests/AsLibELF/SomeDrivers.cpp. Will land this now and followup there.