4 ms·

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.

abainbridge 5y ago
Agreed. I think it was only a problem because of ubsan. I guess ubsan added checks to the generated code that looked at the value being shifted left 24 and saw that overflow occurred and therefore raised its undefined behaviour signal. 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 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 https://github.com/glfw/glfw/pull/1986
- dundarious 5y ago
  Maybe what I'm suggesting isn't what you had in mind. 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.
  - abainbridge 5y ago
    You're right. 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-958000246 https://github.com/glfw/glfw/pull/1986#issuecomment-95800024...).

abainbridge 5y ago

Agreed. I think it was only a problem because of ubsan. I guess ubsan added checks to the generated code that looked at the value being shifted left 24 and saw that overflow occurred and therefore raised its undefined behaviour signal. 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 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 https://github.com/glfw/glfw/pull/1986

dundarious 5y ago
Maybe what I'm suggesting isn't what you had in mind. 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.
- abainbridge 5y ago
  You're right. 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-958000246 https://github.com/glfw/glfw/pull/1986#issuecomment-95800024...).