Re: r244488 - [dllimport] A non-imported class with an imported key can't have a key
Awesome, thanks! On Tue, Aug 18, 2015 at 4:27 PM, Reid Kleckner wrote: > I merged both of them and tweaked the test case to make it work. > > On Tue, Aug 18, 2015 at 2:55 PM, Hans Wennborg wrote: >> >> On Tue, Aug 11, 2015 at 9:40 AM, Hans Wennborg wrote: >> > On Mon, Aug 10, 2015 at 12:39 PM, Reid Kleckner via cfe-commits >> > wrote: >> >> Author: rnk >> >> Date: Mon Aug 10 14:39:01 2015 >> >> New Revision: 244488 >> >> >> >> URL: http://llvm.org/viewvc/llvm-project?rev=244488&view=rev >> >> Log: >> >> [dllimport] A non-imported class with an imported key can't have a key >> >> >> >> Summary: >> >> The vtable takes its DLL storage class from the class, not the key >> >> function. When they disagree, the vtable won't be exported by the DLL >> >> that defines the key function. The easiest way to ensure that importers >> >> of the class emit their own vtable is to say that the class has no key >> >> function. >> >> >> >> Reviewers: hans, majnemer >> >> >> >> Subscribers: cfe-commits >> >> >> >> Differential Revision: http://reviews.llvm.org/D11913 >> > >> > Should we merge this and r244266 to 3.7? >> >> As pointed out on the r244266 thread, these patches are still not >> merged as the tests don't pass on 3.7. I was hoping someone more >> familiar with the patches could take a look. > > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: r244488 - [dllimport] A non-imported class with an imported key can't have a key
I merged both of them and tweaked the test case to make it work. On Tue, Aug 18, 2015 at 2:55 PM, Hans Wennborg wrote: > On Tue, Aug 11, 2015 at 9:40 AM, Hans Wennborg wrote: > > On Mon, Aug 10, 2015 at 12:39 PM, Reid Kleckner via cfe-commits > > wrote: > >> Author: rnk > >> Date: Mon Aug 10 14:39:01 2015 > >> New Revision: 244488 > >> > >> URL: http://llvm.org/viewvc/llvm-project?rev=244488&view=rev > >> Log: > >> [dllimport] A non-imported class with an imported key can't have a key > >> > >> Summary: > >> The vtable takes its DLL storage class from the class, not the key > >> function. When they disagree, the vtable won't be exported by the DLL > >> that defines the key function. The easiest way to ensure that importers > >> of the class emit their own vtable is to say that the class has no key > >> function. > >> > >> Reviewers: hans, majnemer > >> > >> Subscribers: cfe-commits > >> > >> Differential Revision: http://reviews.llvm.org/D11913 > > > > Should we merge this and r244266 to 3.7? > > As pointed out on the r244266 thread, these patches are still not > merged as the tests don't pass on 3.7. I was hoping someone more > familiar with the patches could take a look. > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: r244488 - [dllimport] A non-imported class with an imported key can't have a key
On Tue, Aug 11, 2015 at 9:40 AM, Hans Wennborg wrote: > On Mon, Aug 10, 2015 at 12:39 PM, Reid Kleckner via cfe-commits > wrote: >> Author: rnk >> Date: Mon Aug 10 14:39:01 2015 >> New Revision: 244488 >> >> URL: http://llvm.org/viewvc/llvm-project?rev=244488&view=rev >> Log: >> [dllimport] A non-imported class with an imported key can't have a key >> >> Summary: >> The vtable takes its DLL storage class from the class, not the key >> function. When they disagree, the vtable won't be exported by the DLL >> that defines the key function. The easiest way to ensure that importers >> of the class emit their own vtable is to say that the class has no key >> function. >> >> Reviewers: hans, majnemer >> >> Subscribers: cfe-commits >> >> Differential Revision: http://reviews.llvm.org/D11913 > > Should we merge this and r244266 to 3.7? As pointed out on the r244266 thread, these patches are still not merged as the tests don't pass on 3.7. I was hoping someone more familiar with the patches could take a look. ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: r244488 - [dllimport] A non-imported class with an imported key can't have a key
On Aug 11, 2015 9:45 AM, "Reid Kleckner" wrote: > > On Mon, Aug 10, 2015 at 6:40 PM, Richard Smith wrote: >> >> On Mon, Aug 10, 2015 at 5:43 PM, Reid Kleckner wrote: >>> >>> On Mon, Aug 10, 2015 at 5:00 PM, Richard Smith wrote: On Mon, Aug 10, 2015 at 12:39 PM, Reid Kleckner via cfe-commits < cfe-commits@lists.llvm.org> wrote: > > +// If the key function is dllimport but the class isn't, then the class has > +// no key function. The DLL that exports the key function won't export the > +// vtable in this case. > +if (MD->hasAttr() && !RD->hasAttr()) > + return nullptr; Does the same apply if the key function is DLLExport and the class is not? (Presumably this would just lead us to export a vtable that we don't need to, which is presumably harmless?) >>> >>> >>> No, there's no issue with dllexport. If the key function is dllexport, then it must be part of the same DLL as the current TU, and we can rely on it to provide (potentially non-exported) vtable and RTTI symbols. >> >> >> Even if a different compiler is used to compile the other TUs in this DLL? It seems to me that if the rule really is "the vtable takes its DLL storage class from the class, not the key function", then the TU with the key function need not actually provide a vtable symbol if the class is not dllexport but the key function is. > > > GCC (and presumably other Itanium compilers that support dllexport) still emit the vtable with the key function when the key is exported. It's just not necessarily exported. All the TUs that are statically linked into the DLL containing an exported key function can rely on the vtable to be there, so I don't see any reason to add logic around dllexport here. Make sense? Yes, thanks. ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: r244488 - [dllimport] A non-imported class with an imported key can't have a key
Yeah, let's do that. On Tue, Aug 11, 2015 at 9:40 AM, Hans Wennborg wrote: > On Mon, Aug 10, 2015 at 12:39 PM, Reid Kleckner via cfe-commits > wrote: > > Author: rnk > > Date: Mon Aug 10 14:39:01 2015 > > New Revision: 244488 > > > > URL: http://llvm.org/viewvc/llvm-project?rev=244488&view=rev > > Log: > > [dllimport] A non-imported class with an imported key can't have a key > > > > Summary: > > The vtable takes its DLL storage class from the class, not the key > > function. When they disagree, the vtable won't be exported by the DLL > > that defines the key function. The easiest way to ensure that importers > > of the class emit their own vtable is to say that the class has no key > > function. > > > > Reviewers: hans, majnemer > > > > Subscribers: cfe-commits > > > > Differential Revision: http://reviews.llvm.org/D11913 > > Should we merge this and r244266 to 3.7? > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: r244488 - [dllimport] A non-imported class with an imported key can't have a key
On Mon, Aug 10, 2015 at 6:40 PM, Richard Smith wrote: > On Mon, Aug 10, 2015 at 5:43 PM, Reid Kleckner wrote: > >> On Mon, Aug 10, 2015 at 5:00 PM, Richard Smith >> wrote: >> >>> On Mon, Aug 10, 2015 at 12:39 PM, Reid Kleckner via cfe-commits < >>> cfe-commits@lists.llvm.org> wrote: +// If the key function is dllimport but the class isn't, then the class has +// no key function. The DLL that exports the key function won't export the +// vtable in this case. +if (MD->hasAttr() && !RD->hasAttr()) + return nullptr; >>> >>> Does the same apply if the key function is DLLExport and the class is >>> not? (Presumably this would just lead us to export a vtable that we don't >>> need to, which is presumably harmless?) >>> >> >> No, there's no issue with dllexport. If the key function is dllexport, >> then it must be part of the same DLL as the current TU, and we can rely on >> it to provide (potentially non-exported) vtable and RTTI symbols. >> > > Even if a different compiler is used to compile the other TUs in this DLL? > It seems to me that if the rule really is "the vtable takes its DLL storage > class from the class, not the key function", then the TU with the key > function need not actually provide a vtable symbol if the class is not > dllexport but the key function is. > GCC (and presumably other Itanium compilers that support dllexport) still emit the vtable with the key function when the key is exported. It's just not necessarily exported. All the TUs that are statically linked into the DLL containing an exported key function can rely on the vtable to be there, so I don't see any reason to add logic around dllexport here. Make sense? ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: r244488 - [dllimport] A non-imported class with an imported key can't have a key
On Mon, Aug 10, 2015 at 12:39 PM, Reid Kleckner via cfe-commits wrote: > Author: rnk > Date: Mon Aug 10 14:39:01 2015 > New Revision: 244488 > > URL: http://llvm.org/viewvc/llvm-project?rev=244488&view=rev > Log: > [dllimport] A non-imported class with an imported key can't have a key > > Summary: > The vtable takes its DLL storage class from the class, not the key > function. When they disagree, the vtable won't be exported by the DLL > that defines the key function. The easiest way to ensure that importers > of the class emit their own vtable is to say that the class has no key > function. > > Reviewers: hans, majnemer > > Subscribers: cfe-commits > > Differential Revision: http://reviews.llvm.org/D11913 Should we merge this and r244266 to 3.7? ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: r244488 - [dllimport] A non-imported class with an imported key can't have a key
On Mon, Aug 10, 2015 at 5:43 PM, Reid Kleckner wrote: > On Mon, Aug 10, 2015 at 5:00 PM, Richard Smith > wrote: > >> On Mon, Aug 10, 2015 at 12:39 PM, Reid Kleckner via cfe-commits < >> cfe-commits@lists.llvm.org> wrote: >> >>> Author: rnk >>> Date: Mon Aug 10 14:39:01 2015 >>> New Revision: 244488 >>> >>> URL: http://llvm.org/viewvc/llvm-project?rev=244488&view=rev >>> Log: >>> [dllimport] A non-imported class with an imported key can't have a key >>> >>> Summary: >>> The vtable takes its DLL storage class from the class, not the key >>> function. When they disagree, the vtable won't be exported by the DLL >>> that defines the key function. The easiest way to ensure that importers >>> of the class emit their own vtable is to say that the class has no key >>> function. >>> >>> Reviewers: hans, majnemer >>> >>> Subscribers: cfe-commits >>> >>> Differential Revision: http://reviews.llvm.org/D11913 >>> >>> Modified: >>> cfe/trunk/lib/AST/RecordLayoutBuilder.cpp >>> cfe/trunk/test/CodeGenCXX/dllimport-rtti.cpp >>> >>> Modified: cfe/trunk/lib/AST/RecordLayoutBuilder.cpp >>> URL: >>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/RecordLayoutBuilder.cpp?rev=244488&r1=244487&r2=244488&view=diff >>> >>> == >>> --- cfe/trunk/lib/AST/RecordLayoutBuilder.cpp (original) >>> +++ cfe/trunk/lib/AST/RecordLayoutBuilder.cpp Mon Aug 10 14:39:01 2015 >>> @@ -2008,6 +2008,12 @@ static const CXXMethodDecl *computeKeyFu >>> continue; >>> } >>> >>> +// If the key function is dllimport but the class isn't, then the >>> class has >>> +// no key function. The DLL that exports the key function won't >>> export the >>> +// vtable in this case. >>> +if (MD->hasAttr() && !RD->hasAttr()) >>> + return nullptr; >>> >> >> Does the same apply if the key function is DLLExport and the class is >> not? (Presumably this would just lead us to export a vtable that we don't >> need to, which is presumably harmless?) >> > > No, there's no issue with dllexport. If the key function is dllexport, > then it must be part of the same DLL as the current TU, and we can rely on > it to provide (potentially non-exported) vtable and RTTI symbols. > Even if a different compiler is used to compile the other TUs in this DLL? It seems to me that if the rule really is "the vtable takes its DLL storage class from the class, not the key function", then the TU with the key function need not actually provide a vtable symbol if the class is not dllexport but the key function is. ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: r244488 - [dllimport] A non-imported class with an imported key can't have a key
On Mon, Aug 10, 2015 at 5:00 PM, Richard Smith wrote: > On Mon, Aug 10, 2015 at 12:39 PM, Reid Kleckner via cfe-commits < > cfe-commits@lists.llvm.org> wrote: > >> Author: rnk >> Date: Mon Aug 10 14:39:01 2015 >> New Revision: 244488 >> >> URL: http://llvm.org/viewvc/llvm-project?rev=244488&view=rev >> Log: >> [dllimport] A non-imported class with an imported key can't have a key >> >> Summary: >> The vtable takes its DLL storage class from the class, not the key >> function. When they disagree, the vtable won't be exported by the DLL >> that defines the key function. The easiest way to ensure that importers >> of the class emit their own vtable is to say that the class has no key >> function. >> >> Reviewers: hans, majnemer >> >> Subscribers: cfe-commits >> >> Differential Revision: http://reviews.llvm.org/D11913 >> >> Modified: >> cfe/trunk/lib/AST/RecordLayoutBuilder.cpp >> cfe/trunk/test/CodeGenCXX/dllimport-rtti.cpp >> >> Modified: cfe/trunk/lib/AST/RecordLayoutBuilder.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/RecordLayoutBuilder.cpp?rev=244488&r1=244487&r2=244488&view=diff >> >> == >> --- cfe/trunk/lib/AST/RecordLayoutBuilder.cpp (original) >> +++ cfe/trunk/lib/AST/RecordLayoutBuilder.cpp Mon Aug 10 14:39:01 2015 >> @@ -2008,6 +2008,12 @@ static const CXXMethodDecl *computeKeyFu >> continue; >> } >> >> +// If the key function is dllimport but the class isn't, then the >> class has >> +// no key function. The DLL that exports the key function won't >> export the >> +// vtable in this case. >> +if (MD->hasAttr() && !RD->hasAttr()) >> + return nullptr; >> > > Does the same apply if the key function is DLLExport and the class is not? > (Presumably this would just lead us to export a vtable that we don't need > to, which is presumably harmless?) > No, there's no issue with dllexport. If the key function is dllexport, then it must be part of the same DLL as the current TU, and we can rely on it to provide (potentially non-exported) vtable and RTTI symbols. ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: r244488 - [dllimport] A non-imported class with an imported key can't have a key
On Mon, Aug 10, 2015 at 12:39 PM, Reid Kleckner via cfe-commits <
cfe-commits@lists.llvm.org> wrote:
> Author: rnk
> Date: Mon Aug 10 14:39:01 2015
> New Revision: 244488
>
> URL: http://llvm.org/viewvc/llvm-project?rev=244488&view=rev
> Log:
> [dllimport] A non-imported class with an imported key can't have a key
>
> Summary:
> The vtable takes its DLL storage class from the class, not the key
> function. When they disagree, the vtable won't be exported by the DLL
> that defines the key function. The easiest way to ensure that importers
> of the class emit their own vtable is to say that the class has no key
> function.
>
> Reviewers: hans, majnemer
>
> Subscribers: cfe-commits
>
> Differential Revision: http://reviews.llvm.org/D11913
>
> Modified:
> cfe/trunk/lib/AST/RecordLayoutBuilder.cpp
> cfe/trunk/test/CodeGenCXX/dllimport-rtti.cpp
>
> Modified: cfe/trunk/lib/AST/RecordLayoutBuilder.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/RecordLayoutBuilder.cpp?rev=244488&r1=244487&r2=244488&view=diff
>
> ==
> --- cfe/trunk/lib/AST/RecordLayoutBuilder.cpp (original)
> +++ cfe/trunk/lib/AST/RecordLayoutBuilder.cpp Mon Aug 10 14:39:01 2015
> @@ -2008,6 +2008,12 @@ static const CXXMethodDecl *computeKeyFu
> continue;
> }
>
> +// If the key function is dllimport but the class isn't, then the
> class has
> +// no key function. The DLL that exports the key function won't
> export the
> +// vtable in this case.
> +if (MD->hasAttr() && !RD->hasAttr())
> + return nullptr;
>
Does the same apply if the key function is DLLExport and the class is not?
(Presumably this would just lead us to export a vtable that we don't need
to, which is presumably harmless?)
> +
> // We found it.
> return MD;
>}
>
> Modified: cfe/trunk/test/CodeGenCXX/dllimport-rtti.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/dllimport-rtti.cpp?rev=244488&r1=244487&r2=244488&view=diff
>
> ==
> --- cfe/trunk/test/CodeGenCXX/dllimport-rtti.cpp (original)
> +++ cfe/trunk/test/CodeGenCXX/dllimport-rtti.cpp Mon Aug 10 14:39:01 2015
> @@ -22,3 +22,11 @@ struct __declspec(dllimport) V {
> // GNU-DAG: @_ZTV1V = available_externally dllimport
> // GNU-DAG: @_ZTS1V = linkonce_odr
> // GNU-DAG: @_ZTI1V = linkonce_odr
> +
> +struct W {
> + __declspec(dllimport) virtual void f();
> + virtual void g();
> +} w;
> +// GNU-DAG: @_ZTV1W = linkonce_odr
> +// GNU-DAG: @_ZTS1W = linkonce_odr
> +// GNU-DAG: @_ZTI1W = linkonce_odr
>
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r244488 - [dllimport] A non-imported class with an imported key can't have a key
Author: rnk
Date: Mon Aug 10 14:39:01 2015
New Revision: 244488
URL: http://llvm.org/viewvc/llvm-project?rev=244488&view=rev
Log:
[dllimport] A non-imported class with an imported key can't have a key
Summary:
The vtable takes its DLL storage class from the class, not the key
function. When they disagree, the vtable won't be exported by the DLL
that defines the key function. The easiest way to ensure that importers
of the class emit their own vtable is to say that the class has no key
function.
Reviewers: hans, majnemer
Subscribers: cfe-commits
Differential Revision: http://reviews.llvm.org/D11913
Modified:
cfe/trunk/lib/AST/RecordLayoutBuilder.cpp
cfe/trunk/test/CodeGenCXX/dllimport-rtti.cpp
Modified: cfe/trunk/lib/AST/RecordLayoutBuilder.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/RecordLayoutBuilder.cpp?rev=244488&r1=244487&r2=244488&view=diff
==
--- cfe/trunk/lib/AST/RecordLayoutBuilder.cpp (original)
+++ cfe/trunk/lib/AST/RecordLayoutBuilder.cpp Mon Aug 10 14:39:01 2015
@@ -2008,6 +2008,12 @@ static const CXXMethodDecl *computeKeyFu
continue;
}
+// If the key function is dllimport but the class isn't, then the class has
+// no key function. The DLL that exports the key function won't export the
+// vtable in this case.
+if (MD->hasAttr() && !RD->hasAttr())
+ return nullptr;
+
// We found it.
return MD;
}
Modified: cfe/trunk/test/CodeGenCXX/dllimport-rtti.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/dllimport-rtti.cpp?rev=244488&r1=244487&r2=244488&view=diff
==
--- cfe/trunk/test/CodeGenCXX/dllimport-rtti.cpp (original)
+++ cfe/trunk/test/CodeGenCXX/dllimport-rtti.cpp Mon Aug 10 14:39:01 2015
@@ -22,3 +22,11 @@ struct __declspec(dllimport) V {
// GNU-DAG: @_ZTV1V = available_externally dllimport
// GNU-DAG: @_ZTS1V = linkonce_odr
// GNU-DAG: @_ZTI1V = linkonce_odr
+
+struct W {
+ __declspec(dllimport) virtual void f();
+ virtual void g();
+} w;
+// GNU-DAG: @_ZTV1W = linkonce_odr
+// GNU-DAG: @_ZTS1W = linkonce_odr
+// GNU-DAG: @_ZTI1W = linkonce_odr
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
