Closed Bug 1485224 Opened Last year Closed Last year

Update Visual Studio 2017 on builders to Update 8

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(firefox64 fixed)

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: emk, Assigned: emk)

References

Details

Attachments

(3 files, 6 obsolete files)

Clang-cl still depends on headers, libraries, and some tools from Visual Studio.

Can anyone upload VS2017 Update 8 (and Windows SDK 17134) to tooltool? (I have no privilege.)
I think RyanVM has generated the zips in the past?  Or if RyanVM can provide directions, you can generate them, Firefox Send them to me, and I can upload them.  We'll figure something out!
Flags: needinfo?(ryanvm)
There's an in-tree script to create them.
See build/windows_toolchain.py. It needs some changes for versions.
(In reply to Nathan Froyd [:froydnj] from comment #1)
> I think RyanVM has generated the zips in the past?  Or if RyanVM can provide
> directions, you can generate them, Firefox Send them to me, and I can upload
> them.  We'll figure something out!

Sent a private mail.
Uploaded the archive Kimura-san sent:

{
"size": 349578286,
"visibility": "internal",
"digest": "1c7bba22a8def72dadf7caff418373b7cc7dca45a7d45cdc0041d2dfea7e590bd87163308ffe0d9c42af78783cf46b95fe8b31f692ad51790fe03bc37a8528d7",
"algorithm": "sha512",
"filename": "vs2017_15.8.0.zip"
}
Flags: needinfo?(ryanvm)
Mmm, apparently this archive contains Japanese localized MSVC toolchains...
Uploaded a new one:
[
  {
    "size": 349594612,
    "visibility": "internal",
    "digest": "c28588327efd079c547e844c907d4e2fdd05877550dfcd576e55a504de046978d0263ddcb03b6add514c24c9f09a433c3509164f019a9fddebddcc047922a18c",
    "algorithm": "sha512",
    "filename": "vs2017_15.8.1.zip"
  }
]
triaging, assigning to emk since attached patches
Assignee: nobody → VYV03354
MSVC PGO builds fail with LNK1107 :(
Unassigning myself because I could not resolve or workaround the issue.
Assignee: VYV03354 → nobody
Maybe I found the cause. If we are generating a profile and -OUT: specifies a filename that contains a directory, link.exe will fail.

OK: link.exe -NOLOGO -OUT:plugin-hang-ui.exe ...
Error: link.exe -NOLOGO -OUT:../../../../dist/bin/plugin-hang-ui.exe ...

Obviously this is a bug of MSVC 15.8, but we can work around.
(In reply to Masatoshi Kimura [:emk] from comment #11)
> Maybe I found the cause. If we are generating a profile and -OUT: specifies
> a filename that contains a directory, link.exe will fail.
> 
> OK: link.exe -NOLOGO -OUT:plugin-hang-ui.exe ...
> Error: link.exe -NOLOGO -OUT:../../../../dist/bin/plugin-hang-ui.exe ...
> 
> Obviously this is a bug of MSVC 15.8, but we can work around.

That's...pretty bad.  Did this break in 15.8?  Did you report a bug?
(In reply to Nathan Froyd [:froydnj] from comment #12)
> Did this break in 15.8?

I don't know about 15.7, but it worked in 15.6. Otherwise the current MSVC builders would have already broken.

> Did you report a bug?

Not yet.
MSVC 15.8 linker busted if it is generating a profile and /OUT: specifies a filename that contains a directory path (even if it is ".\")

To workaround the problem, generate the output file to current directory and move it to the proper target directory.
Attachment #9004900 - Flags: review?(core-build-config-reviews)
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Attachment #9004901 - Flags: review?(core-build-config-reviews)
Attachment #9003103 - Attachment is obsolete: true
Comment on attachment 9004900 [details] [diff] [review]
Workaround a linker bug of MSVC 2017 Update 8

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

::: config/rules.mk
@@ +558,5 @@
>  	@$(RM) $@.manifest
>  ifeq (_WINNT,$(GNU_CC)_$(OS_ARCH))
> +ifeq ($(CC_TYPE)_$(MOZ_PROFILE_GENERATE),msvc_1)
> +# Workaround a bug of MSVC 2017 Update 8 (see bug 1485224)
> +	$(LINKER) -NOLOGO -OUT:$(@F) -PDB:$(LINK_PDBFILE) -IMPLIB:$(basename $(@F)).lib $(WIN32_EXE_LDFLAGS) $(LDFLAGS) $(MOZ_PROGRAM_LDFLAGS) $($(notdir $@)_$(OBJS_VAR_SUFFIX)) $(RESFILE) $(STATIC_LIBS) $(SHARED_LIBS) $(OS_LIBS)

did you try an absolute path instead of a relative path?
(In reply to Mike Hommey [:glandium] from comment #16)
> did you try an absolute path instead of a relative path?

I already tried and failed, unfortunately.
Attachment #9004900 - Flags: review?(core-build-config-reviews) → review?(mh+mozilla)
Attachment #9004901 - Flags: review?(core-build-config-reviews) → review?(mh+mozilla)
(In reply to Masatoshi Kimura [:emk] from comment #17)
> (In reply to Mike Hommey [:glandium] from comment #16)
> > did you try an absolute path instead of a relative path?
> 
> I already tried and failed, unfortunately.

Did you try -PGD with a full path, in case the problem is when the linker tries to derive the path on its own from -OUT?
Comment on attachment 9004901 [details] [diff] [review]
Update Windows builders to VS 2017 Update 8 and Windows SDK 17134

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

I'd rather leave non-Firefox builds out. For them, eventually, we should pull out the SDK, and switch them to clang-cl.
Attachment #9004901 - Flags: review?(mh+mozilla)
Comment on attachment 9004900 [details] [diff] [review]
Workaround a linker bug of MSVC 2017 Update 8

(In reply to Mike Hommey [:glandium] from comment #18)
> (In reply to Masatoshi Kimura [:emk] from comment #17)
> > (In reply to Mike Hommey [:glandium] from comment #16)
> > > did you try an absolute path instead of a relative path?
> > 
> > I already tried and failed, unfortunately.
> 
> Did you try -PGD with a full path, in case the problem is when the linker
> tries to derive the path on its own from -OUT?

I tried and it did not work either. But maybe I found another work around without using mv. Stay tuned.
Attachment #9004900 - Flags: review?(mh+mozilla)
MSVC 15.8 linker dislikes forwar slashes in the /OUT: parameter when it is generating a profile

To workaround the problem, generate the output file to current directory and move it to the proper target directory.
Attachment #9007173 - Flags: review?(mh+mozilla)
Attachment #9004900 - Attachment is obsolete: true
Attachment #9004901 - Attachment is obsolete: true
(In reply to Masatoshi Kimura [:emk] from comment #22)
> Created attachment 9007173 [details] [diff] [review]
> Workaround a linker bug of MSVC 2017 Update 8
> 
> MSVC 15.8 linker dislikes forwar slashes in the /OUT: parameter when it is
> generating a profile
> 
> To workaround the problem, generate the output file to current directory and
> move it to the proper target directory.

I forgot to remove the second sentence. Please disregard it.
Comment on attachment 9007171 [details] [diff] [review]
Update Windows builders to VS 2017 Update 8 and Windows SDK 17134

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

r+ assuming the following works:

::: build/moz.configure/toolchain.configure
@@ +518,5 @@
>          elif info.type == 'clang-cl' and info.language_version != cxx14_version:
>              append_flag('-std=c++14')
>  
> +    # We force clang-cl to emulate Visual C++ 2017 version 15.8.1
> +    msvc_version = '19.15.26726'

Can you do a try build with just this change? (as in, does it break building with clang-cl with the SDK from msvc 15.6)
Attachment #9007171 - Flags: review?(mh+mozilla) → review+
Comment on attachment 9007173 [details] [diff] [review]
Workaround a linker bug of MSVC 2017 Update 8

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

::: config/rules.mk
@@ +558,5 @@
>  	@$(RM) $@.manifest
>  ifeq (_WINNT,$(GNU_CC)_$(OS_ARCH))
> +ifeq ($(CC_TYPE)_$(MOZ_PROFILE_GENERATE),msvc_1)
> +# Workaround a bug of MSVC 2017 Update 8 (see bug 1485224)
> +	$(LINKER) -NOLOGO -OUT:$(subst /,\,$@) -PDB:$(LINK_PDBFILE) -IMPLIB:$(basename $(@F)).lib $(WIN32_EXE_LDFLAGS) $(LDFLAGS) $(MOZ_PROGRAM_LDFLAGS) $($(notdir $@)_$(OBJS_VAR_SUFFIX)) $(RESFILE) $(STATIC_LIBS) $(SHARED_LIBS) $(OS_LIBS)

It would be better to put that in a variable like:

if (...)
LINKER_OUT=$(subst /,\,$1)
else
LINKER_OUT=$1
endif

and use -OUT:$(call LINKER_OUT,$@)

As for the condition, it should also include somewhere that the _host_ system is windows.
Attachment #9007173 - Flags: review?(mh+mozilla)
(In reply to Mike Hommey [:glandium] from comment #25)
> ::: build/moz.configure/toolchain.configure
> @@ +518,5 @@
> >          elif info.type == 'clang-cl' and info.language_version != cxx14_version:
> >              append_flag('-std=c++14')
> >  
> > +    # We force clang-cl to emulate Visual C++ 2017 version 15.8.1
> > +    msvc_version = '19.15.26726'
> 
> Can you do a try build with just this change? (as in, does it break building
> with clang-cl with the SDK from msvc 15.6)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=585a10dd24bc523c98c846b5b15d5c07886399d3
* Added LINKER_OUT as suggested.
* Added HOST_OS_ARCH to the condition.
Attachment #9008222 - Flags: review?(mh+mozilla)
Attachment #9007173 - Attachment is obsolete: true
Attachment #9008222 - Flags: review?(mh+mozilla) → review+
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7a6e536532e
Workaround a linker bug of MSVC 2017 Update 8. r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe2c607f4af7
Update Windows builders to VS 2017 Update 8 and Windows SDK 17134. r=glandium
Blocks: 1490654
Looks like crashes, possibly due to miscompilation (?).
But the crashing jobs are running on clang-cl builds. Really weird...
Assignee: VYV03354 → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(VYV03354)
The SDK update would have affected clang-cl jobs as well. FWIW, the failures were happening after a bunch of "ASSERTION: Creating widget for MenuPopupFrame with children: '!mGeneratedChildren && !PrincipalChildList().FirstChild()', file z:/build/build/src/layout/xul/nsMenuPopupFrame.cpp, line 259" assertions while running browser/components/extensions/test/browser/test-oop-extensions/browser_ext_browserAction_popup.js.
Same assertions fire on a passing run, though. Also, I triggered more mochitest-bc runs on that push and was able to hit the same failures on browser/components/sessionstore/test/browser_restore_cookies_noOriginAttributes.js.
https://taskcluster-artifacts.net/HaObPD_LRjqlpOrJDi5N0Q/0/public/logs/live_backing.log

Same as before, test asserts and things go sideways from there?
Also, the failures don't occur on Win10. Also, I triggered MSVC builds+tests on comment 29 and can confirm that the same failures affect those builds as well.
Error line from clang-cl builds:
> Exception: Error getting fileid for Z:\task_1536750533ad] WARNING: NS_ENSURE_TRUE(currentIn\build\application\firefox\xul.dll:

MSVC builds:
> Exception: Error getting fileid for Z:\task_1536762326\ build\application\firefox\xul.dll:
                                                         ^space???
Could not obtain a correct path somehow?
Attachment #9008724 - Flags: review?(nfroyd)
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Comment on attachment 9008724 [details] [diff] [review]
Make best efforts to write a stack frame atomically

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

Sure, we can try this.

::: xpcom/base/nsTraceRefcnt.cpp
@@ +785,5 @@
>    fflush(stream);
> +#ifdef XP_WIN
> +  _write(_fileno(stream), buf, len);
> +#else
> +  write(fileno(stream), buf, len);

I would ask "how much of a performance hit is this", but it should be pretty minimal, right, since we were flushing the stream every line in the first place, which would involve the same call to `write`?
Attachment #9008724 - Flags: review?(nfroyd) → review+
FYI, I've uploaded the 15.8.4 release to tooltool now.

"size": 349626009,
"visibility": "internal",
"digest": "ecf1e03f6f98f86775059a43f9e7dc7e326f6643d7c08962d9f614e4f5a65b1ca63fa1cfeb0f1a3c2474bf0d4318dda960b378beb2a44ecf8a91111207f4ece5",
"algorithm": "sha512",
"filename": "vs2017_15.8.4.zip"
Attachment #9007171 - Attachment is obsolete: true
The previous patch caused a compiler error on Linux because it did not see the return value from write(). I also fixed the "short write" problem.
Attachment #9009157 - Flags: review?(nfroyd)
Attachment #9008724 - Attachment is obsolete: true
Comment on attachment 9009156 [details] [diff] [review]
Update Windows builders to VS 2017 15.8.4 and Windows SDK 17134

carrying forward r+
Attachment #9009156 - Flags: review+
Comment on attachment 9009157 [details] [diff] [review]
Make best efforts to write a stack frame atomically

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

::: xpcom/base/nsTraceRefcnt.cpp
@@ +782,5 @@
> +    auto written = _write(fd, aBuf, aLen);
> +#else
> +    auto written = write(fd, aBuf, aLen);
> +#endif
> +    if (written <= 0 || size_t(written) > aLen) {

When does this second case happen?  It seems like it would catch error returns from `write`, but that should be caught by the first case?
Attachment #9009157 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd [:froydnj] from comment #45)
> > +    if (written <= 0 || size_t(written) > aLen) {
> 
> When does this second case happen?  It seems like it would catch error
> returns from `write`, but that should be caught by the first case?

Just in case `written` was larger than aLen even though `write` is not supposed to return such value. Since this is used as a part of assertion code, it would not be a good idea to use assertions here.
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/mozilla-inbound/rev/131188fb2361
Make best efforts to write a stack frame atomically. r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/815d17af6a7e
Workaround a linker bug of MSVC 2017 Update 8. r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/b2a536ba5d4b
Update Windows builders to VS 2017 15.8.4 and Windows SDK 17134. r=glandium
https://hg.mozilla.org/mozilla-central/rev/131188fb2361
https://hg.mozilla.org/mozilla-central/rev/815d17af6a7e
https://hg.mozilla.org/mozilla-central/rev/b2a536ba5d4b
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/9724c66105bf
Port bug 1485224 - Update Windows builders to VS 2017 15.8.4 and Windows SDK 17134. rs=bustage-fix DONTBUILD
Pushed by dluca@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e088bb62f286
Fixup for broken tests when combining bug 1485224 from inbound and bug 1490549 from autoland. r=me,a=build-bustage-fix

Comment on attachment 9008222 [details] [diff] [review]
Workaround a linker bug of MSVC 2017 Update 8

ESR Uplift Approval Request

If this is not a sec:{high,crit} bug, please state case for ESR consideration

Reducing intermittent failures on ESR 60 automations, see bug 1527038

User impact if declined

No user impact, only affecting stack frame dump.

Fix Landed on Version

64

Risk to taking this patch

Low

Why is the change risky/not risky? (and alternatives if risky)

This change would only affect stack frame dump.

String or UUID changes made by this patch

none

Attachment #9008222 - Flags: approval-mozilla-esr60?
Comment on attachment 9008222 [details] [diff] [review]
Workaround a linker bug of MSVC 2017 Update 8

Sorry, wrong patch.
Attachment #9008222 - Flags: approval-mozilla-esr60?

Comment on attachment 9009157 [details] [diff] [review]
Make best efforts to write a stack frame atomically

ESR Uplift Approval Request

If this is not a sec:{high,crit} bug, please state case for ESR consideration

Reducing intermittent failures on ESR 60 automations, see bug 1527038

User impact if declined

No user impact, only affecting stack frame dump.

Fix Landed on Version

64

Risk to taking this patch

Low

Why is the change risky/not risky? (and alternatives if risky)

This change would only affect stack frame dump.

String or UUID changes made by this patch

none

Attachment #9009157 - Flags: approval-mozilla-esr60?
Comment on attachment 9009157 [details] [diff] [review]
Make best efforts to write a stack frame atomically

Essentially a NPOTB change needed to fix frequently-failing tests on ESR60 after the Win10 worker upgrade. Approved for ESR60.
Attachment #9009157 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Comment on attachment 9009157 [details] [diff] [review]
Make best efforts to write a stack frame atomically

https://hg.mozilla.org/releases/mozilla-esr60/rev/afe14f79474a

Clearing the ESR60 approval to get it off the needs-uplift radar since setting the esr60 status to fixed doesn't really make sense in this case either.
Attachment #9009157 - Flags: approval-mozilla-esr60+ → checkin+
You need to log in before you can comment on or make changes to this bug.