Closed Bug 1001332 Opened 6 years ago Closed 6 years ago

Add Windows version into /SUBSYSTEM

Categories

(Firefox Build System :: General, defect)

x86
Windows 8.1
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla34

People

(Reporter: m_kato, Assigned: dmajor)

References

Details

Attachments

(2 files, 4 obsolete files)

When building VS2012+, subsystem version becomes 6.0, not 5.01 by linker.

We should set subsystem version keeps 5.01 even if VS2012 or VS2013.
Also, _USING_V110_SDK71_ is for XP
Blocks: VC12
_USING_V110_SDK71_ may be unnecessary.  If necessary, I will file a new bug.
Summary: Add Windows version into /SUBSYSTEM and define _USING_V110_SDK71_ → Add Windows version into /SUBSYSTEM
Attached patch for ICU (obsolete) — Splinter Review
Assignee: nobody → m_kato
Attachment #8415736 - Attachment is obsolete: true
Attachment #8413582 - Attachment is obsolete: true
Attachment #8415757 - Flags: review?(mh+mozilla)
Attachment #8415758 - Flags: review?(mh+mozilla)
VS2012 and VS2013's default is subsystem 6.0.  To run on XP, subsystem version should be 5.01.
Comment on attachment 8415757 [details] [diff] [review]
Part 1. Set subsystem version to 5.01

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

r+ with the following fixed.

::: config/config.mk
@@ +655,3 @@
>  else
> +# For setting subsystem version
> +WIN32_EXE_LDFLAGS	+= $(WIN32_CONSOLE_EXE_LDFLAGS)

This branch was not there originally. So this means this is trying to replace the default. The default subsystem is windows according to http://msdn.microsoft.com/en-us/library/fcc1zstk%28v=vs.100%29.aspx, so that would be GUI, not CONSOLE.
Attachment #8415757 - Flags: review?(mh+mozilla) → review+
Attachment #8415758 - Flags: review?(mh+mozilla) → review+
According to <http://msdn.microsoft.com/en-us/library/fcc1zstk.aspx>, the lowest possible value is 5.02 for x64.
Can we get this ready for checkin please?
I'll help land this since it's blocking VS2013 rollout. Will address comment 8 and comment 9 and do a sanity check on the build.
Assignee: m_kato → dmajor
(In reply to Mike Hommey [:glandium] (out from Sep 6 to Sep 22) from comment #8)
> This branch was not there originally. So this means this is trying to
> replace the default. The default subsystem is windows according to
> http://msdn.microsoft.com/en-us/library/fcc1zstk%28v=vs.100%29.aspx, so that
> would be GUI, not CONSOLE.

My reading of the documentation is that the default depends on whether you export main or WinMain. The current patch matches the expectations of all our executables, and builds successfully.

If I try to invert the default, I get failures:
error LNK2019: unresolved external symbol _WinMain@16 referenced in function ___tmainCRTStartup
in host_object_int_extract.exe, host_jskwgen.exe, and host_ListCSSProperties.exe.
The current patches don't cover all of our binaries (e.g. freebl3 from NSS), but it seems the loader doesn't care about DLLs, only .exe files. So I propose that we drop the ICU patch and just go with this.

See comment 12 for why I kept the default as console in config.mk.
Attachment #8415757 - Attachment is obsolete: true
Attachment #8415758 - Attachment is obsolete: true
Attachment #8479607 - Flags: review?(mh+mozilla)
Attachment #8479607 - Flags: review?(mh+mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/437f9ad3fec9
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
These changes weren't enough. We're missing a subsystem on certutil.exe.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to David Major [:dmajor] from comment #16)
> These changes weren't enough. We're missing a subsystem on certutil.exe.

It's a) not built with our build system, but NSS's, b) not shipped (only used on automation). Do we care?
The mochitests do. https://tbpl.mozilla.org/?tree=Try&rev=637499d15eb8
This makes the Mochitests green on WinXP with VS2013: https://tbpl.mozilla.org/?tree=Try&rev=14cf7e66e2c1 (I pushed as 2013 mozconfig as an ancestor but TBPL doesn't show it)
Attachment #8482140 - Flags: review?(wtc)
Comment on attachment 8482140 [details] [diff] [review]
Subsystem for NSS utils

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

r=wtc.

Patch checked in: https://hg.mozilla.org/projects/nss/rev/7ee8791bd370
Attachment #8482140 - Flags: review?(wtc)
Attachment #8482140 - Flags: review+
Attachment #8482140 - Flags: checkin+
Comment on attachment 8482140 [details] [diff] [review]
Subsystem for NSS utils

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

Patch (as part of NSS_3_17_1_BETA2) pushed to mozilla-central:
https://hg.mozilla.org/mozilla-central/rev/e1c5c185b707
Thanks Wan-Teh! The XP tests are working with latest m-c.
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
I think NSS change is not enough, because the change doesn't affect freebl3.dll, nssdbm3.dll, softokn3.dll, which use WIN32.mk's OS_DLLFLAGS
For icuin52.dll, icuuc52.dll, icudt52.dll, the patches are not enough, too.
They use intl/icu/source/common/Makefile.in, intl/icu/source/i18n/Makefile.in 's LDFLAGS, and intl/icu/source/tools/pkgdata/pkgdata.cpp's LINK_FLAGS
It's not necessary to fix the .dll files. The loader only looks at the flags on .exe files. (If this is ever not true, we're screwed, because msvcr120.dll is subsystem 6.00)
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.