118830 - NSS public header files should be C++ safe

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P3)

Description

[Wan-Teh Chang]

NSS public header files should be able to be safely included
by a C++ file without the need for an extern "C" { } block.

I just noticed that several NSS headers are included by PSM
files (C++) in extern "C" blocks:

pkcs11.h
pkcs12.h
p12plcy.h
pk11pqg.h
secdert.h
keydbt.h
crmf.h

We should get them fixed.

Comment 1

[Julien Pierre]

Wan-Teh,

Did you try to remove the extern "C" and build PSM ?
I know there have been issues in the past with headers' C++ safety but many got
resolved. Usually a temporary workaround was to put extern "C" before fixes were
made. That was certainly the case in the web server, and those workarounds were
never removed since their presence did not harm anything in the build process. 

Comment 2

[Wan-Teh Chang]

No, I didn't try to remove the extern "C" and see if it works.

I filed this bug so that I don't forget about this problem.
It is low priority and has an easy workaround.

Comment 3

[Wan-Teh Chang]

Changed the QA contact to Bishakha.

Comment 4

[Wan-Teh Chang]

Set target milestone to NSS 3.5.

Comment 5

[Wan-Teh Chang]

Moved to target milestone 3.8 because the original
NSS 3.7 release has been renamed 3.8.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Remove target milestone of 3.8, since these bugs didn't get into that release.

Comment 7

[Julien Pierre]

I checked all the header files named above. keydbt.h no longer exists. All the others have been fixed and are C++-safe, except for secdert.h which has some data symbols outside that are not in a SEC_BEGIN_PROTOS / SEC_END_PROTOS block . Somehow I doubt this really poses much of a problem because the data symbols wouldn't even be visible on Windows, so the name mangling would probably not even come into play. But just for consistency, I will provide a patch for that header.

Comment 8

[Julien Pierre]

Attachment: Add SEC_BEGIN_PROTOS / SEC_END_PROTOS

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 271781 [details] [diff] [review]
Add SEC_BEGIN_PROTOS / SEC_END_PROTOS

Doesn't the ``extern "C" { '' need to enclose all the typedefs and struct
type declarations, to prevent their type names from being mangled, and
to prevent the structs from being treated as public classes?  
If so, then the BEGIN_PROTOS is in the wrong place.
If not, then this is OK, but I'd prefer that the BEGIN_PROTOS be put at 
the front of the file anyway.

Comment 10

[Julien Pierre]

Nelson,

extern "C" doesn't have any effect on typedefs and structs, it is only for function and variable symbol names, but it is harmless to move it up.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 271781 [details] [diff] [review]
Add SEC_BEGIN_PROTOS / SEC_END_PROTOS

r=nelson
Move BEGIN_PROTOS up or not as you see fit.

Comment 12

[Wan-Teh Chang]

Right.  I observed that NSS headers have been careful to
exclude typedefs, structs, and macros from extern "C".
So it is a good idea to preserve that convention.

Comment 13

[Julien Pierre]

Thanks for the reviews. I checked in the patch as-is to the trunk.

Checking in secdert.h;
/cvsroot/mozilla/security/nss/lib/util/secdert.h,v  <--  secdert.h
new revision: 1.3; previous revision: 1.2
done

Comment 14

[Kai Engert (:KaiE:)]

This patch breaks compilation of PSM-with-NSS-3.12

nsKeygenHandler.cpp
../../../../dist/public/nss/secdert.h:163: error: expected constructor, destructor, or type conversion before ‘extern’
../../../../dist/include/system_wrappers/secdert.h:4: error: ‘#pragma’ is not allowed here

It was necessary to add
  #include "seccomon.h"
to file secdert.h

Comment 15

[Kai Engert (:KaiE:)]

The compilation error was addressed in bug 388120, setting bug back to resolved.