1485224 - Update Visual Studio 2017 on builders to Update 8

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Masatoshi Kimura [:emk]]

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.)

Comment 1

[Nathan Froyd [:froydnj]]

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!

Comment 2

[Mike Hommey [:glandium]]

There's an in-tree script to create them.

Comment 3

[Mike Hommey [:glandium]]

See build/windows_toolchain.py. It needs some changes for versions.

Comment 4

[Masatoshi Kimura [:emk]]

(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.

Comment 5

[Masatoshi Kimura [:emk]]

Attachment: Update windows_toolchain.py to VS 2017 u8 and SDK 17134

Comment 6

[Nathan Froyd [:froydnj]]

Uploaded the archive Kimura-san sent:

{
"size": 349578286,
"visibility": "internal",
"digest": "1c7bba22a8def72dadf7caff418373b7cc7dca45a7d45cdc0041d2dfea7e590bd87163308ffe0d9c42af78783cf46b95fe8b31f692ad51790fe03bc37a8528d7",
"algorithm": "sha512",
"filename": "vs2017_15.8.0.zip"
}

Comment 7

[Masatoshi Kimura [:emk]]

Mmm, apparently this archive contains Japanese localized MSVC toolchains...

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Uploaded a new one:
[
  {
    "size": 349594612,
    "visibility": "internal",
    "digest": "c28588327efd079c547e844c907d4e2fdd05877550dfcd576e55a504de046978d0263ddcb03b6add514c24c9f09a433c3509164f019a9fddebddcc047922a18c",
    "algorithm": "sha512",
    "filename": "vs2017_15.8.1.zip"
  }
]

Comment 9

[Kim Moir [:kmoir] ET]

triaging, assigning to emk since attached patches

Comment 10

[Masatoshi Kimura [:emk]]

MSVC PGO builds fail with LNK1107 :(
Unassigning myself because I could not resolve or workaround the issue.

Comment 11

[Masatoshi Kimura [:emk]]

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.

Comment 12

[Nathan Froyd [:froydnj]]

(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?

Comment 13

[Masatoshi Kimura [:emk]]

(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.

Comment 14

[Masatoshi Kimura [:emk]]

Attachment: Workaround a linker bug of MSVC 2017 Update 8

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.

Comment 15

[Masatoshi Kimura [:emk]]

Attachment: Update Windows builders to VS 2017 Update 8 and Windows SDK 17134

Comment 16

[Mike Hommey [:glandium]]

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?

Comment 17

[Masatoshi Kimura [:emk]]

(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.

Comment 18

[Mike Hommey [:glandium]]

(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 19

[Mike Hommey [:glandium]]

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.

Comment 20

[Masatoshi Kimura [:emk]]

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.

Comment 21

[Masatoshi Kimura [:emk]]

Attachment: Update Windows builders to VS 2017 Update 8 and Windows SDK 17134

Comment 22

[Masatoshi Kimura [:emk]]

Attachment: 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.

Comment 23

[Masatoshi Kimura [:emk]]

Filed an issue at Developer Community:
https://developercommunity.visualstudio.com/content/problem/330992/msvc-158-linker-cannot-handle-forward-slashes-when.html

Comment 24

[Masatoshi Kimura [:emk]]

(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 25

[Mike Hommey [:glandium]]

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)

Comment 26

[Mike Hommey [:glandium]]

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.

Comment 27

[Masatoshi Kimura [:emk]]

(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

Comment 28

[Masatoshi Kimura [:emk]]

Attachment: Workaround a linker bug of MSVC 2017 Update 8

* Added LINKER_OUT as suggested.
* Added HOST_OS_ARCH to the condition.

Comment 29

[Pulsebot]

Pushed by [e-mail]:
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

Comment 30

[Eliza Balazs [:ebalazs_]]

Backed out 2 changesets (Bug 1485224) for perma bc3 failures e.g.: Error getting fileid for Z:\task_1536750533ad] WARNING: NS_ENSURE_TRUE CLOSED TREE

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed,busted,exception&revision=fe2c607f4af7e2bd13f687cc3f2b9e2192e89ce2&selectedJob=198831446

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=198831446&repo=mozilla-inbound

Backout: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed,busted,exception&revision=bfed465371545545c4e1a79f351ddd60c92733c2

Comment 31

[Nathan Froyd [:froydnj]]

Looks like crashes, possibly due to miscompilation (?).

Comment 32

[Masatoshi Kimura [:emk]]

But the crashing jobs are running on clang-cl builds. Really weird...

Comment 33

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

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?

Comment 35

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 36

[Masatoshi Kimura [:emk]]

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?

Comment 37

[Masatoshi Kimura [:emk]]

Attachment: Make best efforts to write a stack frame atomically

Comment 38

[Nathan Froyd [:froydnj]]

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`?

Comment 39

[Masatoshi Kimura [:emk]]

Try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b3110b328da5ddf86015a913a9330dd7f6ba7393

Comment 40

[Ryan VanderMeulen [:RyanVM]]

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"

Comment 41

[Masatoshi Kimura [:emk]]

Attachment: Update Windows builders to VS 2017 15.8.4 and Windows SDK 17134

Comment 42

[Masatoshi Kimura [:emk]]

Attachment: Make best efforts to write a stack frame atomically

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.

Comment 43

[Masatoshi Kimura [:emk]]

Try runs:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=85601b9c7be1317f4a9c6ddd9e48167b16a02fce
https://treeherder.mozilla.org/#/jobs?repo=try&revision=19e8e60b4cf0a657b61a2501a27c2c5f23464d13

Comment 44

[Masatoshi Kimura [:emk]]

Comment on attachment 9009156 [details] [diff] [review]
Update Windows builders to VS 2017 15.8.4 and Windows SDK 17134

carrying forward r+

Comment 45

[Nathan Froyd [:froydnj]]

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?

Comment 46

[Masatoshi Kimura [:emk]]

(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.

Comment 47

[Pulsebot]

Pushed by [e-mail]:
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

Comment 48

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/131188fb2361
https://hg.mozilla.org/mozilla-central/rev/815d17af6a7e
https://hg.mozilla.org/mozilla-central/rev/b2a536ba5d4b

Comment 49

[Pulsebot]

Pushed by [e-mail]:
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

Comment 50

[Pulsebot]

Pushed by [e-mail]:
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 51

[Masatoshi Kimura [:emk]]

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

Comment 52

[Masatoshi Kimura [:emk]]

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

Sorry, wrong patch.

Comment 53

[Masatoshi Kimura [:emk]]

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

Comment 54

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 55

[Ryan VanderMeulen [:RyanVM]]

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.