[Clang][Cygwin] Remove erroneous _WIN32 define and clean up Cygwin code

[mati865]

With this define present and building with Clang the build fails:

In file included from /h/projects/llvm-project/clang/tools/libclang/CIndexer.cpp:36:
In file included from /usr/include/w32api/windows.h:69:
In file included from /usr/include/w32api/windef.h:9:
In file included from /usr/include/w32api/minwindef.h:163:
In file included from /usr/include/w32api/winnt.h:1658:
In file included from /usr/lib/clang/20/include/x86intrin.h:15:
In file included from /usr/lib/clang/20/include/immintrin.h:24:
In file included from /usr/lib/clang/20/include/xmmintrin.h:31:
/usr/lib/clang/20/include/mm_malloc.h:45:22: error: use of undeclared identifier '_aligned_malloc'; did you mean 'aligned_alloc'?
   45 |   __mallocedMemory = _aligned_malloc(__size, __align);
      |                      ^

Removing it allows the build with Clang to succeed and doesn't break build with GCC.

The "#define _WIN32" was originally "#define LLVM_ON_WIN32", but this was changed into a plain "#define _WIN32" (which not necessarily is something that is supported to do) in 1865df4 as part of a larger refactoring.

This cygwin specific code isn't needed for converting paths anymore, as dladdr takes care of it. (dladdr was added to cygwin in 2017, according to the tags in version 2.8.0.)

[llvmbot]

@llvm/pr-subscribers-clang

Author: Mateusz Mikuła (mati865)

Changes

With this define present and building with Clang the build fails:

In file included from /h/projects/llvm-project/clang/tools/libclang/CIndexer.cpp:36:
In file included from /usr/include/w32api/windows.h:69:
In file included from /usr/include/w32api/windef.h:9:
In file included from /usr/include/w32api/minwindef.h:163:
In file included from /usr/include/w32api/winnt.h:1658:
In file included from /usr/lib/clang/20/include/x86intrin.h:15:
In file included from /usr/lib/clang/20/include/immintrin.h:24:
In file included from /usr/lib/clang/20/include/xmmintrin.h:31:
/usr/lib/clang/20/include/mm_malloc.h:45:22: error: use of undeclared identifier '_aligned_malloc'; did you mean 'aligned_alloc'?
   45 |   __mallocedMemory = _aligned_malloc(__size, __align);
      |                      ^

Removing it allows the build with Clang to succeed and doesn't break build with GCC.

Split out from #134494

---

Full diff: https://github.com/llvm/llvm-project/pull/138120.diff

1 Files Affected:

• (modified) clang/tools/libclang/CIndexer.cpp (-16)

diff --git a/clang/tools/libclang/CIndexer.cpp b/clang/tools/libclang/CIndexer.cpp
index 12d9d418dea51..11d9312b64849 100644
--- a/clang/tools/libclang/CIndexer.cpp
+++ b/clang/tools/libclang/CIndexer.cpp
@@ -26,12 +26,6 @@
 #include <cstdio>
 #include <mutex>
 
-#ifdef __CYGWIN__
-#include <cygwin/version.h>
-#include <sys/cygwin.h>
-#define _WIN32 1
-#endif
-
 #ifdef _WIN32
 #include <windows.h>
 #elif defined(_AIX)
@@ -112,16 +106,6 @@ const std::string &CIndexer::getClangResourcesPath() {
                sizeof(mbi));
   GetModuleFileNameA((HINSTANCE)mbi.AllocationBase, path, MAX_PATH);
 
-#ifdef __CYGWIN__
-  char w32path[MAX_PATH];
-  strcpy(w32path, path);
-#if CYGWIN_VERSION_API_MAJOR > 0 || CYGWIN_VERSION_API_MINOR >= 181
-  cygwin_conv_path(CCP_WIN_A_TO_POSIX, w32path, path, MAX_PATH);
-#else
-  cygwin_conv_to_full_posix_path(w32path, path);
-#endif
-#endif
-
   LibClangPath += path;
 #elif defined(_AIX)
   getClangResourcesPathImplAIX(LibClangPath);

[mstorsjo]

This change feels a little more unclear to me.

Defining _WIN32 in cygwin mode compilations, and including w32api headers from there, feels brittle; I'm not familiar enough with cygwin to know if this is something that one usually can do, or whether it is commonly done, or what caveats that involves (doesn't e.g. cygwin have an incompatible definition of the type long?) - so getting rid of that probably is good in itself.

Was this an issue only when building Clang with Clang - did this succeed if building Clang with GCC?

I presume that the removed codepaths did serve some purpose at some point initially. Can we dig up that purpose from the git history and recheck whether it's needed/relevant still at this point.

While building may be broken right now, the individual issue of missing _aligned_malloc() probably is fixable as well, so I'd like to know a bit more about the reasoning for why this cygwin specific codepath should be removed.

[mati865]

Defining _WIN32 in cygwin mode compilations, and including w32api headers from there, feels brittle; I'm not familiar enough with cygwin to know if this is something that one usually can do, or whether it is commonly done, or what caveats that involves (doesn't e.g. cygwin have an incompatible definition of the type long?) - so getting rid of that probably is good in itself.

The rule of thumb is to avoid using Win32 APIs unless absolutely necessary, but I'm not familiar with the reasons.

Was this an issue only when building Clang with Clang - did this succeed if building Clang with GCC?

Indeed, it builds fine with GCC.
I think the difference here is Clang being a cross-compiler, has more robust headers that cover all targets. GCC can drop bits irrelevant to its target at the build time.

If I grep through /usr for _aligned_malloc there are no matches for GCC and Clang guards it based on _WIN32:

$ grep -R _aligned_malloc /usr -C2
/usr/lib/clang/20/include/mm_malloc.h-  void *__mallocedMemory;
/usr/lib/clang/20/include/mm_malloc.h-#if defined(__MINGW32__)
/usr/lib/clang/20/include/mm_malloc.h:  __mallocedMemory = __mingw_aligned_malloc(__size, __align);
/usr/lib/clang/20/include/mm_malloc.h-#elif defined(_WIN32)
/usr/lib/clang/20/include/mm_malloc.h:  __mallocedMemory = _aligned_malloc(__size, __align);
/usr/lib/clang/20/include/mm_malloc.h-#else
/usr/lib/clang/20/include/mm_malloc.h-  if (posix_memalign(&__mallocedMemory, __align, __size))
--
/usr/lib/gcc/x86_64-pc-msys/13.3.0/include/c++/x86_64-pc-msys/bits/c++config.h-/* #undef _GLIBCXX_HAVE__ACOSL */
/usr/lib/gcc/x86_64-pc-msys/13.3.0/include/c++/x86_64-pc-msys/bits/c++config.h-
/usr/lib/gcc/x86_64-pc-msys/13.3.0/include/c++/x86_64-pc-msys/bits/c++config.h:/* Define to 1 if you have the `_aligned_malloc' function. */
/usr/lib/gcc/x86_64-pc-msys/13.3.0/include/c++/x86_64-pc-msys/bits/c++config.h-/* #undef _GLIBCXX_HAVE__ALIGNED_MALLOC */
/usr/lib/gcc/x86_64-pc-msys/13.3.0/include/c++/x86_64-pc-msys/bits/c++config.h-
--
/usr/share/aclocal/ax_check_page_aligned_malloc.m4-# =================================================================================
/usr/share/aclocal/ax_check_page_aligned_malloc.m4:#  https://www.gnu.org/software/autoconf-archive/ax_check_page_aligned_malloc.html
/usr/share/aclocal/ax_check_page_aligned_malloc.m4-# =================================================================================
/usr/share/aclocal/ax_check_page_aligned_malloc.m4-#

GCC has some _WIN32 checks with special __CYGWIN__ condition, but I think these code paths (except fs_path.h) are not that relevant:

$ grep -R _WIN32 /usr/lib/gcc
/usr/lib/gcc/x86_64-pc-msys/13.3.0/include/c++/bits/fs_path.h:#if defined(_WIN32) && !defined(__CYGWIN__)
/usr/lib/gcc/x86_64-pc-msys/13.3.0/include/c++/experimental/bits/fs_path.h:#if defined(_WIN32) && !defined(__CYGWIN__)
/usr/lib/gcc/x86_64-pc-msys/13.3.0/include/c++/experimental/internet:#if defined _WIN32 && __has_include(<ws2tcpip.h>)
/usr/lib/gcc/x86_64-pc-msys/13.3.0/include/c++/parallel/compatibility.h:#if !defined(_WIN32) || defined (__CYGWIN__)
/usr/lib/gcc/x86_64-pc-msys/13.3.0/include/c++/parallel/compatibility.h:#if defined (_WIN32) && !defined (__CYGWIN__)
/usr/lib/gcc/x86_64-pc-msys/13.3.0/include/c++/x86_64-pc-msys/bits/c++config.h:/* #undef _GLIBCXX_USE_WIN32_SLEEP */
/usr/lib/gcc/x86_64-pc-msys/13.3.0/include/c++/x86_64-pc-msys/bits/os_defines.h:#define _GLIBCXX_THREAD_ATEXIT_WIN32 1

It would be possible to apply similar && !defined(__CYGWIN__) here, but I don't know how many other places would hit the same problem. While using Win32 APIs is sometimes necessary, I think defining _WIN32 is a red flag, although I don't have that much experience with Cygwin.

I presume that the removed codepaths did serve some purpose at some point initially. Can we dig up that purpose from the git history and recheck whether it's needed/relevant still at this point.

It was introduced by aa63fdf

I'd expect Cygwin API missing the required bits back then, these days it can use the same code as Linux in this file.
@jeremyd2019 pointed that out in: #134494 (comment)

While building may be broken right now, the individual issue of missing _aligned_malloc() probably is fixable as well, so I'd like to know a bit more about the reasoning for why this cygwin specific codepath should be removed.

There is another error for _aligned_free but I didn't include it to shorten the message.

[mstorsjo]

If I grep through /usr for _aligned_malloc there are no matches for GCC and Clang guards it based on _WIN32:

$ grep -R _aligned_malloc /usr -C2
/usr/lib/clang/20/include/mm_malloc.h-  void *__mallocedMemory;
/usr/lib/clang/20/include/mm_malloc.h-#if defined(__MINGW32__)
/usr/lib/clang/20/include/mm_malloc.h:  __mallocedMemory = __mingw_aligned_malloc(__size, __align);
/usr/lib/clang/20/include/mm_malloc.h-#elif defined(_WIN32)
/usr/lib/clang/20/include/mm_malloc.h:  __mallocedMemory = _aligned_malloc(__size, __align);
/usr/lib/clang/20/include/mm_malloc.h-#else
/usr/lib/clang/20/include/mm_malloc.h-  if (posix_memalign(&__mallocedMemory, __align, __size))
--
/usr/lib/gcc/x86_64-pc-msys/13.3.0/include/c++/x86_64-pc-msys/bits/c++config.h-/* #undef _GLIBCXX_HAVE__ACOSL */
/usr/lib/gcc/x86_64-pc-msys/13.3.0/include/c++/x86_64-pc-msys/bits/c++config.h-
/usr/lib/gcc/x86_64-pc-msys/13.3.0/include/c++/x86_64-pc-msys/bits/c++config.h:/* Define to 1 if you have the `_aligned_malloc' function. */
/usr/lib/gcc/x86_64-pc-msys/13.3.0/include/c++/x86_64-pc-msys/bits/c++config.h-/* #undef _GLIBCXX_HAVE__ALIGNED_MALLOC */
/usr/lib/gcc/x86_64-pc-msys/13.3.0/include/c++/x86_64-pc-msys/bits/c++config.h-
--
/usr/share/aclocal/ax_check_page_aligned_malloc.m4-# =================================================================================
/usr/share/aclocal/ax_check_page_aligned_malloc.m4:#  https://www.gnu.org/software/autoconf-archive/ax_check_page_aligned_malloc.html
/usr/share/aclocal/ax_check_page_aligned_malloc.m4-# =================================================================================
/usr/share/aclocal/ax_check_page_aligned_malloc.m4-#

GCC has some _WIN32 checks with special __CYGWIN__ condition, but I think these code paths (except fs_path.h) are not that relevant:

$ grep -R _WIN32 /usr/lib/gcc
/usr/lib/gcc/x86_64-pc-msys/13.3.0/include/c++/bits/fs_path.h:#if defined(_WIN32) && !defined(__CYGWIN__)
/usr/lib/gcc/x86_64-pc-msys/13.3.0/include/c++/experimental/bits/fs_path.h:#if defined(_WIN32) && !defined(__CYGWIN__)
/usr/lib/gcc/x86_64-pc-msys/13.3.0/include/c++/experimental/internet:#if defined _WIN32 && __has_include(<ws2tcpip.h>)
/usr/lib/gcc/x86_64-pc-msys/13.3.0/include/c++/parallel/compatibility.h:#if !defined(_WIN32) || defined (__CYGWIN__)
/usr/lib/gcc/x86_64-pc-msys/13.3.0/include/c++/parallel/compatibility.h:#if defined (_WIN32) && !defined (__CYGWIN__)
/usr/lib/gcc/x86_64-pc-msys/13.3.0/include/c++/x86_64-pc-msys/bits/c++config.h:/* #undef _GLIBCXX_USE_WIN32_SLEEP */
/usr/lib/gcc/x86_64-pc-msys/13.3.0/include/c++/x86_64-pc-msys/bits/os_defines.h:#define _GLIBCXX_THREAD_ATEXIT_WIN32 1

It would be possible to apply similar && !defined(__CYGWIN__) here, but I don't know how many other places would hit the same problem.

I agree; it may be a way to go overall, but not necessarily the primary solution here (and it may end up requiring many more tweaks).

While using Win32 APIs is sometimes necessary, I think defining _WIN32 is a red flag, although I don't have that much experience with Cygwin.

Yes, I think so too. In this case here, the history seems to be a bit more tricky, as this originated as #define LLVM_ON_WIN32 1 which may have been a bit more kosher to do, than an outright #define _WIN32 1. The original case here might have been fine, when 1865df4 got rid of LLVM_ON_WIN32 we ended up with a more problematic case here.

I presume that the removed codepaths did serve some purpose at some point initially. Can we dig up that purpose from the git history and recheck whether it's needed/relevant still at this point.

It was introduced by aa63fdf

I'd expect Cygwin API missing the required bits back then, these days it can use the same code as Linux in this file. @jeremyd2019 pointed that out in: #134494 (comment)

Ah, I hadn't read the earlier discussions of these patches (or I have read and promptly forgot), now I understand this a bit better. So yeah, I would also first have taken the direction of just removing the #define _WIN32.

Anyway, the comments from @jeremyd2019 there seal the case here I think. So if you update the PR description (i.e. future commit message) explaining why this no longer is necessary, I think this PR is good to go!

[jeremyd2019]

dladdr was added to cygwin in 2017, according to the tags in version 2.8.0.

[mstorsjo]

I updated the PR description with the details I wanted to have included in the commit message.

[mati865]

Looks good, thanks.