[WebAssembly] Define __funcref_call_table in generated asm and objects#180900
[WebAssembly] Define __funcref_call_table in generated asm and objects#180900QuantumSegfault wants to merge 1 commit intollvm:mainfrom
__funcref_call_table in generated asm and objects#180900Conversation
|
@llvm/pr-subscribers-backend-webassembly Author: Demetrius Kanios (QuantumSegfault) ChangesCurrently, calls into funcref ( It functions just fine when This fixes said error by "defining" it during Full diff: https://github.com/llvm/llvm-project/pull/180900.diff 2 Files Affected:
diff --git a/llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp b/llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp
index 1cacdb04fa74d..afe834b077909 100644
--- a/llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp
+++ b/llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp
@@ -392,6 +392,22 @@ void WebAssemblyAsmPrinter::emitEndOfAsmFile(Module &M) {
// emitDecls() is not called until now.
emitDecls(M);
+ {
+ StringRef Name = "__funcref_call_table";
+ auto *Sym = static_cast<MCSymbolWasm *>(OutContext.lookupSymbol(Name));
+ if (Sym) {
+ if (!Sym->isFunctionTable())
+ OutContext.reportError(SMLoc(), "symbol is not a wasm funcref table");
+
+ if (Sym->isWeak()) {
+ OutStreamer->emitSymbolAttribute(Sym, MCSA_Weak);
+ }
+ if (!Sym->isDefined()) {
+ OutStreamer->emitLabel(Sym);
+ OutStreamer->addBlankLine();
+ }
+ }
+ }
// When a function's address is taken, a TABLE_INDEX relocation is emitted
// against the function symbol at the use site. However the relocation
// doesn't explicitly refer to the table. In the future we may want to
diff --git a/llvm/test/CodeGen/WebAssembly/funcref-call.ll b/llvm/test/CodeGen/WebAssembly/funcref-call.ll
index 9904df2280e81..4f6025de4759c 100644
--- a/llvm/test/CodeGen/WebAssembly/funcref-call.ll
+++ b/llvm/test/CodeGen/WebAssembly/funcref-call.ll
@@ -1,6 +1,7 @@
; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
; RUN: llc < %s --mtriple=wasm32-unknown-unknown -fast-isel=0 -mattr=+reference-types | FileCheck %s
; RUN: llc < %s --mtriple=wasm32-unknown-unknown -fast-isel=1 -mattr=+reference-types | FileCheck %s
+; RUN: llc < %s --mtriple=wasm32-unknown-unknown --filetype=obj
%funcref = type ptr addrspace(20) ;; addrspace 20 is nonintegral
|
|
Requesting review |
|
Follow up to the problems encountered during #162227 I have absolutely no idea if this is an appropriate fix...but it seems to work. Though this places the definition at the end of file when dumping assembly...there must be a better place for it. Low priority I guess, since clearly nobody is using this broken feature. On the upside that means there's little resistance to changing it. |
|
I think we should maybe be able to treat this like the regular function table symbol ( |
Well, it is marking it as weak, but that doesn't define it. It's still undefined (hence the error that "undefined symbols can't be weak" when trying to emit object files). Not sure exactly what you mean about treating it the same as the other? When CallIndirectOverlong is enabled, call_indirect takes a table argument, so the reference to the The problem is right now we are creating the |
|
Ping |
|
@sbc100 |
I think it just made sense at the time (and still does). Since both the the linear memory and the indirection function table are synthesized by the linker, its makes sense that that symbols that point to them also be created by the linker. In particular, no external object could define them because only the linker knows how big they need to be. |
| OutStreamer->emitSymbolAttribute(Sym, MCSA_Weak); | ||
| } | ||
| if (!Sym->isDefined()) { | ||
| OutStreamer->emitLabel(Sym); |
There was a problem hiding this comment.
This seems rather odd. It seems like we are emitting definition for the symbol exactly then its not defined (i.e. when no definition should be emitted).
In general this seems like the wrong place to be doing this kind of thing. It seems a little too magical. How/where is the definition of __indirect_function_table handled?
There was a problem hiding this comment.
...did I never press send?
emitLabel is the very thing that marks a given symbol as "defined". I took the approach from the handling of globals in emitGlobalVariable.
A symbol is never defined until it has been given a Fragment to? Which emitLabel does. Since we have a free floating symbol, there's no other reason for it to be emitted, so we need to call emitLabel (which confusingly, also has an effect in object emission) somewhere. Though I agree that the position (at the end of the file) is odd. Don't know where else to stick it though. Is there a more appropriate hook?
__indirect_function_table is handled directly in lld, and is never defined in the intermediate objects. It directly creates a DefinedTable symbol in the linker. Here I'm trying to emit a weak definition of it into every object instead, as there's nothing complex about it.
There was a problem hiding this comment.
I see. So you are trying to essentially convert this symbols from "weak undefined" to "weak defined" during ASM generation?
This still seems wrong. Can/should we not instead change the original definition of __indirect_function_table from weak undefined to weak defined?
Better still shouldn't the symbol be "strong" + "undefined"? i.e. the symbol is not optional, right?
There was a problem hiding this comment.
Perhaps there's some confusion? I'm trying to fix __funcref_call_table...not __indirect_function_table.
Ever since it's inception __funcref_call_table was declared like this.
MCSymbolWasm *WebAssembly::getOrCreateFuncrefCallTableSymbol(
MCContext &Ctx, const WebAssemblySubtarget *Subtarget) {
StringRef Name = "__funcref_call_table";
auto *Sym = static_cast<MCSymbolWasm *>(Ctx.lookupSymbol(Name));
if (Sym) {
if (!Sym->isFunctionTable())
Ctx.reportError(SMLoc(), "symbol is not a wasm funcref table");
} else {
Sym = static_cast<MCSymbolWasm *>(Ctx.getOrCreateSymbol(Name));
// Setting Weak ensure only one table is left after linking when multiple
// modules define the table.
Sym->setWeak(true);
wasm::WasmLimits Limits = {0, 1, 1, 0};
wasm::WasmTableType TableType = {wasm::ValType::FUNCREF, Limits};
Sym->setType(wasm::WASM_SYMBOL_TYPE_TABLE);
Sym->setTableType(TableType);
}
// MVP object files can't have symtab entries for tables.
if (!(Subtarget && Subtarget->hasCallIndirectOverlong()))
Sym->setOmitFromLinkingSection();
return Sym;
}If any exist, they can all be merged into the same. However when outputting -filetype=obj directly from llc, it would just error out with LLVM ERROR: undefined table symbol cannot be weak. It never worked.
I do want to set "weak" + "defined", but my approach was the only one I found. emitLabel IS what defines the MCSymbol, strangely enough. Do you know another way to define an MCSymbol?
There was a problem hiding this comment.
I see, so the problem is that you have defined symbol that is not being correctly emitted in the assembly?
Wouldn't we want a solution that works for all table symbols, not just __funcref_call_table?
Also, it seem that Sym->isDefined is wrong about this table its it returning false, since this is a defined symbol, right?
There was a problem hiding this comment.
I think I prefer that solution actually since it matches what we do for other symbols.
There was a problem hiding this comment.
libc doesn't make sense here.
Is compiler-rt linked in ALL invocations of wasm-ld? If not, then it's not a viable spot either.
So the only other option if we REALLY don't want to weak define the symbol in every relevant object is to have lld define it in a similar manner as __indirect_function_table. It just didn't seem necessary to complicate that when the definition of the table is nothing special (i.e. no non-default initialization of the table; just a simple, 1 element, ref.null initialized table).
There was a problem hiding this comment.
There was a problem hiding this comment.
I think we should depend on it being defined by the toolchain somewhere, but not defined in clang itself or in wasm-ld.
libcompiler-rt seems like the most logical place, but maybe just libc (since it recently came to my attention that compiler-rt is designed to be statically linked into each shared object, so if it was to be defined in compiler-rt it would need to be a weak def).
There was a problem hiding this comment.
No. We can't rely on libc. Because then we tie anybody wanting to call through funcref's to (wasi) libc for no good reason.
Just a small .S or .ll file in libcompiler-rt might do it (yes, defined weak).
I guess, what do you have against the PR right now? Why is weakly defined per-object such a bad thing?
4 participants
Currently, calls into funcref (
call addrspace(20)) are lowered into a store of the ref into a special single-element__funcref_call_table, a call_indirect into said table, then a store of null into it.It functions just fine when
-filetype=asm(default), but when emitting an object file directly from llc it fails withLLVM ERROR: undefined table symbol cannot be weakThis fixes said error by "defining" it during
WebAssemblyAsmPrinter, and ensuring it is definitely emitted as weak (so duplicate tables in multiple objects are merged into a single one).