This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] call_indirect causes indirect function table import
ClosedPublic

Authored by wingo on Dec 8 2020, 5:22 AM.

Details

Summary

For wasm-ld table linking work to proceed, object files should indicate
if they use an indirect function table. In the future this will be done
by the usual symbols and relocations mechanism, but until that support
lands in the linker, the presence of an __indirect_function_table in
the object file's import section shows that the object file needs an
indirect function table.

Prior to https://reviews.llvm.org/D91637, this condition was met by all
object files residualizing an __indirect_function_table import.

Since https://reviews.llvm.org/D91637, the intention has been that only
those object files needing an indirect function table would have the
__indirect_function_table import. However, we missed the case of
object files which use the table via call_indirect but which
themselves do not declare any indirect functions.

This changeset makes it so that when we lower a call to call_indirect,
that we ensure that a __indirect_function_table symbol is present and
that it will be propagated to the linker.

A followup patch will revise this mechanism to make an explicit link
between call_indirect and its associated indirect function table; see
https://reviews.llvm.org/D90948.

Diff Detail

Event Timeline

wingo created this revision.Dec 8 2020, 5:22 AM
wingo requested review of this revision.Dec 8 2020, 5:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 8 2020, 5:22 AM
wingo updated this revision to Diff 310149.Dec 8 2020, 5:30 AM

Fix comments

sbc100 added a reviewer: dschuff.

lgtm.

I'm not sure how to address the duplication of got and the associated FIXME. Adding some others to the review list who might know a way to deal with that without duplication.

aheejin added inline comments.Dec 9 2020, 12:38 AM
llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp
168

I'm not sure if there's an easy way to factor out so all three places can use it, but at least can't we use WebAssemblyUtilities.cpp's getOrCreateFunctionTableSymbol here and not define it again? Can't we do `#include "WebAssemblyUtilities.h" and use its function?

wingo added inline comments.Dec 9 2020, 1:16 AM
llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp
168

Sadly we can't directly re-use it here, for reasons I don't fully understand. If you #include the WebAssemblyUtilities.h file, it compiles fine but llvm-mc fails to link. It would seem that one might need to factor WebAssemblyUtilities to a separate cmake component.

aheejin added inline comments.Dec 9 2020, 1:28 AM
llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp
168

Does adding WebAssemblyCodeGen under LINK_COMPONENTS in llvm/lib/Target/WebAssembly/AsmParser/CMakeLists.txt work?

wingo added inline comments.Dec 9 2020, 1:49 AM
llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp
168

I'll give it a try, but note fwiw that other Target/$foo/AsmParser/CMakeLists.txt don't include the codegen components

wingo added inline comments.Dec 9 2020, 2:14 AM
llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp
168

OK so this works :) It will lead to more work for the linker though. @aheejin I can factor out a Target/WebAssembly/Utils/ dir and library component, as e.g. AArch64 and RISCV have, or I can just add WebAssemblyCodeGen to the LINK_COMPONENTS. Do you have a preference?

aheejin added inline comments.Dec 9 2020, 2:23 AM
llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp
168

Making Utils directory sounds good to me. As you said, there seems to be already four targets which does that: AArch64, AMDGPU, ARM, and RISCV.

Maybe it would be good to land this as is and do the Utils refactoring as a followup CL.

wingo added inline comments.Dec 9 2020, 6:18 AM
llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp
168

Really sorry for the noise here but given that the existing WebAssemblyUtilities pulls in all of CodeGen anyway via use of MachineInstr in the other utility routines, it would appear to be just useless noodling to put it in Utils. I will just make AsmParser use WebAssemblyCodeGen and call it a day. Further feedback still welcome of course!

wingo updated this revision to Diff 310513.Dec 9 2020, 6:27 AM

AsmParser uses WebAssemblyUtilities, links to WebAssemblyCodeGen

sbc100 added inline comments.Dec 9 2020, 10:07 AM
llvm/lib/Target/WebAssembly/AsmParser/CMakeLists.txt
7

It could be that MC may be designed to specifically not include CodeGen stuff? Separation of concerns and all that.

Perhaps this oversteps some boundary? I'm not familiar enough the design goals of llvm WRT compartmentalization to know if this is some kind of violation. Does anyone know if this is problem?

aheejin added inline comments.Dec 9 2020, 4:05 PM
llvm/lib/Target/WebAssembly/AsmParser/CMakeLists.txt
7

I don't know; I just discovered the linking works that way, but I'm not sure whether it violates some LLVM convention or something. Other AsmParser directories don't seem to make use of their corresponding CodeGen libraries though.

wingo added inline comments.Dec 10 2020, 12:28 AM
llvm/lib/Target/WebAssembly/AsmParser/CMakeLists.txt
7

I am both heartened and disheartened to see that you all share my ignorance in this regard :) I am happy to do whatever you all think best. The possibilities I see are:

  1. method on MCContext (discussion: https://reviews.llvm.org/D90948?id=305170#inline-851687)
  2. WebAssemblyUtilities, and link to codegen
  3. just copy-paste the functionality
  4. make a new utils library including just this one function.

Let me know your preference please :)

Lets just go with duplicating the function for now (along with a TODO to followup).

Other than that this lgtm

aardappel added inline comments.Dec 10 2020, 9:52 AM
llvm/lib/Target/WebAssembly/AsmParser/CMakeLists.txt
7

Yes, this doesn't seem like a good idea. The MC code is used by quite a few utilities and is intended to be separate from CodeGen. We've had to jump thru hoops a few times before to keep the two separate. I believe there is already a header that contains shared utility functions (either in the WebAssemblyInfo target, or a header like WebAssemblyMCTargetDesc.h)

wingo updated this revision to Diff 314538.Jan 5 2021, 1:57 AM

Copy/paste GetOrCreateFunctionTableSymbol into WebAssemblyAsmParser

wingo added a comment.Jan 5 2021, 2:08 AM

Landing, given @sbc100's lgtm-with-nit. Thanks all for your patience and happy new year :)

This revision was not accepted when it landed; it landed in state Needs Review.Jan 5 2021, 2:24 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.

This commit appears to be the source of ongoing object file ABI breakage that is causing older versions of lld that do not know about table index relocations to fail to link objects produced by versions of LLVM after this change. This can be fixed by only adding the __indirect_function_table import when the reference-types feature is enabled.

cc @sbc100

@wingo can we revert this for the llvm 12 branch which is about to occur (tomorrow I think)? And then we can look at moving forward after the branch?