-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
[rtextures.c] ColorToInt(RED) is UB #3987
Comments
Potential fix? int ColorToInt(Color color)
{
return (int)(((unsigned)color.r << 24) | ((unsigned)color.g << 16) | ((unsigned)color.b << 8) | color.a);
} The core problem stems from explicit implicit upcast from unsigned char goes to int for operator << I believe (from my little testing). Neat issue though! |
Yes. And it only now occurs to me that this assumes |
I am probably the wrong person to ask on such matters as UB.
(https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3096.pdf) So, regarding the standard the behaviour is implementation-defined. So I looked up what clang does (did not find it) but here is gcc:
(https://gcc.gnu.org/onlinedocs/gcc-5.3.0/gcc/Integers-implementation.html#Integers-implementation) I am guessing clang does the same as gcc. And clang's UBSAN seems to be happy when I run it as well. It is probably the best solution, you know I opened an issue instead of a PR because I dislike my memcpy/branchy concoction. So yes, I am in favour of @JayLCypher's implementation. |
This also could be not quite correct for big endian systems (the return type, anyway). Not sure there are any good reasons to call this function. |
It is interesting that |
My initital use case was to compare colors, which could be done differently (i. e. by comparing all the fields individually, or maybe with memcmp). in my implementation, I could have also used my own enum of Colors and used that before converting them with a switch statement to the Raylib color shortly before rendering. So in my case, you may be right, but I have no idea what are people are using it for. Regarding endian-ness, the documentation is not exactly clear in which order the fields go, it just states it would convert it to a "hexadecimal" value with GetColor doing the reverse. Furthermore, the implementation is endian-neutral, like it should be, it would produce the same value on a big endian vs. a little endian system. |
I'm a pedantic fellow so I enjoy things written "flawlessly", which is what drew me to this issue. In terms of endianness and bit-width, the structure for color requires 4 * sizeof (unsigned char) to be represented as a single value. I noticed in my tests that endian order mattered memory layout wise using a union, which is sub-optimal. As for a use case of comparing colors, you could definitively roll a simple inline helper function. Something like: bool IsSameColor(register const Color a, register const Color b)
{
return (a.r == b.r) & (a.g == b.g) & (a.b == b.b) & (a.a == b.a);
} (You can use int or bool return type, depending on your support. If you know the scope you can also make it static inline. As for the library function, it is a common issue I see of using non-fixed width integers where widths are needed (such as this case) and using integers as a fundamental type when you don't need signedness. That is old-school type C for ya. :) The main issue is really that the upcast of an unsigned char ends up being integer when doing operator << on it, which is the bonkers thing to me. So that's why it's each color variable that is explicitly upcasted to 32-bit width unsigned before <<. Once the final value is set, it can be casted to int for return type which just changes the representation (it's now a negative number on LE). You can do this implicitly by binding the variables to a new (32-bit unsigned) variable too. Cliteral version because I like to write C :) int ColorToInt(register const Color c)
{
return (int)((c.a) | (CLITERAL(unsigned){c.b} << 8) | (CLITERAL(unsigned){c.g} << 16) | (CLITERAL(unsigned){c.r} << 24));
} |
what I meant, is that code like this should work regardless of endianness: assert(7991807 == ColorToInt(BLUE));
int red = 0xFF'00'00'00;
assert(ColorToInt((Color){.r=255}) == red); Does it not? I used ColorToInt because it was easier than writing three lines of a dedicated function. bool is_same_color(Color a, Color b) {
return a.r == b.r && a.g == b.g && a.b == b.b && a.a == b.a;
}
I find your C code really interesting, it really showcases differences in style and I appreciate that Anyway I went with your original ColorToInt function, I see no need to initialize new unsigned integers using compound literals. |
Agreed. There is no reason to ever write "register". I don't think it helps compilers today at all
const in general has extremely questionable value. I don't use it.
I decided to try, and it seems like it does: |
@OetkenPurveyorOfCode
Thanks for the compliment though. :) |
Yes. But why would you restrict another programmer from taking its address? I struggle to think of a reason here or a scenario where this adds value.
the color_is_same function would be in my codebase, so my style. If I were to add it to Raylib, I would obviously call it ColorIsSame and use raylib's style.
Logical and short-circuits, yours does not. But I said, that this is not relevant here. It was to "communicate intent" as you would say.
I would use restrict only to hint to the optimiser, it can enable simd optimisations and reduce rendundant loads, but I have not used it so far (I would have to have a suitable performance problem in the first place). Anyway, I haven't been programming in C for long, so don't take my advise too seriously. Have fun! |
Minimal example:
Produces the following error:
The issue is that any color with a high enough RED value causes the signed integer to overflow which is undefined as per the C standard.
This happens here:
I tired rewriting it either to:
which assumes that int is 4 bytes and stored as two's complement. (That would be UB on another machine but that does not affect me, the signed overflow affects me though)
Or this ugly dance around (https://stackoverflow.com/questions/5129498/how-to-cast-or-convert-an-unsigned-int-to-int-in-c):
(which also assume 2's complement which is only guaranteed in the latest C standard (but I am compiling with -std=c2x, so that would not be an issue for bleeding edge people like me)
Both run fine without triggering UBSAN for the entire range of uints (though retyping them in this issue may have introduced errors)
Or should the ColorToInt signature to be changed to return an unsigned int (probably a breaking change)?
The text was updated successfully, but these errors were encountered: