[libcxx][modules] Fix missing includes for windows

[matts1]

Previously, I was getting the following error when attempting to compile libc++ on windows with modules enabled.

While building module 'std':
In file included from <module-includes>:1:
In file included from gen/third_party/libc++/src/include/algorithm:1865:
In file included from gen/third_party/libc++/src/include/__algorithm/inplace_merge.h:28:
In file included from gen/third_party/libc++/src/include/__memory/unique_temporary_buffer.h:17:
In file included from gen/third_party/libc++/src/include/__memory/allocator.h:19:
gen/third_party/libc++/src/include/__new/allocate.h(40,73): error: 
      declaration of 'align_val_t' must be imported from module
      'sys_stage1.sysroot_vcruntime_new_h' before it is required
   40 |     return static_cast<_Tp*>(__builtin_operator_new(__size, static_cast<align_val...
      |                                                                         ^
../../third_party/depot_tools/win_toolchain/vs_files/e4305f407e/VC/Tools/MSVC/14.44.35207/include/vcruntime_new.h(27,33): note: 
      declaration here is not visible
   27 |     _VCRT_EXPORT_STD enum class align_val_t : size_t {};
      |                                 ^

[llvmbot]

@llvm/pr-subscribers-clang-modules

@llvm/pr-subscribers-libcxx

Author: Matt (matts1)

Changes

Previously, I was getting the following error when attempting to compile libc++ on windows with modules enabled.

While building module 'std':
In file included from <module-includes>:1:
In file included from gen/third_party/libc++/src/include/algorithm:1865:
In file included from gen/third_party/libc++/src/include/__algorithm/inplace_merge.h:28:
In file included from gen/third_party/libc++/src/include/__memory/unique_temporary_buffer.h:17:
In file included from gen/third_party/libc++/src/include/__memory/allocator.h:19:
gen/third_party/libc++/src/include/__new/allocate.h(40,73): error: 
      declaration of 'align_val_t' must be imported from module
      'sys_stage1.sysroot_vcruntime_new_h' before it is required
   40 |     return static_cast<_Tp*>(__builtin_operator_new(__size, static_cast<align_val...
      |                                                                         ^
../../third_party/depot_tools/win_toolchain/vs_files/e4305f407e/VC/Tools/MSVC/14.44.35207/include/vcruntime_new.h(27,33): note: 
      declaration here is not visible
   27 |     _VCRT_EXPORT_STD enum class align_val_t : size_t {};
      |                                 ^

---

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

1 Files Affected:

• (modified) libcxx/include/__new/align_val_t.h (+7-2)

diff --git a/libcxx/include/__new/align_val_t.h b/libcxx/include/__new/align_val_t.h
index 03ab7cb143a2b..36fc9f3365062 100644
--- a/libcxx/include/__new/align_val_t.h
+++ b/libcxx/include/__new/align_val_t.h
@@ -16,14 +16,19 @@
 #  pragma GCC system_header
 #endif
 
+#if _LIBCPP_HAS_LIBRARY_ALIGNED_ALLOCATION
+#if defined(_LIBCPP_ABI_VCRUNTIME)
+// std::align_val_t is defined in vcruntime_new.h
+#include <vcruntime_new.h>
+#else
 _LIBCPP_BEGIN_UNVERSIONED_NAMESPACE_STD
-#if _LIBCPP_HAS_LIBRARY_ALIGNED_ALLOCATION && !defined(_LIBCPP_ABI_VCRUNTIME)
 #  ifndef _LIBCPP_CXX03_LANG
 enum class align_val_t : size_t {};
 #  else
 enum align_val_t { __zero = 0, __max = (size_t)-1 };
 #  endif
-#endif
 _LIBCPP_END_UNVERSIONED_NAMESPACE_STD
+#endif
+#endif
 
 #endif // _LIBCPP___NEW_ALIGN_VAL_T_H

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[frederick-vs-ja]

#150182 is related. Do you have any idea why did the error only happen with modules? Can we add a test case or unblock an existing one?

[matts1]

I don't know for sure, but my suspicion would be that with a non-modules build, __memory/allocator.h (or something that includes it) includes a file which includes vcruntime_new.h transitively (eg. vcruntime_exception.h).

Adding a testcase is probably not feasible because last I checked, libc++ wasn't set up to compile the whole of std as a clang module, since compiling std as a clang module requires the underlying platform to be a clang module.

It only works for me because I've gone to the effort of generating modulemaps for the target platform for all the sysroots we use in chromium.

[philnik777]

@matts1 Is there any reason not to have these modulemaps upstream? I assume you'd want to maintain them in the future and having them upstream would make it significantly less likely that we regress that configuration.

[matts1]

Unfortunately, that's more complex than I'd like. Modulemaps aren't specific to a platform, so much as they're specific to a (platform, sysroot version) pair. This works for us because we only support one version of a sysroot at a time, and the sysroot version is stored in version control, so we just update the sysroot version and the modulemaps at the same time, and we have a script to generate the updated modulemap.

The real solution here is to have have the sysroots provide their own modulemap files similar to how libc++ provides a module.modulemap. Apple already does this and provides modulemap files for their sysroot, but they're currently the only platform to do so.

We could, of course, do something similar in libc++, but there's a lot of obstacles:

• My generation script for modulemaps + BUILD.gn files is designed to work specifically for chromium. It's definitely feasible, but certainly not trivial, to adjust this for other platforms.
• Manual maintainence of a simple modulemap file would probably be my recommended solution, but it would probably be flaky
• Getting some tests to work in CI might not be as difficult, but it'd be a nightmare to get it to work locally on a dev's machine, since it would be highly dependent on the version of the OS that you have installed.

[matts1]

FWIW, I'm certainly not opposed to adding sysroot modulemaps, but it's something that I can't personally contribute development time to (though I could contribute my expertise and reviews).

In the current state this is blocking us from being able to use modules on windows, so I'd really like to submit this without an associated test, since adding a test for it would take setting up a very large amount of infra.

[zmodem]

The change itself looks good to me.

Can we get a +1 from a libc++ maintainer?

[zmodem]

@ldionne ping?