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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
