STL wrappers break VS 2015 (type_traits errors)

RESOLVED FIXED in Firefox 43

Status

defect
RESOLVED FIXED
4 years ago
Last year

People

(Reporter: bas.schouten, Assigned: bas.schouten)

Tracking

unspecified
mozilla43
Dependency tree / graph

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

From https://bugzilla.mozilla.org/show_bug.cgi?id=1120793#c7:

'The next issue I found in the VS 2015 rabbit hole:

<xstddef> includes <cstdlib>, but the mozilla build arranges for its own wrapper file 'cstdlib' to get loaded instead. The wrapper 'cstdlib' has a number of layering violations, carefully #defined and ordered so everything works.

By 'layering violation', I mean that <cstdlib> just includes bits of the 'C' standard library, and typically gets pulled in before the C++ std library headers. So it doesn't/shouldn't assume the presence of C++ std library definitions.

Our 'cstdlib' wrapper and its header dependency tree, however, does pull in C++ std library definitions. These C++ headers try to pull in <xstddef>, which gets skipped because the compiler doesn't like infinite loops. Then the C++ headers try to use the content from <xstddef>, leading to errors like this:

type_traits(104): error C3646: '_Bool_type': unknown override specifier

So, I can probably hack the #defines and the order of includes to make it work again, but perhaps a more robust approach would be to move the C++ content out of the 'cstdlib' wrapper into some more layer-appropriate headers.

Either way risks other build targets and environments, since the changes mess with core header files. Option 1 (hack it to work) seems easier but more fragile, while option 2 is a larger change, more likely to disrupt years of carefully arranged header files.

I'm hoping that more experienced Mozilla contributors will point out options 3 and 4 - I'd love some feedback at this point.

Also, let me know if I should go with side-option A: open a new bug for this issue.'

This can be worked around by making sure we only attempt to include C++ headers the first time we include a wrapper.
Attachment #8641800 - Flags: feedback?(mh+mozilla)
Attachment #8641800 - Attachment is patch: true
Blocks: VC14
Comment on attachment 8641800 [details] [diff] [review]
Only include STL stuff on the first include

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

::: config/msvc-stl-wrapper.template.h
@@ +11,5 @@
>  #if _HAS_EXCEPTIONS
>  #  error "STL code can only be used with -fno-exceptions"
>  #endif
>  
> +#ifndef HAVE_INCLUDED_ALLOC

You can also wrap the _HAS_EXCEPTIONS above.
a mozilla_ or MOZ_ or __MOZ_ prefix would be better.
Attachment #8641800 - Flags: feedback?(mh+mozilla) → feedback+
Assignee: nobody → bas
Attachment #8641800 - Attachment is obsolete: true
Attachment #8644162 - Flags: review?(mh+mozilla)
Comment on attachment 8644162 [details] [diff] [review]
Only include STL stuff on the first include v2

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

Sorry I didn't look at this one earlier, I thought it was the tricky one.

::: config/msvc-stl-wrapper.template.h
@@ +34,5 @@
>  #  include "mozilla/mozalloc.h"
>  #else
>  #  error "STL code can only be used with infallible ::operator new()"
>  #endif
> +#endif

/* MOZ_HAVE_INCLUDED_ALLOC */
Attachment #8644162 - Flags: review?(mh+mozilla) → review+
Duplicate of this bug: 1181726
This fixes the error seen when the first patch landed. This also happens to be required for bug 1189967.
Flags: needinfo?(bas)
Attachment #8650764 - Flags: review?(dkeeler)
Comment on attachment 8650764 [details] [diff] [review]
Include <string> from pkix/Input.h

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

::: security/pkix/include/pkix/Input.h
@@ +30,5 @@
> +// That also triggers C4350 warnings on MSVC 2013, which we simply ignore.
> +// https://connect.microsoft.com/VisualStudio/feedback/details/767960/warning-c4350-behavior-change-when-including-string-and-no-precompiled-header
> +#pragma warning(push)
> +#pragma warning(disable:4350)
> +#include <string>

First, this should all be guarded with `#if defined(_MSC_VER) && _MSC_VER >= 1900`

We should not include <string> in this header file, because mozilla::pkix is not supposed to use <string> (except in code under test/), and this would just make that confusing.

I would rather have the include of <cstring> removed, if the include of <cstring> is the problem. e.g. we can maybe use <algorithms> to deal with the std::memcmp calls, or just reimplement memcmp in the header file using a for loop.

RE: warning C4350: It is best to disable this warning in security/pkix/warnings.mozbuild and anywhere else that it needs to be disabled. These "behavior change" warnings are not helpful to us.
(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #8)
> Comment on attachment 8650764 [details] [diff] [review]
> Include <string> from pkix/Input.h
> 
> Review of attachment 8650764 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: security/pkix/include/pkix/Input.h
> @@ +30,5 @@
> > +// That also triggers C4350 warnings on MSVC 2013, which we simply ignore.
> > +// https://connect.microsoft.com/VisualStudio/feedback/details/767960/warning-c4350-behavior-change-when-including-string-and-no-precompiled-header
> > +#pragma warning(push)
> > +#pragma warning(disable:4350)
> > +#include <string>
> 
> First, this should all be guarded with `#if defined(_MSC_VER) && _MSC_VER >=
> 1900`

No, because it affects 2013 as well. This is why the other patch in this bug was backed out.
 
> We should not include <string> in this header file, because mozilla::pkix is
> not supposed to use <string> (except in code under test/), and this would
> just make that confusing.
> 
> I would rather have the include of <cstring> removed, if the include of
> <cstring> is the problem. e.g. we can maybe use <algorithms> to deal with
> the std::memcmp calls, or just reimplement memcmp in the header file using a
> for loop.
> 
> RE: warning C4350: It is best to disable this warning in
> security/pkix/warnings.mozbuild and anywhere else that it needs to be
> disabled. These "behavior change" warnings are not helpful to us.

IMHO, it's best to disable it exactly where it's being caused. Or globally, like we disable many others.
So, for something interesting:
- There are three calls to memcmp in that header. Two are namespaced, one is not.
- Removing the cstring include on Windows just works, it doesn't complain.
- On Linux, doing the same doesn't work, but replacing the two std::memcmp with memcmp does (without a warning), because string.h ends up being included from mozalloc.h, which is included from our STL wrappers.
Attachment #8650764 - Attachment is obsolete: true
Attachment #8650764 - Flags: review?(dkeeler)
Attachment #8650795 - Flags: review?(brian)
Attachment #8650795 - Flags: review?(brian) → review+
Can't have nice things. This broke pkix tests, so backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/eaa9d14c60af
https://hg.mozilla.org/mozilla-central/rev/491c4ae1ea4a
https://hg.mozilla.org/mozilla-central/rev/97207786c908
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.