-
-
Notifications
You must be signed in to change notification settings - Fork 429
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
Enum relation assert on windows with clang #716
Comments
Here is the cmake output :
|
Looks like this is a clang 13 issue, looking into it |
Yep, clang 13 changed the format of |
Fixed! Turns out Apple clang was one minor version behind llvm clang. |
Hello ! Sorry for reopening this issue. The code still asserts on Windows with clang 13 in debug mode. In release mode, Following this comment : #716 (comment) To tried to pinpoint where the problem is, I tried to follow the behaviour of The only difference in behaviour I could find is in this piece of code in enum.hpp : // 1
template <E Value, flecs::if_not_t< enum_constant_is_valid<E, Value>() > = 0>
static void init_constant(flecs::world_t*) { std::cout << "Hello" << ; }
// 2
template <E Value, flecs::if_t< enum_constant_is_valid<E, Value>() > = 0>
static void init_constant(flecs::world_t *world) {
int v = to_int<Value>();
const char *name = enum_constant_to_name<E, Value>();
data.constants[v].next = data.min;
data.min = v;
if (!data.max) {
data.max = v;
}
data.constants[v].id = ecs_cpp_enum_constant_register(
world, data.id, data.constants[v].id, name, v);
}
template <E Value = FLECS_ENUM_MAX(E) >
static void init(flecs::world_t *world) {
init_constant<Value>(world); // here
if (is_not_0<Value>()) {
init<from_int<to_int<Value>() - is_not_0<Value>()>()>(world);
}
} During You'll find attached a screenshot of local variables during this call. Should Hope it can helps ! |
Circling back to this, I couldn't reproduce on clang 14/Linux but couldn't. Could you try to run and compile this code with clang? #include <stdio.h>
enum SparseEnum {
Black = 1, White = 3, Grey = 5
};
template <typename E, E C>
void enum_constant() {
printf("%s\n", __PRETTY_FUNCTION__);
}
int main()
{
enum_constant<SparseEnum, Black>();
enum_constant<SparseEnum, (SparseEnum)0>();
return 0;
} The number |
I tested this code on clang 13 and 14 (on windows) and it works correctly : void enum_constant() [E = SparseEnum, C = Black]
void enum_constant() [E = SparseEnum, C = (SparseEnum)0] I'll investigate more ! |
So when calling During component.hpp:151 Later on, we end up in flecs_cpp.c : char* ecs_cpp_get_constant_name(
char *constant_name,
const char *func_name,
size_t func_name_len)
{
ecs_size_t f_len = flecs_uto(ecs_size_t, func_name_len);
const char *start = cpp_func_rchr(func_name, f_len, ' ');
start = cpp_func_max(start, cpp_func_rchr(func_name, f_len, ')'));
start = cpp_func_max(start, cpp_func_rchr(func_name, f_len, ':'));
start = cpp_func_max(start, cpp_func_rchr(func_name, f_len, ','));
ecs_assert(start != NULL, ECS_INVALID_PARAMETER, func_name);
start ++;
ecs_size_t len = flecs_uto(ecs_size_t,
(f_len - (start - func_name) - flecs_uto(ecs_size_t, ECS_FUNC_NAME_BACK)));
ecs_os_memcpy_n(constant_name, start, char, len);
constant_name[len] = '\0';
return constant_name;
} and func_name is equal to :
and afterwards, If it's more convenient for you, I can share my screen on the discord server (if you want to direct me when debugging). My intuition was there was something wrong in this piece of code : #if defined(__clang__)
#if __clang_major__ < 13 || (defined(__APPLE__) && __clang_minor__ < 1)
template <typename E, E C>
constexpr bool enum_constant_is_valid() {
return !(
(ECS_FUNC_NAME[ECS_FUNC_NAME_FRONT(bool, enum_constant_is_valid) +
enum_type_len<E>() + 6 /* ', C = ' */] >= '0') &&
(ECS_FUNC_NAME[ECS_FUNC_NAME_FRONT(bool, enum_constant_is_valid) +
enum_type_len<E>() + 6 /* ', C = ' */] <= '9'));
}
#else
template <typename E, E C>
constexpr bool enum_constant_is_valid() {
return (ECS_FUNC_NAME[ECS_FUNC_NAME_FRONT(bool, enum_constant_is_valid) +
enum_type_len<E>() + 6 /* ', E C = ' */] != '(');
}
#endif
#elif defined(__GNUC__)
template <typename E, E C>
constexpr bool enum_constant_is_valid() {
return (ECS_FUNC_NAME[ECS_FUNC_NAME_FRONT(constepxr bool, enum_constant_is_valid) +
enum_type_len<E>() + 8 /* ', E C = ' */] != '(');
}
#else
/* Use different trick on MSVC, since it uses hexadecimal representation for
* invalid enum constants. We can leverage that msvc inserts a C-style cast
* into the name, and the location of its first character ('(') is known. */
template <typename E, E C>
constexpr bool enum_constant_is_valid() {
return ECS_FUNC_NAME[ECS_FUNC_NAME_FRONT(bool, enum_constant_is_valid) +
enum_type_len<E>() + 1] != '(';
}
#endif Like maybe we do not enter the correct version on windows (see : https://stackoverflow.com/questions/28017400/why-isnt-clang-defined-when-using-llvmclang-in-visual-studio). For example, when running this code (clang and windows): int main()
{
#ifdef _MSC_VER
std::cout << "_MSC_VER is defined\n"; // Is called
#endif
#ifdef __clang__
std::cout << "__clang__ is defined\n"; // Is also called
#endif
} Maybe the fact that both are true is creating bug somewhere else ? Like maybe for this (enum.hpp): #ifdef ECS_TARGET_MSVC
#define ECS_SIZE_T_STR "unsigned __int64" // This is defined
#elif defined(__clang__)
#define ECS_SIZE_T_STR "size_t" // and not this
#else
#define ECS_SIZE_T_STR "constexpr size_t; size_t = long unsigned int"
#endif |
Well I think I found the culprit. From last comment, I pointed that Undefing In this piece of code : #ifdef ECS_TARGET_MSVC
#define ECS_SIZE_T_STR "unsigned __int64" // This is defined
#elif defined(__clang__)
#define ECS_SIZE_T_STR "size_t" // and not this
#else
#define ECS_SIZE_T_STR "constexpr size_t; size_t = long unsigned int"
#endif MSVC is checked first, clang afterwards. Which is not the case for : #if defined(__clang__)
#if __clang_major__ < 13 || (defined(__APPLE__) && __clang_minor__ < 1)
template <typename E, E C>
constexpr bool enum_constant_is_valid() {
return !(
(ECS_FUNC_NAME[ECS_FUNC_NAME_FRONT(bool, enum_constant_is_valid) +
enum_type_len<E>() + 6 /* ', C = ' */] >= '0') &&
(ECS_FUNC_NAME[ECS_FUNC_NAME_FRONT(bool, enum_constant_is_valid) +
enum_type_len<E>() + 6 /* ', C = ' */] <= '9'));
}
#else
template <typename E, E C>
constexpr bool enum_constant_is_valid() {
return (ECS_FUNC_NAME[ECS_FUNC_NAME_FRONT(bool, enum_constant_is_valid) +
enum_type_len<E>() + 6 /* ', E C = ' */] != '(');
}
#endif
#elif defined(__GNUC__)
template <typename E, E C>
constexpr bool enum_constant_is_valid() {
return (ECS_FUNC_NAME[ECS_FUNC_NAME_FRONT(constepxr bool, enum_constant_is_valid) +
enum_type_len<E>() + 8 /* ', E C = ' */] != '(');
}
#else
/* Use different trick on MSVC, since it uses hexadecimal representation for
* invalid enum constants. We can leverage that msvc inserts a C-style cast
* into the name, and the location of its first character ('(') is known. */
template <typename E, E C>
constexpr bool enum_constant_is_valid() {
return ECS_FUNC_NAME[ECS_FUNC_NAME_FRONT(bool, enum_constant_is_valid) +
enum_type_len<E>() + 1] != '(';
}
#endif So I thought that maybe by switching so that clang is checked first and msvc afterwards, it would solve the problem : #if defined(__clang__)
#define ECS_SIZE_T_STR "size_t"
#elif defined(ECS_TARGET_MSVC)
#define ECS_SIZE_T_STR "unsigned __int64"
#else
#define ECS_SIZE_T_STR "constexpr size_t; size_t = long unsigned int"
#endif Turns out it works now ! I haven't done an extensive check to see if there was other places where this would be a problem. I tried this : (api_defines.h) #if defined(_MSC_VER) && !defined(__clang__)
#define ECS_TARGET_MSVC
#endif But there are many compiler errors. |
Thanks for looking into this! That sounds like the cause of the problem, I'll see if I can come up with a fix :) |
Can you try if this branch fixes the issue? https://github.com/SanderMertens/flecs/tree/clang_windows_fix |
I've got some compiler errors : There are In file line 173 #ifndef ECS_TARGET_MSVC // Shouldn't this be ECS_TARGET_WINDOWS now ?
ssize_t send_bytes = send(sock, buf, flecs_itosize(size), flags);
return flecs_itoi32(send_bytes);
#else
int send_bytes = send(sock, buf, size, flags);
return flecs_itoi32(send_bytes);
#endif and line 190 #ifndef ECS_TARGET_MSVC // idem
ssize_t recv_bytes = recv(sock, buf, flecs_itosize(size), flags);
ret = flecs_itoi32(recv_bytes);
#else
int recv_bytes = recv(sock, buf, size, flags);
ret = flecs_itoi32(recv_bytes);
#endif
if (ret == -1) {
ecs_dbg("recv failed: %s (sock = %d)", ecs_os_strerror(errno), sock);
} else if (ret == 0) {
ecs_dbg("recv: received 0 bytes (sock = %d)", sock);
} In file #ifdef ECS_TARGET_MSVC // idem ?
//FIXME
#else
#include <sys/param.h> /* attempt to define endianness */
#endif
#ifdef ECS_TARGET_LINUX
# include <endian.h> /* attempt to define endianness */
#endif EDIT : now that I think about it, it may not be this simple. I thought |
Yup you're right. The I just checked in an update to the branch which I think follows your suggestions, let me know if it works! |
Yep, it works (on clang 14 and windows) ! Thank you 🥳 ! |
Awesome! I'll merge it to master then. Thanks for your help! |
Describe the bug
Adding an enum relation, e.g flecs example, assert on windows when building with clang (tested only with version 13.0) :
Here is the assert :
fatal: entity.c: 2027: assert: name_assigned == ecs_has_pair( world, result, ecs_id(EcsIdentifier), EcsName) INTERNAL_ERROR
To Reproduce
A minimal repo is available here : github repo.
Or, here is the code :
Expected behavior
For the above code to not assert during line 1 or 2.
Additional context
Tested on master
The above code works on other configurations :
In release mode, there seem to be no problem, the above code works. However, it seems in more complex configuration, (if we added reflection for example), the code crash (access error).
Also, here are the logs with
flecs::log::set_level(3)
:The text was updated successfully, but these errors were encountered: