Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
404853002:
Disable exceptions on Windows also in the shared_library build (Closed)
|
Created:
6 years, 5 months ago by hans Modified:
6 years, 5 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionDisable exceptions on Windows also in the shared_library build
They were previously only disabled in static_library builds. This is believed
to have been due to problems with old MSVC versions, but it should work now.
This change is relevant for Clang, which currently doesn't support exceptions
on Windows.
Last time I attempted to do this, it broke some tests due to a debug assertion.
Gtest had previously been catching the exception, but with exceptions disabled
it crashed. Those tests were actually broken in static_library Debug builds
too, but we don't seem to have bots for that. After Blink r176189, we no longer
hit that debug assertion, so the tests now pass.
BUG=82385
TEST=blink_platform_unittests --gtest_filter=DateTimeFormatTest.CommonPattern,
content_browsertests --gtest_filter=RenderViewImplTest.SetEditableSelectionAndComposition
webkit_unit_tests
R=thakis@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284519
Patch Set 1 #
Total comments: 4
Patch Set 2 : Add comments as suggested by thakis #Messages
Total messages: 21 (0 generated)
Note to self: Previous attempt was here: https://codereview.chromium.org/287193012 It got reverted due to errors reported here: https://code.google.com/p/chromium/issues/detail?id=377008
Tryjobs looking good so far. Please take a look.
On 2014/07/18 21:10:38, Nico (away) wrote: > http://src.chromium.org/viewvc/blink/trunk/.gitignore?revision=176189 <- this > one? No, https://src.chromium.org/viewvc/blink?revision=176189&view=revision
I'm not good with computers :-) lgtm https://codereview.chromium.org/404853002/diff/1/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/404853002/diff/1/build/common.gypi#newcode5143 build/common.gypi:5143: '_HAS_EXCEPTIONS=0', Maybe add a note here that this must match the Exceptions setting below. https://codereview.chromium.org/404853002/diff/1/build/common.gypi#newcode5293 build/common.gypi:5293: 'ExceptionHandling': '0', ...and add a note here that this must match _HAS_EXCEPTIONS above.
Oh! And can you try patching in https://codereview.chromium.org/404803005/ in third_party/pdfium with this enabled and verify that it still adds back /EHsc and removes /D_HAS_EXCEPTIONS=0 from out\Release\obj\third_party\pdfium.ninja ? On Fri, Jul 18, 2014 at 2:17 PM, <thakis@chromium.org> wrote: > I'm not good with computers :-) > > lgtm > > > https://codereview.chromium.org/404853002/diff/1/build/common.gypi > File build/common.gypi (right): > > https://codereview.chromium.org/404853002/diff/1/build/ > common.gypi#newcode5143 > build/common.gypi:5143: '_HAS_EXCEPTIONS=0', > Maybe add a note here that this must match the Exceptions setting below. > > https://codereview.chromium.org/404853002/diff/1/build/ > common.gypi#newcode5293 > build/common.gypi:5293: 'ExceptionHandling': '0', > ...and add a note here that this must match _HAS_EXCEPTIONS above. > > https://codereview.chromium.org/404853002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I tested the pdfium thing, it works with and without this change. On Jul 18, 2014 2:20 PM, "Nico Weber" <thakis@chromium.org> wrote: > Oh! And can you try patching in https://codereview.chromium.org/404803005/ > in third_party/pdfium with this enabled and verify that it still adds back > /EHsc and removes /D_HAS_EXCEPTIONS=0 from > out\Release\obj\third_party\pdfium.ninja ? > > > On Fri, Jul 18, 2014 at 2:17 PM, <thakis@chromium.org> wrote: > >> I'm not good with computers :-) >> >> lgtm >> >> >> https://codereview.chromium.org/404853002/diff/1/build/common.gypi >> File build/common.gypi (right): >> >> https://codereview.chromium.org/404853002/diff/1/build/ >> common.gypi#newcode5143 >> build/common.gypi:5143: '_HAS_EXCEPTIONS=0', >> Maybe add a note here that this must match the Exceptions setting below. >> >> https://codereview.chromium.org/404853002/diff/1/build/ >> common.gypi#newcode5293 >> build/common.gypi:5293: 'ExceptionHandling': '0', >> ...and add a note here that this must match _HAS_EXCEPTIONS above. >> >> https://codereview.chromium.org/404853002/ >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/07/18 21:20:55, Nico (away) wrote: > Oh! And can you try patching in https://codereview.chromium.org/404803005/ > in third_party/pdfium with this enabled and verify that it still adds back > /EHsc and removes /D_HAS_EXCEPTIONS=0 from > out\Release\obj\third_party\pdfium.ninja ? Verified!
https://codereview.chromium.org/404853002/diff/1/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/404853002/diff/1/build/common.gypi#newcode5143 build/common.gypi:5143: '_HAS_EXCEPTIONS=0', On 2014/07/18 21:17:17, Nico (away) wrote: > Maybe add a note here that this must match the Exceptions setting below. Done. https://codereview.chromium.org/404853002/diff/1/build/common.gypi#newcode5293 build/common.gypi:5293: 'ExceptionHandling': '0', On 2014/07/18 21:17:17, Nico (away) wrote: > ...and add a note here that this must match _HAS_EXCEPTIONS above. Done.
The CQ bit was checked by hans@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hans@chromium.org/404853002/20001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
Message was sent while issue was closed.
Committed patchset #2 manually as r284519 (presubmit successful).
Message was sent while issue was closed.
Could this be the culprit of Win7 x64 chrome_elf_unittests failure? http://build.chromium.org/p/chromium.win/builders/Win%207%20Tests%20x64%20%28...
Message was sent while issue was closed.
On 2014/07/22 04:05:12, Kunihiko Sakamoto wrote: > Could this be the culprit of Win7 x64 chrome_elf_unittests failure? > http://build.chromium.org/p/chromium.win/builders/Win%207%20Tests%20x64%20%28... Feel free to revert and see if it helps. If it doesn't, it's easy to reland :-)
Message was sent while issue was closed.
On 2014/07/22 04:05:12, Kunihiko Sakamoto wrote: > Could this be the culprit of Win7 x64 chrome_elf_unittests failure? > http://build.chromium.org/p/chromium.win/builders/Win%207%20Tests%20x64%20%28... I'm not familiar with this test. I can look into it tomorrow morning, but if this is breaking something right now, please revert my patch.
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/410613002/ by ksakamoto@chromium.org. The reason for reverting is: Speculative revert to try to fix chrome_elf_unittests on Win7 x64. http://build.chromium.org/p/chromium.win/builders/Win%207%20Tests%20x64%20%28... AddDllsFromRegistryToBlacklist LoadBlacklistedLibrary I'll reland if that's not it. .
Message was sent while issue was closed.
After reverting this CL, the test starts to pass again. http://build.chromium.org/p/chromium.win/builders/Win%207%20Tests%20x64%20(2)
Message was sent while issue was closed.
On 2014/07/22 06:39:41, Kunihiko Sakamoto wrote: > After reverting this CL, the test starts to pass again. > > http://build.chromium.org/p/chromium.win/builders/Win%207%20Tests%20x64%20(2) Great! Sorry about the breakage, and thanks for reverting. |
