This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Allow import and export of TLS symbols between DSOs
ClosedPublic

Authored by sbc100 on Aug 28 2021, 2:00 PM.

Diff Detail

Event Timeline

sbc100 created this revision.Aug 28 2021, 2:00 PM
sbc100 requested review of this revision.Aug 28 2021, 2:00 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 28 2021, 2:00 PM
sbc100 updated this revision to Diff 370347.Sep 2 2021, 11:51 AM
  • rebase
sbc100 updated this revision to Diff 370351.Sep 2 2021, 11:56 AM
  • rebase
sbc100 retitled this revision from WIP: [WebAssembly] Allow import and export of TLS betwen DSOs to [WebAssembly] Allow import and export of TLS symbols between DSOs.Sep 2 2021, 11:58 AM

It would be great to have tests for the new subsections.

lld/wasm/SyntheticSections.cpp
73

Why do we check sym->isTLS() for exports but not for imports?

llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
1566–1567

Should these asserts be report_fatal_errors instead?

dschuff added inline comments.Sep 3 2021, 10:25 AM
lld/wasm/Writer.cpp
990

what if we have TLS in the input but not config->sharedMemory?

sbc100 updated this revision to Diff 371317.Sep 8 2021, 5:00 AM

More testing in tls-non-shared-memory.s

sbc100 added inline comments.
lld/wasm/Writer.cpp
990

In this case all the TLS handling is basically internalize.

See lld/test/wasm/tls-non-shared-memory.s. I've added an extra case to that test to verify that the imported/GOT usage of the TLS variable is also internalized.

sbc100 added inline comments.Sep 8 2021, 5:06 AM
lld/wasm/SyntheticSections.cpp
73

I'm getting ahead of myself here. The TLS flag is needed when exporting (since we need to tell the dynamic linker to adjust the exported value according to the tls base).

However for imports we don't need to know if a given global is TLS or not since all the GOT.mem imports are of absolute values (its up to the dynamic linker to ensure they are all absolute addresses).

For imports however, we are going to need to tell the dynamic linker about which imports are weak (so it can provide 0 and not error out).

I think I will completely remove IMPORT_INFO from this change since its not used yet.

sbc100 updated this revision to Diff 371318.Sep 8 2021, 5:11 AM
  • remove IMPORT_INFO
sbc100 updated this revision to Diff 371360.Sep 8 2021, 9:05 AM

Add more tests

tlively added inline comments.Sep 8 2021, 3:23 PM
lld/wasm/SyntheticSections.cpp
64

"toe" => "to"

llvm/lib/Object/WasmObjectFile.cpp
366–367

Do we need to be storing this information somewhere so it can be round-tripped?

sbc100 updated this revision to Diff 371649.Sep 9 2021, 10:26 AM

Add srialization via yaml and more testing

sbc100 updated this revision to Diff 371652.Sep 9 2021, 10:27 AM
sbc100 marked an inline comment as done.

typo

sbc100 marked an inline comment as done.Sep 9 2021, 10:28 AM

This change is much more complete now.

  • removed ImportInfo for now (will add that later)
  • added obj2yaml support for dumping the new subsection
  • added a lot more testing
sbc100 updated this revision to Diff 371663.Sep 9 2021, 11:07 AM
  • revert part
sbc100 updated this revision to Diff 371664.Sep 9 2021, 11:11 AM
  • comment

I think the code is looking good.
I do think it will be cleaner in the long run to make the dylink section properly subsectioned, rather than the backwards-compatible version you have here. We've never declared any kind of backwords compat yet for dylibs, have we?

sbc100 updated this revision to Diff 371936.Sep 10 2021, 8:57 AM
  • rebase

ptal.. should be good to land this now

tlively accepted this revision.Sep 13 2021, 6:26 PM

LGTM!

This revision is now accepted and ready to land.Sep 13 2021, 6:26 PM
This revision was landed with ongoing or failed builds.Sep 14 2021, 6:48 AM
This revision was automatically updated to reflect the committed changes.

Looks like this change caused llvm/test/ObjectYAML/wasm/dylink_section.yaml to start failing. Working on a fix now.

lld/test/wasm/tls-export.s