1120793 - Remove outdated _Throw() hack

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

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

+++ This bug was initially created as a clone of Bug #560723 +++

In bug 1119072 comment 2, Brian Smith wrote:
> When VS2015 comnpiles the <exception> header, it fails
> to recognize the _THROW0() macro, causing a syntax error. The strange thing
> is that there is a "#inclue <xstddef>" in <exception> which should cause
> _THROW0() to always be defined. It may be a bug in VS2015's "#pragma once"
> support.

In bug 1119072 comment 11, Brian Smith wrote:
>  I noticed that the logic for replacing _Raise includes
> <xstddef> and <exception>, and the exact problem we're
> having is that <exception> seems to not always recognize
> the macros defined in <xstddef>. So, I think it is likely
> that the overriding of _Raise needs to be redone for
> VS2015. But, that's beyond my current knowledge level.
> Help is appreciated.

I noticed that this doesn't start happening until about 5 minutes into the build, so it might be something specific to how certain modules/directories are configured.

Comment 1

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

Attachment: remove-raise-throw.patch

Need to investigate whether the CRT stuff from bug 560723 is still relevant in today's Gecko. If not, it seems like we can just do this, or similar.

Comment 2

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

I don't have time to work on this. It would be great if somebody else could help out. There's now only a few reasons why the MSVC build fails, and this is the most urgent one.

Comment 3

[Bruce Williams]

I drilled into this a bit, and found the following:

The break occurs when building memory/mozalloc/msvc_raise_wrappers.cpp.

The build implicitly pulls in the header file <mozilla-config.h> before any of the headers explicitly specified in the .cpp file.

The .cpp file explicitly includes its own header file, <msvc_raise_wrappers.h>. Since it wants to patch some parts of the std library, it verifies that nobody has already loaded the std headers containing the patch targets.

This check fails because <mozilla-config.h> pulls in Char16.h, and that file's include behavior changes under VS 2015, as described below:

Char16.h includes a helper class (char16ptr_t) that specifically targets Windows built using VS 2015 or later (_MSC_VER >= 1900). This code pulls in <string>. <string>, in turn, deep in its header dependency tree, pulls in <xstddef>, which causes the check described above to fail.

I propose moving the include of <msvc_raise_wrappers.h> into <mozilla-config.h>, somewhere before <Char16.h> is included. I'll try this locally and see how it flies.

Comment 4

[(Away)]

Thanks for the analysis, Bruce!

I wonder if we could do the same avoid-Char16.h hack for msvc_raise_wrappers.cpp as in bug 1135583.

Comment 5

[Bruce Williams]

The fix for bug 1135583 also works when applied to this build target. I have the fix here, but being new to Mozilla, I have no idea what review/checkin procedure is. I'll look through the Mozilla dev site docs, and feel free to just let me know.

Comment 6

[Bruce Williams]

I should also mention, applying that fix merely gets you to the next set of errors - it doesn't totally solve VS 2015 build.

Comment 7

[Bruce Williams]

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.

Comment 8

[Bruce Williams]

Attachment: skip-string-for-mozalloc.patch

Comment 9

[(Away)]

Comment on attachment 8612690 [details] [diff] [review]
skip-string-for-mozalloc.patch

Thanks for the patch! Glandium is the best reviewer for Windows build changes.

Given that the build succeeds with the workaround in comment 1, I suspect that all of the remaining issues must be related to the _THROW wrappers. IMO we should continue the investigation here in this bug, and solve the entire problem before landing the fixes. That's my take, at least.

Glandium do you have any advice re comment 7?

Comment 10

[Bruce Williams]

Unfortunately the the "skip the contents of Char16.h" workaround used in bug 1135583 and in the mozalloc.h patch above, does not work on this latest <cstdlib> problem. I suspected this would be the case - the circular header dependencies I described in comment 7 exist regardless of how <xstddef> gets included. Removing Char16.h just pushes the failure to the next guy to include <xstddef>, and it is such a core library its probably a dependency for tons of header files.

I've attached traces of the header dependencies leading to xpcom/glue failure both with and without the "skip char16.h" patch.

Comment 11

[Bruce Williams]

Attachment: glue_header_dependencies_without_char16.txt

Comment 12

[Bruce Williams]

Attachment: glue_header_dependencies_with_char16.txt

Comment 13

[(Away)]

Aha - that helps explain the reason why this only just now started appearing. Previously, VS2013's <xstddef> included <stdlib.h>, but in VS2015 <xstddef> now includes <cstdlib>.

Comment 14

[Mike Hommey [:glandium]]

Comment on attachment 8612690 [details] [diff] [review]
skip-string-for-mozalloc.patch

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

I think you should look into comment 2, though.

Comment 15

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #14)
> Comment on attachment 8612690 [details] [diff] [review]
> skip-string-for-mozalloc.patch
> 
> Review of attachment 8612690 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think you should look into comment 2, though.

Err, I meant comment 1.

Comment 16

[Bruce Williams]

(In reply to Mike Hommey [:glandium] from comment #15)

> > I think you should look into comment 2, though.
> 
> Err, I meant comment 1.

Will do.

Comment 17

[Avi Halachmi (:avih)]

vs2015, now officially released, still gives the following error (x86 build):

> 12:42.80 msvc_raise_wrappers.cpp
> 12:42.80 [...]\obj-i686-pc-mingw32\dist\include\mozilla\msvc_raise_wrappers.h(12): fatal error C1189: #error:  "Unable to wrap _RAISE(); CRT _RAISE() already defined"

However, the patch from comment 1 no longer applies cleanly. FYI.

Comment 18

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #1)
> Created attachment 8587182 [details] [diff] [review]
> remove-raise-throw.patch
> 
> Need to investigate whether the CRT stuff from bug 560723 is still relevant
> in today's Gecko. If not, it seems like we can just do this, or similar.

With my brief investigation, it seems to me:

1. We probably no longer need to wrap _RAISE:

In VS2013 and VS2015, when _HAS_EXCEPTIONS=0, the macro _RAISE calls function _invoke_watson (see xstddef:71). I investigate this function a bit, and find some interesting code from msvcr120.dll: https://pastebin.mozilla.org/8840527 . To get this code, you need VS2013, and:
1) explicitly call _invoke_watson(0, 0, 0, 0, 0); in you code
2) run the code, and require downloading symbols from the symbols server when it breaks

In this code, we can see that one branch goes to __fastfail, which according to the official document [2], just invokes "int 0x29" to terminate the process. The comment there indicates that the other branch also goes to terminate the process.

It seems to me that we wrap this macro mainly because "we don't want to invoke atexit handlers, destructors, library unload handlers, and so on when our process might be in a compromised state" [3]. If that's the only reason, than the _RAISE in the header probably already meets our requirement. (I wonder the system would do something other than that, though)


2. We probably still need the wrappers for _X[exception] functions:

With _HAS_EXCEPTIONS=0, the _X[exception] functions in CRT calls _THROW_NCEE. In <xstddef>, this macros is defined as "#define _THROW_NCEE(x, y) x(y)._Raise()". It is not immediately what happens here, but it seems to me it does much more things than a fast fail. So I reasonably guess it doesn't meet our requirement of a fast fail.


3. We should be able to remove _Throw hack:

That was for VS7/8/9, and has been a dead code since VC10. Since we are requiring VC12 now, I don't think we need to keep those code.


[1] https://pastebin.mozilla.org/8840527
[2] https://msdn.microsoft.com/en-us/library/dn774154.aspx?
[3] http://hg.mozilla.org/mozilla-central/file/2a563492ca66/mfbt/Assertions.h#l179

Comment 19

[Bas Schouten (:bas.schouten)]

Can we land the part here that has r+?

Comment 20

[Bas Schouten (:bas.schouten)]

Hrm, I guess that part isn't really that valuable with the other cstdlib related problems still being there.

Comment 21

[Mike Hommey [:glandium]]

Turns out this bug is all intertwined with bug 1189967. I'm inclined to leave it to that bug to fix the VC15 build, and to repurpose this present bug to actually remove the _Throw stuff that was for VC < 10.

Comment 22

[Mike Hommey [:glandium]]

Attachment: Remove obsolete _Throw wrapping

Comment 23

[Nathan Froyd [:froydnj]]

Comment on attachment 8650388 [details] [diff] [review]
Remove obsolete _Throw wrapping

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

Judging from bug 557060 comment 23 on what this _Throw stuff is doing, and Xidorn's analysis in comment 18, I think it is correct that we don't need this anymore.

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/50bdc607693d

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/50bdc607693d