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