[lld][wasm] Clear lazyBitcodeFiles while resetting context

[anutosh491]

Hi @sbc100

I was looking into a use case involving the link function (which got my attention to reset).

I see that lazyBitcodeFiles variable was introduced here #114327 but I don't see it being reset while destroying the context eventually. Hopefully this should be the correct way to address it.

[llvmbot]

@llvm/pr-subscribers-lld

Author: Anutosh Bhat (anutosh491)

Changes

Hi @sbc100

I was looking into a use case involving the link function (which got my attention to reset).

I see that lazyBitcodeFiles variable was introduced here #114327 but I don't see it being reset while destroying the context eventually. Hopefully this should be the correct way to address it.

---

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

1 Files Affected:

• (modified) lld/wasm/Driver.cpp (+1)

diff --git a/lld/wasm/Driver.cpp b/lld/wasm/Driver.cpp
index 8d01ff839ddfcf..37a0156c728f6f 100644
--- a/lld/wasm/Driver.cpp
+++ b/lld/wasm/Driver.cpp
@@ -59,6 +59,7 @@ void Ctx::reset() {
   stubFiles.clear();
   sharedFiles.clear();
   bitcodeFiles.clear();
+  lazyBitcodeFiles.clear();
   syntheticFunctions.clear();
   syntheticGlobals.clear();
   syntheticTables.clear();

[llvmbot]

@llvm/pr-subscribers-lld-wasm

Author: Anutosh Bhat (anutosh491)

Changes

Hi @sbc100

I was looking into a use case involving the link function (which got my attention to reset).

I see that lazyBitcodeFiles variable was introduced here #114327 but I don't see it being reset while destroying the context eventually. Hopefully this should be the correct way to address it.

---

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

1 Files Affected:

• (modified) lld/wasm/Driver.cpp (+1)

diff --git a/lld/wasm/Driver.cpp b/lld/wasm/Driver.cpp
index 8d01ff839ddfcf..37a0156c728f6f 100644
--- a/lld/wasm/Driver.cpp
+++ b/lld/wasm/Driver.cpp
@@ -59,6 +59,7 @@ void Ctx::reset() {
   stubFiles.clear();
   sharedFiles.clear();
   bitcodeFiles.clear();
+  lazyBitcodeFiles.clear();
   syntheticFunctions.clear();
   syntheticGlobals.clear();
   syntheticTables.clear();

[sbc100]

Do we have tests for re-using contexts?

[sbc100]

lgtm % testing question

[anutosh491]

Do we have tests for re-using contexts?

Hmmm, not sure :(
Do you think we need one ? Being new to contributing to llvm, maybe you could suggest where and what kind of test should be added, if we need one.

[sbc100]

I guess we can land this without a test, especially since #78640 also didn't have any test.

@MaskRay do you know of any tests for context reuse/reset?

[anutosh491]

My use case of running clang-repl completely in browser technically supports my claim that context resetting works as expected.
Attaching a small screen recorded demo for the same (I should have this in jupyterlite by the weekend I think)

Screen.Recording.2024-12-04.at.11.03.06.AM.mov

[MaskRay]

I guess we can land this without a test, especially since #78640 also didn't have any test.

@MaskRay do you know of any tests for context reuse/reset?

We have basic unittests in lld/unittest/AsLibELF. We should have wasm tests as well, but it's difficult and probably unnecessary to cover just lazyBitcodeFiles.

If lld/wasm constructs Ctx and drops reset, this pitfall will be removed. https://maskray.me/blog/2024-11-17-removing-global-state-from-lld Eliminating the global ctx variable

[sbc100]

I guess we can land this without a test, especially since #78640 also didn't have any test.
@MaskRay do you know of any tests for context reuse/reset?

We have basic unittests in lld/unittest/AsLibELF. We should have wasm tests as well, but it's difficult and probably unnecessary to cover just lazyBitcodeFiles.

If lld/wasm constructs Ctx and drops reset, this pitfall will be removed. https://maskray.me/blog/2024-11-17-removing-global-state-from-lld Eliminating the global ctx variable

Both of those sounds like good ideas, thank you. I don't think we need to block this change on those though.

[anutosh491]

Hey @sbc100 @MaskRay

I just skimmed through this blog post pasted above. I think we could try the following out

If lld/wasm constructs Ctx and drops reset, this pitfall will be removed. https://maskray.me/blog/2024-11-17-removing-global-state-from-lld Eliminating the global ctx variable

Let me know if I have the green flag from your side and I'll try to address this on a separate branch.

[anutosh491]

I guess we can land this without a test, especially since #78640 also didn't have any test.
@MaskRay do you know of any tests for context reuse/reset?

We have basic unittests in lld/unittest/AsLibELF. We should have wasm tests as well, but it's difficult and probably unnecessary to cover just lazyBitcodeFiles.

If lld/wasm constructs Ctx and drops reset, this pitfall will be removed. https://maskray.me/blog/2024-11-17-removing-global-state-from-lld Eliminating the global ctx variable

I opened an issue to track the progress on this
#119180

Y'all could let me know your thoughts on the same and let me know if my thinking on the issue makes sense.