[clang] [Clang][Cygwin] Remove erroneous _WIN32 define and clean up Cygwin code (PR #138120)

Thu May 1 06:44:50 PDT 2025

mati865 wrote:

> 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 https://github.com/llvm/llvm-project/commit/aa63fdf3b57ab5701e951b4bf20a9dca3d8da419

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: https://github.com/llvm/llvm-project/pull/134494#discussion_r2035957416

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

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