Skip to content
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

[rcore] sinfl_bsr fix for TCC #4807

Merged
merged 4 commits into from
Mar 11, 2025
Merged

[rcore] sinfl_bsr fix for TCC #4807

merged 4 commits into from
Mar 11, 2025

Conversation

RicoP
Copy link
Contributor

@RicoP RicoP commented Mar 2, 2025

under TCC sinfl_bsr returned no value.

@raysan5
Copy link
Owner

raysan5 commented Mar 2, 2025

@RicoP This change could be changing the behaviour for other configurations and result in unexpected issues. Are you completely sure that it can be replaced by an else? It means "no other possible compiler other than".

Also, note that this change is on an external library, it shouldn't be implemented on raylib side...

@raysan5 raysan5 changed the title sinfl_bsr fix for TCC [rcore] sinfl_bsr fix for TCC Mar 2, 2025
@raysan5 raysan5 added the external This issue depends on external lib label Mar 2, 2025
@RicoP
Copy link
Contributor Author

RicoP commented Mar 2, 2025

@RicoP This change could be changing the behaviour for other configurations and result in unexpected issues. Are you completely sure that it can be replaced by an else? It means "no other possible compiler other than".

Also, note that this change is on an external library, it shouldn't be implemented on raylib side...

Well the current implementation leaves a function with no return statement causing undefined behavior

static int
sinfl_bsr(unsigned n) {
#if defined(_MSC_VER) && !defined(__clang__)
  _BitScanReverse(&n, n);
  return n;
#elif defined(__GNUC__) || defined(__clang__)
  return 31 - __builtin_clz(n);
#endif
}

an alternative fix could be this with an explicit error

static int
sinfl_bsr(unsigned n) {
#if defined(_MSC_VER) && !defined(__clang__)
  _BitScanReverse(&n, n);
  return n;
#elif defined(__GNUC__) || defined(__clang__) || defined(__TINYC__)
  return 31 - __builtin_clz(n);
#else  
#error "unknown compiler."
#endif
}

another alternative is to of course implement __builtin_clz by hand in the else branch.

I could also open a PR on the original repo but I figured it's quicker to do here.

@RicoP
Copy link
Contributor Author

RicoP commented Mar 2, 2025

static int
sinfl_bsr(unsigned n) {
#if defined(_MSC_VER) && !defined(__clang__)
  _BitScanReverse(&n, n);
  return n;
#elif defined(__GNUC__) || defined(__clang__) || defined(__TINYC__)
  return 31 - __builtin_clz(n);
#else  
    static const unsigned char table_2_32[32] = {
        31, 22, 30, 21, 18, 10, 29,  2, 20, 17, 15, 13,  9,  6, 28,  1,
        23, 19, 11,  3, 16, 14,  7, 24, 12,  4,  8, 25,  5, 26, 27,  0
    };
    n |= n >> 1;  
    n |= n >> 2;  
    n |= n >> 4;  
    n |= n >> 8;  
    n |= n >> 16; 
    n = table_2_32[(n * 0x07c4acddu) >> 27];
    return 31 - n;
#endif
}

I got this from https://github.com/TinyCC/tinycc/blob/6ec4a106524ab6a6e3a4178774fd1e884d3fcce2/lib/builtin.c#L50

@RicoP
Copy link
Contributor Author

RicoP commented Mar 2, 2025

I created a PR for the original lib, let's see what the author says. vurtun/lib#54

@RicoP
Copy link
Contributor Author

RicoP commented Mar 7, 2025

@raysan5 seems like my pr got merged

@raysan5
Copy link
Owner

raysan5 commented Mar 10, 2025

@RicoP nice, can you update this PR accordingly? thanks!

@RicoP
Copy link
Contributor Author

RicoP commented Mar 10, 2025

@raysan5 done

@raysan5
Copy link
Owner

raysan5 commented Mar 10, 2025

Screenshot_2025-03-10-21-22-19-39_40deb401b9ffe8e1df2f1cc5ba480b12~2
It seems there is some issue with building...

@RicoP
Copy link
Contributor Author

RicoP commented Mar 11, 2025

OK seems MSVC didn't liked a C++ cast

@raysan5 raysan5 merged commit b80250b into raysan5:master Mar 11, 2025
15 checks passed
@raysan5
Copy link
Owner

raysan5 commented Mar 11, 2025

@RicoP thanks for the review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
external This issue depends on external lib
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants