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.

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
base: master
from

Conversation

Projects
None yet
8 participants
@danbev
Member

danbev commented May 12, 2017

I came across this template class but I don't understand why it is
there. It is not used in the template specialization following it.

I just wanted to bring it up just in case this is something that
has been overlooked.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

crypto

crypto: remove unnecessary template class
I came across this template class but I don't understand why it is
there. It is not used in the template specialization following it.

I just wanted to bring it up just in case this is something that
has been overlooked.
@danbev

This comment has been minimized.

Show comment
Hide comment
@jasnell

LGTM but let's get @indutny to weigh in

@@ -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>;

This comment has been minimized.

@addaleax

addaleax May 12, 2017

Member

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.)

@addaleax

addaleax May 12, 2017

Member

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.)

This comment has been minimized.

@danbev

danbev May 15, 2017

Member

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?

@danbev

danbev May 15, 2017

Member

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?

This comment has been minimized.

@addaleax

addaleax May 18, 2017

Member

@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?

@addaleax

addaleax May 18, 2017

Member

@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?

This comment has been minimized.

@danbev

danbev May 22, 2017

Member

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?

@danbev

danbev May 22, 2017

Member

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?

This comment has been minimized.

@addaleax

addaleax May 23, 2017

Member

@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.

@addaleax

addaleax May 23, 2017

Member

@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.

@refack

This comment has been minimized.

Show comment
Hide comment
@refack

refack May 12, 2017

Member

ping @indutny it's your add b9a0eb0
<ins I didn't see @jasnell's comment. Ooof GitHub is so inconsistent with comments left as review summaries>

Member

refack commented May 12, 2017

ping @indutny it's your add b9a0eb0
<ins I didn't see @jasnell's comment. Ooof GitHub is so inconsistent with comments left as review summaries>

@refack refack requested a review from indutny May 12, 2017

@indutny

LGTM, if it builds.

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax May 23, 2017

Member

Landed in 4f4184c

Member

addaleax commented May 23, 2017

Landed in 4f4184c

@addaleax addaleax closed this May 23, 2017

addaleax added a commit that referenced this pull request May 23, 2017

crypto: remove unnecessary template class
I came across this template class but I don't understand why it is
there. It is not used in the template specialization following it.

I just wanted to bring it up just in case this is something that
has been overlooked.

PR-URL: #12993
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

jasnell added a commit that referenced this pull request May 23, 2017

crypto: remove unnecessary template class
I came across this template class but I don't understand why it is
there. It is not used in the template specialization following it.

I just wanted to bring it up just in case this is something that
has been overlooked.

PR-URL: #12993
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

jasnell added a commit that referenced this pull request May 28, 2017

crypto: remove unnecessary template class
I came across this template class but I don't understand why it is
there. It is not used in the template specialization following it.

I just wanted to bring it up just in case this is something that
has been overlooked.

PR-URL: #12993
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

@jasnell jasnell referenced this pull request May 28, 2017

Closed

8.0.0 Release Proposal #12220

@gibfahn gibfahn referenced this pull request Jun 15, 2017

Closed

Auditing for 6.11.1 #230

2 of 3 tasks complete

@danbev danbev deleted the danbev:remove-unnecessary-template-class branch Jun 28, 2017

MylesBorins added a commit that referenced this pull request Jul 17, 2017

crypto: remove unnecessary template class
I came across this template class but I don't understand why it is
there. It is not used in the template specialization following it.

I just wanted to bring it up just in case this is something that
has been overlooked.

PR-URL: #12993
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Jul 17, 2017

Member

I opted to not land this into LTS... please let me know if it should land

Member

MylesBorins commented Jul 17, 2017

I opted to not land this into LTS... please let me know if it should land

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment