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