Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

clang "cannot select" when using wasm table builtins and -fPIC #65191

Closed
hoodmane opened this issue Sep 2, 2023 · 10 comments · Fixed by #67545
Closed

clang "cannot select" when using wasm table builtins and -fPIC #65191

hoodmane opened this issue Sep 2, 2023 · 10 comments · Fixed by #67545
Assignees

Comments

@hoodmane
Copy link

hoodmane commented Sep 2, 2023

Example:

Source file:

static __externref_t table[0];

__externref_t test_builtin_wasm_table_get() {
  return __builtin_wasm_table_get(table, 0);
}

Compiling as follows works:

clang -c -target wasm32 -mreference-types builtins-table.c

but if I add -fPIC, it says:

fatal error: error in backend: Cannot select: intrinsic %llvm.wasm.table.get.externref

Bad/good IR

The IR looks as follows:

@table = internal addrspace(1) global [0 x ptr addrspace(10)] zeroinitializer, align 1

; Function Attrs: noinline nounwind optnone
define hidden ptr addrspace(10) @test_builtin_wasm_table_get() #0 {
entry:
  %0 = call ptr addrspace(10) @llvm.wasm.table.get.externref(ptr addrspace(1) @table, i32 0)
  ret ptr addrspace(10) %0
}

; Function Attrs: nocallback nofree nosync nounwind willreturn memory(read)
declare ptr addrspace(10) @llvm.wasm.table.get.externref(ptr addrspace(1), i32) #1

If I change @table = internal addrspace(1) to @table = local_unnamed_addr addrspace(1) it seems to fix the problem.

@pmatos

@hoodmane hoodmane changed the title clang cannot select when using wasm table builtins and -fPIC clang "cannot select" when using wasm table builtins and -fPIC Sep 2, 2023
@llvmbot
Copy link
Member

llvmbot commented Sep 2, 2023

@llvm/issue-subscribers-backend-webassembly

@hoodmane
Copy link
Author

hoodmane commented Sep 2, 2023

Actually, even with local_unnamed_addr it still doesn't work quite as desired with -fPIC, since it generates relocations of type R_WASM_GLOBAL_INDEX_LEB instead of type R_WASM_TABLE_NUMBER_LEB and so linking does not work. But it moves the problem a bit later.

@pmatos
Copy link
Contributor

pmatos commented Sep 2, 2023

Hi. I am currently on holidays until Sept 11. If nobody looks at this until then, I will upon my return.

@pmatos
Copy link
Contributor

pmatos commented Sep 13, 2023

I can reproduce the problem and will work on a fix.

@pmatos pmatos self-assigned this Sep 13, 2023
@pmatos
Copy link
Contributor

pmatos commented Sep 26, 2023

@sbc100 looking at some code you wrote some time ago:

if (GV->getValueType()->isFunctionTy()) {

Before Wasm tables being implemented, we used a single table for functions, which explains the condition GV->getValueType()->isFunctionTy(). The issue with this issue is that the symbol for a table is handled in the else condition because it's not a function.

I am not sure this is the correct behaviour. The symbol for a table is compiled to a reference to a table that is then declared with .tabletype. Unsure what this __table_base refers to in the code and it can be used by multiple tables. In any case I think we can solve this issue by adding a code path for Wasm Tables here and appropriately implementing node selection the node. PIC was something I didn't test when implementing tables support.

@sbc100
Copy link
Collaborator

sbc100 commented Sep 26, 2023

Under -fPIC (i.e. dynamic linking) the __table_base symbol is the linker-created global which represents the offset into the __indirect_function_table at which the current DLL's static function addresses are stored. Its similar to __memory_base for the static data section. It should only be used when the address of function is taken and it should only refer to __indirect_function_table.

@sbc100
Copy link
Collaborator

sbc100 commented Sep 26, 2023

Looks like we just need another branch in that if/else to handle table symbols. When that code was written there were only two types of symbols: function symbols and data symbols.

@pmatos
Copy link
Contributor

pmatos commented Sep 27, 2023

Looks like we just need another branch in that if/else to handle table symbols. When that code was written there were only two types of symbols: function symbols and data symbols.

Thanks. It seems like the issue is straightforward given tables cannot be export/imported but it will become more complicated when we can have two modules sharing a table.

@pmatos
Copy link
Contributor

pmatos commented Sep 27, 2023

@hoodmane I submitted a PR attempting to fix this. Let me know if it solves the problem you were seeing. Apologies for the delay on the fix.

pmatos added a commit to pmatos/llvm-project that referenced this issue Sep 29, 2023
Currently tables cannot be shared between compilation units, therefore
no special treatment is needed for tables.

Fixes llvm#65191
pmatos added a commit that referenced this issue Oct 3, 2023
Currently tables cannot be shared between compilation units, therefore
no special treatment is needed for tables.

Fixes #65191
@hoodmane
Copy link
Author

hoodmane commented Oct 3, 2023

Thanks so much for fixing this @pmatos!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants