mstorsjo wrote: > 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 1865df49960e34cc90d0083b0e0cd4771c0feb35 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](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: > [#134494 > (comment)](https://github.com/llvm/llvm-project/pull/134494#discussion_r2035957416) 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! https://github.com/llvm/llvm-project/pull/138120 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
