1189967 - Char16.h includes <string> even when MOZ_NO_MOZALLOC is defined

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Bas Schouten (:bas.schouten)]

Attachment: Don't include string when MOZ_NO_MOZALLOC is defined

This doesn't work since our <string> wrapper errors out when MOZ_NO_MOZALLOC is defined.

Comment 1

[Mike Hommey [:glandium]]

Comment on attachment 8641911 [details] [diff] [review]
Don't include string when MOZ_NO_MOZALLOC is defined

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

::: mfbt/Char16.h
@@ +47,5 @@
>  #    define MOZ_USE_CHAR16_WRAPPER
>  #  endif
>  #endif
>  
> +#if defined(MOZ_USE_CHAR16_WRAPPER) && !defined(MOZ_NO_MOZALLOC)

... except the result is also to use char16_t* for char16ptr_t, which is likely not desirable.

You're failing to mention what you're trying to fix here. MOZ_NO_MOZALLOC should almost never be used.

Comment 2

[Bas Schouten (:bas.schouten)]

(In reply to Mike Hommey [:glandium] from comment #1)
> Comment on attachment 8641911 [details] [diff] [review]
> Don't include string when MOZ_NO_MOZALLOC is defined
> 
> Review of attachment 8641911 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mfbt/Char16.h
> @@ +47,5 @@
> >  #    define MOZ_USE_CHAR16_WRAPPER
> >  #  endif
> >  #endif
> >  
> > +#if defined(MOZ_USE_CHAR16_WRAPPER) && !defined(MOZ_NO_MOZALLOC)
> 
> ... except the result is also to use char16_t* for char16ptr_t, which is
> likely not desirable.
> 
> You're failing to mention what you're trying to fix here. MOZ_NO_MOZALLOC
> should almost never be used.

Fair point, so xpcom/typelib/xpt is compiled with MOZ_NO_ALLOC, yet it also includes mozilla-config.h which in turn includes Char16.h. On earlier versions of Visual Studio this would not include <string> and therefor not touch any of our STL wrappers. In newer versions it could, this means mozilla-config.h will now conflict with MOZ_NO_MOZALLOC since out <string> STL wrapper #errors on MOZ_NO_MOZALLOC not being defined.

Comment 3

[Mike Hommey [:glandium]]

Sounds to me it's time to fix bug 1138321.

Comment 4

[Bas Schouten (:bas.schouten)]

(In reply to Mike Hommey [:glandium] from comment #3)
> Sounds to me it's time to fix bug 1138321.

Anything I can do to help there? I'd like VS2015 to build :).

Comment 5

[Bas Schouten (:bas.schouten)]

So this causes additional issues, since we force-include mozilla-config.h in a bunch of places, this causes us to draw in mozalloc declarations all over the place (for example stuff in testing/tools) where we don't actually link against MOZALLOC or libxul.. Causing linking issues later on.

Comment 6

[Bas Schouten (:bas.schouten)]

Basically currently the only way to make this work is defining MOZ_NO_MOZALLOC in all the tests that don't link against mozalloc (since we include mozilla-config.h in all of them), or start linking them against mozalloc..

Comment 7

[Bas Schouten (:bas.schouten)]

Attachment: Allow specifying not including the char16ptr wrapper to avoid string usage

So this is one of the solutions to the problem I can come up with, a lot of these cases have stand-alone tests, they get built before mozalloc even exists and so can't easily be linked against it without changing build order. At the same time we probably don't just want to disable STL_WRAPPERS as some of these files are actually also linked into xul.dll. Having said that it's questionable whether there's a point in allowing the STL wrappers as any use of STL headers would cause the links of these stand-alone tests to fail anyway.

Comment 8

[Mike Hommey [:glandium]]

Attachment: Stop including mozilla/Char16.h from mozilla-config.h

The snippet including mozilla/Char16.h was added in bug 895047, which
also made mozilla/Char16.h define __PRUNICHAR__ and typedef PRUnichar
itself. That part was removed in bug 956507, and there is actually no
reason anymore to ensure mozilla/Char16.h is included before prtypes.h.
In fact, doing so presently doesn't change anything for prtypes.h.

This simplifies the problem Bas is trying to fix, and I suspect it's
actually enough to fix it. Bas. Can you check?

Comment 9

[Bas Schouten (:bas.schouten)]

Comment on attachment 8647283 [details] [diff] [review]
Stop including mozilla/Char16.h from mozilla-config.h

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

Interestingly this patch makes things worse, as it causes a bunch of other places in the build to now include out STL wrappers in more inconvenient places where they cause more, new and exciting build errors :-(. (of the type in bug 1189891)

Comment 10

[Mike Hommey [:glandium]]

I think it just triggers bug 1120793 earlier than it normally happens.

Comment 11

[Bas Schouten (:bas.schouten)]

(In reply to Mike Hommey [:glandium] from comment #10)
> I think it just triggers bug 1120793 earlier than it normally happens.

I had the second patch of that bug in my tree (skip string for mozalloc one), it also built fine with my patch (from this bug) applied which shouldn't really fix anything bug 1120793 related, but all this include stuff is somewhat of a mystery to me so who knows.

Comment 12

[Mike Hommey [:glandium]]

Attachment: Be slightly more explicit about some conversions to std::wstring

Comment 13

[Mike Hommey [:glandium]]

Attachment: Avoid including <string> from Char16.h

Since Char16.h is included everywhere, and MSVC 2015 uses the char16ptr_t trick
it contains, we include <string> everywhere, but that has the side effect of
breaking the build in subtle ways. One way around this would be to avoid including
Char16.h in the first place, but that requires more work than I was ready to put
in. So instead, just avoid including <string> by removing the conversion operator
for std::wstring.

Comment 14

[Mike Hommey [:glandium]]

Attachment: Add cmath to the wrapped STL headers

Somehow, TimeStamp.cpp fails to build with MSVC 2015 without this.

Comment 15

[Mike Hommey [:glandium]]

Note, I think we should remove the char16ptr_t thing, and switch uses of std::wstring to std::u16string or some other type.

Comment 16

[Mike Hommey [:glandium]]

Attachment: Be slightly more explicit about some conversions to std::wstring

Fixup for OSX.

Comment 17

[Mike Hommey [:glandium]]

Attachment: Avoid conflicting declarations for our raise wrappers on Windows

This fixes warnings that we are actually already getting, but that adding cmath to the STL wrappers makes a hard error because TimeStamp.cpp is built with warnings as errors.

Comment 18

[Nathan Froyd [:froydnj]]

Comment on attachment 8650885 [details] [diff] [review]
Avoid including <string> from Char16.h

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

::: mfbt/Char16.h
@@ -92,5 @@
>    operator bool() const
>    {
>      return mPtr != nullptr;
>    }
> -  operator std::wstring() const

I wonder if it's worth |= delete|'ing this, just so we have a helpful place to hang a "this is why we don't provide conversion to wstring" comment.

Comment 19

[Nathan Froyd [:froydnj]]

Comment on attachment 8650959 [details] [diff] [review]
Avoid conflicting declarations for our raise wrappers on Windows

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

::: memory/mozalloc/msvc_raise_wrappers.h
@@ +17,5 @@
>  
>  #include "mozilla/mozalloc_abort.h"
>  
> +// xutility will declare those functions in the std namespace, which we
> +// will define in msvc_raise_wrappers.cpp

This comment reads a little funny to me (mostly around "those functions", how about:

"xutility will declare the following functions in the std namespace.  We #define them to be named differently so we can ensure the exception throwing semantics of these functions work exactly the way we want, by defining our own versions in msvc_raise_wrappers.cpp."

Comment 20

[Mike Hommey [:glandium]]

(In reply to Nathan Froyd [:froydnj][:nfroyd] from comment #18)
> Comment on attachment 8650885 [details] [diff] [review]
> Avoid including <string> from Char16.h
> 
> Review of attachment 8650885 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mfbt/Char16.h
> @@ -92,5 @@
> >    operator bool() const
> >    {
> >      return mPtr != nullptr;
> >    }
> > -  operator std::wstring() const
> 
> I wonder if it's worth |= delete|'ing this, just so we have a helpful place
> to hang a "this is why we don't provide conversion to wstring" comment.

How do you = delete it without including <string> for the definition of wstring? The problem being with the include, not wstring.

Comment 21

[Nathan Froyd [:froydnj]]

(In reply to Mike Hommey [:glandium] from comment #20)
> (In reply to Nathan Froyd [:froydnj][:nfroyd] from comment #18)
> > I wonder if it's worth |= delete|'ing this, just so we have a helpful place
> > to hang a "this is why we don't provide conversion to wstring" comment.
> 
> How do you = delete it without including <string> for the definition of
> wstring? The problem being with the include, not wstring.

Fair point!  Can't even forward-declare wstring, since it's a typedef...

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/73bd8b894c6b
https://hg.mozilla.org/integration/mozilla-inbound/rev/6fa4f5768491
https://hg.mozilla.org/integration/mozilla-inbound/rev/3cb2f2b870f4
https://hg.mozilla.org/integration/mozilla-inbound/rev/187097ffcdc6

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/73bd8b894c6b
https://hg.mozilla.org/mozilla-central/rev/6fa4f5768491
https://hg.mozilla.org/mozilla-central/rev/3cb2f2b870f4
https://hg.mozilla.org/mozilla-central/rev/187097ffcdc6