Always define _HAS_EXCEPTIONS=0 on Windows

Always define _HAS_EXCEPTIONS=0 on Windows

Previously we would not define it in component builds because it didn't work with old versions of MSVC. These days it should work.

The macro is extra important for Clang, which doesn't currently support exceptions on Windows.

BUG=82385
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272478

[hans, 2014-05-22 17:11:29]

Please take a look.

[Nico, 2014-05-22 17:50:14]

maruel, I think _HAS_EXCEPTIONS=0 isn't defined in component builds on windows
due to your advice in
https://codereview.chromium.org/2836013/diff/1/2#newcode696 . Do you remember
why you gave that advice?

[M-A Ruel, 2014-05-22 17:55:12]

On 2014/05/22 17:50:14, Nico wrote:
> maruel, I think _HAS_EXCEPTIONS=0 isn't defined in component builds on windows
> due to your advice in
> https://codereview.chromium.org/2836013/diff/1/2#newcode696 . Do you remember
> why you gave that advice?

It's because when msvcrt.dll or msvcrtd.dll (or whatever new versionned name it
has) is being used, exceptions must not be disabled, since it changes the import
signature and cause a runtime failure (IIRC).

On clang, this may or may not apply. If it works, fine with me. In particular,
it may now work with VS2013 dll while it didn't in previous versions, so what I
describe above may be stale.

[Reid Kleckner, 2014-05-22 18:42:39]

On 2014/05/22 17:55:12, M-A Ruel wrote:
> On 2014/05/22 17:50:14, Nico wrote:
> > maruel, I think _HAS_EXCEPTIONS=0 isn't defined in component builds on
windows
> > due to your advice in
> > https://codereview.chromium.org/2836013/diff/1/2#newcode696 . Do you
remember
> > why you gave that advice?
> 
> It's because when msvcrt.dll or msvcrtd.dll (or whatever new versionned name
it
> has) is being used, exceptions must not be disabled, since it changes the
import
> signature and cause a runtime failure (IIRC).
> 
> On clang, this may or may not apply. If it works, fine with me. In particular,
> it may now work with VS2013 dll while it didn't in previous versions, so what
I
> describe above may be stale.

Do you have any recollection of what part of the CRT interface tickles this? 
Hans is working on dllimport/dllexport at the moment, so we probably haven't hit
this yet.  It'd be good to start with a small program using /MDd
/D_HAS_EXCEPTIONS=0 and see if we can make that work.

[M-A Ruel, 2014-05-22 18:52:50]

On 2014/05/22 18:42:39, Reid Kleckner wrote:
> On 2014/05/22 17:55:12, M-A Ruel wrote:
> > On 2014/05/22 17:50:14, Nico wrote:
> > > maruel, I think _HAS_EXCEPTIONS=0 isn't defined in component builds on
> windows
> > > due to your advice in
> > > https://codereview.chromium.org/2836013/diff/1/2#newcode696 . Do you
> remember
> > > why you gave that advice?
> > 
> > It's because when msvcrt.dll or msvcrtd.dll (or whatever new versionned name
> it
> > has) is being used, exceptions must not be disabled, since it changes the
> import
> > signature and cause a runtime failure (IIRC).
> > 
> > On clang, this may or may not apply. If it works, fine with me. In
particular,
> > it may now work with VS2013 dll while it didn't in previous versions, so
what
> I
> > describe above may be stale.
> 
> Do you have any recollection of what part of the CRT interface tickles this? 
> Hans is working on dllimport/dllexport at the moment, so we probably haven't
hit
> this yet.  It'd be good to start with a small program using /MDd
> /D_HAS_EXCEPTIONS=0 and see if we can make that work.

IIRC, some stl symbols were defined differently in the header file depending on
the value of this macro. But it's been years and the toolset changed since then,
so it's worth having a fresh look at this.  Just understand that it is a
possibility that you have to revert this CL in the end but I don't think it's a
big deal as of now. As long as you are simply compiling and not yet linking,
lgtm.

[Nico, 2014-05-22 18:54:55]

Unless there's a good reason why this should be different for clang and msvs, I
think this should be changed either for both or none. (Maybe there is a good
reason, in that case it should be in the CL description.)

[chromium-reviews, 2014-05-22 19:09:36]

Maybe we should kick off some win+dbg+shared_library try jobs with a
_HAS_EXCEPTIONS=0 change for MSVC?


On Thu, May 22, 2014 at 11:54 AM, <thakis@chromium.org> wrote:

> Unless there's a good reason why this should be different for clang and
> msvs, I
> think this should be changed either for both or none. (Maybe there is a
> good
> reason, in that case it should be in the CL description.)
>
> https://codereview.chromium.org/287193012/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[hans, 2014-05-22 19:36:56]

> On Thu, May 22, 2014 at 11:54 AM, <mailto:thakis@chromium.org> wrote:
> 
> > Unless there's a good reason why this should be different for clang and
> > msvs, I
> > think this should be changed either for both or none. (Maybe there is a
> > good
> > reason, in that case it should be in the CL description.)

On 2014/05/22 19:09:36, chromium-reviews_chromium.org wrote:
> Maybe we should kick off some win+dbg+shared_library try jobs with a
> _HAS_EXCEPTIONS=0 change for MSVC?

I tried a vanilla msvc dbg shared_library build of base_unittests locally, and
that seemed to work, so fingers crossed. I'll run some tryjobs.

[hans, 2014-05-23 00:07:49]

The tryjobs look good, and I've updated the CL description.

OK to take this to the commit queue?

[Nico, 2014-05-23 00:27:27]

On 2014/05/23 00:07:49, hans wrote:
> The tryjobs look good, and I've updated the CL description.
> 
> OK to take this to the commit queue?

Lgtm

[Nico, 2014-05-23 00:28:32]

On 2014/05/23 00:27:27, Nico wrote:
> On 2014/05/23 00:07:49, hans wrote:
> > The tryjobs look good, and I've updated the CL description.
> > 
> > OK to take this to the commit queue?
> 
> Lgtm

(Are targets that remove this again keyed off component too?)

[hans, 2014-05-23 00:41:52]

On 2014/05/23 00:28:32, Nico wrote:
> On 2014/05/23 00:27:27, Nico wrote:
> > On 2014/05/23 00:07:49, hans wrote:
> > > The tryjobs look good, and I've updated the CL description.
> > > 
> > > OK to take this to the commit queue?
> > 
> > Lgtm
> 
> (Are targets that remove this again keyed off component too?)

I couldn't find any that remove this define.

There are a couple of other places that define it like this, keyed off
component, e.g. v8/build/standalone.gypi, native_client/build/common.gypi. I
guess those can be simplified too.

[hans, 2014-05-23 00:41:58]

The CQ bit was checked by hans@chromium.org

[commit-bot: I haz the power, 2014-05-23 00:42:53]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hans@chromium.org/287193012/20001

[commit-bot: I haz the power, 2014-05-23 10:54:59]

Change committed as 272478

[Nico, 2014-05-23 20:51:57]

https://codereview.chromium.org/287193012/diff/20001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/287193012/diff/20001/build/common.gypi#newcod...
build/common.gypi:4965: }],
Elsewhere, I noticed today that this block here also disables exceptions only in
static builds. This should very likely be in sync with the define.