Closed Bug 1033887 Opened 5 years ago Closed 5 years ago

Disable the usage of try/catch in TestSTLWrappers.cpp with clang-cl

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: ehsan, Assigned: ehsan)

References

Details

Attachments

(2 files)

No description provided.
Assignee: nobody → ehsan
Blocks: winclang
Attachment #8449917 - Flags: review?(nfroyd)
Isn't the try/catch the whole point of this test?
Comment on attachment 8449917 [details] [diff] [review]
Disable the usage of try/catch in TestSTLWrappers.cpp with clang-cl

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

How are we getting WRAP_STL_INCLUDES set in this case?  This code:

http://mxr.mozilla.org/mozilla-central/source/configure.in#443

shouldn't let us get to the point where we're setting WRAP_STL_INCLUDES.  Or is this another case where compiling with clang-cl bypasses the configury that tries to decide if we're compiling with clang?
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #2)
> Isn't the try/catch the whole point of this test?

We build with _HAS_EXCEPTIONS=0, so this test really has no point in the first place.  :-)

(In reply to Nathan Froyd (:froydnj) from comment #3)
> How are we getting WRAP_STL_INCLUDES set in this case?  This code:
> 
> http://mxr.mozilla.org/mozilla-central/source/configure.in#443
> 
> shouldn't let us get to the point where we're setting WRAP_STL_INCLUDES.  Or
> is this another case where compiling with clang-cl bypasses the configury
> that tries to decide if we're compiling with clang?

We get it from here: http://mxr.mozilla.org/mozilla-central/source/configure.in#610.

But I'm not sure what you mean really.  We wrap STL includes on Windows with MSVC, and clang-cl should be no different (and rightly so.)  The issue here is that clang-cl still doesn't know how to compile code which tries to catch exceptions, which is fine, since this code won't throw exceptions because we build with _HAS_EXCEPTIONS=0.  :-)
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #4)
> We get it from here:
> http://mxr.mozilla.org/mozilla-central/source/configure.in#610.
> 
> But I'm not sure what you mean really.  We wrap STL includes on Windows with
> MSVC, and clang-cl should be no different (and rightly so.)

Sure.  But that WRAP_STL_INCLUDES is guarded by a CLANG_CC check (and so is the |else| arm a little farther down).  Is that check meant to be testing for a mingw32-esque clang, or is that check just useless now that clang-cl is semi-functional?
Comment on attachment 8449917 [details] [diff] [review]
Disable the usage of try/catch in TestSTLWrappers.cpp with clang-cl

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

I assume if clang-cl ever grows the necessary support, this check will be removed.
Attachment #8449917 - Flags: review?(nfroyd) → review+
The point of this test and the STL wrappers is that the CRT is built with exceptions enabled, even when our code isn't. So in order to prevent C++ exceptions from leaking into code which is built without, we catch and abort.

If we are using a clang CRT with no exceptions, then we need neither the STL wrappers nor this test.

If we are using a clang CRT with exceptions, then we need the wrappers, but perhaps the test makes no sense and we should just not build it in this config.

Either way, I don't think the patch here makes sense.
Attachment #8449917 - Flags: review+ → review-
(In reply to comment #5)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #4)
> > We get it from here:
> > http://mxr.mozilla.org/mozilla-central/source/configure.in#610.
> > 
> > But I'm not sure what you mean really.  We wrap STL includes on Windows with
> > MSVC, and clang-cl should be no different (and rightly so.)
> 
> Sure.  But that WRAP_STL_INCLUDES is guarded by a CLANG_CC check (and so is the
> |else| arm a little farther down).  Is that check meant to be testing for a
> mingw32-esque clang, or is that check just useless now that clang-cl is
> semi-functional?

That was added by me a very long time ago before clang-cl IIRC, back when I was trying to get the gcc style driver to work on Windows.  That variable is not set in clang-cl, and I should actually remove this old cruft.
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #7)
> The point of this test and the STL wrappers is that the CRT is built with
> exceptions enabled, even when our code isn't. So in order to prevent C++
> exceptions from leaking into code which is built without, we catch and abort.
> 
> If we are using a clang CRT with no exceptions, then we need neither the STL
> wrappers nor this test.
> 
> If we are using a clang CRT with exceptions, then we need the wrappers, but
> perhaps the test makes no sense and we should just not build it in this
> config.
> 
> Either way, I don't think the patch here makes sense.

There is no clang CRT involved here, we use the usual MSVC CRT headers and libraries.  Specifically the same STL.  My point was that setting _HAS_EXCEPTIONS=0 is the well known (and actually undocumented) way to get the Microsoft STL to not throw exceptions (that is, by #ifdef-ing throw statements out).  So if I understand you correctly, this test is attempting to test whether that works?  If that's the case, I still think this is a silly test as Microsoft won't be able to remove support for _HAS_EXCEPTIONS without breaking everybody that depends on it.

(FWIW I'm fine with not taking this workaround and just falling back to cl.exe on this test, this is really not worth much debate! ;-)
Flags: needinfo?(benjamin)
I think we should just avoid building this test entirely for clang-cl.
Flags: needinfo?(benjamin)
Attachment #8450216 - Flags: review?(benjamin)
Attachment #8450216 - Flags: review?(benjamin) → review+
https://hg.mozilla.org/mozilla-central/rev/aa4c2a2605f8
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.