Sandbox MinGW Compilation errors: __try / __except

RESOLVED FIXED in Firefox 60

Status

()

enhancement
P1
normal
RESOLVED FIXED
2 years ago
10 months ago

People

(Reporter: tjr, Assigned: tjr)

Tracking

unspecified
mozilla60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

(Whiteboard: [tor][mingw-upstream-pending][sb+])

Attachments

(2 attachments)

MinGW doesn't support __try / __except.

Tor has successfully shipped Tor Browser replacing __try with if(true) and commenting out all __except blocks.

Can I take this approach? Would you accept patches to this effect bob? (Step after that will be trying to get it into chromium...)
Flags: needinfo?(bobowencode)
I thought the patches I saw were using __try1 and removing blocks that were needed only for debug or ignored exceptions that possibly shouldn't be.
(I'm guessing this is something they might not take upstream, but we could possibly do it with a force included header.)

Did that approach not work out?
Flags: needinfo?(bobowencode) → needinfo?(tom)
(In reply to Bob Owen (:bobowen) from comment #1)
> I thought the patches I saw were using __try1 and removing blocks that were
> needed only for debug or ignored exceptions that possibly shouldn't be.
> Did that approach not work out?

That was a guesswork approach. It might work, but we haven't tested it. What I'm doing is trying to port the patches Tor Browser has tested (and is running with) into -central keeping the other builds working. (Their patch is https://gitweb.torproject.org/tor-browser.git/patch/?id=258b15f9 FWIW)  They didn't use the __try1 approach.  

Right now, our MinGW build of -central isn't actually running, we're working on tracking that down Bug 1411401

> (I'm guessing this is something they might not take upstream, but we could
> possibly do it with a force included header.)

Yea, this part will probably involve some patches we have to carry, but I'll try to minimize it.
Flags: needinfo?(tom)
Flags: needinfo?(bobowencode)
OK, I see.

So if we add a force included file like [1], but just for MinGW, with those two #defines I guess that should work.
Probably ought to get a build peer review on that as well.

[1] https://searchfox.org/mozilla-central/source/mozilla-config.h.in#59
Flags: needinfo?(bobowencode)
Priority: -- → P1
Whiteboard: [tor][mingw-upstream-pending] → [tor][mingw-upstream-pending][sb+]
Okay bob, I reviewed the cases more carefully and I think this is ready for review now.
Comment on attachment 8944751 [details]
Bug 1431803 Disable a specific __try block on MinGW

https://reviewboard.mozilla.org/r/214910/#review221344

Patch file as well please.
Attachment #8944751 - Flags: review?(bobowencode) → review+
Comment on attachment 8944748 [details]
Bug 1431803 Turn __try into if(true) and __except into else in the chromium sandbox code

https://reviewboard.mozilla.org/r/214908/#review221342

We should probably have a build peer review as well because of mozilla-config.h.in changes.
Maybe glandium.

::: security/sandbox/chromium-shim/base/win/mingw.h:1
(Diff revision 4)
> +#ifdef __MINGW32__

emacs and vim lines, license header and #if header guard please.

Also, a comment explaining how this is included and how it can potentially be used for other MinGW build issues.
Attachment #8944748 - Flags: review?(bobowencode)
Depends on: 1431797
Comment on attachment 8944748 [details]
Bug 1431803 Turn __try into if(true) and __except into else in the chromium sandbox code

https://reviewboard.mozilla.org/r/214908/#review221384

Thanks.
Attachment #8944748 - Flags: review?(bobowencode) → review+
Attachment #8944748 - Flags: review?(mh+mozilla)
Comment on attachment 8944748 [details]
Bug 1431803 Turn __try into if(true) and __except into else in the chromium sandbox code

https://reviewboard.mozilla.org/r/214908/#review222260

A cursory grep __try says there are some outside security/sandbox. Are all of those in msvc-only sections?

::: security/sandbox/chromium-shim/base/win/mingw.h:26
(Diff revision 5)
> +#define __try if(true)
> +#define __except(x) else

Why not simply set that in mozilla-include.h.in?
Attachment #8944748 - Flags: review?(mh+mozilla)
(In reply to Mike Hommey [:glandium] from comment #16)
> Comment on attachment 8944748 [details]
> Bug 1431803 Force-include a header to turn __try into if(true) and __except
> into else
> 
> https://reviewboard.mozilla.org/r/214908/#review222260
> 
> A cursory grep __try says there are some outside security/sandbox. Are all
> of those in msvc-only sections?

Yes (otherwise the MinGW build wouldn't build.)

> ::: security/sandbox/chromium-shim/base/win/mingw.h:26
> (Diff revision 5)
> > +#define __try if(true)
> > +#define __except(x) else
> 
> Why not simply set that in mozilla-include.h.in?

Moved!
Flags: needinfo?(mh+mozilla)
Comment on attachment 8944748 [details]
Bug 1431803 Turn __try into if(true) and __except into else in the chromium sandbox code

https://reviewboard.mozilla.org/r/214908/#review222628

::: commit-message-4c8d4:1
(Diff revision 6)
> +Bug 1431803 Force-include a header to turn __try into if(true) and __except into else r?bobowen

You should adjust the commit message.
Attachment #8944748 - Flags: review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/c0de1027fbda
Turn __try into if(true) and __except into else in the chromium sandbox code r=bobowen,glandium
https://hg.mozilla.org/integration/autoland/rev/5506a531ce36
Disable a specific __try block on MinGW r=bobowen
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c0de1027fbda
https://hg.mozilla.org/mozilla-central/rev/5506a531ce36
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Flags: needinfo?(mh+mozilla)
Blocks: 1498676
Blocks: 1498693
You need to log in before you can comment on or make changes to this bug.