|Summary:||Switch windows build to Visual Studio 2017|
|Severity:||Normal||CC:||achristensen, bfulgham, commit-queue, don.olmstead, ews-watchlist, Hironori.Fujii, pvollan, stephan.szabo, webkit-bug-importer, ysuzuki|
|Version:||WebKit Nightly Build|
|Bug Depends on:||174979|
Description isaac+webkit 2017-05-19 23:57:20 PDT
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++
Comment 2 isaac+webkit 2017-05-22 19:46:34 PDT
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")
Comment 3 isaac+webkit 2017-05-22 19:47:55 PDT
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
Comment 4 Fujii Hironori 2017-07-26 23:08:11 PDT
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?
Comment 5 Fujii Hironori 2017-07-27 18:15:33 PDT
Oh, no. Just removing pragma init_seg causes a crash when destructors are called at shutdown.
Comment 6 Fujii Hironori 2017-07-30 16:03:50 PDT
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.
Comment 7 Yusuke Suzuki 2017-07-30 19:49:50 PDT
For a long term solution, I think we should use LazyNeverDestroyed instead of DEFINE_GLOBAL hack.
Comment 8 Fujii Hironori 2017-07-31 02:26:35 PDT
Sounds very interesting. I'll try it in Bug 174979. Thank you.
Comment 9 Fujii Hironori 2017-08-15 23:15:00 PDT
The pragma init_seg issue will be solved in Bug 175209. This bug doesn't block Bug 174195 for now.
Comment 10 Stephan Szabo 2017-11-30 11:26:40 PST
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.
Comment 11 EWS Watchlist 2017-11-30 11:28:46 PST
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]  Total errors found: 1 in 14 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 12 Stephan Szabo 2017-11-30 13:12:08 PST
Created attachment 328010 [details] Require VS2017 for building
Comment 13 Per Arne Vollan 2017-12-05 14:23:06 PST
(In reply to Stephan Szabo from comment #12) > Created attachment 328010 [details] > Require VS2017 for building Does this patch need rebasing?
Comment 14 Stephan Szabo 2017-12-05 14:59:54 PST
Created attachment 328508 [details] Require VS2017 for building Rebased to re-check on bot
Comment 15 Per Arne Vollan 2017-12-05 15:25:19 PST
Comment on attachment 328508 [details] Require VS2017 for building Looks good! R=me. Please wait until bots are green before committing.
Comment 16 Stephan Szabo 2017-12-05 16:06:12 PST
Created attachment 328518 [details] Require VS2017 for building - reup
Comment 17 Stephan Szabo 2017-12-05 17:47:13 PST
Created attachment 328539 [details] Require VS2017 for building - setting reviewer based on pre-reupload r=me
Comment 18 WebKit Commit Bot 2017-12-05 18:19:01 PST
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>
Comment 19 WebKit Commit Bot 2017-12-05 18:19:03 PST
All reviewed patches have been landed. Closing bug.