r352930 - [WebAssembly] Add an import_field function attribute

Dan Gohman via cfe-commits cfe-commits at lists.llvm.org
Fri Feb 1 17:27:04 PST 2019


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.


>
> > 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190201/cf962bea/attachment-0001.html>


More information about the cfe-commits mailing list