7 ms·

The problem isn’t overflow, the problem is the backwards logic of the compiler assuming there will never be any execution that leads to overflow, so the code th

by wrs 3y ago

The problem isn’t overflow, the problem is the backwards logic of the compiler assuming there will never be any execution that leads to overflow, so the code that overflows just vanishes completely. In other words, bool overflowed = (x+1)<x; should be meaningful. It may or may not do what you want on any given architecture, but it shouldn’t just be assumed false.

dzaima 3y ago
How about if the x+1 was from an inlined function? a macro? perhaps even just a variable defined 10 lines up? In a 7-character sequence it may seem pretty obvious that it's probably intended to mean something, but when joining together multiple independent things doing redundant operations it's much less trivial. int game_logic(int prev_score, int bonus) { if (bonus < 0) { // check for bad argument return -1; } int new_score = prev_score + bonus; // ... if (new_score < prev_score) { // sanity check abort(); } return new_score; } // in the above the abort path can (and is) already optimized out, but an invocation of game_logic(x,1) becomes exactly x+1<x
zzo38computer 3y ago
I think the -fwrapv switch in GCC allows this to work properly. I don't actually know about this specific case, but -fwrapv makes signed and unsigned arithmetic to wrap around so that only the appropriate number of low bits are kept. (I commonly use -fwrapv when writing programs in C.)
- deleted 3y ago
  [deleted]
nlewycky 3y ago
bool overflowed = (x+1)<x; I was convinced that some warning, probably -Wtautological-compare, already handled this and I plugged it into godbolt to see which one, but got no warnings with either gcc or clang. Frankly, I'm stunned and even a bit annoyed. Clearly this code deserves a warning, unless there's some good reason I'm just blind to right now. Nevertheless, I don't know what to do about the suggestion "it may or may not do what you want on any given architecture, but it shouldn’t just be assumed false." The compiler needs to know what it can and can't do. The common advice of "just do what the CPU does" doesn't work, we need to know what to do when cross-compiling, when constant folding, and we need to know which instructions are valid to select. If I selected PADDSW for this add (a saturating addition operation) but then later when you use x+1 I select a non-saturating addition, would you be happy with that? Probably not. The compiler needs actual rules to follow. I don't know how to apply the suggestion about warnings when we end up deleting dead code. The compiler deletes dead code all the time, consider a case like "vector<t> v; v.push_back(a); v.push_back(b);", each push_back begins with an if-statement on whether reallocation is required, and that becomes constant with inlining. Tracking "this code became dead because", well, because which situations exactly?
- mjevans 3y ago
  If code is dead or unreached, and therefore deleted / no instructions emitted; clearly THAT is of a level a warning should cover, if not an error.
  - nlewycky 3y ago
    Dead code happens all the time. Adjusting the example in my comment you're replying to: vector<t> v; v.push_back(a); v.push_back(b); v.push_back(c); v.push_back(d); Let's suppose the definition of our vector here looks something like this: template <typename T> struct vector<t> { size_t len = 0; size_t storage_len = 0; T *storage = nullptr; void push_back(T t) { if (len + 1 > storage_len) { storage_len = storage_len ? storage_len * 2 : 1; storage = realloc(storage, storage_len); } storage[len] = t; ++len; } }; So here's what happens. vector<t> v; v.push_back(a); v.push_back(b); v.push_back(c); v.push_back(d); becomes len = 0; storage_len = 0; storage = nullptr; if (len + 1 > storage_len) { storage_len = storage_len ? storage_len * 2 : 1; storage = realloc(storage, storage_len); } storage[len] = a; ++len; if (len + 1 > storage_len) { storage_len = storage_len ? storage_len * 2 : 1; storage = realloc(storage, storage_len); } storage[len] = b; ++len; if (len + 1 > storage_len) { storage_len = storage_len ? storage_len * 2 : 1; storage = realloc(storage, storage_len); } storage[len] = c; ++len; if (len + 1 > storage_len) { storage_len = storage_len ? storage_len * 2 : 1; storage = realloc(storage, storage_len); } storage[len] = d; ++len; becomes len = 0; storage_len = 0; storage = nullptr; if (0 + 1 > 0) { storage_len = 1; storage = realloc(storage, 1); } storage[0] = a; len = 1; if (1 + 1 > 1) { storage_len = 2; storage = realloc(storage, 2); } storage[1] = b; len = 2; if (2 + 1 > 2) { storage_len = 4; storage = realloc(storage, 4); } storage[2] = c; len = 3; if (3 + 1 > 4) { storage_len = 8; storage = realloc(storage, 8); } storage[3] = d; len = 4; In that last one, you can see that the if-expression is false and the body becomes dead code. If I understand the rule you're proposing, you want to get a warning or error for that?

by wrs 3y ago

dzaima 3y ago

How about if the x+1 was from an inlined function? a macro? perhaps even just a variable defined 10 lines up? In a 7-character sequence it may seem pretty obvious that it's probably intended to mean something, but when joining together multiple independent things doing redundant operations it's much less trivial. int game_logic(int prev_score, int bonus) { if (bonus < 0) { // check for bad argument return -1; } int new_score = prev_score + bonus; // ... if (new_score < prev_score) { // sanity check abort(); } return new_score; } // in the above the abort path can (and is) already optimized out, but an invocation of game_logic(x,1) becomes exactly x+1<x

zzo38computer 3y ago

I think the -fwrapv switch in GCC allows this to work properly. I don't actually know about this specific case, but -fwrapv makes signed and unsigned arithmetic to wrap around so that only the appropriate number of low bits are kept. (I commonly use -fwrapv when writing programs in C.)

deleted 3y ago
[deleted]

nlewycky 3y ago

bool overflowed = (x+1)<x; I was convinced that some warning, probably -Wtautological-compare, already handled this and I plugged it into godbolt to see which one, but got no warnings with either gcc or clang. Frankly, I'm stunned and even a bit annoyed. Clearly this code deserves a warning, unless there's some good reason I'm just blind to right now. Nevertheless, I don't know what to do about the suggestion "it may or may not do what you want on any given architecture, but it shouldn’t just be assumed false." The compiler needs to know what it can and can't do. The common advice of "just do what the CPU does" doesn't work, we need to know what to do when cross-compiling, when constant folding, and we need to know which instructions are valid to select. If I selected PADDSW for this add (a saturating addition operation) but then later when you use x+1 I select a non-saturating addition, would you be happy with that? Probably not. The compiler needs actual rules to follow. I don't know how to apply the suggestion about warnings when we end up deleting dead code. The compiler deletes dead code all the time, consider a case like "vector<t> v; v.push_back(a); v.push_back(b);", each push_back begins with an if-statement on whether reallocation is required, and that becomes constant with inlining. Tracking "this code became dead because", well, because which situations exactly?

mjevans 3y ago
If code is dead or unreached, and therefore deleted / no instructions emitted; clearly THAT is of a level a warning should cover, if not an error.
- nlewycky 3y ago
  Dead code happens all the time. Adjusting the example in my comment you're replying to: vector<t> v; v.push_back(a); v.push_back(b); v.push_back(c); v.push_back(d); Let's suppose the definition of our vector here looks something like this: template <typename T> struct vector<t> { size_t len = 0; size_t storage_len = 0; T *storage = nullptr; void push_back(T t) { if (len + 1 > storage_len) { storage_len = storage_len ? storage_len * 2 : 1; storage = realloc(storage, storage_len); } storage[len] = t; ++len; } }; So here's what happens. vector<t> v; v.push_back(a); v.push_back(b); v.push_back(c); v.push_back(d); becomes len = 0; storage_len = 0; storage = nullptr; if (len + 1 > storage_len) { storage_len = storage_len ? storage_len * 2 : 1; storage = realloc(storage, storage_len); } storage[len] = a; ++len; if (len + 1 > storage_len) { storage_len = storage_len ? storage_len * 2 : 1; storage = realloc(storage, storage_len); } storage[len] = b; ++len; if (len + 1 > storage_len) { storage_len = storage_len ? storage_len * 2 : 1; storage = realloc(storage, storage_len); } storage[len] = c; ++len; if (len + 1 > storage_len) { storage_len = storage_len ? storage_len * 2 : 1; storage = realloc(storage, storage_len); } storage[len] = d; ++len; becomes len = 0; storage_len = 0; storage = nullptr; if (0 + 1 > 0) { storage_len = 1; storage = realloc(storage, 1); } storage[0] = a; len = 1; if (1 + 1 > 1) { storage_len = 2; storage = realloc(storage, 2); } storage[1] = b; len = 2; if (2 + 1 > 2) { storage_len = 4; storage = realloc(storage, 4); } storage[2] = c; len = 3; if (3 + 1 > 4) { storage_len = 8; storage = realloc(storage, 8); } storage[3] = d; len = 4; In that last one, you can see that the if-expression is false and the body becomes dead code. If I understand the rule you're proposing, you want to get a warning or error for that?