Enable IDN on Windows when building with clang #12606

mraunak · 2023-12-28T22:13:42Z

This PR aims to enable IDN on the Windows platform with CLANG compiler version 17 which is required to successfully
build TensorFlow-CPU on the Windows
Changed https://github.com/curl/curl/blob/master/lib/idn.c to use an inbuilt Windows function IdnToAscii on the Clang Compiler

Below is the screenshot of the error:

gvanem · 2023-12-29T06:25:41Z

So why isn't -D_WIN32_WINNT >= 0x600 defined for Clang?

vszakats · 2023-12-29T12:30:00Z

It doesn't look right to enable Windows SDK-specific code based on compiler vendor.
The official Windows binaries are built with clang 17 + IDN without this issue.

What needs to be seen is why this build env is missing these Windows declarations
or why it sets USE_WIN32_IDN without _WIN32, or why its Windows headers
are missing these declarations without a matching _WIN32_WINNT?

What build tool, build config, curl version is this happening with?

bagder · 2024-01-04T22:26:22Z

@mraunak: this PR awaits info from you...

mraunak · 2024-01-08T21:30:18Z

Hi Team, Thank you very much for your input.

idn.c gets compiled successfully with CLANG 15.0.7 but has the error shown above when compiled with CLANG 17

Build tool: Bazel version 6.4.0

Build config:
ERROR: D:/ockagzuo/external/curl/BUILD.bazel:31:11: Compiling lib/idn.c failed: (Exit 1): clang-cl.exe failed: error executing command (from target @curl//:curl)
cd /d D:/ockagzuo/execroot/org_tensorflow
SET INCLUDE=C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.38.33130\include;C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.38.33130\ATLMFC\include;C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Auxiliary\VS\include;C:\Program Files (x86)\Windows Kits\10\include\10.0.22621.0\ucrt;C:\Program Files (x86)\Windows Kits\10\include\10.0.22621.0\um;C:\Program Files (x86)\Windows Kits\10\include\10.0.22621.0\shared;C:\Program Files (x86)\Windows Kits\10\include\10.0.22621.0\winrt;C:\Program Files (x86)\Windows Kits\10\include\10.0.22621.0\cppwinrt;C:\Program Files (x86)\Windows Kits\NETFXSDK\4.8\include\um;D:\user\mraunak\LLVM\lib\clang\17\include
SET PATH=C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.38.33130\bin\HostX64\x64;C:\Program Files\Microsoft Visual Studio\2022\Community\Common7\IDE\VC\VCPackages;C:\Program Files\Microsoft Visual Studio\2022\Community\Common7\IDE\CommonExtensions\Microsoft\TestWindow;C:\Program Files\Microsoft Visual Studio\2022\Community\Common7\IDE\CommonExtensions\Microsoft\TeamFoundation\Team Explorer;C:\Program Files\Microsoft Visual Studio\2022\Community\MSBuild\Current\bin\Roslyn;C:\Program Files\Microsoft Visual Studio\2022\Community\Team Tools\Performance Tools\x64;C:\Program Files\Microsoft Visual Studio\2022\Community\Team Tools\Performance Tools;C:\Program Files (x86)\Microsoft Visual Studio\Shared\Common\VSPerfCollectionTools\vs2019\x64;C:\Program Files (x86)\Microsoft Visual Studio\Shared\Common\VSPerfCollectionTools\vs2019;C:\Program Files (x86)\Microsoft SDKs\Windows\v10.0A\bin\NETFX 4.8 Tools\x64;C:\Program Files\Microsoft Visual Studio\2022\Community\Team Tools\DiagnosticsHub\Collector;C:\Program Files (x86)\Windows Kits\10\bin\10.0.22621.0\x64;C:\Program Files (x86)\Windows Kits\10\bin\x64;C:\Program Files\Microsoft Visual Studio\2022\Community\MSBuild\Current\Bin\amd64;C:\Windows\Microsoft.NET\Framework64\v4.0.30319;C:\Program Files\Microsoft Visual Studio\2022\Community\Common7\IDE;C:\Program Files\Microsoft Visual Studio\2022\Community\Common7\Tools;;C:\Windows\system32;C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\Llvm\x64\bin;C:\Program Files\Microsoft Visual Studio\2022\Community\Common7\IDE\CommonExtensions\Microsoft\CMake\CMake\bin;C:\Program Files\Microsoft Visual Studio\2022\Community\Common7\IDE\CommonExtensions\Microsoft\CMake\Ninja;C:\Program Files\Microsoft Visual Studio\2022\Community\Common7\IDE\VC\Linux\bin\ConnectionManagerExe
SET PWD=/proc/self/cwd
SET PYTHON_BIN_PATH=D:/user/mraunak/venv310_new1/Scripts/python.exe
SET PYTHON_LIB_PATH=D:/user/mraunak/venv310_new1/lib/site-packages
SET TEMP=C:\Users\mraunak\AppData\Local\Temp\2
SET TF2_BEHAVIOR=1
SET TMP=C:\Users\mraunak\AppData\Local\Temp\2
SET VSLANG=1033
D:\user\mraunak\LLVM\bin\clang-cl.exe @bazel-out/x64_windows-opt/bin/external/curl/_objs/curl/idn.obj.params
"#Configuration: d9c220ac8112bcb55e4eecee5f7704306287d5558f345559f8f07e36e16a82c0"
"#Execution platform: //tensorflow/tools/toolchains/win:x64_windows-clang-cl"

Curl version: curl 8.4.0 (Windows) libcurl/8.4.0 Schannel WinIDN
Release-Date: 2023-10-11

Clang version: 17.0.6

dfandrich · 2024-01-08T23:37:10Z

Could you show the actual compile error? That's not included in that snippet.

mraunak · 2024-01-08T23:54:21Z

clang-17.txt

Hi @dfandrich, please find the attached log with the error. Please let me know if any further information is needed from my end

dfandrich · 2024-01-09T00:09:00Z

The underlying errors seem to be the same as in your initial screenshot:

error: call to undeclared function 'IdnToUnicode';
error: call to undeclared function 'IdnToAscii

So, the question from vszakats still stands: why this build env is missing these Windows declarations?

mraunak · 2024-01-09T09:05:32Z

Hi Team,

IdnToUnicode and IdnToASCII function is handled by WinNls.h
https://github.com/tpn/winsdk-10/blob/master/Include/10.0.10240.0/um/WinNls.h#L2445
https://learn.microsoft.com/en-us/cpp/porting/modifying-winver-and-win32-winnt?view=msvc-170. As per the above documentation, WINVER <0x600 was added to fix the issue. Please let me know if it is an acceptable solution

The other solution is to update the TensorFlow configuration file for the Windows platform, for this approach, there is no need for a PR to Curl.

vszakats · 2024-01-09T10:04:00Z

If this is caused by mismatched _WIN32_WINNT and WINVER values setup by the build environment, I'd vote to fix this in the build configuration.

Edit: As seen in Windows SDK 7.1 and mingw-w64 Windows headers, if undefined, WINVER is made equal to _WIN32_WINNT in the header sdkddkver.h, which is one of the first headers included by windows.h. Meaning this issue should be fixed by Windows headers automatically. It's still a pending question why this built-in mechanism fails in this case. Based on what we know so far, this isn't a curl issue, and as such, the correct place to fix this isn't curl's idn.c.

mihaimaruseac · 2024-01-10T17:01:35Z

This could be caused by missing defines/includes/files to compile&link in https://github.com/tensorflow/tensorflow/blob/master/third_party/curl.BUILD

vszakats · 2024-01-10T18:18:40Z

Is this correct though?
https://github.com/tensorflow/tensorflow/blob/83b3fbfa32de0ff95f52e86a1370d7e3156acac0/third_party/curl.BUILD#L17-L21

According to the comment it's trying to fix something in curl
(would be useful to know what), but with this particular setup,
which isn't SDK 7.1, but SDK 10 and not MSVC but clang-cl, it
might rather be breaking things.

The Tensorflow build hack triggers some logic in curl, which
messes with WINVER:

curl/lib/config-win32.h

Lines 320 to 380 in 24ae4a0

 /* Define some minimum and default build targets for Visual Studio */ 

 #if defined(_MSC_VER) 

 /* Officially, Microsoft's Windows SDK versions 6.X does not support Windows 

  2000 as a supported build target. VS2008 default installations provides 

  an embedded Windows SDK v6.0A along with the claim that Windows 2000 is a 

  valid build target for VS2008. Popular belief is that binaries built with 

  VS2008 using Windows SDK versions v6.X and Windows 2000 as a build target 

  are functional. */ 

 # define VS2008_MIN_TARGET 0x0500 

 /* The minimum build target for VS2012 is Vista unless Update 1 is installed 

  and the v110_xp toolset is chosen. */ 

 # if defined(_USING_V110_SDK71_) 

 # define VS2012_MIN_TARGET 0x0501 

 # else 

 # define VS2012_MIN_TARGET 0x0600 

 # endif 

 /* VS2008 default build target is Windows Vista. We override default target 

  to be Windows XP. */ 

 # define VS2008_DEF_TARGET 0x0501 

 /* VS2012 default build target is Windows Vista unless Update 1 is installed 

  and the v110_xp toolset is chosen. */ 

 # if defined(_USING_V110_SDK71_) 

 # define VS2012_DEF_TARGET 0x0501 

 # else 

 # define VS2012_DEF_TARGET 0x0600 

 # endif 

 #endif 

 /* VS2008 default target settings and minimum build target check. */ 

 #if defined(_MSC_VER) && (_MSC_VER >= 1500) && (_MSC_VER <= 1600) 

 # ifndef _WIN32_WINNT 

 # define _WIN32_WINNT VS2008_DEF_TARGET 

 # endif 

 # ifndef WINVER 

 # define WINVER VS2008_DEF_TARGET 

 # endif 

 # if (_WIN32_WINNT < VS2008_MIN_TARGET) || (WINVER < VS2008_MIN_TARGET) 

 # error VS2008 does not support Windows build targets prior to Windows 2000 

 # endif 

 #endif 

 /* VS2012 default target settings and minimum build target check. */ 

 #if defined(_MSC_VER) && (_MSC_VER >= 1700) 

 # ifndef _WIN32_WINNT 

 # define _WIN32_WINNT VS2012_DEF_TARGET 

 # endif 

 # ifndef WINVER 

 # define WINVER VS2012_DEF_TARGET 

 # endif 

 # if (_WIN32_WINNT < VS2012_MIN_TARGET) || (WINVER < VS2012_MIN_TARGET) 

 # if defined(_USING_V110_SDK71_) 

 # error VS2012 does not support Windows build targets prior to Windows XP 

 # else 

 # error VS2012 does not support Windows build targets prior to Windows \ 

 Vista 

 # endif 

 # endif 

 #endif

This logic in curl seems dangerous and possibly obsolete, but
since it's MSVC-specific, I haven't looked at it. Touching either
_WIN32_WINNT or WINVER by any project is generally
best to avoid and these are up to the builders to set (or leave as is).
If there is a combination that is broken, I'd vote to put out an
#error rather than trying to fix it locally.

I haven't tried but this problem might be fixed by deleting either
the Tensorflow hack or the curl hack that it triggers. Or both.

The other logic in curl that touches WINVER is this in autotools:

curl/configure.ac

Lines 2471 to 2488 in 24ae4a0

 dnl WinIDN requires a minimum supported OS version of at least Vista (0x0600) 

 AC_COMPILE_IFELSE([ 

  AC_LANG_PROGRAM([[ 

  #include <windows.h> 

  ]],[[ 

  #if (WINVER < 0x600) && (_WIN32_WINNT < 0x600) 

  #error 

  #endif 

  ]]) 

  ],[ 

  ],[ 

  CFLAGS=`echo $CFLAGS | $SED -e 's/-DWINVER=[[^ ]]*//g'` 

  CFLAGS=`echo $CFLAGS | $SED -e 's/-D_WIN32_WINNT=[[^ ]]*//g'` 

  CPPFLAGS=`echo $CPPFLAGS | $SED -e 's/-DWINVER=[[^ ]]*//g'` 

  CPPFLAGS=`echo $CPPFLAGS | $SED -e 's/-D_WIN32_WINNT=[[^ ]]*//g'` 

  WINIDN_CPPFLAGS="$WINIDN_CPPFLAGS -DWINVER=0x0600" 

  ]) 

 #

Added as part of a commit fixing legacy-mingw issues in 37f1c21.
The commit message doesn't mention IDN while this logic specifically
tries to address some issues with it. If this is legacy-mingw specific,
I think it'd be best to delete it, because we no longer support legacy-mingw.

mraunak · 2024-01-11T20:27:02Z

Hi Team, Thank you very much for your input, removing the hack (D_USING_V110_SDK71_) works.
https://github.com/tensorflow/tensorflow/blob/83b3fbfa32de0ff95f52e86a1370d7e3156acac0/third_party/curl.BUILD#L17-L21
If we remove line 21, we won't see the IDNTOASCII/IDNTOUNICODE error.

- If the minimum Windows target OS is pre-Vista (_WIN32_WINNT < 0x600) then error. - Stop comparing against WINVER when making the minimum OS determinations (ie check only _WIN32_WINNT instead of both since that's the one that matters). Prior to this change, if --with-winidn was used in such a case configure would change the minimum target OS to Vista (0x600) so the build would compile. That was done primarily as a workaround for original MinGW which we no longer support. Bug: curl#12606 (comment) Reported-by: Viktor Szakats Closes #xxxxx

jay · 2024-01-12T00:18:05Z

This logic in curl seems dangerous and possibly obsolete, but
since it's MSVC-specific, I haven't looked at it. Touching either
_WIN32_WINNT or WINVER by any project is generally
best to avoid and these are up to the builders to set (or leave as is).
If there is a combination that is broken, I'd vote to put out an
#error rather than trying to fix it locally.

I will check this with some old Visual Studio versions. I think it was primarily meant as workarounds for old versions (which are still supported via winbuild and pregenerated projects). I don't know why it would be applied to new versions. Regardless that tensor hack is fishy.

Added as part of a commit fixing legacy-mingw issues in 37f1c21.
The commit message doesn't mention IDN while this logic specifically
tries to address some issues with it. If this is legacy-mingw specific,
I think it'd be best to delete it, because we no longer support legacy-mingw.

#12684

- If the minimum Windows target OS is pre-Vista (_WIN32_WINNT < 0x600) then error. - Stop comparing against WINVER when making the minimum OS determinations (ie check only _WIN32_WINNT instead of both since that's the one that matters). Prior to this change, if --with-winidn was used in such a case configure would change the minimum target OS to Vista (0x600) so the build would compile. That was done primarily as a workaround for original MinGW which we no longer support. Bug: curl#12606 (comment) Reported-by: Viktor Szakats Closes #xxxxx

bagder · 2024-04-11T14:52:03Z

@mraunak I take it this PR is no longer actually necessary or wanted?

mraunak · 2024-04-11T20:44:48Z

Hi @bagder, yes, we can close it. Thank you!

bagder · 2024-04-11T20:52:22Z

Thanks!

Add Clang support Windows

9633705

github-actions bot added the name lookup DNS and related tech label Dec 28, 2023

bagder added the Windows Windows-specific label Dec 28, 2023

bagder changed the title ~~Add Clang support Windows~~ enable IDN on Windows when building with clang Dec 28, 2023

bagder added the needs-info label Dec 29, 2023

vszakats added the build label Jan 2, 2024

mraunak changed the title ~~enable IDN on Windows when building with clang~~ Enable IDN on Windows when building with clang Jan 2, 2024

Resolve TensorFlow build error

c8aa601

jay mentioned this pull request Jan 12, 2024

configure.ac: error if --with-winidn and target OS too old #12684

Open

bagder closed this Apr 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable IDN on Windows when building with clang #12606

Enable IDN on Windows when building with clang #12606

mraunak commented Dec 28, 2023 •

edited

gvanem commented Dec 29, 2023

vszakats commented Dec 29, 2023 •

edited

bagder commented Jan 4, 2024

mraunak commented Jan 8, 2024 •

edited

dfandrich commented Jan 8, 2024

mraunak commented Jan 8, 2024

dfandrich commented Jan 9, 2024

mraunak commented Jan 9, 2024 •

edited

vszakats commented Jan 9, 2024 •

edited

mihaimaruseac commented Jan 10, 2024

vszakats commented Jan 10, 2024 •

edited

mraunak commented Jan 11, 2024 •

edited

jay commented Jan 12, 2024

bagder commented Apr 11, 2024

mraunak commented Apr 11, 2024

bagder commented Apr 11, 2024

Enable IDN on Windows when building with clang #12606

Enable IDN on Windows when building with clang #12606

Conversation

mraunak commented Dec 28, 2023 • edited

gvanem commented Dec 29, 2023

vszakats commented Dec 29, 2023 • edited

bagder commented Jan 4, 2024

mraunak commented Jan 8, 2024 • edited

dfandrich commented Jan 8, 2024

mraunak commented Jan 8, 2024

dfandrich commented Jan 9, 2024

mraunak commented Jan 9, 2024 • edited

vszakats commented Jan 9, 2024 • edited

mihaimaruseac commented Jan 10, 2024

vszakats commented Jan 10, 2024 • edited

mraunak commented Jan 11, 2024 • edited

jay commented Jan 12, 2024

bagder commented Apr 11, 2024

mraunak commented Apr 11, 2024

bagder commented Apr 11, 2024

mraunak commented Dec 28, 2023 •

edited

vszakats commented Dec 29, 2023 •

edited

mraunak commented Jan 8, 2024 •

edited

mraunak commented Jan 9, 2024 •

edited

vszakats commented Jan 9, 2024 •

edited

vszakats commented Jan 10, 2024 •

edited

mraunak commented Jan 11, 2024 •

edited