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

• Component: CMake
• Status: RESOLVED FIXED

[QuellaZhang]

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

[Fujii Hironori]

Created attachment 380731 [details]
Patch

[QuellaZhang]

Created attachment 380824 [details]
webkit patch

new error was generated after applying this webkit.patch file

[QuellaZhang]

Created attachment 380825 [details]
webkit_result_log

The webkit result log was generated after applying patch.(please see attached patch file)

[Fujii Hironori]

Created attachment 447302 [details]
Patch

[Darin Adler]

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 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 on attachment 447302 [details]
Patch

My bad. this is a wrong patch.

[Fujii Hironori]

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

Created attachment 447430 [details]
Patch

[Darin Adler]

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

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]

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

[Darin Adler]

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]

Created attachment 447463 [details]
Patch

[Don Olmstead]

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

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]

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]

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

[Fujii Hironori]

Created attachment 447570 [details]
Patch

[EWS]

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