This allows __wasilibc_populate_libpreopen to be GC'd in more cases
where it isn't needed, including when linked from Rust's libstd.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
To be clear this change only relates to object files at are part of ar archives and are not part of the link? Perhaps mention that in the PR title/description.
lld/wasm/InputFiles.h | ||
---|---|---|
105 | My understanding is that any file that is created as an ObjFile is by definition live. and that all files in symtab->objectFiles are also by definition live. Archive files don't create any ObjFiles until they are pulled into the link for some reason (i.e. they are live). What am I missing here? |
Yes, this relates to functions in objects which don't, after GC, contribute any functions to the link. I've now added "archive" to the title.
lld/wasm/InputFiles.h | ||
---|---|---|
105 | There are effectively two GC algorithms in wasm-ld today. The first selects the objects that aren't in archives, plus the objects in archives they (transitively) reference. The second one is MarkLive.cpp, which selects exported functions, plus functions they (transitively) reference. What this patch is saying is, if a constructor function gets pulled in because its object is selected in the first phase, but MarkLive.cpp's GC determines that no functions in that object are transitively called from an export in the second phase, the constructor doesn't need to be called. |
lld/wasm/InputFiles.h | ||
---|---|---|
105 | I see .. so something like this:
If I'm understanding correctly, one possible problem with this is that is makes --gc-sections observable. I could get a static ctor that runs with --no-gc-sections but then is not run with --gc-sections.... maybe this is so subtle as not to matter? But I would normally expect those two builds with be identical in their behaviour. |
lld/wasm/InputFiles.h | ||
---|---|---|
105 | That's correct. You can get similar observable behavior changes from any optimization that can delete code, which can lead fewer object files being pulled in. Consider C code that does this: int x = 0; if (x) { foo(); } Plain -O2 will remove the call to foo here. If that's the only call to foo and foo is defined in an object which has a static constructor, the constructor will run at -O0 and won't run at -O2. This patch has a similar effect. This patch upholds the rule that if the constructor is defined in a .o file which contributes to the final link, it'll run. It's just doing more optimization before making that determination. |
- Reorganize the code a little so that we don't have to call mark multiple times.
- Fix a bug where we weren't considering calls from constructors as keeping other constructors live.
- Add a few more tests.
lld/test/wasm/Inputs/ctor-ctor.s | ||
---|---|---|
14 | Can we call this something less generic like myctor or test_ctor so its not confused with maybe keyword or magic symbol? | |
lld/test/wasm/Inputs/ctor-lib.s | ||
6 | What do you think about renaming usethis -> lib_func and bar -> unused_lib_func? I think you can also from the .hidden lines in all these tests to keep them short. | |
lld/test/wasm/Inputs/ctor-start.s | ||
4 | I think you can drop the .section and .type lines for functions which i think makes the tests more readable. | |
7 | Extra spaces there and in other .s files? | |
lld/test/wasm/ctor-gc-setup.test | ||
11 | Why not put both these in the same archive file? Seems like it would at least make the test a little more concise. | |
lld/wasm/InputFiles.h | ||
63 | How about a comment along the lines of. "An InputFile is considered live if any of the symbols defined by it are live"? | |
lld/wasm/MarkLive.cpp | ||
62 | So here we are detecting the first time a given object is marked as live and then marking its init functions at that time. The guess this is inherently iterative since queuing the init functions creates more marking work? | |
72 | isn't cast<> an equivalent way of saying these two lines? (in which case this if body can probably just be one line) | |
113 | Maybe move the above three lines of comment to the declaration of enqueueInitFunctions since it seems like a general comment not specific to this callsite. | |
lld/wasm/Writer.cpp | ||
1115 | I think !sym->isLive() is enough since all discarded symbols will be non-live. oh. wait.. I think I forgot about --no-gc-sections in which case everything is live but stuff can still get discarded. | |
1116–1117 | I guess you can drop this assert now. |
lld/test/wasm/Inputs/ctor-start.s | ||
---|---|---|
7 | These were tabs, because that's what LLVM emits. I've now changed them to single plain spaces. | |
lld/wasm/MarkLive.cpp | ||
62 | Yes, it's iterative in the same way that the broader mark-and-sweep is iterative. When it sees an edge to something that's already live, it doesn't traverse that edge. |
Looks like we have a specific emscripten test for this since it started failing on our llvm roller:
https://github.com/emscripten-core/emscripten/blob/bd8e5b5296721e33cfad98322e6b70b9985cfd2d/tests/test_other.py#L900
Was this change supposed to effect archive object included via --whole-archive too?
Oops, I accidentally raced here and posted https://reviews.llvm.org/D89293. Looks like our fixes are very similar, so I'll review yours and you can land whichever you prefer.
Can we call this something less generic like myctor or test_ctor so its not confused with maybe keyword or magic symbol?