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