Probably why it's never been caught or been a problem.
Can't think of a reasonable case where a compiler would generate "bad" code because of this.
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...).