Bug 202842

Summary:

MSVC reports "wtf/RetainPtr.h(196): error C3861: 'CFAutorelease': identifier not found " with /permissive- on Windows

Product:

WebKit

Reporter:

QuellaZhang <1998zhangyi>

Component:

CMake

Assignee:

Fujii Hironori <fujii.hironori>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

achristensen, benjamin, bfulgham, cdumez, cmarcelo, darin, dbates, don.olmstead, ews-watchlist, fujii.hironori, pvollan, ross.kirsling

Priority:

Version:

WebKit Local Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Build log	none
Patch	none
webkit patch	none
webkit_result_log	none
Patch	darin: review-
Patch	none
Patch	none
Patch	none

QuellaZhang

Reported 2019-10-10 23:35:46 PDT

Created attachment 380730 [details] Build log Hi all, WebKit failed with lots of "error C3861: 'CFAutorelease': identifier not found" under /permissive- mode when built by msvc on Windows, I use latest source afa5598 on master branch. Could you help take a look at this? Repro steps: 1. git clone https://github.com/WebKit/webkit d:\WebKit\src 2. open a VS 2017 x86 command prompt as admin and browse to D:\Webkit 3. extra WebKitLibrary\win to D:\WebKit\src\WebKitLibraries\win 4. set path=C:\gnuwin32\bin;%path% 5. set _CL_=/permissive- 6. cmake -G "Visual Studio 15 2017" -DCMAKE_SYSTEM_VERSION=10.0.17134.0 -DCMAKE_BUILD_TYPE=Release -DRUBY_LIBRARY=C:\Ruby25-x64\lib -DPORT="AppleWin" ..\src 7. msbuild /p:Configuration=Release;Platform=Win32 WebKit.sln /t:Rebuild Failures(detail see attachment): DateMath.cpp D:\WebKit\src\Source\WTF\wtf/RetainPtr.h(196): error C3861: 'CFAutorelease': identifier not found D:\WebKit\src\Source\WTF\wtf/RetainPtr.h(196): note: 'CFAutorelease': function declaration must be available as none of the arguments depend on a template parameter

Attachments
Build log (243.73 KB, text/plain) 2019-10-10 23:35 PDT, QuellaZhang	no flags	Details
Patch (1.46 KB, patch) 2019-10-11 00:24 PDT, Fujii Hironori	no flags	Details Formatted Diff Diff
webkit patch (653 bytes, patch) 2019-10-12 02:40 PDT, QuellaZhang	no flags	Details Formatted Diff Diff
webkit_result_log (459.82 KB, text/plain) 2019-10-12 03:02 PDT, QuellaZhang	no flags	Details
Patch (1.33 KB, patch) 2021-12-15 17:10 PST, Fujii Hironori	darin: review-	Details Formatted Diff Diff
Patch (1.65 KB, patch) 2021-12-16 23:00 PST, Fujii Hironori	no flags	Details Formatted Diff Diff
Patch (1.73 KB, patch) 2021-12-17 10:55 PST, Alex Christensen	no flags	Details Formatted Diff Diff
Patch (2.00 KB, patch) 2021-12-19 18:12 PST, Fujii Hironori	no flags	Details Formatted Diff Diff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.

Fujii Hironori

Comment 1 2019-10-11 00:24:22 PDT

Created attachment 380731 [details] Patch

QuellaZhang

Comment 2 2019-10-12 02:40:36 PDT

Created attachment 380824 [details] webkit patch new error was generated after applying this webkit.patch file

QuellaZhang

Comment 3 2019-10-12 03:02:18 PDT

Created attachment 380825 [details] webkit_result_log The webkit result log was generated after applying patch.(please see attached patch file)

Fujii Hironori

Comment 4 2021-12-15 17:10:02 PST

Created attachment 447302 [details] Patch

Darin Adler

Comment 5 2021-12-15 17:21:34 PST

Comment on attachment 447302 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=447302&action=review > Source/WTF/ChangeLog:10 > + * wtf/RetainPtr.h: Enclosed RetainPtr<T>::autorelease with #if !OS(WINDOWS). This patch wraps the entire RetainPtr class in #if !OS(WINDOWS), but that’s not what the comment says.

Darin Adler

Comment 6 2021-12-15 17:23:24 PST

Comment on attachment 447302 [details] Patch I think it’s OK to put a condition in here around the parts that won’t work on Windows, but: 1) That’s not what this patch does (and so that’s why review-). 2) I don’t think OS(WINDOWS) is the best conditional to use. I think we should use HAVE(CFAUTORELEASE) or HAVE(AUTORELEASE) even though we need to add to PlatformHave.h to do that.

Fujii Hironori

Comment 7 2021-12-15 17:27:24 PST

Comment on attachment 447302 [details] Patch My bad. this is a wrong patch.

Fujii Hironori

Comment 8 2021-12-16 22:14:37 PST

(In reply to Darin Adler from comment #6) > 2) I don’t think OS(WINDOWS) is the best conditional to use. I think we > should use HAVE(CFAUTORELEASE) or HAVE(AUTORELEASE) even though we need to > add to PlatformHave.h to do that. I checked AppleWin's latest WebKitSupportLibrary.zip and WebKitAuxiliaryLibrary.zip. They don't have CFAutorelease at least for now. I think OS(WINDOWS) is a good condition at least as now now. If AppleWin will add it in the future version, it will be a good time to add the new macro.

Fujii Hironori

Comment 9 2021-12-16 23:00:32 PST

Created attachment 447430 [details] Patch

Darin Adler

Comment 10 2021-12-17 10:39:57 PST

(In reply to Fujii Hironori from comment #8) > I think OS(WINDOWS) is a good condition at least as now now. I think it’s "hacky" to put just OS(WINDOWS). Over time, decisions like this make it hard to code correctly on the project. I feel the same way about overuse of PLATFORM(COCOA), PLATFORM(MAC), etc. But even more, "OS" is not supposed to be used to understand the availability of higher level frameworks. This is a misuse.

Darin Adler

Comment 11 2021-12-17 10:40:54 PST

I suggest: #if PLATFORM(COCOA) instead of #if !OS(WINDOWS) I still don’t think it’s great, but I think it’s better.

Alex Christensen

Comment 12 2021-12-17 10:42:18 PST

And you should probably do the same for the declaration: PtrType autorelease();

Darin Adler

Comment 13 2021-12-17 10:53:04 PST

That’s also OK, but not required. I guess it could save a programmer some time since compile will fail rather than link.

Alex Christensen

Comment 14 2021-12-17 10:55:16 PST

Created attachment 447463 [details] Patch

Don Olmstead

Comment 15 2021-12-17 11:04:48 PST

(In reply to Darin Adler from comment #6) > Comment on attachment 447302 [details] > Patch > > I think it’s OK to put a condition in here around the parts that won’t work > on Windows, but: > > 1) That’s not what this patch does (and so that’s why review-). > 2) I don’t think OS(WINDOWS) is the best conditional to use. I think we > should use HAVE(CFAUTORELEASE) or HAVE(AUTORELEASE) even though we need to > add to PlatformHave.h to do that. You should be able to do a check for this in the `OptionsWin.cmake` file. There's a macro for seeing if a symbol is present in the CMake code. I think doing something along the lines of this should work if (USE_CF) set(CMAKE_REQUIRED_INCLUDES ${WEBKIT_LIBRARIES_INCLUDE_DIR}) set(CMAKE_REQUIRED_LIBRARIES "${WEBKIT_LIBRARIES_LINK_DIR}/CoreFoundation${DEBUG_SUFFIX}.lib") WEBKIT_CHECK_HAVE_SYMBOLE(HAVE_AUTORELEASE CFAutorelease CoreFoundation/CoreFoundation.h) endif () See https://github.com/WebKit/WebKit/blob/main/Source/cmake/OptionsAppleWin.cmake#L50-L94

EWS

Comment 16 2021-12-17 11:54:59 PST

Committed r287200 (245367@main): <https://commits.webkit.org/245367@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 447463 [details].

Darin Adler

Comment 17 2021-12-17 13:22:07 PST

In the shorter term, if we want to use HAVE(CFAUTORELEASE) in the short term instead of PLATFORM, the main thing we would need to do is not at all Windows-specific; we’d add this to PlatforHave.h: #if PLATFORM(COCOA) #define HAVE_CFAUTORELEASE 1 #endif (In reply to Don Olmstead from comment #15) > You should be able to do a check for this in the `OptionsWin.cmake` file. > There's a macro for seeing if a symbol is present in the CMake code. That would be a nice refinement some day if we ever do have a Windows-platform variant that has CFAutorelease, but I think that’s not likely, so this would be of theoretical value, but not practical value. Or if we ever reach the point where we use CMake on all Cocoa platforms, then cross platform we could consider adding this to some platform-independent CMake source file: if (USE_CF) WEBKIT_CHECK_HAVE_SYMBOL(HAVE_AUTORELEASE CFAutorelease CoreFoundation/CoreFoundation.h) endif ()

Fujii Hironori

Comment 18 2021-12-17 14:44:08 PST

Thank you for the feedback. I reopen this ticket to revise.

Fujii Hironori

Comment 19 2021-12-19 18:12:55 PST

Created attachment 447570 [details] Patch

EWS

Comment 20 2021-12-20 09:59:04 PST

Committed r287265 (245421@main): <https://commits.webkit.org/245421@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 447570 [details].

Note You need to log in before you can comment on or make changes to this bug.