Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(692)

Issue 861273003: Reland "msvs/ninja win: Fix support for ImageHasSafeExceptionHandlers" (Closed)

Created:
5 years, 11 months ago by Shezan Baig (Bloomberg)
Modified:
5 years ago
Reviewers:
Nico, bradn, scottmg
CC:
gyp-developer_googlegroups.com
Target Ref:
refs/heads/master
Visibility:
Public.

Description

Reland "msvs/ninja win: Fix support for ImageHasSafeExceptionHandlers" This commit fixes the x64 issue and adds a test. The underlying issue was that the msvs default value for ImageHasSafeExceptionHandlers is None for x64 targets. In order to add the x64 test, we need to update the ninja generator to use ml64.exe instead of ml.exe when building x64 targets. BUG= R=scottmg@chromium.org Committed: https://code.google.com/p/gyp/source/detail?r=2031

Patch Set 1 #

Total comments: 1

Patch Set 2 : Rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+81 lines, -12 lines) Patch
M pylib/gyp/MSVSSettings.py View 1 4 chunks +17 lines, -0 lines 0 comments Download
M pylib/gyp/generator/msvs.py View 5 chunks +12 lines, -1 line 0 comments Download
M pylib/gyp/generator/ninja.py View 1 3 chunks +3 lines, -3 lines 0 comments Download
M pylib/gyp/msvs_emulation.py View 1 1 chunk +8 lines, -1 line 0 comments Download
M pylib/gyp/win_tool.py View 1 chunk +0 lines, -3 lines 0 comments Download
M test/win/gyptest-link-safeseh.py View 2 chunks +8 lines, -2 lines 0 comments Download
M test/win/linker-flags/safeseh.gyp View 2 chunks +33 lines, -1 line 0 comments Download
A + test/win/linker-flags/safeseh_zero64.asm View 1 chunk +0 lines, -1 line 0 comments Download

Messages

Total messages: 23 (3 generated)
Shezan Baig (Bloomberg)
* safeseh defaults to None for x64 targets, still defaults to 'true' for Win32 targets ...
5 years, 11 months ago (2015-01-21 22:37:32 UTC) #2
Shezan Baig (Bloomberg)
Does anyone know what's up with the trybots? :)
5 years, 11 months ago (2015-01-21 22:38:22 UTC) #3
scottmg
https://codereview.chromium.org/861273003/diff/1/pylib/gyp/generator/ninja.py File pylib/gyp/generator/ninja.py (left): https://codereview.chromium.org/861273003/diff/1/pylib/gyp/generator/ninja.py#oldcode992 pylib/gyp/generator/ninja.py:992: # Asm files only get auto assembled for x86 ...
5 years, 11 months ago (2015-01-21 22:42:43 UTC) #4
Shezan Baig (Bloomberg)
On 2015/01/21 22:42:43, scottmg wrote: > https://codereview.chromium.org/861273003/diff/1/pylib/gyp/generator/ninja.py > File pylib/gyp/generator/ninja.py (left): > > https://codereview.chromium.org/861273003/diff/1/pylib/gyp/generator/ninja.py#oldcode992 > ...
5 years, 11 months ago (2015-01-21 22:46:25 UTC) #5
scottmg
+bradn explicitly, as I think he wrote the ml handling (and might remember the x86 ...
5 years, 11 months ago (2015-01-21 22:46:38 UTC) #7
scottmg
On 2015/01/21 22:46:25, Shezan Baig (Bloomberg) wrote: > On 2015/01/21 22:42:43, scottmg wrote: > > ...
5 years, 11 months ago (2015-01-21 22:52:43 UTC) #8
Shezan Baig (Bloomberg)
test/assembly succeeds locally for me. I'll land maybe tomorrow/Friday, hopefully the trybots will be back ...
5 years, 11 months ago (2015-01-21 23:16:07 UTC) #9
Shezan Baig (Bloomberg)
Committed patchset #2 (id:20001) manually as 2031 (presubmit successful).
5 years, 10 months ago (2015-02-02 23:31:50 UTC) #10
Shezan Baig (Bloomberg)
An interesting fallout from this is that Debug builds now produce the following linker warning: ...
5 years, 10 months ago (2015-02-03 15:18:09 UTC) #11
scottmg
On 2015/02/03 15:18:09, Shezan Baig (Bloomberg) wrote: > An interesting fallout from this is that ...
5 years, 10 months ago (2015-02-03 17:59:55 UTC) #12
Nico
A new test I'm adding is failing with [4/4] LINK_EMBED(DLL) mysolib.dll FAILED: C:\python_27_amd64\files\python.exe gyp-win-tool link-with-manifests ...
5 years ago (2015-12-14 15:02:43 UTC) #14
Shezan Baig (Bloomberg)
On 2015/12/14 15:02:43, Nico wrote: > A new test I'm adding is failing with > ...
5 years ago (2015-12-14 15:15:28 UTC) #15
Nico
I don't explicitly specify an arch, it's msvs that decides to default to x64, see ...
5 years ago (2015-12-14 15:22:52 UTC) #16
Nico
Since MSVC seems to default to x64, maybe GetArch() should return x64 instead of x86 ...
5 years ago (2015-12-14 15:24:43 UTC) #17
Nico
(…but why isn't this a problem for all the other tests using shared_libraries?)
5 years ago (2015-12-14 15:24:59 UTC) #18
Shezan Baig (Bloomberg)
On 2015/12/14 15:24:59, Nico wrote: > (…but why isn't this a problem for all the ...
5 years ago (2015-12-14 15:33:19 UTC) #19
Nico
On 2015/12/14 15:33:19, Shezan Baig (Bloomberg) wrote: > On 2015/12/14 15:24:59, Nico wrote: > > ...
5 years ago (2015-12-14 15:51:33 UTC) #20
Shezan Baig (Bloomberg)
Ok, I can reproduce this locally too. If you add a source file to the ...
5 years ago (2015-12-14 16:44:20 UTC) #21
Shezan Baig (Bloomberg)
On 2015/12/14 16:44:20, Shezan Baig (Bloomberg) wrote: > Ok, I can reproduce this locally too. ...
5 years ago (2015-12-14 16:45:09 UTC) #22
Shezan Baig (Bloomberg)
5 years ago (2015-12-14 16:48:17 UTC) #23
Message was sent while issue was closed.
On 2015/12/14 16:45:09, Shezan Baig (Bloomberg) wrote:
> On 2015/12/14 16:44:20, Shezan Baig (Bloomberg) wrote:
> > Ok, I can reproduce this locally too.
> > 
> > If you add a source file to the shared_library, it links fine (but obviously
> > defeats the purpose of the test).
> > 
> > The MSDN page for /MACHINE says: "Usually, you don't have to specify the
> > /MACHINE option. LINK infers the machine type from the .obj files."
> > 
> > So when there is a source file, then the fact that it was compiled with
> > environment.x86 means that link.exe will default to x86.
> > 
> > But in the absence of .obj files, it maybe defaults to the host arch?  (this
> > doesn't seem to be documented).
> > 
> > Not sure what would be the cleanest way to fix this.  I think the solution
> that
> > would work would be: GetArch should return x86 by default, unless you are
> > building a linkable target with no obj files, in that case, return the host
> arch
> > by default.  Seems messy :(
> 
> Another solution would be to always specify /MACHINE

Or make it always specify /MACHINE if no obj files are present

Powered by Google App Engine
This is Rietveld 408576698