<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Feb 1, 2019 at 2:31 PM Aaron Ballman <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Fri, Feb 1, 2019 at 5:25 PM Dan Gohman via cfe-commits<br>
<<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<br>
><br>
> Author: djg<br>
> Date: Fri Feb 1 14:25:23 2019<br>
> New Revision: 352930<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=352930&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=352930&view=rev</a><br>
> Log:<br>
> [WebAssembly] Add an import_field function attribute<br>
><br>
> This is similar to import_module, but sets the import field name<br>
> instead.<br>
><br>
> By default, the import field name is the same as the C/asm/.o symbol<br>
> name. However, there are situations where it's useful to have it be<br>
> different. For example, suppose I have a wasm API with a module named<br>
> "pwsix" and a field named "read". There's no risk of namespace<br>
> collisions with user code at the wasm level because the generic name<br>
> "read" is qualified by the module name "pwsix". However in the C/asm/.o<br>
> namespaces, the module name is not used, so if I have a global function<br>
> named "read", it is intruding on the user's namespace.<br>
><br>
> With the import_field module, I can declare my function (in libc) to be<br>
> "__read", and then set the wasm import module to be "pwsix" and the wasm<br>
> import field to be "read". So at the C/asm/.o levels, my symbol is<br>
> outside the user namespace.<br>
><br>
> Differential Revision: <a href="https://reviews.llvm.org/D57602" rel="noreferrer" target="_blank">https://reviews.llvm.org/D57602</a><br>
<br>
Btw, this review never went to cfe-commits, but it should have. :-)<br></blockquote><div><br></div><div>Yes, my mistake.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> Added:<br>
> cfe/trunk/test/CodeGen/wasm-import-name.c<br>
> - copied, changed from r352781, cfe/trunk/test/CodeGen/wasm-import-module.c<br>
> Modified:<br>
> cfe/trunk/include/clang/Basic/Attr.td<br>
> cfe/trunk/include/clang/Basic/AttrDocs.td<br>
> cfe/trunk/lib/CodeGen/TargetInfo.cpp<br>
> cfe/trunk/lib/Sema/SemaDeclAttr.cpp<br>
> cfe/trunk/test/Misc/pragma-attribute-supported-attributes-list.test<br>
><br>
> Modified: cfe/trunk/include/clang/Basic/Attr.td<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/Attr.td?rev=352930&r1=352929&r2=352930&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/Attr.td?rev=352930&r1=352929&r2=352930&view=diff</a><br>
> ==============================================================================<br>
> --- cfe/trunk/include/clang/Basic/Attr.td (original)<br>
> +++ cfe/trunk/include/clang/Basic/Attr.td Fri Feb 1 14:25:23 2019<br>
> @@ -1514,11 +1514,19 @@ def AMDGPUNumVGPR : InheritableAttr {<br>
> def WebAssemblyImportModule : InheritableAttr,<br>
> TargetSpecificAttr<TargetWebAssembly> {<br>
> let Spellings = [Clang<"import_module">];<br>
> - let Args = [StringArgument<"ImportModuleName">];<br>
> + let Args = [StringArgument<"ImportModule">];<br>
> let Documentation = [WebAssemblyImportModuleDocs];<br>
> let Subjects = SubjectList<[Function], ErrorDiag>;<br>
> }<br>
><br>
> +def WebAssemblyImportName : InheritableAttr,<br>
> + TargetSpecificAttr<TargetWebAssembly> {<br>
> + let Spellings = [Clang<"import_name">];<br>
> + let Args = [StringArgument<"ImportName">];<br>
> + let Documentation = [WebAssemblyImportNameDocs];<br>
> + let Subjects = SubjectList<[Function], ErrorDiag>;<br>
> +}<br>
> +<br>
> def NoSplitStack : InheritableAttr {<br>
> let Spellings = [GCC<"no_split_stack">];<br>
> let Subjects = SubjectList<[Function], ErrorDiag>;<br>
><br>
> Modified: cfe/trunk/include/clang/Basic/AttrDocs.td<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/AttrDocs.td?rev=352930&r1=352929&r2=352930&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/AttrDocs.td?rev=352930&r1=352929&r2=352930&view=diff</a><br>
> ==============================================================================<br>
> --- cfe/trunk/include/clang/Basic/AttrDocs.td (original)<br>
> +++ cfe/trunk/include/clang/Basic/AttrDocs.td Fri Feb 1 14:25:23 2019<br>
> @@ -3730,6 +3730,23 @@ reuqest a specific module name be used i<br>
> }];<br>
> }<br>
><br>
> +def WebAssemblyImportNameDocs : Documentation {<br>
> + let Category = DocCatFunction;<br>
> + let Content = [{<br>
> +Clang supports the ``__attribute__((import_name(<name>)))``<br>
> +attribute for the WebAssembly target. This attribute may be attached to a<br>
> +function declaration, where it modifies how the symbol is to be imported<br>
> +within the WebAssembly linking environment.<br>
> +<br>
> +WebAssembly imports use a two-level namespace scheme, consisting of a module<br>
> +name, which typically identifies a module from which to import, and a field<br>
> +name, which typically identifies a field from that module to import. By<br>
> +default, field names for C/C++ symbols are the same as their C/C++ symbol<br>
> +names. This attribute can be used to override the default behavior, and<br>
> +reuqest a specific field name be used instead.<br>
> + }];<br>
> +}<br>
> +<br>
> def ArtificialDocs : Documentation {<br>
> let Category = DocCatFunction;<br>
> let Content = [{<br>
><br>
> Modified: cfe/trunk/lib/CodeGen/TargetInfo.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/TargetInfo.cpp?rev=352930&r1=352929&r2=352930&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/TargetInfo.cpp?rev=352930&r1=352929&r2=352930&view=diff</a><br>
> ==============================================================================<br>
> --- cfe/trunk/lib/CodeGen/TargetInfo.cpp (original)<br>
> +++ cfe/trunk/lib/CodeGen/TargetInfo.cpp Fri Feb 1 14:25:23 2019<br>
> @@ -765,7 +765,13 @@ public:<br>
> if (const auto *Attr = FD->getAttr<WebAssemblyImportModuleAttr>()) {<br>
> llvm::Function *Fn = cast<llvm::Function>(GV);<br>
> llvm::AttrBuilder B;<br>
> - B.addAttribute("wasm-import-module", Attr->getImportModuleName());<br>
> + B.addAttribute("wasm-import-module", Attr->getImportModule());<br>
> + Fn->addAttributes(llvm::AttributeList::FunctionIndex, B);<br>
> + }<br>
> + if (const auto *Attr = FD->getAttr<WebAssemblyImportNameAttr>()) {<br>
> + llvm::Function *Fn = cast<llvm::Function>(GV);<br>
> + llvm::AttrBuilder B;<br>
> + B.addAttribute("wasm-import-name", Attr->getImportName());<br>
> Fn->addAttributes(llvm::AttributeList::FunctionIndex, B);<br>
> }<br>
> }<br>
><br>
> Modified: cfe/trunk/lib/Sema/SemaDeclAttr.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclAttr.cpp?rev=352930&r1=352929&r2=352930&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclAttr.cpp?rev=352930&r1=352929&r2=352930&view=diff</a><br>
> ==============================================================================<br>
> --- cfe/trunk/lib/Sema/SemaDeclAttr.cpp (original)<br>
> +++ cfe/trunk/lib/Sema/SemaDeclAttr.cpp Fri Feb 1 14:25:23 2019<br>
> @@ -5732,6 +5732,29 @@ static void handleWebAssemblyImportModul<br>
> AL.getAttributeSpellingListIndex()));<br>
> }<br>
><br>
> +static void handleWebAssemblyImportNameAttr(Sema &S, Decl *D, const ParsedAttr &AL) {<br>
> + if (!isFunctionOrMethod(D)) {<br>
> + S.Diag(D->getLocation(), diag::warn_attribute_wrong_decl_type)<br>
> + << "'import_name'" << ExpectedFunction;<br>
> + return;<br>
> + }<br>
<br>
This code is redundant and can be removed.<br></blockquote><div><br></div><div>Ok, cool. I'll clean it up in a follow-up patch.</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> +<br>
> + auto *FD = cast<FunctionDecl>(D);<br>
> + if (FD->isThisDeclarationADefinition()) {<br>
> + S.Diag(D->getLocation(), diag::err_alias_is_definition) << FD << 0;<br>
> + return;<br>
> + }<br>
<br>
This diagnostic does not make sense to me -- what does this have to do<br>
with aliases?<br></blockquote><div><br></div><div>It appears we're currently abusing an existing error code rather than creating a new one. I'll fix this in a follow-up patch. <br></div><br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> --- cfe/trunk/test/Misc/pragma-attribute-supported-attributes-list.test (original)<br>
> +++ cfe/trunk/test/Misc/pragma-attribute-supported-attributes-list.test Fri Feb 1 14:25:23 2019<br>
> @@ -138,6 +138,7 @@<br>
> // CHECK-NEXT: WarnUnusedResult (SubjectMatchRule_objc_method, SubjectMatchRule_enum, SubjectMatchRule_record, SubjectMatchRule_hasType_functionType)<br>
> // CHECK-NEXT: Weak (SubjectMatchRule_variable, SubjectMatchRule_function, SubjectMatchRule_record)<br>
> // CHECK-NEXT: WeakRef (SubjectMatchRule_variable, SubjectMatchRule_function)<br>
> +// CHECK-NEXT: WebAssemblyImportName (SubjectMatchRule_function)<br>
> // CHECK-NEXT: WebAssemblyImportModule (SubjectMatchRule_function)<br>
> // CHECK-NEXT: WorkGroupSizeHint (SubjectMatchRule_function)<br>
> // CHECK-NEXT: XRayInstrument (SubjectMatchRule_function, SubjectMatchRule_objc_method)<br>
<br>
This is missing all the Sema tests that I would have expected. You<br>
should add tests for the situations you are diagnosing, including<br>
applying the attribute to the wrong entity, giving it zero args, 2 or<br>
more args, etc.</blockquote><div><br></div><div> Indeed, and the existing import_module attribute needs these tests as well. I'll write some and add them in a follow-up patch.<br></div><div><br></div><div>Thanks for the review!</div><div><br></div><div>Dan</div><div><br></div></div></div>