Bug 172412 – Switch windows build to Visual Studio 2017

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

Created attachment 310766 [details]
Patch to change to use Visual Studio 2017 for building

Switch the target compiler and build environment to Visual Studio 2017.

N.B. The attached patch seems to cause MiniBrowser at startup with the following stack trace:
WebKit\WebKitBuild\Debug\DerivedSources\WebCore\HTMLNames.cpp

 	WTF.dll!WTFCrash() Line 292	C++
 	WebKit.dll!WTF::StringImpl::assertHashIsCorrect() Line 889	C++
>	WebKit.dll!WebCore::HTMLNames::init() Line 1666	C++
 	WebKit.dll!WebCore::Frame::Frame(WebCore::Page & page, WebCore::HTMLFrameOwnerElement * ownerElement, WebCore::FrameLoaderClient & frameLoaderClient) Line 175	C++
 	WebKit.dll!WebCore::MainFrame::MainFrame(WebCore::Page & page, WebCore::PageConfiguration & configuration) Line 56	C++
 	WebKit.dll!WebCore::MainFrame::create(WebCore::Page & page, WebCore::PageConfiguration & configuration) Line 68	C++
 	WebKit.dll!WebCore::Page::Page(WebCore::PageConfiguration && pageConfiguration) Line 270	C++
 	WebKit.dll!WebView::initWithFrame(tagRECT frame, wchar_t * frameName, wchar_t * groupName) Line 3122	C++
 	MiniBrowserLib.dll!MiniBrowser::prepareViews(HWND__ * mainWnd, const tagRECT & clientRect, wchar_t * const & requestedURL, HWND__ * & viewHwnd) Line 100	C++
 	MiniBrowserLib.dll!wWinMain(HINSTANCE__ * hInstance, HINSTANCE__ * hPrevInstance, wchar_t * lpstrCmdLine, int nCmdShow) Line 160	C++
 	MiniBrowserLib.dll!dllLauncherEntryPoint(HINSTANCE__ * hInstance, HINSTANCE__ * hPrevInstance, wchar_t * lpstrCmdLine, int nCmdShow) Line 857	C++
 	MiniBrowser.exe!wWinMain(HINSTANCE__ * hInstance, HINSTANCE__ * hPrevInstance, wchar_t * lpstrCmdLine, int nCmdShow) Line 249	C++

[Fujii Hironori]

Your callstack looks similar with Bug 171800.

Yes, I thought so too. I've experimented with re-enabling static constructors by removing the line below from Source/WTF/wtf/StaticConstructors and it then works.
#pragma init_seg(".unwantedstaticinits")

Also, I've managed to launch MiniBrowser in the debugger and it appears that the aData statics aren't initialized, unless that #pragma is removed from StaticConstructors.h

[Fujii Hironori]

The init_seg trick doesn't work for VS 2017 as expected.
How should we do?

1) Look for the solution or ask MS how to solve this
2) Stop using the init_seg trick for both VS 2015 and 2017
3) Use the init_seg trick only for VS 2015

Anyone have an idea?

[Fujii Hironori]

Oh, no. Just removing pragma init_seg causes a crash when destructors are called at shutdown.

[Fujii Hironori]

Created attachment 316752 [details]
WIP patch (Moved #pragma init_seg)

In the compilation units of generated *Names.cpp files, some global variable need to be constructed, but other doesn't.
Put the global variable needing to be constructed in the front of one doesn't, and put #pragma init_seg just after the former.

[Yusuke Suzuki]

For a long term solution, I think we should use LazyNeverDestroyed instead of DEFINE_GLOBAL hack.

[Fujii Hironori]

Sounds very interesting. I'll try it in Bug 174979. Thank you.

[Fujii Hironori]

The pragma init_seg issue will be solved in Bug 175209. This bug doesn't block Bug 174195 for now.

[Stephan Szabo]

Created attachment 327998 [details]
Require VS2017 for building

This merges together pieces of the earlier patch and https://bug-174195-attachments.webkit.org/attachment.cgi?id=314698 and updates the webkitdirs pieces to deal with the vswhere changes. 

I ended up not giving a specific location relating to the location of VS2017 for vcvars.bat in some messages as both previous patches seemed to update the text to a location I didn't actually get with a VS2017 install and from my install it seemed like the location would be dependent on the particular VS2017 flavor (enterprise, professional, etc) you installed.

Uploading first to throw at bots.

[EWS Watchlist]

Attachment 327998 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Stephan Szabo]

Created attachment 328010 [details]
Require VS2017 for building

[Per Arne Vollan]

(In reply to Stephan Szabo from comment #12)
> Created attachment 328010 [details]
> Require VS2017 for building

Does this patch need rebasing?

[Stephan Szabo]

Created attachment 328508 [details]
Require VS2017 for building

Rebased to re-check on bot

[Per Arne Vollan]

Comment on attachment 328508 [details]
Require VS2017 for building

Looks good! R=me. Please wait until bots are green before committing.

[Stephan Szabo]

Created attachment 328518 [details]
Require VS2017 for building - reup

[Stephan Szabo]

Created attachment 328539 [details]
Require VS2017 for building - setting reviewer based on pre-reupload r=me

[WebKit Commit Bot]

Comment on attachment 328539 [details]
Require VS2017 for building - setting reviewer based on pre-reupload r=me

Clearing flags on attachment: 328539

Committed r225563: <https://trac.webkit.org/changeset/225563>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35896610>

[Radar WebKit Bug Importer]

<rdar://problem/35896611>

[Don Olmstead]

*** Bug 174195 has been marked as a duplicate of this bug. ***