I was expecting a compile time warning and thought ubsan shouldn't be necessary. But CUViper explains why the UB can only be caught at runtime.
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...).