1220016 - Don't mix-and-match debug/non-debug MS CRT in NSS

Status: RESOLVED FIXED

(NSS :: Build, defect)

Description

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

The NSS makefiles, for reasons lost to history (or at least to hg blame), build on Windows with /D_DEBUG whenever BUILD_OPT=0, regardless of whether a debug CRT library is being used (controlled by USE_DEBUG_RTL in the NSS/NSPR makefiles).

For C this appears to be tolerated, but it causes some of the C++ standard library headers to enable error-checking containing calls to _CrtDbgReportW, which exists only in the debug CRT.

The current workaround is to force building gtests and the gtest library with /MTd.  This isn't the best idea to begin with — https://support.microsoft.com/en-us/kb/140584 explains some of the footguns of CRT-mixing — but more immediately this prevents statically linking gtests with the code under test, to allow unit tests to cover internal interfaces.

(Additional side-effect: forcing debug mode on ssl_gtest is hiding an instance of 
optimization warning C4800 that would otherwise break the build.)

I'm not sure why we're changing the definedness of _DEBUG at all, rather than leaving that to the /M flags, but it's been like that since 2000 so I'm hesistant to touch it.  One alternative would be to just filter it out for C++ files or for directories using C++.

Comment 1

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Attachment: Patch: remove explicit -D_DEBUG and -U_DEBUG from all compilation.

This is one approach; changing the flags just for the gtest directories also works.

Comment 2

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Comment on attachment 8681572 [details] [diff] [review]
Patch: remove explicit -D_DEBUG and -U_DEBUG from all compilation.

(Applying suggestions I got about asking whether this patch is reasonable.)

Comment 3

[Martin Thomson [:mt:]]

Comment on attachment 8681572 [details] [diff] [review]
Patch: remove explicit -D_DEBUG and -U_DEBUG from all compilation.

Seems fine.

Comment 4

[Kate McKinley [:kmckinley, :Kate]]

Comment on attachment 8681572 [details] [diff] [review]
Patch: remove explicit -D_DEBUG and -U_DEBUG from all compilation.

Review of attachment 8681572 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.

Comment 5

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Comment on attachment 8681572 [details] [diff] [review]
Patch: remove explicit -D_DEBUG and -U_DEBUG from all compilation.

Thanks for the feedback.  It's passed all 2h20m of NSS tests and doesn't break Gecko on Windows, so r?.

Comment 6

[Eric Rescorla (:ekr)]

Comment on attachment 8681572 [details] [diff] [review]
Patch: remove explicit -D_DEBUG and -U_DEBUG from all compilation.

Review of attachment 8681572 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM

Comment 7

[Martin Thomson [:mt:]]

Comment on attachment 8681572 [details] [diff] [review]
Patch: remove explicit -D_DEBUG and -U_DEBUG from all compilation.

http://hg.mozilla.org/projects/nss/rev/24d118f3c692