-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[libc] Some MSVC compatibility fixes in src/__support. #159428
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
Conversation
@llvm/pr-subscribers-libc Author: None (lntue) ChangesFull diff: https://github.com/llvm/llvm-project/pull/159428.diff 6 Files Affected:
diff --git a/libc/src/__support/CMakeLists.txt b/libc/src/__support/CMakeLists.txt
index 0ef09a9b8c9d0..73d0ad15584fc 100644
--- a/libc/src/__support/CMakeLists.txt
+++ b/libc/src/__support/CMakeLists.txt
@@ -123,6 +123,7 @@ add_header_library(
libc.src.__support.CPP.type_traits
libc.src.__support.macros.attributes
libc.src.__support.macros.config
+ libc.src.__support.macros.properties.compiler
)
add_header_library(
diff --git a/libc/src/__support/CPP/CMakeLists.txt b/libc/src/__support/CPP/CMakeLists.txt
index a9cb67df0b427..bdfbc6151c773 100644
--- a/libc/src/__support/CPP/CMakeLists.txt
+++ b/libc/src/__support/CPP/CMakeLists.txt
@@ -20,6 +20,7 @@ add_header_library(
libc.hdr.stdint_proxy
libc.src.__support.macros.attributes
libc.src.__support.macros.sanitizer
+ libc.src.__support.macros.properties.compiler
)
add_header_library(
@@ -209,6 +210,7 @@ add_object_library(
libc.hdr.func.malloc
libc.hdr.func.aligned_alloc
libc.src.__support.common
+ libc.src.__support.macros.properties.compiler
libc.src.__support.macros.properties.os
)
diff --git a/libc/src/__support/CPP/bit.h b/libc/src/__support/CPP/bit.h
index 8dbb30047faec..f602a7447ec10 100644
--- a/libc/src/__support/CPP/bit.h
+++ b/libc/src/__support/CPP/bit.h
@@ -16,6 +16,7 @@
#include "src/__support/CPP/type_traits.h"
#include "src/__support/macros/attributes.h"
#include "src/__support/macros/config.h"
+#include "src/__support/macros/properties/compiler.h"
#include "src/__support/macros/sanitizer.h"
namespace LIBC_NAMESPACE_DECL {
@@ -36,7 +37,7 @@ LIBC_INLINE constexpr cpp::enable_if_t<
To>
bit_cast(const From &from) {
MSAN_UNPOISON(&from, sizeof(From));
-#if __has_builtin(__builtin_bit_cast)
+#if __has_builtin(__builtin_bit_cast) || defined(LIBC_COMPILER_IS_MSVC)
return __builtin_bit_cast(To, from);
#else
To to{};
diff --git a/libc/src/__support/CPP/new.h b/libc/src/__support/CPP/new.h
index fe36de29468a8..67344b09e3833 100644
--- a/libc/src/__support/CPP/new.h
+++ b/libc/src/__support/CPP/new.h
@@ -14,6 +14,7 @@
#include "hdr/func/malloc.h"
#include "src/__support/common.h"
#include "src/__support/macros/config.h"
+#include "src/__support/macros/properties/compiler.h"
#include "src/__support/macros/properties/os.h"
#include <stddef.h> // For size_t
@@ -109,8 +110,12 @@ LIBC_INLINE void *operator new[](size_t, void *p) { return p; }
// header file in all libc source files where operator delete is called ensures
// that only libc call sites use these replacement operator delete functions.
+#ifndef LIBC_COMPILER_IS_MSVC
#define DELETE_NAME(name) \
__asm__(LIBC_MACRO_TO_STRING(LIBC_NAMESPACE) "_" LIBC_MACRO_TO_STRING(name))
+#else
+#define DELETE_NAME(name)
+#endif // LIBC_COMPILER_IS_MSVC
void operator delete(void *) noexcept DELETE_NAME(delete);
void operator delete(void *, std::align_val_t) noexcept
diff --git a/libc/src/__support/big_int.h b/libc/src/__support/big_int.h
index 10e35ef4fd24a..bb9cefd67b552 100644
--- a/libc/src/__support/big_int.h
+++ b/libc/src/__support/big_int.h
@@ -95,10 +95,10 @@ LIBC_INLINE constexpr DoubleWide<word> mul2(word a, word b) {
#endif
else {
using half_word = half_width_t<word>;
- const auto shiftl = [](word value) -> word {
+ constexpr auto shiftl = [](word value) -> word {
return value << cpp::numeric_limits<half_word>::digits;
};
- const auto shiftr = [](word value) -> word {
+ constexpr auto shiftr = [](word value) -> word {
return value >> cpp::numeric_limits<half_word>::digits;
};
// Here we do a one digit multiplication where 'a' and 'b' are of type
@@ -111,19 +111,19 @@ LIBC_INLINE constexpr DoubleWide<word> mul2(word a, word b) {
// c result
// We convert 'lo' and 'hi' from 'half_word' to 'word' so multiplication
// doesn't overflow.
- const word a_lo = lo(a);
- const word b_lo = lo(b);
- const word a_hi = hi(a);
- const word b_hi = hi(b);
- const word step1 = b_lo * a_lo; // no overflow;
- const word step2 = b_lo * a_hi; // no overflow;
- const word step3 = b_hi * a_lo; // no overflow;
- const word step4 = b_hi * a_hi; // no overflow;
+ word a_lo = lo(a);
+ word b_lo = lo(b);
+ word a_hi = hi(a);
+ word b_hi = hi(b);
+ word step1 = b_lo * a_lo; // no overflow;
+ word step2 = b_lo * a_hi; // no overflow;
+ word step3 = b_hi * a_lo; // no overflow;
+ word step4 = b_hi * a_hi; // no overflow;
word lo_digit = step1;
word hi_digit = step4;
- const word no_carry = 0;
- word carry;
- word _; // unused carry variable.
+ word no_carry = 0;
+ word carry = 0;
+ [[maybe_unused]] word _ = 0; // unused carry variable.
lo_digit = add_with_carry<word>(lo_digit, shiftl(step2), no_carry, carry);
hi_digit = add_with_carry<word>(hi_digit, shiftr(step2), carry, _);
lo_digit = add_with_carry<word>(lo_digit, shiftl(step3), no_carry, carry);
diff --git a/libc/src/__support/math_extras.h b/libc/src/__support/math_extras.h
index d4dc6dcb4acf6..093a3371de3cd 100644
--- a/libc/src/__support/math_extras.h
+++ b/libc/src/__support/math_extras.h
@@ -15,6 +15,7 @@
#include "src/__support/CPP/type_traits.h" // is_unsigned_v, is_constant_evaluated
#include "src/__support/macros/attributes.h" // LIBC_INLINE
#include "src/__support/macros/config.h"
+#include "src/__support/macros/properties/compiler.h"
namespace LIBC_NAMESPACE_DECL {
@@ -25,7 +26,17 @@ LIBC_INLINE constexpr cpp::enable_if_t<cpp::is_unsigned_v<T>, T>
mask_trailing_ones() {
constexpr unsigned T_BITS = CHAR_BIT * sizeof(T);
static_assert(count <= T_BITS && "Invalid bit index");
+#ifndef LIBC_COMPILER_IS_MSVC
return count == 0 ? 0 : (T(-1) >> (T_BITS - count));
+#else
+ // MSVC complains about out of range shifts.
+ if constexpr (count == 0)
+ return 0;
+ else if constexpr (count >= T_BITS)
+ return T(-1);
+ else
+ return T(-1) >> (T_BITS - count);
+#endif // !LIBC_COMPILER_IS_MSVC
}
// Create a bitmask with the count left-most bits set to 1, and all other bits
|
bit_cast(const From &from) { | ||
MSAN_UNPOISON(&from, sizeof(From)); | ||
#if __has_builtin(__builtin_bit_cast) | ||
#if __has_builtin(__builtin_bit_cast) || defined(LIBC_COMPILER_IS_MSVC) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we not just
#if defined(LIBC_COMPILER_IS_MSVC)
#define __has_builtin(x) 0
#endif
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already define __has_builtin(x) 0
for MSVC https://github.com/llvm/llvm-project/blob/main/libc/src/__support/macros/config.h#L26
The problem here is that MSVC does have __builtin_bit_cast
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What a pain
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM
libc/src/__support/math_extras.h
Outdated
#ifndef LIBC_COMPILER_IS_MSVC | ||
return count == 0 ? 0 : (T(-1) >> (T_BITS - count)); | ||
#else | ||
// MSVC complains about out of range shifts. | ||
if constexpr (count == 0) | ||
return 0; | ||
else if constexpr (count >= T_BITS) | ||
return T(-1); | ||
else | ||
return T(-1) >> (T_BITS - count); | ||
#endif // !LIBC_COMPILER_IS_MSVC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason we need to keep the non-MSVC version? It seems like the new version should work on other platforms as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess not? Probably just remove the static_assert
and keep the new version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nvm, we can actually keep the static assert.
No description provided.