Remove outdated _Throw() hack

RESOLVED FIXED in Firefox 43

Status

()

Core
XPCOM
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: briansmith, Assigned: glandium)

Tracking

({helpwanted})

Trunk
Future
x86
Windows 7
helpwanted
Points:
---

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

+++ 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.
Assignee: nobody → brian
Status: NEW → ASSIGNED
Target Milestone: --- → Future
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.
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.
Assignee: brian → nobody
Keywords: helpwanted

Comment 3

3 years ago
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.
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

3 years ago
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

3 years ago
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

3 years ago
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

3 years ago
Created attachment 8612690 [details] [diff] [review]
skip-string-for-mozalloc.patch
Attachment #8612690 - Flags: review?(dmajor)
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?
Attachment #8612690 - Flags: review?(dmajor) → review?(mh+mozilla)

Comment 10

3 years ago
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

3 years ago
Created attachment 8613082 [details]
glue_header_dependencies_without_char16.txt

Comment 12

3 years ago
Created attachment 8613093 [details]
glue_header_dependencies_with_char16.txt
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>.
(Assignee)

Comment 14

3 years ago
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.
Attachment #8612690 - Flags: review?(mh+mozilla) → review+
(Assignee)

Comment 15

3 years ago
(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

3 years ago
(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.
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.
(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
Can we land the part here that has r+?
Hrm, I guess that part isn't really that valuable with the other cstdlib related problems still being there.
(Assignee)

Comment 21

3 years ago
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.
(Assignee)

Updated

3 years ago
No longer blocks: 1119082
Summary: Sort out why the _Throw() hack doesn't work on VC14 (MSVS 2015), and find an alternative → Remove outdated _Throw() hack
(Assignee)

Comment 22

3 years ago
Created attachment 8650388 [details] [diff] [review]
Remove obsolete _Throw wrapping
Assignee: nobody → mh+mozilla
Attachment #8587182 - Attachment is obsolete: true
Attachment #8612690 - Attachment is obsolete: true
Attachment #8613082 - Attachment is obsolete: true
Attachment #8613093 - Attachment is obsolete: true
Attachment #8650388 - Flags: review?(nfroyd)
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.
Attachment #8650388 - Flags: review?(nfroyd) → review+
https://hg.mozilla.org/mozilla-central/rev/50bdc607693d
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.