Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

crypto: remove unnecessary template class #12993

Closed
wants to merge 1 commit into from
Closed

crypto: remove unnecessary template class #12993

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion src/node_crypto.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,6 @@ std::string extra_root_certs_file; // NOLINT(runtime/string)
X509_STORE* root_cert_store;

// Just to generate static methods
template class SSLWrap<TLSWrap>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could it be that this was supposed to generate the vtable or something? But I agree, I don’t really see the point in having any of the methods listed here … there should be an extern template matching them, or they should not be there at all, right? (and I would prefer the latter.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that the explicit instantiation for the class template is not required as it would only create template instantiations for the members of the class, but not for the static class members.

Without the explicit instantiation for the static members my understanding is that the compiler will not instantiate the template. If the usage of the template is in a separate file and compiled separately it will just have a symbol left for the linker to resolve, but since the compiler never compiled anything for the function there will be a link error.

If I'm not mistaken the only member functions of SSLWrap are the following:

void DestroySSL();
void WaitForCertCb(CertCb cb, void* arg);
void SetSNIContext(SecureContext* sc);
int SetCACerts(SecureContext* sc);

which are also explicitly instantiated. The above functions are protected members of SSLWrap. If they had been public member I believe that they would have been instantiated using template class SSLWrap<TLSWrap. So I still think that it would safe to remove that, but keep the rest of the instantiations and perhaps adding a comment. Does this make sense to you?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@danbev Actually, I’m a bit confused. 😄 For a linker error to be generated, the compiler would have first needed to be told that there are explicit instantiations generated elsewhere; but I don’t see anything like that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I’m a bit confused. 😄

Welcome to my world 😄 .
I think I did a bad job of trying to explain what I think is going on. Let me try again.

For a linker error to be generated, the compiler would have first needed to be told that there are explicit instantiations generated elsewhere; but I don’t see anything like that?

Lets focus on the removal of the explicit class instantiations which this PR is about:

template class SSLWrap<TLSWrap>;

And the explicit instantiations of its protected member functions:

template void SSLWrap<TLSWrap>::DestroySSL();
template void SSLWrap<TLSWrap>::WaitForCertCb(CertCb cb, void* arg);
template void SSLWrap<TLSWrap>::SetSNIContext(SecureContext* sc);
template int SSLWrap<TLSWrap>::SetCACerts(SecureContext* sc);

Lets say we want to also remove the explicit template instantiations of the protected members as well.

Lets comment out DestroySSL which is used in tls_wrap.cc:

TLSWrap* wrap;
...
wrap->SSLWrap<TLSWrap>::DestroySSL();

If we take a look at the unresolved symbols for tls_wrap.o we can find the following:

$ nm -gU out/Debug/obj.target/node/src/tls_wrap.o|grep DestroySSL
00000000000048f0 T __ZN4node7TLSWrap10DestroySSLERKN2v820FunctionCallbackInfoINS1_5ValueEEE

$ c++filt __ZN4node7TLSWrap10DestroySSLERKN2v820FunctionCallbackInfoINS1_5ValueEEE
node::TLSWrap::DestroySSL(v8::FunctionCallbackInfo<v8::Value> const&)

My understanding is that the compiler will check the types of the template when compiling tls_wrap.cc, but then just create a symbol to be resolve later by the linker.

If we take a look at the symbols created for DestroySSL in node_crypto.o we find:

$ nm -g out/Debug/obj.target/node/src/node_crypto.o | grep DestroySSL
0000000000009250 T __ZN4node6crypto7SSLWrapINS0_10ConnectionEE10DestroySSLEv

$ c++filt __ZN4node6crypto7SSLWrapINS0_10ConnectionEE10DestroySSLEv
node::crypto::SSLWrap<node::crypto::Connection>::DestroySSL()

In this case the compiler only instantiated (generated the function specialised for type node::crypto::Connection) which was done implicitly/automatic as it is used in node_crypto.cc. But there is no such instantiation of DestroySSL for a type of TLSWrap as there is no usage of it (it is used in a separate compilation unit) which will lead to a link error.

If we comment back the explicit instantiation of DestroySSL we can inspect node_crypto.o once again and see that it generated the function specialised for TLSWrap:

$ nm -g out/Debug/obj.target/node/src/node_crypto.o | grep DestroySSL
0000000000009250 T __ZN4node6crypto7SSLWrapINS0_10ConnectionEE10DestroySSLEv
000000000001bbb0 T __ZN4node6crypto7SSLWrapINS_7TLSWrapEE10DestroySSLEv

$ c++filt __ZN4node6crypto7SSLWrapINS_7TLSWrapEE10DestroySSLEv
node::crypto::SSLWrap<node::TLSWrap>::DestroySSL()

Now, this is my understanding of what is going on so please correct me if I'm wrong about anything here.

@addaleax Does that make sense?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@danbev Sorry, yes, it does. :) It’s not really good practice to use template functions without having definitions available, that’s why I got confused, but I see that you’re right now.

template void SSLWrap<TLSWrap>::AddMethods(Environment* env,
Local<FunctionTemplate> t);
template void SSLWrap<TLSWrap>::InitNPN(SecureContext* sc);
Expand Down