Page MenuHomePhabricator

[WebAssembly] GC constructor functions in otherwise unused archive objects
ClosedPublic

Authored by sunfish on Jul 31 2020, 5:23 PM.

Details

Summary

This allows __wasilibc_populate_libpreopen to be GC'd in more cases
where it isn't needed, including when linked from Rust's libstd.

Diff Detail

Event Timeline

sunfish created this revision.Jul 31 2020, 5:23 PM
sunfish requested review of this revision.Jul 31 2020, 5:23 PM

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?

sunfish retitled this revision from [WebAssembly] GC constructor functions in otherwise unused objects to [WebAssembly] GC constructor functions in otherwise unused archive objects.Jul 31 2020, 7:24 PM

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.

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.

sbc100 added inline comments.Jul 31 2020, 8:16 PM
lld/wasm/InputFiles.h
105

I see .. so something like this:

  1. Transitive dependency pulls object out of archive.
  2. Source of dependency turns out to not be live in the final link due to --gc-sections.
  3. Object file should no longer be considered part of the link after all, reversing the decision made in (1).

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.

sunfish added inline comments.Jul 31 2020, 10:06 PM
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.

sunfish updated this revision to Diff 282449.Aug 2 2020, 7:34 AM
  • 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.
sbc100 added inline comments.Aug 11 2020, 10:28 AM
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.

sunfish updated this revision to Diff 295460.Sep 30 2020, 8:38 PM
sunfish marked 7 inline comments as done.

Address review feedback.

sunfish marked 3 inline comments as done.Sep 30 2020, 8:39 PM
sunfish added inline comments.
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.

sunfish marked an inline comment as done.Oct 12 2020, 2:53 PM

I've finished addressing the review comments, so this is now ready for review again.

sbc100 accepted this revision.Oct 12 2020, 3:04 PM

lgtm % test nits

lld/test/wasm/Inputs/ctor-setup-call-def.s
22

I've been putting these external declaration in column 0

lld/test/wasm/Inputs/ctor-setup.s
5

No need to .section for functions since each one implicitly gets its own one anyway.

This revision is now accepted and ready to land.Oct 12 2020, 3:04 PM

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?

Fix (I believe) is in https://reviews.llvm.org/D89290

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.