Re: r352930 - [WebAssembly] Add an import_field function attribute
On Mon, Mar 18, 2019 at 7:08 PM Dan Gohman wrote: > > > > On Fri, Mar 15, 2019 at 10:55 AM Aaron Ballman wrote: >> >> >> Ping. > > > I apologize for the extraordinarily delays here. I've now posted a patch to > address the review comments here: > > https://reviews.llvm.org/D59520 Delays happen to all of us; thank you for addressing the comments -- I appreciate it! ~Aaron > > Dan > ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: r352930 - [WebAssembly] Add an import_field function attribute
On Fri, Mar 15, 2019 at 10:55 AM Aaron Ballman wrote: > > Ping. > I apologize for the extraordinarily delays here. I've now posted a patch to address the review comments here: https://reviews.llvm.org/D59520 Dan ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: r352930 - [WebAssembly] Add an import_field function attribute
On Fri, Mar 8, 2019 at 5:06 PM Aaron Ballman wrote: > > On Wed, Feb 6, 2019 at 12:43 AM Dan Gohman wrote: > > > > > > > > On Tue, Feb 5, 2019 at 11:20 AM Aaron Ballman > > wrote: > >> > >> On Fri, Feb 1, 2019 at 8:27 PM Dan Gohman wrote: > >> > > >> > > >> > 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? > > > > > > Yes, it's still on my radar. > > It's been a month and it still seems like this hasn't been taken care > of. The usual expectation is to handle post-commit feedback > "immediately". Can you please address these concerns? Ping. ~Aaron > > ~Aaron > > > > > Dan > > ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: r352930 - [WebAssembly] Add an import_field function attribute
On Wed, Feb 6, 2019 at 12:43 AM Dan Gohman wrote: > > > > On Tue, Feb 5, 2019 at 11:20 AM Aaron Ballman wrote: >> >> On Fri, Feb 1, 2019 at 8:27 PM Dan Gohman wrote: >> > >> > >> > 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? > > > Yes, it's still on my radar. It's been a month and it still seems like this hasn't been taken care of. The usual expectation is to handle post-commit feedback "immediately". Can you please address these concerns? ~Aaron > > Dan > ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: r352930 - [WebAssembly] Add an import_field function attribute
On Tue, Feb 5, 2019 at 11:20 AM Aaron Ballman wrote: > On Fri, Feb 1, 2019 at 8:27 PM Dan Gohman wrote: > > > > > > 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? > Yes, it's still on my radar. Dan ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: r352930 - [WebAssembly] Add an import_field function attribute
On Fri, Feb 1, 2019 at 8:27 PM Dan Gohman wrote: > > > > On Fri, Feb 1, 2019 at 2:31 PM Aaron Ballman wrote: >> >> On Fri, Feb 1, 2019 at 5:25 PM Dan Gohman via cfe-commits >> wrote: >> > >> > Author: djg >> > Date: Fri Feb 1 14:25:23 2019 >> > New Revision: 352930 >> > >> > URL: http://llvm.org/viewvc/llvm-project?rev=352930=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=352929=352930=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 { >> >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 { >> > + 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=352929=352930=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()))`` >> > +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: >> >
Re: r352930 - [WebAssembly] Add an import_field function attribute
On Fri, Feb 1, 2019 at 2:31 PM Aaron Ballman wrote: > On Fri, Feb 1, 2019 at 5:25 PM Dan Gohman via cfe-commits > wrote: > > > > Author: djg > > Date: Fri Feb 1 14:25:23 2019 > > New Revision: 352930 > > > > URL: http://llvm.org/viewvc/llvm-project?rev=352930=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=352929=352930=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 { > >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 { > > + 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=352929=352930=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()))`` > > +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=352929=352930=diff > > > == > > --- cfe/trunk/lib/CodeGen/TargetInfo.cpp (original) > > +++ cfe/trunk/lib/CodeGen/TargetInfo.cpp Fri Feb 1 14:25:23 2019 > >
Re: r352930 - [WebAssembly] Add an import_field function attribute
On Fri, Feb 1, 2019 at 5:25 PM Dan Gohman via cfe-commits wrote: > > Author: djg > Date: Fri Feb 1 14:25:23 2019 > New Revision: 352930 > > URL: http://llvm.org/viewvc/llvm-project?rev=352930=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. :-) > 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=352929=352930=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 { >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 { > + 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=352929=352930=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()))`` > +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=352929=352930=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()) { > llvm::Function *Fn = cast(GV); > llvm::AttrBuilder B; > -B.addAttribute("wasm-import-module", Attr->getImportModuleName()); > +B.addAttribute("wasm-import-module",
r352930 - [WebAssembly] Add an import_field function attribute
Author: djg Date: Fri Feb 1 14:25:23 2019 New Revision: 352930 URL: http://llvm.org/viewvc/llvm-project?rev=352930=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 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=352929=352930=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 { 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 { + 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=352929=352930=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()))`` +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=352929=352930=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()) { llvm::Function *Fn = cast(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()) { +llvm::Function *Fn = cast(GV); +llvm::AttrBuilder B; +B.addAttribute("wasm-import-name", Attr->getImportName()); Fn->addAttributes(llvm::AttributeList::FunctionIndex, B);