This is an archive of the discontinued LLVM Phabricator instance.

[lld][WebAssembly] Handle TLS symbols in older object file
ClosedPublic

Authored by sbc100 on Jan 27 2022, 3:58 PM.

Details

Summary

In older versions of llvm (e.g. llvm 13), symbols were not individually
flagged as TLS. In this case, the indent was to implicitly mark any
symbols defined in TLS segments as TLS. However, we were not performing
this implicit conversion if the segment was explicitly marked as TLS

As it happens, llvm 13 was branched between the addition of the segment
flag and the addition of the symbol flag. See:

Testing this is tricky because the assembler will imply the TLS status
of the symbol based on the segment its declared in, so we are forced to
use a yaml file here.

Fixes: https://github.com/emscripten-core/emscripten/issues/15891

Diff Detail

Event Timeline

sbc100 created this revision.Jan 27 2022, 3:58 PM
sbc100 requested review of this revision.Jan 27 2022, 3:58 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 27 2022, 3:58 PM
dschuff added inline comments.Jan 27 2022, 4:22 PM
lld/test/wasm/tls-implicit.yaml
6
8
lld/wasm/InputFiles.cpp
582–585
583
sbc100 updated this revision to Diff 403832.Jan 27 2022, 4:27 PM
sbc100 marked 4 inline comments as done.
  • feedback
dschuff accepted this revision.Jan 27 2022, 4:29 PM

I guess the existing tls.s test case still covers the case where neither the symbol nor the segment is marked as TLS but the name is .tdata?

This revision is now accepted and ready to land.Jan 27 2022, 4:29 PM

I guess the existing tls.s test case still covers the case where neither the symbol nor the segment is marked as TLS but the name is .tdata?

Yes I believe that is covered here:

https://github.com/llvm/llvm-project/blob/bddc814b442ae9f30d62e2f881274d6255411225/lld/test/wasm/tls.s#L54-L61

tlively accepted this revision.Jan 27 2022, 4:34 PM

Should we try to get some patch backported to the 13.x.x releases to get them out of the intermediate state that was causing this bug? I guess it's not really important after this fix. Or maybe this fix should be backported?

LGTM modulo comments

llvm/lib/ObjectYAML/WasmEmitter.cpp
588

This looks unrelated. Would it make sense to split out?

sbc100 added inline comments.Jan 27 2022, 4:36 PM
llvm/lib/ObjectYAML/WasmEmitter.cpp
588

Its needed in order to make yaml2obj work for my new test case... so in a sense in related... in some way my new test acts a test for this..

tlively added inline comments.Jan 27 2022, 4:42 PM
llvm/lib/ObjectYAML/WasmEmitter.cpp
588

Sounds good!

This revision was landed with ongoing or failed builds.Jan 27 2022, 5:27 PM
This revision was automatically updated to reflect the committed changes.