1189891 - STL wrappers break VS 2015 (type_traits errors)

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Bas Schouten (:bas.schouten)]

Attachment: Only include STL stuff on the first include

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.

Comment 1

[Mike Hommey [:glandium]]

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.

Comment 2

[Bas Schouten (:bas.schouten)]

Attachment: Only include STL stuff on the first include v2

Comment 3

[Mike Hommey [:glandium]]

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 */

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/151d70cfe45f

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/d50e07c09227 for windows build bustage:

https://treeherder.mozilla.org/logviewer.html#?job_id=13104958&repo=mozilla-inbound

Comment 7

[Mike Hommey [:glandium]]

Attachment: Include <string> from pkix/Input.h

This fixes the error seen when the first patch landed. This also happens to be required for bug 1189967.

Comment 8

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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.

Comment 9

[Mike Hommey [:glandium]]

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

Comment 10

[Mike Hommey [:glandium]]

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.

Comment 11

[Mike Hommey [:glandium]]

Attachment: Avoid including <cstring> from pkix/Input.h

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/af1b36497559
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d52ab626597

Comment 13

[Mike Hommey [:glandium]]

Can't have nice things. This broke pkix tests, so backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/eaa9d14c60af

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/491c4ae1ea4a
https://hg.mozilla.org/integration/mozilla-inbound/rev/97207786c908

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/491c4ae1ea4a
https://hg.mozilla.org/mozilla-central/rev/97207786c908