Page MenuHomePhabricator

[WebAssembly] Add wasm-exported function attribute
AcceptedPublic

Authored by sbc100 on Mar 21 2020, 10:57 AM.

Details

Reviewers
sunfish
kripken
Summary

This matches the existing export-name attribute but exports that symbol
but its llvm symbol name. This corresponds directly to the existing
WASM_SYMBOL_EXPORTED symbol flag.

This allows the existing emscripgten macro EMSCERIPTEN_KEEPALIVE to be
implemented in terms of this attribute rather then the current
workaround which uses the used attribute.

Diff Detail

Unit TestsFailed

TimeTest
40 msClang.CodeGen::wasm-export.c
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/clang -cc1 -internal-isystem /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/lib/clang/11.0.0/include -nostdsysteminc -triple wasm32-unknown-unknown-wasm -emit-llvm -o - /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/clang/test/CodeGen/wasm-export.c | /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/FileCheck /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/clang/test/CodeGen/wasm-export.c
30 msClang.Misc::pragma-attribute-supported-attributes-list.test
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/clang-tblgen -gen-clang-test-pragma-attribute-supported-attributes -I/mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/clang/include /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/clang/include/clang/Basic/Attr.td -o - | /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/FileCheck /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/clang/test/Misc/pragma-attribute-supported-attributes-list.test

Event Timeline

sbc100 created this revision.Mar 21 2020, 10:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 21 2020, 10:57 AM
kripken accepted this revision.Mar 23 2020, 4:11 PM
kripken added inline comments.
clang/include/clang/Basic/AttrDocs.td
4173

"the be" => "the function to be"

This revision is now accepted and ready to land.Mar 23 2020, 4:11 PM

Instead of creating a new LLVM-IR-level attribute here, could you have clang translate the attribute to "wasm-export-name", to keep the LLVM-IR level simpler?

Also, I myself would be more comfortable with this change if it were restricted to Emscripten for now. export_name already exists and works in both Emscripten and non-Emscripten targets. If there's demand for this new syntax outside of Emscripten, I'm happy to reconsider, but until then it seems better to be conservative. Obviously it's not possible to completely prevent people from becoming dependent on C++ ABI details, but we can avoid giving them tools that make it easy to do the wrong thing. And we can keep the ecosystem simpler if we don't have multiple ways to do the same thing.

What about your idea of using the default keyword rather than adding a new clang attr? I quite liked that approach.

I was thinking of dropping the new clang attr in favor of the default keyword in the current attr, but actually keeping the llvm attr, since it corresponds quite nicely to the existing EXPORTED symbol attribute in the binary format and avoid duplication in the .ll, .s and .o formats.