Page MenuHomePhabricator

[WebAssembly] Update WebAssemblyLowerEmscriptenEHSjLj to handle separate compilation
ClosedPublic

Authored by sbc100 on Jul 12 2018, 11:59 AM.

Details

Summary

Previously we were assuming whole program compilation. Now that
separate compilation is a thing we need to update this pass.
Firstly, it can no longer assert on the existence of malloc and free.
This functions might not be in the current translation unit. If we
need them then we will generate not imports for them.

Secondly the global helper function we create should be marked as
weak since we will be generating a separate copy in each translation
unit.

Finally the names of the symbols used must be unique and fixed since
they need to agree across translation units.

Diff Detail

Repository
rL LLVM

Event Timeline

sbc100 created this revision.Jul 12 2018, 11:59 AM
sbc100 updated this revision to Diff 155248.Jul 12 2018, 12:01 PM
  • cleanup
sbc100 edited the summary of this revision. (Show Details)

I think I actually prefer the approach of linking this code in from a library rather than generating N copies of it. It's more consistent with how system code is usually included into programs (i.e. linking it in rather than generating it magically from the compiler). It's also easier to read and understand for non-compiler developers.

I agree it that the other approach is more elegant. However, I decided to go with this one for now since:
(1) Its a smaller incremental change
(2) Hopefully all this code will be removed pretty soon anyway

We can always improve on this incrementally too.

aheejin added inline comments.Jul 12 2018, 1:09 PM
lib/Target/WebAssembly/WebAssemblyLowerEmscriptenEHSjLj.cpp
310 ↗(On Diff #155248)

Why delete and inline these strings in code?

336 ↗(On Diff #155248)

Nit: Maybe createInt32GlobalVariable? Because the int32 type is hardcoded here

723 ↗(On Diff #155248)

What happens now with the new linker when malloc and free are not in the module?

sbc100 updated this revision to Diff 155727.Jul 16 2018, 11:40 AM
sbc100 marked an inline comment as done.
  • feedback
sbc100 updated this revision to Diff 155771.Jul 16 2018, 3:28 PM
  • feedback
sbc100 added inline comments.Jul 16 2018, 3:28 PM
lib/Target/WebAssembly/WebAssemblyLowerEmscriptenEHSjLj.cpp
310 ↗(On Diff #155248)

I find this pattern needlessly complex for single use strings. At least in lld we try to avoid this.

723 ↗(On Diff #155248)

If you don't include malloc/free (i.e. if you don't link with libc) and this pass adds references you will get "undefined symbol: malloc" at link time. Seems reasonable?

aheejin added inline comments.Jul 16 2018, 3:42 PM
lib/Target/WebAssembly/WebAssemblyLowerEmscriptenEHSjLj.cpp
310 ↗(On Diff #155248)

Hmm, I see. Then maybe it would be better to inline other strings to be consistent, not necessarily in this CL though...

723 ↗(On Diff #155248)

Yeah that sounds good.

aheejin accepted this revision.Jul 16 2018, 3:43 PM

I'm OK with this, but @dschuff might prefer the library approach..?

This revision is now accepted and ready to land.Jul 16 2018, 3:43 PM

I think we can still do that library option, with this as an intermediate step. This change has the nice property of not requiring an emscripten-side change.

This revision was automatically updated to reflect the committed changes.