[PATCH] D76547: [WebAssembly] Add `wasm-exported` function attribute

[Aaron Ballman via Phabricator via llvm-commits]

aaron.ballman added a reviewer: erichkeane.
aaron.ballman added a comment.

The changes need a release note at some point, and this is missing all of the usual sema diagnostic tests (wrong subject, wrong number of args, wrong target, etc).

That said, are we sure this attribute is sufficiently compelling to add it in the first place? This seems like an attribute needed for one? use case, but it's using a pretty general name for the attribute (`exported`). For example, it's not clear to me why this cannot be solved with the existing `export_name` attribute (e.g., `__attribute__((export_name("foo"))) void foo();`)



================
Comment at: clang/include/clang/Basic/AttrDocs.td:5533-5536
+Clang supports the ``__attribute__((exported))``
+attribute for the WebAssembly target. This attribute may be attached to a
+function declaration, where it causes the symbol to be exported from the
+linked WebAssembly module.
----------------
I think more details would be helpful here. The docs for `export_name`:

> WebAssembly functions are exported via string name. By default when a symbol is exported, the export name for C/C++ symbols are the same as their C/C++ symbol names. This attribute can be used to override the default behavior, and request a specific string name be used instead.

make it sound like there's already a way to export the symbol, so it's not clear to me why this attribute is needed (unless this attribute is actually the one intended to provide that functionality!).

Also, the docs don't mention important things like that this attribute only works for emscripten.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:7561-7568
+  if (!isFunctionOrMethod(D)) {
+    S.Diag(D->getLocation(), diag::warn_attribute_wrong_decl_type)
+        << "'exported'" << ExpectedFunction;
+    return;
+  }
+
+  if (S.Context.getTargetInfo().getTriple().isOSEmscripten()) {
----------------
These can both be dropped, they're handled automatically for you.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:7572
+  if (FD->isThisDeclarationADefinition()) {
+    S.Diag(D->getLocation(), diag::err_alias_is_definition) << FD << 0;
+    return;
----------------
Is this diagnostic actually correct? It's for use with the alias and ifunc attributes, so I'm surprised to see it here.


================
Comment at: clang/test/CodeGen/WebAssembly/wasm-exported.c:12
+
+// MISSING: error: unknown attribute 'exported' ignored
+
----------------
This should be handled in a sema test along with the other sema testing, rather than squirreled away here in the codegen tests.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76547/new/

https://reviews.llvm.org/D76547