> Anybody using GLFW could have enabled UBSan in their C compiler. Anybody could have run into this same crash and debugged it in the last 6 years. But they didn’t. Only because Zig has good defaults, because it places so much emphasis on things being right out of the box, and because there is such an emphasis on having safety checks for undefined behavior - were we able to catch this undefined behavior that went unnoticed in GLFW for the last 6 years.
(the point on good defaults stays valid though)
unsigned char foo = 0x80; long bar = foo << 24;
It overflows and bar is set to -2147483648. If foo was initialized with 0x7f, there'd be no overflow and bar would be set to 2130706432.
In the original problem, the value of foo isn't known at compile time.
Can't think of a reasonable case where a compiler would generate "bad" code because of this.
The code would never fail on a two's compliment machine. What ubsan is saying is that the rules of C don't guarantee this code to work - it only works because the overflow writes into the sign bit, which is where the next thing expected it to be anyway.
If the above is true, I don't think their "fix" helps: https://github.com/glfw/glfw/pull/1986/files. I would have thought that the important part is to change the longs to unsigned longs. Edit: Ah, right, that's what the discussion says: https://github.com/glfw/glfw/pull/1986
The fix for glfw is to manually promote to an appropriate unsigned type, to override the potentially UB-inducing default signed promotion. Since that potentially UB-inducing default can't change now, I was suggesting a warning for it.
It would be an aggressive warning and would probably trigger on a lot of existing code, but honestly, I would enable it for all new code. I really dislike the promotion logic in C.
Their expression (simplified) was: *target = pixels[j] << 24, where pixels contains items of type unsigned char.
I hadn't realized that the pixels[j] was being promoted to an int rather than unsigned int. Eeep.
> I really dislike the promotion logic in C.
Agreed. I'll add that to my list of C dislikes.
I still think that even with manually promoting to the appropriate unsigned type isn't sufficient. Because target has type long*, so coercing the result of (unsigned int)pixels[j] << 24 to long still causes overflow and therefore UB... unless there's yet more I've missed.
Edit: Damn it, again, that's what the discussion says (https://github.com/glfw/glfw/pull/1986#issuecomment-95800024...).
For example, if a u64 would overflow through addition, then instead of just allowing the addition to wrap the u64 around to 0, Zig will rather detect this and crash at runtime with an error, unless you explicitly indicate through the "%+" operator that you do intend to allow the addition to wrap.
If this is an unknown unknown for you, you don't need to worry about it because Zig will detect it, simply because the default is correct.
Whereas in other languages that support something like this (and I think even Zig's checked arithmetic is still among the most extensive across all operators), you have to actually opt-in to checked arithmetic, it's not the default in safe builds, which always surprises me.
https://github.com/glfw/glfw/pull/1986#issuecomment-95578417...
#include <stdint.h>
void foo() {
uint32_t foo = 1;
foo <<= 32;
}
warning: left shift count >= width of type [-Wshift-count-overflow]
warning: shift count >= width of type [-Wshift-count-overflow]
Note that with foo = 0 there's no UB.
int main() {
unsigned char foo = 0x80;
unsigned int x = foo << 24;
}
/app/example.c:5:30: runtime error: left shift of 128 by 24 places cannot be represented in type 'int'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /app/example.c:5:30 in