Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions libc/cmake/modules/LLVMLibCArchitectures.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -206,5 +206,12 @@ if(explicit_target_triple AND
endif()
endif()


# Windows does not support full mode build.
if (LIBC_TARGET_OS_IS_WINDOWS AND LLVM_LIBC_FULL_BUILD)
message(FATAL_ERROR "Windows does not support full mode build.")
endif ()


message(STATUS
"Building libc for ${LIBC_TARGET_ARCHITECTURE} on ${LIBC_TARGET_OS}")
3 changes: 2 additions & 1 deletion libc/include/llvm-libc-macros/float16-macros.h
Original file line number Diff line number Diff line change
Expand Up @@ -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) && \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

!defined(_WIN32)
#define LIBC_TYPES_HAS_FLOAT16

// TODO: This would no longer be required if HdrGen let us guard function
Expand Down
6 changes: 4 additions & 2 deletions libc/include/llvm-libc-macros/stdckdint-macros.h
Original file line number Diff line number Diff line change
Expand Up @@ -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__
Expand Down
1 change: 1 addition & 0 deletions libc/src/__support/CPP/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -199,4 +199,5 @@ add_object_library(
DEPENDS
libc.include.stdlib
libc.src.__support.common
libc.src.__support.macros.properties.os
)
16 changes: 15 additions & 1 deletion libc/src/__support/CPP/new.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

::_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 LIBC_TARGET_OS_IS_WINDOWS
::_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 LIBC_TARGET_OS_IS_WINDOWS
::_aligned_free(mem);
#else
::free(mem);
#endif
}
9 changes: 9 additions & 0 deletions libc/src/__support/CPP/new.h
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

#include "src/__support/common.h"
#include "src/__support/macros/config.h"
#include "src/__support/macros/properties/os.h"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.


#include <stddef.h> // For size_t
#include <stdlib.h> // For malloc, free etc.
Expand Down Expand Up @@ -47,7 +48,15 @@ class AllocChecker {

LIBC_INLINE static void *aligned_alloc(size_t s, std::align_val_t align,
AllocChecker &ac) {
#ifdef LIBC_TARGET_OS_IS_WINDOWS
// 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;
}
Expand Down
2 changes: 2 additions & 0 deletions libc/src/__support/OSUtil/io.h
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
10 changes: 10 additions & 0 deletions libc/src/__support/OSUtil/windows/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
add_object_library(
windows_util
SRCS
exit.cpp
io.cpp
HDRS
io.h
DEPENDS
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

libc.src.__support.macros.config
)
23 changes: 23 additions & 0 deletions libc/src/__support/OSUtil/windows/exit.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
//===-- Windows implementation of an exit function ------------------------===//
//
// 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"

// On Windows we cannot make direct syscalls since Microsoft changes system call
// IDs periodically. We must rely on functions exported from ntdll.dll or
// kernel32.dll to invoke system service procedures.
#define WIN32_LEAN_AND_MEAN
#include <Windows.h>
Copy link
Contributor

@SchrodingerZhu SchrodingerZhu Aug 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it be better to surround it with

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


namespace LIBC_NAMESPACE_DECL {
namespace internal {

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

} // namespace internal
} // namespace LIBC_NAMESPACE_DECL
25 changes: 25 additions & 0 deletions libc/src/__support/OSUtil/windows/io.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
//===------------- 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 "io.h"
#include "src/__support/macros/config.h"

// On Windows we cannot make direct syscalls since Microsoft changes system call
// IDs periodically. We must rely on functions exported from ntdll.dll or
// kernel32.dll to invoke system service procedures.
#define WIN32_LEAN_AND_MEAN
#include <Windows.h>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


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
21 changes: 21 additions & 0 deletions libc/src/__support/OSUtil/windows/io.h
Original file line number Diff line number Diff line change
@@ -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
2 changes: 1 addition & 1 deletion libc/src/__support/macros/properties/types.h
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
#endif // UINT64_MAX

// int128 / uint128 support
#if defined(__SIZEOF_INT128__)
#if defined(__SIZEOF_INT128__) && !defined(LIBC_TARGET_OS_IS_WINDOWS)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

#define LIBC_TYPES_HAS_INT128
#endif // defined(__SIZEOF_INT128__)

Expand Down
7 changes: 6 additions & 1 deletion libc/test/src/__support/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -140,9 +140,14 @@ add_libc_test(
arg_list_test.cpp
DEPENDS
libc.src.__support.arg_list
libc.src.__support.macros.properties.os
)

if(NOT LIBC_TARGET_ARCHITECTURE_IS_NVPTX)
# TODO: clang-cl generates calls into runtime library functions to
# handle 128-bit integer arithmetics and conversions which are not yet
# available on Windows. Re-enable 128-bit integer support on Windows once
# these functions are ready.
if(NOT LIBC_TARGET_ARCHITECTURE_IS_NVPTX AND NOT LIBC_TARGET_OS_IS_WINDOWS)
add_libc_test(
big_int_test
SUITE
Expand Down
1 change: 1 addition & 0 deletions libc/test/src/__support/FPUtil/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ add_libc_test(
libc.src.__support.FPUtil.fp_bits
libc.src.__support.FPUtil.fpbits_str
libc.src.__support.integer_literals
libc.src.__support.macros.properties.types
libc.src.__support.sign
)

Expand Down
6 changes: 2 additions & 4 deletions libc/test/src/__support/FPUtil/fpbits_test.cpp
Copy link
Member

@overmighty overmighty Aug 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should have updated all cmake scripts.

Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include "src/__support/FPUtil/FPBits.h"
#include "src/__support/FPUtil/fpbits_str.h"
#include "src/__support/integer_literals.h"
#include "src/__support/macros/properties/types.h"
#include "src/__support/sign.h" // Sign
#include "test/UnitTest/Test.h"

Expand Down Expand Up @@ -425,13 +426,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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

TEST(LlvmLibcFPBitsTest, X86LongDoubleType) {
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(),
Expand Down
3 changes: 2 additions & 1 deletion libc/test/src/__support/arg_list_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
//===----------------------------------------------------------------------===//

#include "src/__support/arg_list.h"
#include "src/__support/macros/properties/os.h"

#include "test/UnitTest/Test.h"

Expand Down Expand Up @@ -120,7 +121,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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.


using int1 = int __attribute__((ext_vector_type(1)));
using int2 = int __attribute__((ext_vector_type(2)));
Expand Down
1 change: 1 addition & 0 deletions libc/test/src/fenv/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ add_libc_unittest(
libc.src.fenv.fesetenv
libc.src.fenv.fesetround
libc.src.__support.FPUtil.fenv_impl
libc.src.__support.macros.properties.os
LINK_LIBRARIES
LibcFPTestHelpers
)
Expand Down
5 changes: 4 additions & 1 deletion libc/test/src/fenv/getenv_and_setenv_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,15 @@
#include "src/fenv/fesetround.h"

#include "src/__support/FPUtil/FEnvImpl.h"
#include "src/__support/macros/properties/os.h"
#include "test/UnitTest/FEnvSafeTest.h"
#include "test/UnitTest/Test.h"

#include "excepts.h"

using LlvmLibcFEnvTest = LIBC_NAMESPACE::testing::FEnvSafeTest;

#ifndef LIBC_TARGET_OS_IS_WINDOWS
TEST_F(LlvmLibcFEnvTest, GetEnvAndSetEnv) {
// We will disable all exceptions to prevent invocation of the exception
// handler.
Expand Down Expand Up @@ -71,8 +73,9 @@ TEST_F(LlvmLibcFEnvTest, Set_FE_DFL_ENV) {
int rm = LIBC_NAMESPACE::fegetround();
EXPECT_EQ(rm, FE_TONEAREST);
}
#endif

#ifdef _WIN32
#ifdef LIBC_TARGET_OS_IS_WINDOWS
TEST_F(LlvmLibcFEnvTest, Windows_Set_Get_Test) {
// If a valid fenv_t is written, then reading it back out should be identical.
fenv_t setEnv = {0x7e00053e, 0x0f00000f};
Expand Down
3 changes: 3 additions & 0 deletions libc/test/src/math/smoke/AddTest.h
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include "hdr/errno_macros.h"
#include "hdr/fenv_macros.h"
#include "src/__support/FPUtil/BasicOperations.h"
#include "src/__support/macros/properties/os.h"
#include "test/UnitTest/FEnvSafeTest.h"
#include "test/UnitTest/FPMatcher.h"
#include "test/UnitTest/Test.h"
Expand Down Expand Up @@ -53,6 +54,7 @@ class AddTest : public LIBC_NAMESPACE::testing::FEnvSafeTest {
}

void test_range_errors(AddFunc func) {
#ifndef LIBC_TARGET_OS_IS_WINDOWS
using namespace LIBC_NAMESPACE::fputil::testing;

if (ForceRoundingMode r(RoundingMode::Nearest); r.success) {
Expand Down Expand Up @@ -121,6 +123,7 @@ class AddTest : public LIBC_NAMESPACE::testing::FEnvSafeTest {
FE_UNDERFLOW | FE_INEXACT);
EXPECT_MATH_ERRNO(ERANGE);
}
#endif
}

void test_inexact_results(AddFunc func) {
Expand Down
Loading
Loading