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 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 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 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, 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&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. :-) > 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 { >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&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()))`` > +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()) { > llvm::Function *Fn = cast(GV); > llvm::AttrBuilder B; > -B.addAttribute("wasm-import-module", Attr->getImportModuleName()); > +B.addAttribute(
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&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 { > >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&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()))`` > > +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/Target
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&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 { >> >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&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()))`` >> > +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
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