[libc] Add osutils for Windows and make libc and its tests build on Windows target #104676

Lancern · 2024-08-17T16:38:39Z

This PR first adds osutils for Windows, and changes some libc code to make libc and its tests build on the Windows target. It then temporarily disables some libc tests that are currently problematic on Windows.

Specifically, the changes besides the addition of osutils include:

Macro LIBC_TYPES_HAS_FLOAT16 is disabled on Windows. clang-cl generates calls to functions in compiler-rt to handle float16 arithmetic and these functions are currently not linked in on Windows.
Macro LIBC_TYPES_HAS_INT128 is disabled on Windows.
The invocation to ::aligned_malloc is changed to an invocation to ::_aligned_malloc.
The following unit tests are temporarily disabled because they currently fail on Windows:
- test.src.__support.big_int_test
- test.src.__support.arg_list_test
- test.src.fenv.getenv_and_setenv_test
- Tests involving __m128i, __m256i, and __m512i in test.src.string.memory_utils.op_tests.cpp
- test_range_errors in libc/test/src/math/smoke/AddTest.h and libc/test/src/math/smoke/SubTest.h

llvmbot · 2024-08-17T16:39:08Z

@llvm/pr-subscribers-libc

Author: Sirui Mu (Lancern)

Changes

This PR first adds osutils for Windows, and changes some libc code to make libc and its tests build on the Windows target. It then temporarily disables some libc tests that are currently problematic on Windows.

Specifically, the changes besides the addition of osutils include:

Macro LIBC_TYPES_HAS_FLOAT16 is disabled on Windows. clang-cl generates calls to functions in compiler-rt to handle float16 arithmetic and these functions are currently not linked in on Windows.
Macro LIBC_TYPES_HAS_INT128 is disabled on Windows.
The invocation to ::aligned_malloc is changed to an invocation to ::_aligned_malloc.
The following unit tests are temporarily disabled because they currently fail on Windows:
- test.src.__support.big_int_test
- test.src.__support.arg_list_test
- test.src.fenv.getenv_and_setenv_test
- Tests involving __m128i, __m256i, and __m512i in test.src.string.memory_utils.op_tests.cpp
- test_range_errors in libc/test/src/math/smoke/AddTest.h and libc/test/src/math/smoke/SubTest.h

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

17 Files Affected:

(modified) libc/include/llvm-libc-macros/float16-macros.h (+2-1)
(modified) libc/include/llvm-libc-macros/stdckdint-macros.h (+4-2)
(modified) libc/src/__support/CPP/new.cpp (+15-1)
(modified) libc/src/__support/CPP/new.h (+8)
(modified) libc/src/__support/OSUtil/io.h (+2)
(added) libc/src/__support/OSUtil/windows/CMakeLists.txt (+9)
(added) libc/src/__support/OSUtil/windows/exit.cpp (+21)
(added) libc/src/__support/OSUtil/windows/io.cpp (+21)
(added) libc/src/__support/OSUtil/windows/io.h (+21)
(modified) libc/src/__support/macros/properties/types.h (+1-1)
(modified) libc/test/src/__support/CMakeLists.txt (+1-1)
(modified) libc/test/src/__support/FPUtil/fpbits_test.cpp (+2-3)
(modified) libc/test/src/__support/arg_list_test.cpp (+1-1)
(modified) libc/test/src/fenv/getenv_and_setenv_test.cpp (+2)
(modified) libc/test/src/math/smoke/AddTest.h (+2)
(modified) libc/test/src/math/smoke/SubTest.h (+2)
(modified) libc/test/src/string/memory_utils/op_tests.cpp (+1-1)

diff --git a/libc/include/llvm-libc-macros/float16-macros.h b/libc/include/llvm-libc-macros/float16-macros.h
index 9a11ecc49307e2..229e3e62f2aedf 100644
--- a/libc/include/llvm-libc-macros/float16-macros.h
+++ b/libc/include/llvm-libc-macros/float16-macros.h
@@ -13,7 +13,8 @@
 
 #if defined(__FLT16_MANT_DIG__) &&                                             \
     (!defined(__GNUC__) || __GNUC__ >= 13 || defined(__clang__)) &&            \
-    !defined(__arm__) && !defined(_M_ARM) && !defined(__riscv)
+    !defined(__arm__) && !defined(_M_ARM) && !defined(__riscv) &&              \
+    !defined(_WIN32)
 #define LIBC_TYPES_HAS_FLOAT16
 
 // TODO: This would no longer be required if HdrGen let us guard function
diff --git a/libc/include/llvm-libc-macros/stdckdint-macros.h b/libc/include/llvm-libc-macros/stdckdint-macros.h
index 694412290bbca0..17e4ccdc2d5f8e 100644
--- a/libc/include/llvm-libc-macros/stdckdint-macros.h
+++ b/libc/include/llvm-libc-macros/stdckdint-macros.h
@@ -10,8 +10,10 @@
 #define LLVM_LIBC_MACROS_STDCKDINT_MACROS_H
 
 // We need to use __builtin_*_overflow from GCC/Clang to implement the overflow
-// macros. Check __GNUC__ for availability of such builtins.
-#ifdef __GNUC__
+// macros. Check __GNUC__ or __clang__ for availability of such builtins.
+// Note that clang-cl defines __clang__ only and does not define __GNUC__ so we
+// have to check for both.
+#if defined(__GNUC__) || defined(__clang__)
 // clang/gcc overlay may provides similar macros, we need to avoid redefining
 // them.
 #ifndef __STDC_VERSION_STDCKDINT_H__
diff --git a/libc/src/__support/CPP/new.cpp b/libc/src/__support/CPP/new.cpp
index 5a40d4a6d3b272..7792a53453c3e3 100644
--- a/libc/src/__support/CPP/new.cpp
+++ b/libc/src/__support/CPP/new.cpp
@@ -16,15 +16,29 @@ void operator delete(void *mem, std::align_val_t) noexcept { ::free(mem); }
 void operator delete(void *mem, size_t) noexcept { ::free(mem); }
 
 void operator delete(void *mem, size_t, std::align_val_t) noexcept {
+#ifdef _WIN32
+  ::_aligned_free(mem);
+#else
   ::free(mem);
+#endif
 }
 
 void operator delete[](void *mem) noexcept { ::free(mem); }
 
-void operator delete[](void *mem, std::align_val_t) noexcept { ::free(mem); }
+void operator delete[](void *mem, std::align_val_t) noexcept {
+#ifdef _WIN32
+  ::_aligned_free(mem);
+#else
+  ::free(mem);
+#endif
+}
 
 void operator delete[](void *mem, size_t) noexcept { ::free(mem); }
 
 void operator delete[](void *mem, size_t, std::align_val_t) noexcept {
+#ifdef _WIN32
+  ::_aligned_free(mem);
+#else
   ::free(mem);
+#endif
 }
diff --git a/libc/src/__support/CPP/new.h b/libc/src/__support/CPP/new.h
index 94a8466a39677b..9bea833665d207 100644
--- a/libc/src/__support/CPP/new.h
+++ b/libc/src/__support/CPP/new.h
@@ -47,7 +47,15 @@ class AllocChecker {
 
   LIBC_INLINE static void *aligned_alloc(size_t s, std::align_val_t align,
                                          AllocChecker &ac) {
+#ifdef _WIN32
+    // std::aligned_alloc is not available on Windows because std::free on
+    // Windows cannot deallocate any over-aligned memory. Microsoft provides an
+    // alternative for std::aligned_alloc named _aligned_malloc, but it must be
+    // paired with _aligned_free instead of std::free.
+    void *mem = ::_aligned_malloc(static_cast<size_t>(align), s);
+#else
     void *mem = ::aligned_alloc(static_cast<size_t>(align), s);
+#endif
     ac = (mem != nullptr);
     return mem;
   }
diff --git a/libc/src/__support/OSUtil/io.h b/libc/src/__support/OSUtil/io.h
index cb7e748fc64426..80119da77fc027 100644
--- a/libc/src/__support/OSUtil/io.h
+++ b/libc/src/__support/OSUtil/io.h
@@ -19,6 +19,8 @@
 #include "linux/io.h"
 #elif defined(__Fuchsia__)
 #include "fuchsia/io.h"
+#elif defined(_WIN32)
+#include "windows/io.h"
 #elif defined(__ELF__)
 // TODO: Ideally we would have LIBC_TARGET_OS_IS_BAREMETAL.
 #include "baremetal/io.h"
diff --git a/libc/src/__support/OSUtil/windows/CMakeLists.txt b/libc/src/__support/OSUtil/windows/CMakeLists.txt
new file mode 100644
index 00000000000000..9ae4535d4aaebd
--- /dev/null
+++ b/libc/src/__support/OSUtil/windows/CMakeLists.txt
@@ -0,0 +1,9 @@
+add_object_library(
+  windows_util
+  SRCS
+    exit.cpp
+    io.cpp
+  HDRS
+    io.h
+  DEPENDS
+)
diff --git a/libc/src/__support/OSUtil/windows/exit.cpp b/libc/src/__support/OSUtil/windows/exit.cpp
new file mode 100644
index 00000000000000..07b776ce1cea2f
--- /dev/null
+++ b/libc/src/__support/OSUtil/windows/exit.cpp
@@ -0,0 +1,21 @@
+//===----------- Windows implementation of an exit function -----*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "src/__support/macros/config.h"
+
+#include <Windows.h>
+
+namespace LIBC_NAMESPACE_DECL {
+namespace internal {
+
+__attribute__((noreturn)) void exit(int status) {
+  ExitProcess(status);
+}
+
+} // namespace internal
+} // namespace LIBC_NAMESPACE_DECL
diff --git a/libc/src/__support/OSUtil/windows/io.cpp b/libc/src/__support/OSUtil/windows/io.cpp
new file mode 100644
index 00000000000000..c71f296f61734c
--- /dev/null
+++ b/libc/src/__support/OSUtil/windows/io.cpp
@@ -0,0 +1,21 @@
+//===------------- Windows implementation of IO utils -----------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "src/__support/macros/config.h"
+#include "io.h"
+
+#include <Windows.h>
+
+namespace LIBC_NAMESPACE_DECL {
+
+void write_to_stderr(cpp::string_view msg) {
+  HANDLE stream = GetStdHandle(STD_ERROR_HANDLE);
+  WriteFile(stream, msg.data(), msg.size(), nullptr, nullptr);
+}
+
+} // namespace LIBC_NAMESPACE_DECL
\ No newline at end of file
diff --git a/libc/src/__support/OSUtil/windows/io.h b/libc/src/__support/OSUtil/windows/io.h
new file mode 100644
index 00000000000000..bafc00254a7cff
--- /dev/null
+++ b/libc/src/__support/OSUtil/windows/io.h
@@ -0,0 +1,21 @@
+//===------------- Windows implementation of IO utils -----------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_LIBC_SRC___SUPPORT_OSUTIL_WINDOWS_IO_H
+#define LLVM_LIBC_SRC___SUPPORT_OSUTIL_WINDOWS_IO_H
+
+#include "src/__support/CPP/string_view.h"
+#include "src/__support/macros/config.h"
+
+namespace LIBC_NAMESPACE_DECL {
+
+void write_to_stderr(cpp::string_view msg);
+
+} // namespace LIBC_NAMESPACE_DECL
+
+#endif // LLVM_LIBC_SRC___SUPPORT_OSUTIL_WINDOWS_IO_H
diff --git a/libc/src/__support/macros/properties/types.h b/libc/src/__support/macros/properties/types.h
index 69ddc912238e74..7d57300116e1bf 100644
--- a/libc/src/__support/macros/properties/types.h
+++ b/libc/src/__support/macros/properties/types.h
@@ -35,7 +35,7 @@
 #endif // UINT64_MAX
 
 // int128 / uint128 support
-#if defined(__SIZEOF_INT128__)
+#if defined(__SIZEOF_INT128__) && !defined(_WIN32)
 #define LIBC_TYPES_HAS_INT128
 #endif // defined(__SIZEOF_INT128__)
 
diff --git a/libc/test/src/__support/CMakeLists.txt b/libc/test/src/__support/CMakeLists.txt
index 90de520405981b..b739779e3878cc 100644
--- a/libc/test/src/__support/CMakeLists.txt
+++ b/libc/test/src/__support/CMakeLists.txt
@@ -142,7 +142,7 @@ add_libc_test(
     libc.src.__support.arg_list
 )
 
-if(NOT LIBC_TARGET_ARCHITECTURE_IS_NVPTX)
+if(NOT LIBC_TARGET_ARCHITECTURE_IS_NVPTX AND NOT LIBC_TARGET_OS_IS_WINDOWS)
   add_libc_test(
     big_int_test
     SUITE
diff --git a/libc/test/src/__support/FPUtil/fpbits_test.cpp b/libc/test/src/__support/FPUtil/fpbits_test.cpp
index 99acc03010344f..c0dc5420f6c110 100644
--- a/libc/test/src/__support/FPUtil/fpbits_test.cpp
+++ b/libc/test/src/__support/FPUtil/fpbits_test.cpp
@@ -427,11 +427,9 @@ TEST(LlvmLibcFPBitsTest, DoubleType) {
 
 #ifdef LIBC_TARGET_ARCH_IS_X86
 TEST(LlvmLibcFPBitsTest, X86LongDoubleType) {
+#ifndef LIBC_TYPES_LONG_DOUBLE_IS_FLOAT64
   using LongDoubleBits = FPBits<long double>;
 
-  if constexpr (sizeof(long double) == sizeof(double))
-    return; // The tests for the "double" type cover for this case.
-
   EXPECT_STREQ(LIBC_NAMESPACE::str(LongDoubleBits::inf(Sign::POS)).c_str(),
                "(+Infinity)");
   EXPECT_STREQ(LIBC_NAMESPACE::str(LongDoubleBits::inf(Sign::NEG)).c_str(),
@@ -501,6 +499,7 @@ TEST(LlvmLibcFPBitsTest, X86LongDoubleType) {
 
   LongDoubleBits quiet_nan = LongDoubleBits::quiet_nan();
   EXPECT_EQ(quiet_nan.is_quiet_nan(), true);
+#endif
 }
 #else
 TEST(LlvmLibcFPBitsTest, LongDoubleType) {
diff --git a/libc/test/src/__support/arg_list_test.cpp b/libc/test/src/__support/arg_list_test.cpp
index 4f229e2bfe6940..8a5f581e8586b5 100644
--- a/libc/test/src/__support/arg_list_test.cpp
+++ b/libc/test/src/__support/arg_list_test.cpp
@@ -120,7 +120,7 @@ TEST(LlvmLibcArgListTest, TestStructTypes) {
 }
 
 // Test vector extensions from clang.
-#if __has_attribute(ext_vector_type)
+#if !defined(_WIN32) && __has_attribute(ext_vector_type)
 
 using int1 = int __attribute__((ext_vector_type(1)));
 using int2 = int __attribute__((ext_vector_type(2)));
diff --git a/libc/test/src/fenv/getenv_and_setenv_test.cpp b/libc/test/src/fenv/getenv_and_setenv_test.cpp
index 8fc2787ecb5b1e..d85fe1a393f491 100644
--- a/libc/test/src/fenv/getenv_and_setenv_test.cpp
+++ b/libc/test/src/fenv/getenv_and_setenv_test.cpp
@@ -20,6 +20,7 @@
 
 using LlvmLibcFEnvTest = LIBC_NAMESPACE::testing::FEnvSafeTest;
 
+#ifndef _WIN32
 TEST_F(LlvmLibcFEnvTest, GetEnvAndSetEnv) {
   // We will disable all exceptions to prevent invocation of the exception
   // handler.
@@ -71,6 +72,7 @@ TEST_F(LlvmLibcFEnvTest, Set_FE_DFL_ENV) {
   int rm = LIBC_NAMESPACE::fegetround();
   EXPECT_EQ(rm, FE_TONEAREST);
 }
+#endif
 
 #ifdef _WIN32
 TEST_F(LlvmLibcFEnvTest, Windows_Set_Get_Test) {
diff --git a/libc/test/src/math/smoke/AddTest.h b/libc/test/src/math/smoke/AddTest.h
index 0b7e395a22d4cd..1516cb5aa1e7cf 100644
--- a/libc/test/src/math/smoke/AddTest.h
+++ b/libc/test/src/math/smoke/AddTest.h
@@ -53,6 +53,7 @@ class AddTest : public LIBC_NAMESPACE::testing::FEnvSafeTest {
   }
 
   void test_range_errors(AddFunc func) {
+#ifndef _WIN32
     using namespace LIBC_NAMESPACE::fputil::testing;
 
     if (ForceRoundingMode r(RoundingMode::Nearest); r.success) {
@@ -121,6 +122,7 @@ class AddTest : public LIBC_NAMESPACE::testing::FEnvSafeTest {
           FE_UNDERFLOW | FE_INEXACT);
       EXPECT_MATH_ERRNO(ERANGE);
     }
+#endif
   }
 
   void test_inexact_results(AddFunc func) {
diff --git a/libc/test/src/math/smoke/SubTest.h b/libc/test/src/math/smoke/SubTest.h
index 9ee4220b382085..c0581a07cfeeef 100644
--- a/libc/test/src/math/smoke/SubTest.h
+++ b/libc/test/src/math/smoke/SubTest.h
@@ -52,6 +52,7 @@ class SubTest : public LIBC_NAMESPACE::testing::FEnvSafeTest {
   }
 
   void test_range_errors(SubFunc func) {
+#ifndef _WIN32
     using namespace LIBC_NAMESPACE::fputil::testing;
 
     if (ForceRoundingMode r(RoundingMode::Nearest); r.success) {
@@ -123,6 +124,7 @@ class SubTest : public LIBC_NAMESPACE::testing::FEnvSafeTest {
                                   FE_UNDERFLOW | FE_INEXACT);
       EXPECT_MATH_ERRNO(ERANGE);
     }
+#endif
   }
 
   void test_inexact_results(SubFunc func) {
diff --git a/libc/test/src/string/memory_utils/op_tests.cpp b/libc/test/src/string/memory_utils/op_tests.cpp
index 978561f31a2961..5e6f6d87b26e0c 100644
--- a/libc/test/src/string/memory_utils/op_tests.cpp
+++ b/libc/test/src/string/memory_utils/op_tests.cpp
@@ -294,7 +294,7 @@ TYPED_TEST(LlvmLibcOpTest, Bcmp, BcmpImplementations) {
 #endif // LIBC_TARGET_ARCH_IS_X86_64
 
 using MemcmpImplementations = testing::TypeList<
-#ifdef LIBC_TARGET_ARCH_IS_X86_64
+#if defined(LIBC_TARGET_ARCH_IS_X86_64) && !defined(LIBC_TARGET_OS_IS_WINDOWS)
 #ifdef __SSE2__
     generic::Memcmp<__m128i>, //
 #endif

github-actions · 2024-08-17T16:42:11Z

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

jhuber6 · 2024-08-17T17:13:12Z

libc/src/__support/OSUtil/windows/exit.cpp

+namespace LIBC_NAMESPACE_DECL {
+namespace internal {
+
+__attribute__((noreturn)) void exit(int status) { ExitProcess(status); }


Suggested change

__attribute__((noreturn)) void exit(int status) { ExitProcess(status); }

[[noreturn]] void exit(int status) { ExitProcess(status); }

jhuber6 · 2024-08-17T17:13:20Z

libc/src/__support/OSUtil/windows/io.cpp

+  WriteFile(stream, msg.data(), msg.size(), nullptr, nullptr);
+}
+
+} // namespace LIBC_NAMESPACE_DECL


Suggested change

} // namespace LIBC_NAMESPACE_DECL

} // namespace LIBC_NAMESPACE_DECL

jhuber6 · 2024-08-17T17:14:20Z

libc/test/src/fenv/getenv_and_setenv_test.cpp

@@ -20,6 +20,7 @@

 using LlvmLibcFEnvTest = LIBC_NAMESPACE::testing::FEnvSafeTest;

+#ifndef _WIN32


We should probably have a macro like LIBC_TARGET_OS_IS_WINDOWS and use that.

We already have LIBC_TARGET_OS_IS_WINDOWS at https://github.com/llvm/llvm-project/blob/main/libc/src/__support/macros/properties/os.h#L25
@Lancern Do you mind updating the OS checks using that macro, and targets' dependency accordingly?

SchrodingerZhu · 2024-08-18T00:14:49Z

hmmm, are memcpy/memset routines actually failing? that sounds concerning.

lntue · 2024-08-18T05:25:27Z

Tests involving __m128i, __m256i, and __m512i in test.src.string.memory_utils.op_tests.cpp

What errors did you get for these? One possibility is that we will need to include <x86_intrin.h>, and pass proper compiler flags /arch:AVX, /arch:AVX2, /arch:AVX512 for Windows. But I think it's ok to create an issue to re-enable these optimizations later.

Lancern · 2024-08-18T12:25:39Z

hmmm, are memcpy/memset routines actually failing? that sounds concerning.

What errors did you get for these? One possibility is that we will need to include <x86_intrin.h>, and pass proper compiler flags /arch:AVX, /arch:AVX2, /arch:AVX512 for Windows.

The problem with the memcpy / memset / __m128i / __m256i / __m512i tests are still not fully investigated. I'll investigate further into them later. Resolving these problems in this PR may make this PR hard to review so I does not resolve them for now.

SchrodingerZhu · 2024-08-18T14:16:30Z

libc/include/llvm-libc-macros/float16-macros.h

@@ -13,7 +13,8 @@

 #if defined(__FLT16_MANT_DIG__) &&                                             \
    (!defined(__GNUC__) || __GNUC__ >= 13 || defined(__clang__)) &&            \
-    !defined(__arm__) && !defined(_M_ARM) && !defined(__riscv)
+    !defined(__arm__) && !defined(_M_ARM) && !defined(__riscv) &&              \


notice that msvc arm64 define __ARM_ARCH rather than these arm macros. It is not a concern for this patch but may cause problem later on

AArch64 is the CPU target with the best float16 support. Its equivalent of the x86-64 F16C extension is part of the base Armv8-A ISA and mandatory, so we never had issues with compiler runtime soft-float conversion functions on AArch64, since they're not used there. We only had issues on 32-bit Arm, and MSVC defines _M_ARM when targeting 32-bit Arm according to https://learn.microsoft.com/en-us/cpp/preprocessor/predefined-macros?view=msvc-170.

SchrodingerZhu

LGTM with some nits

SchrodingerZhu · 2024-08-18T14:19:10Z

libc/src/__support/OSUtil/windows/exit.cpp

+
+#include "src/__support/macros/config.h"
+
+#include <Windows.h>


would it be better to surround it with

#pragma push_macro(“WIN32_LEAN_AND_MEAN”) #define WIN32_LEAN_AND_MEAN #pragma pop_macro(...)

Is the push_macro and pop_macro pragmas necessary? I added WIN32_LEAN_AND_MEAN only.

I think it is fine. This is a .cpp file so adding WIN32_LEAN_AND_MEAN is sufficient.

lntue · 2024-08-19T00:46:05Z

libc/src/__support/OSUtil/windows/CMakeLists.txt

+    io.cpp
+  HDRS
+    io.h
+  DEPENDS


you can add libc.src.__support.macros.config to avoid empty DEPENDS.

lntue · 2024-08-19T00:54:24Z

libc/test/src/__support/FPUtil/fpbits_test.cpp

@@ -427,11 +427,9 @@ TEST(LlvmLibcFPBitsTest, DoubleType) {

 #ifdef LIBC_TARGET_ARCH_IS_X86
 TEST(LlvmLibcFPBitsTest, X86LongDoubleType) {
+#ifndef LIBC_TYPES_LONG_DOUBLE_IS_FLOAT64


Can you replace the above condition LIBC_TARGET_ARCH_IS_X86 with LIBC_TYPES_LONG_DOUBLE_IS_X86_FLOAT80 instead.

lntue

Thanks! The PR LGTM with a few nits.

SchrodingerZhu · 2024-08-19T15:27:29Z

libc/src/__support/OSUtil/windows/io.cpp

+#include "io.h"
+#include "src/__support/macros/config.h"
+
+#include <Windows.h>


Suggested change

#include <Windows.h>

#define WIN32_LEAN_AND_MEAN

#include <Windows.h>

overmighty · 2024-08-19T16:13:50Z

libc/src/__support/CPP/new.h

@@ -11,6 +11,7 @@

 #include "src/__support/common.h"
 #include "src/__support/macros/config.h"
+#include "src/__support/macros/properties/os.h"


Should update dependencies in libc/src/__support/CPP/CMakeLists.txt.

overmighty · 2024-08-19T16:19:34Z

libc/src/__support/CPP/new.cpp

@@ -16,15 +16,29 @@ void operator delete(void *mem, std::align_val_t) noexcept { ::free(mem); }
 void operator delete(void *mem, size_t) noexcept { ::free(mem); }

 void operator delete(void *mem, size_t, std::align_val_t) noexcept {
+#ifdef LIBC_TARGET_OS_IS_WINDOWS


I guess relying on transitive inclusion of src/__support/macros/properties/os.h is fine in this case.

overmighty · 2024-08-19T16:21:54Z

libc/src/__support/OSUtil/windows/exit.cpp

@@ -0,0 +1,20 @@
+//===----------- Windows implementation of an exit function -----*- C++ -*-===//


Nit: should be:

Suggested change

//===----------- Windows implementation of an exit function -----*- C++ -*-===//

//===-- Windows implementation of an exit function ------------------------===//

but there are existing files in OSUtil that don't conform to the standard file header either.

See https://llvm.org/docs/CodingStandards.html#file-headers.

overmighty · 2024-08-19T16:29:25Z

libc/src/__support/OSUtil/windows/io.cpp

+  HANDLE stream = GetStdHandle(STD_ERROR_HANDLE);
+  WriteFile(stream, msg.data(), msg.size(), nullptr, nullptr);


Nit: I'm not sure if it's consistent across the whole codebase, but rg ' ::\w' libc finds occurrences of :: being used for C types and symbols.

Suggested change

HANDLE stream = GetStdHandle(STD_ERROR_HANDLE);

WriteFile(stream, msg.data(), msg.size(), nullptr, nullptr);

::HANDLE stream = ::GetStdHandle(STD_ERROR_HANDLE);

::WriteFile(stream, msg.data(), msg.size(), nullptr, nullptr);

overmighty · 2024-08-19T16:34:31Z

libc/test/src/__support/FPUtil/fpbits_test.cpp

@@ -425,13 +425,10 @@ TEST(LlvmLibcFPBitsTest, DoubleType) {
  EXPECT_EQ(quiet_nan.is_quiet_nan(), true);
 }

-#ifdef LIBC_TARGET_ARCH_IS_X86
+#ifdef LIBC_TYPES_LONG_DOUBLE_IS_X86_FLOAT80


Should probably explicitly #include "libc/src/__support/macros/properties/types.h".

overmighty · 2024-08-19T16:35:05Z

libc/test/src/__support/arg_list_test.cpp

@@ -120,7 +120,7 @@ TEST(LlvmLibcArgListTest, TestStructTypes) {
 }

 // Test vector extensions from clang.
-#if __has_attribute(ext_vector_type)
+#if !defined(LIBC_TARGET_OS_IS_WINDOWS) && __has_attribute(ext_vector_type)


Should probably explicitly #include "libc/src/__support/macros/properties/os.h" in files where use of LIBC_TARGET_OS_IS_WINDOWS is being added.

michaelrj-google · 2024-08-20T17:48:50Z

libc/src/__support/OSUtil/windows/io.cpp

+#include "src/__support/macros/config.h"
+
+#define WIN32_LEAN_AND_MEAN
+#include <Windows.h>


including the Windows headers here works, but it will cause problems if we ever want to do a fully standalone build. Given how windows is set up I'm fine with saying that windows is overlay only for now, but you should add a comment explaining that decision.

It is actually hard to do the pure syscall approach with windows. They intentionally change the call ids.

In that case it sounds like we should stick to overlay mode only.

we probably should add some explicit error for Windows + full build in the cmake so that the errors people get on Windows won't be too confusing.

Yeah, I think a working C23 library without POSIX would be the best but I am not sure whether there are other complications.

we probably should add some explicit error for Windows + full build in the cmake so that the errors people get on Windows won't be too confusing.

Updated in libc/cmake/modules/LLVMLibCArchitectures.cmake and made this a hard cmake configure error.

overmighty · 2024-08-28T14:31:21Z

libc/test/src/__support/FPUtil/fpbits_test.cpp

Should update CMake target dependencies due to includes added in files in libc/test/src/. For AddTest.h and SubTest.h, you would need to update all target definitions in libc/test/src/math/smoke/CMakeLists.txt that have HDRS AddTest.h or HDRS SubTest.h

Should have updated all cmake scripts.

michaelrj-google

Looks good to land after two nits

michaelrj-google · 2024-09-03T23:38:33Z

libc/src/__support/macros/properties/types.h

@@ -35,7 +35,7 @@
 #endif // UINT64_MAX

 // int128 / uint128 support
-#if defined(__SIZEOF_INT128__)
+#if defined(__SIZEOF_INT128__) && !defined(LIBC_TARGET_OS_IS_WINDOWS)


is this necessary? I'm surprised that windows would define sizeof for uint128 without defining the type

Well clang-cl actually supports int128 types, but it emits calls into compiler-rt functions (such as __udivti3 and __umodti3) to handle int128 arithmetics, which are not available in libc currently on Windows. So we disable this for now.

libc/test/src/__support/CMakeLists.txt

This PR first adds osutils for Windows, and changes some libc code to make libc and its tests build on the Windows target. It then temporarily disables some libc tests that are currently problematic on Windows.

michaelrj-google · 2024-09-11T22:32:39Z

Do you need me to merge this for you?

Lancern · 2024-09-12T01:12:04Z

Do you need me to merge this for you?

Yes I don't have commit access.

SchrodingerZhu · 2024-09-12T03:41:28Z

merge according to previous discussion

llvmbot added the libc label Aug 17, 2024

Lancern force-pushed the libc-win-osutils branch from 8f550ff to 5bdf738 Compare August 17, 2024 16:47

tschuett requested a review from michaelrj-google August 17, 2024 16:50

jhuber6 reviewed Aug 17, 2024

View reviewed changes

SchrodingerZhu requested review from lntue and overmighty August 18, 2024 00:09

Lancern force-pushed the libc-win-osutils branch from 5bdf738 to 460a0b2 Compare August 18, 2024 11:43

SchrodingerZhu reviewed Aug 18, 2024

View reviewed changes

SchrodingerZhu approved these changes Aug 18, 2024

View reviewed changes

lntue reviewed Aug 19, 2024

View reviewed changes

lntue approved these changes Aug 19, 2024

View reviewed changes

Lancern force-pushed the libc-win-osutils branch from 460a0b2 to 0e29aee Compare August 19, 2024 13:05

SchrodingerZhu reviewed Aug 19, 2024

View reviewed changes

Lancern force-pushed the libc-win-osutils branch from 0e29aee to 515a4d1 Compare August 19, 2024 15:41

overmighty requested changes Aug 19, 2024

View reviewed changes

michaelrj-google reviewed Aug 20, 2024

View reviewed changes

Lancern force-pushed the libc-win-osutils branch 3 times, most recently from 1cb0cb3 to fd93d4c Compare August 22, 2024 14:20

overmighty reviewed Aug 28, 2024

View reviewed changes

Lancern force-pushed the libc-win-osutils branch from fd93d4c to efb5670 Compare August 29, 2024 14:47

overmighty approved these changes Aug 29, 2024

View reviewed changes

michaelrj-google approved these changes Sep 3, 2024

View reviewed changes

[libc] Add osutils and make libc and its tests build on Windows target

aad2d7f

This PR first adds osutils for Windows, and changes some libc code to make libc and its tests build on the Windows target. It then temporarily disables some libc tests that are currently problematic on Windows.

Lancern force-pushed the libc-win-osutils branch from efb5670 to aad2d7f Compare September 5, 2024 14:07

SchrodingerZhu merged commit ded0801 into llvm:main Sep 12, 2024
7 checks passed

Lancern deleted the libc-win-osutils branch September 12, 2024 05:05

	__attribute__((noreturn)) void exit(int status) { ExitProcess(status); }
	[[noreturn]] void exit(int status) { ExitProcess(status); }

	} // namespace LIBC_NAMESPACE_DECL
	} // namespace LIBC_NAMESPACE_DECL

		@@ -20,6 +20,7 @@

		using LlvmLibcFEnvTest = LIBC_NAMESPACE::testing::FEnvSafeTest;

		#ifndef _WIN32


		#include "src/__support/macros/config.h"

		#include <Windows.h>

	#include <Windows.h>
	#define WIN32_LEAN_AND_MEAN
	#include <Windows.h>

		@@ -0,0 +1,20 @@
		//===----------- Windows implementation of an exit function ------ C++ --===//

	//===----------- Windows implementation of an exit function ------ C++ --===//
	//===-- Windows implementation of an exit function ------------------------===//

		HANDLE stream = GetStdHandle(STD_ERROR_HANDLE);
		WriteFile(stream, msg.data(), msg.size(), nullptr, nullptr);

[libc] Add osutils for Windows and make libc and its tests build on Windows target #104676

[libc] Add osutils for Windows and make libc and its tests build on Windows target #104676

Uh oh!

Conversation

Lancern commented Aug 17, 2024

Uh oh!

llvmbot commented Aug 17, 2024

Uh oh!

github-actions bot commented Aug 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SchrodingerZhu commented Aug 18, 2024

Uh oh!

lntue commented Aug 18, 2024

Uh oh!

Lancern commented Aug 18, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SchrodingerZhu left a comment

Choose a reason for hiding this comment

Uh oh!

SchrodingerZhu Aug 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lntue left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

github-actions bot commented Aug 17, 2024 •

edited

Loading

SchrodingerZhu Aug 18, 2024 •

edited

Loading

overmighty Aug 28, 2024 •

edited

Loading