Pass the /safeseh option to ml.exe.

Add MASM settings support to msvs_settings.

The 'UseSafeExceptionHandlers' setting, if set to 'true', passes the
/safeseh option to ml.exe.

Without it, linking fails with error messages like this:

[182/259] LINK_EMBED(DLL) crnss.dll
FAILED: C:\python_27_amd64\files\python.exe gyp-win-tool link-with-manifests env
ironment.x86 True crnss.dll "C:\python_27_amd64\files\python.exe gyp-win-tool li
nk-wrapper environment.x86 False link.exe /nologo /IMPLIB:crnss.dll.lib /DLL /OU
T:crnss.dll @crnss.dll.rsp" 2 mt.exe rc.exe "obj\third_party\nss\nss.crnss.dll.i
ntermediate.manifest" obj\third_party\nss\nss.crnss.dll.generated.manifest
nss_static.lib(nss_static.intel-aes-x86-masm_asm.obj) : error LNK2026: module un
safe for SAFESEH image.
nss_static.lib(nss_static.intel-gcm-x86-masm_asm.obj) : error LNK2026: module un
safe for SAFESEH image.
crnss.dll : fatal error LNK1281: Unable to generate SAFESEH image.

Add the ImageHasSafeExceptionHandlers setting support for VCLinkerTool. If set
to 'true', it is mapped to the /SAFESEH linker option.

R=scottmg@chromium.org
BUG=none

Committed: https://code.google.com/p/gyp/source/detail?r=1894

[wtc, 2014-03-29 01:13:01]

Scott: please review.

I found the /safeseh option for ML at
http://msdn.microsoft.com/en-us/library/s0ksfwcf.aspx

This is the only solution I found. If you know of any
MASM directive I can add to the assembly code files to
fix this problem, I can also do that. Thanks.

[scottmg, 2014-03-31 16:25:18]

I don't think this should be hardcoded in the generator.

I checked VS2013 and the ml property in the vcxproj looks like

<MASM>
  <UseSafeExceptionHandlers>true</UseSafeExceptionHandlers>
</MASM>

so it should be conditionalized on that.

[wtc, 2014-04-09 02:14:07]

On 2014/03/31 16:25:18, scottmg wrote:
> I don't think this should be hardcoded in the generator.
> 
> I checked VS2013 and the ml property in the vcxproj looks like
> 
> <MASM>
>   <UseSafeExceptionHandlers>true</UseSafeExceptionHandlers>
> </MASM>
> 
> so it should be conditionalized on that.

Thanks for the info. How do I do this in third_party/nss/nss.gyp?
The CL I'm working on is https://codereview.chromium.org/214183004/

[scottmg, 2014-04-09 02:23:30]

On 2014/04/09 02:14:07, wtc wrote:
> On 2014/03/31 16:25:18, scottmg wrote:
> > I don't think this should be hardcoded in the generator.
> > 
> > I checked VS2013 and the ml property in the vcxproj looks like
> > 
> > <MASM>
> >   <UseSafeExceptionHandlers>true</UseSafeExceptionHandlers>
> > </MASM>
> > 
> > so it should be conditionalized on that.
> 
> Thanks for the info. How do I do this in third_party/nss/nss.gyp?
> The CL I'm working on is https://codereview.chromium.org/214183004/

I just meant in this CL, it should be conditional on that being set, rather than
always added to the command line.

In nss.gyp it'd look something like

'msvs_settings': {
  'MASM': {
    'UseSafeExceptionHandlers': 'true',
  },
}

It would be implemented similar to how msvs_emulation.py GetCflags uses
VCCLCompilerTool settings. We don't currently have any MASM settings though, so
it might be a bit of work to plumb it through.

[wtc, 2014-04-09 03:41:08]

Thanks a lot for the hint. Please review patch set 2.

https://codereview.chromium.org/217813007/diff/20001/pylib/gyp/msvs_emulation.py
File pylib/gyp/msvs_emulation.py (right):

https://codereview.chromium.org/217813007/diff/20001/pylib/gyp/msvs_emulation...
pylib/gyp/msvs_emulation.py:349: """Returns the flags that need to be added to
.asm compilations."""

".asm compilations" is not really accurate because an assembler is not a
compiler. I'd appreciate suggestions for better wording. Perhaps ".asm
assemblying"?

[scottmg, 2014-04-09 03:44:59]

Looks fine, please add a test similar to those in test\win.

https://codereview.chromium.org/217813007/diff/20001/pylib/gyp/msvs_emulation.py
File pylib/gyp/msvs_emulation.py (right):

https://codereview.chromium.org/217813007/diff/20001/pylib/gyp/msvs_emulation...
pylib/gyp/msvs_emulation.py:349: """Returns the flags that need to be added to
.asm compilations."""
On 2014/04/09 03:41:08, wtc wrote:
> 
> ".asm compilations" is not really accurate because an assembler is not a
> compiler. I'd appreciate suggestions for better wording. Perhaps ".asm
> assemblying"?

"...added to ml invocations."?

[wtc, 2014-04-10 02:41:59]

I added a test. Please review patch set 3.

https://codereview.chromium.org/217813007/diff/20001/pylib/gyp/msvs_emulation.py
File pylib/gyp/msvs_emulation.py (right):

https://codereview.chromium.org/217813007/diff/20001/pylib/gyp/msvs_emulation...
pylib/gyp/msvs_emulation.py:349: """Returns the flags that need to be added to
.asm compilations."""

On 2014/04/09 03:44:59, scottmg wrote:
>
> "...added to ml invocations."?

Done.

https://codereview.chromium.org/217813007/diff/40001/test/win/gyptest-ml-safe...
File test/win/gyptest-ml-safeseh.py (right):

https://codereview.chromium.org/217813007/diff/40001/test/win/gyptest-ml-safe...
test/win/gyptest-ml-safeseh.py:8: Make sure the /safeseh option can be passed to
ml.

Do I need to add this test (gyptest-ml-safeseh.py) to some file?

https://codereview.chromium.org/217813007/diff/40001/test/win/gyptest-ml-safe...
test/win/gyptest-ml-safeseh.py:15: if sys.platform == 'win32':

This test is only applicable to 32-bit x86 (ia32) builds. How do I disable it
for 64-bit x64 builds?

https://codereview.chromium.org/217813007/diff/40001/test/win/gyptest-ml-safe...
test/win/gyptest-ml-safeseh.py:16: test = TestGyp.TestGyp(formats=['ninja'])

Although I removed 'msvs' from this line, gyptest.py still tries to run with the
'msvs' format, and reports "No result from the following test" at the end. Do
you know how to fix this?

D:\src\chrome6\src\tools\gyp>python gyptest.py test\win\gyptest-ml-safeseh.py
PYTHONPATH=D:\src\chrome6\src\tools\gyp\test\lib
TESTGYP_FORMAT=msvs
C:\python_27_amd64\files\python.exe test\win\gyptest-ml-safeseh.py
Invalid test for 'msvs' format; skipping test.
NO RESULT
TESTGYP_FORMAT=ninja
C:\python_27_amd64\files\python.exe test\win\gyptest-ml-safeseh.py
PASSED

No result from the following test:
        test\win\gyptest-ml-safeseh.py

https://codereview.chromium.org/217813007/diff/40001/test/win/ml-safeseh/ml-s...
File test/win/ml-safeseh/ml-safeseh.gyp (right):

https://codereview.chromium.org/217813007/diff/40001/test/win/ml-safeseh/ml-s...
test/win/ml-safeseh/ml-safeseh.gyp:17: '/SAFESEH'

Is there a setting I can use instead of adding '/SAFESEH' explicitly?

[scottmg, 2014-04-10 03:31:51]

https://codereview.chromium.org/217813007/diff/40001/test/win/gyptest-ml-safe...
File test/win/gyptest-ml-safeseh.py (right):

https://codereview.chromium.org/217813007/diff/40001/test/win/gyptest-ml-safe...
test/win/gyptest-ml-safeseh.py:8: Make sure the /safeseh option can be passed to
ml.
On 2014/04/10 02:42:00, wtc wrote:
> 
> Do I need to add this test (gyptest-ml-safeseh.py) to some file?

No, they're globbed.

https://codereview.chromium.org/217813007/diff/40001/test/win/gyptest-ml-safe...
test/win/gyptest-ml-safeseh.py:16: test = TestGyp.TestGyp(formats=['ninja'])
On 2014/04/10 02:42:00, wtc wrote:
> 
> Although I removed 'msvs' from this line, gyptest.py still tries to run with
the
> 'msvs' format, and reports "No result from the following test" at the end. Do
> you know how to fix this?

That's as expected.

> 
> D:\src\chrome6\src\tools\gyp>python gyptest.py test\win\gyptest-ml-safeseh.py
> PYTHONPATH=D:\src\chrome6\src\tools\gyp\test\lib
> TESTGYP_FORMAT=msvs
> C:\python_27_amd64\files\python.exe test\win\gyptest-ml-safeseh.py
> Invalid test for 'msvs' format; skipping test.
> NO RESULT
> TESTGYP_FORMAT=ninja
> C:\python_27_amd64\files\python.exe test\win\gyptest-ml-safeseh.py
> PASSED
> 
> No result from the following test:
>         test\win\gyptest-ml-safeseh.py

It would be ideal to also add support to generator/msvs.py and make the test run
on both platforms.

(I'm not exactly sure how to do that, it's probably adding a few lines to
pylib/gyp/MSVSSettings.py but it doesn't have to be in this patch if it's not
obvious.)

https://codereview.chromium.org/217813007/diff/40001/test/win/ml-safeseh/ml-s...
File test/win/ml-safeseh/ml-safeseh.gyp (right):

https://codereview.chromium.org/217813007/diff/40001/test/win/ml-safeseh/ml-s...
test/win/ml-safeseh/ml-safeseh.gyp:17: '/SAFESEH'
On 2014/04/10 02:42:00, wtc wrote:
> 
> Is there a setting I can use instead of adding '/SAFESEH' explicitly?

It should be 'ImageHasSafeExceptionHandlers': 'true', but it looks like that's
only support in the VS generator, not the ninja one! (It would need to be added
to GetLdflags)

[wtc, 2014-04-10 18:21:48]

Please review patch set 4. Thanks.

https://codereview.chromium.org/217813007/diff/40001/test/win/gyptest-ml-safe...
File test/win/gyptest-ml-safeseh.py (right):

https://codereview.chromium.org/217813007/diff/40001/test/win/gyptest-ml-safe...
test/win/gyptest-ml-safeseh.py:16: test = TestGyp.TestGyp(formats=['ninja'])

On 2014/04/10 03:31:51, scottmg wrote:
>> It would be ideal to also add support to generator/msvs.py and make the test
run
> on both platforms.
> 
> (I'm not exactly sure how to do that, it's probably adding a few lines to
> pylib/gyp/MSVSSettings.py but it doesn't have to be in this patch if it's not
> obvious.)

I took a quick look at generator/msvs.py. It doesn't even handle source files
with the .asm extension:

def _MapFileToMsBuildSourceType(source, extension_to_rule_name):
  """...
  """
  _, ext = os.path.splitext(source)
  if ext in extension_to_rule_name:
    group = 'rule'
    element = extension_to_rule_name[ext]
  elif ext in ['.cc', '.cpp', '.c', '.cxx']:
    group = 'compile'
    element = 'ClCompile'
  elif ext in ['.h', '.hxx']:
    group = 'include'
    element = 'ClInclude'
  elif ext == '.rc':
    group = 'resource'
    element = 'ResourceCompile'
  elif ext == '.idl':
    group = 'midl'
    element = 'Midl'
  else:
    group = 'none'
    element = 'None'
  return (group, element)

So I'd prefer to *not* add .asm and MASM support to generator/msvs.py in this
CL.

Note: One reason I only worked on the ninja generator is that the Chromium build
instructions page
(http://www.chromium.org/developers/how-tos/build-instructions-windows) only
mentions the use of ninja at the top, and all the old contents that mention msvs
are now marked as deprecated or untested. Did you suggest that I add msvs
support because GYP is also used by non-Chromium projects?

https://codereview.chromium.org/217813007/diff/40001/test/win/ml-safeseh/ml-s...
File test/win/ml-safeseh/ml-safeseh.gyp (right):

https://codereview.chromium.org/217813007/diff/40001/test/win/ml-safeseh/ml-s...
test/win/ml-safeseh/ml-safeseh.gyp:17: '/SAFESEH'

On 2014/04/10 03:31:51, scottmg wrote:
>
> It should be 'ImageHasSafeExceptionHandlers': 'true', but it looks like that's
> only support in the VS generator, not the ninja one! (It would need to be
added
> to GetLdflags)

Done. Thanks for the hint.

[scottmg, 2014-04-10 19:23:21]

Thanks!

Is there something in dumpbin's output that indicates that an image is /SAFESEH?
If so, please add a test similar to
https://code.google.com/p/chromium/codesearch#chromium/src/tools/gyp/test/win...
for the linker option addition.

Otherwise, LGTM as is.

https://codereview.chromium.org/217813007/diff/40001/test/win/gyptest-ml-safe...
File test/win/gyptest-ml-safeseh.py (right):

https://codereview.chromium.org/217813007/diff/40001/test/win/gyptest-ml-safe...
test/win/gyptest-ml-safeseh.py:16: test = TestGyp.TestGyp(formats=['ninja'])
On 2014/04/10 18:21:48, wtc wrote:
> 
> On 2014/04/10 03:31:51, scottmg wrote:
> >> It would be ideal to also add support to generator/msvs.py and make the
test
> run
> > on both platforms.
> > 
> > (I'm not exactly sure how to do that, it's probably adding a few lines to
> > pylib/gyp/MSVSSettings.py but it doesn't have to be in this patch if it's
not
> > obvious.)
> 
> I took a quick look at generator/msvs.py. It doesn't even handle source files
> with the .asm extension:
> 
> def _MapFileToMsBuildSourceType(source, extension_to_rule_name):
>   """...
>   """
>   _, ext = os.path.splitext(source)
>   if ext in extension_to_rule_name:
>     group = 'rule'
>     element = extension_to_rule_name[ext]
>   elif ext in ['.cc', '.cpp', '.c', '.cxx']:
>     group = 'compile'
>     element = 'ClCompile'
>   elif ext in ['.h', '.hxx']:
>     group = 'include'
>     element = 'ClInclude'
>   elif ext == '.rc':
>     group = 'resource'
>     element = 'ResourceCompile'
>   elif ext == '.idl':
>     group = 'midl'
>     element = 'Midl'
>   else:
>     group = 'none'
>     element = 'None'
>   return (group, element)
> 
> So I'd prefer to *not* add .asm and MASM support to generator/msvs.py in this
> CL.
> 
> Note: One reason I only worked on the ninja generator is that the Chromium
build
> instructions page
> (http://www.chromium.org/developers/how-tos/build-instructions-windows) only
> mentions the use of ninja at the top, and all the old contents that mention
msvs
> are now marked as deprecated or untested. Did you suggest that I add msvs
> support because GYP is also used by non-Chromium projects?

Yes, Chromium only uses ninja now, but we have historically tried to keep
msvs.py and ninja.py feature support in gyp as close as possible.

I think .asm files are supported somehow in msvs, but I don't know how. I think
it's fine to let someone else fix this up if they run into it.

https://codereview.chromium.org/217813007/diff/50007/pylib/gyp/msvs_emulation.py
File pylib/gyp/msvs_emulation.py (right):

https://codereview.chromium.org/217813007/diff/50007/pylib/gyp/msvs_emulation...
pylib/gyp/msvs_emulation.py:540: ld('ImageHasSafeExceptionHandlers',
map={'true': '/SAFESEH'})
Thanks for doing this. We should probably add a test for this too, otherwise if
both weren't getting added, the masm test would still pass.

[wtc, 2014-04-10 22:21:43]

Please review patch set 6. I added a new gyptest-link-safeseh.py test.

https://codereview.chromium.org/217813007/diff/90001/test/win/gyptest-link-sa...
File test/win/gyptest-link-safeseh.py (right):

https://codereview.chromium.org/217813007/diff/90001/test/win/gyptest-link-sa...
test/win/gyptest-link-safeseh.py:16: test = TestGyp.TestGyp(formats=['ninja'])

If I add the 'msvs' format here, the test emits the warning message "Warning:
unrecognized setting VCLinkerTool/ImageHasSafeExceptionHandlers while converting
to MSBuild." and then fails.

https://codereview.chromium.org/217813007/diff/90001/test/win/gyptest-link-sa...
test/win/gyptest-link-safeseh.py:24: output = test.run_dumpbin('/LOADCONFIG',
full_path)

The dumpbin flag is /LOADCONFIG:

$ dumpbin -loadconfig foo.exe
Microsoft (R) COFF/PE Dumper Version 10.00.40219.01
Copyright (C) Microsoft Corporation.  All rights reserved.


Dump of file foo.exe

File Type: EXECUTABLE IMAGE

  Section contains the following load config:

            00000048 size
            ...
            004099F0 Safe Exception Handler Table
                   3 Safe Exception Handler Count

    Safe Exception Handler Table

          Address
          --------
          00402AA0
          00405580
          004075F0

https://codereview.chromium.org/217813007/diff/90001/test/win/gyptest-link-sa...
test/win/gyptest-link-safeseh.py:25: return '    Safe Exception Handler Table'
in output

Should I insist on the leading spaces? I am imitating gyptest-link-aslr.py, but
am worried that a future change in dumpbin output formatting may break this
test.

https://codereview.chromium.org/217813007/diff/90001/test/win/linker-flags/sa...
File test/win/linker-flags/safeseh.gyp (right):

https://codereview.chromium.org/217813007/diff/90001/test/win/linker-flags/sa...
test/win/linker-flags/safeseh.gyp:14: 'safeseh_zero.asm',

All object files produced by Visual C++ are apparently compatible with the safe
exception handling feature, so I have to use a .asm file in this test.

https://codereview.chromium.org/217813007/diff/90001/test/win/linker-flags/sa...
test/win/linker-flags/safeseh.gyp:37: 'MASM': {

Ideally I don't want to add the 'MASM' setting, and I want this target to fail
to build. But I don't know make the test to expect a build failure of this
target, so I am taking the easy way out. I've spent more time than I'd like on
this CL :-)

[scottmg, 2014-04-10 22:57:08]

lgtm, thanks for your patience.

https://codereview.chromium.org/217813007/diff/90001/test/win/gyptest-link-sa...
File test/win/gyptest-link-safeseh.py (right):

https://codereview.chromium.org/217813007/diff/90001/test/win/gyptest-link-sa...
test/win/gyptest-link-safeseh.py:16: test = TestGyp.TestGyp(formats=['ninja'])
On 2014/04/10 22:21:43, wtc wrote:
> 
> If I add the 'msvs' format here, the test emits the warning message "Warning:
> unrecognized setting VCLinkerTool/ImageHasSafeExceptionHandlers while
converting
> to MSBuild." and then fails.

OK, I've probably led you astray then, or something needs to be added to the vs
generator. I think it's fine as is for now.

[wtc, 2014-04-10 23:32:01]

The CQ bit was checked by wtc@chromium.org

[wtc, 2014-04-10 23:34:23]

Scott: are the gyp-win64 and gyp-win32 trybot failures caused by my CL? They
don't seem related to my CL.

[scottmg, 2014-04-10 23:36:42]

Committed patchset #6 manually as r1894 (presubmit successful).

[wtc, 2014-04-11 03:13:18]

https://codereview.chromium.org/217813007/diff/90001/pylib/gyp/generator/ninj...
File pylib/gyp/generator/ninja.py (right):

https://codereview.chromium.org/217813007/diff/90001/pylib/gyp/generator/ninj...
pylib/gyp/generator/ninja.py:1873: description='CXX $out',

Scott: when compiling a C++ file, Ninja displays the output filename, but when
assembling an assembly code file, Ninja displays the input filename. See line
1893. I was confused by this.

Why the difference?

[scottmg, 2014-04-11 03:31:43]

https://codereview.chromium.org/217813007/diff/90001/pylib/gyp/generator/ninj...
File pylib/gyp/generator/ninja.py (right):

https://codereview.chromium.org/217813007/diff/90001/pylib/gyp/generator/ninj...
pylib/gyp/generator/ninja.py:1873: description='CXX $out',
On 2014/04/11 03:13:19, wtc wrote:
> 
> Scott: when compiling a C++ file, Ninja displays the output filename, but when
> assembling an assembly code file, Ninja displays the input filename. See line
> 1893. I was confused by this.
> 
> Why the difference?

No reason, probably because Evan/Nico wrote the CC rules for Linux/Mac that
Windows was copied from, and I wrote those Windows-specific rules for IDL/RC. I
guess the input seemed more useful than the output at the time. It's just a
message for the user, so it could be "ASM $in -> $out" or whatever.