r352930 - [WebAssembly] Add an import_field function attribute
Aaron Ballman via cfe-commits
cfe-commits at lists.llvm.org
Tue Feb 5 11:20:11 PST 2019
On Fri, Feb 1, 2019 at 8:27 PM Dan Gohman <sunfish at mozilla.com> wrote:
>
>
>
> On Fri, Feb 1, 2019 at 2:31 PM Aaron Ballman <aaron at aaronballman.com> wrote:
>>
>> On Fri, Feb 1, 2019 at 5:25 PM Dan Gohman via cfe-commits
>> <cfe-commits at 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
More information about the cfe-commits
mailing list