This probably is related to PR71177 and PR57199, but unlike them, this test case doesn't have any external header dependencies, and also affects gcc as well as g++. However though it was in my D frontend that I first noticed this problem. --- typedef __SIZE_TYPE__ size_t; typedef struct Array { size_t length; size_t* ptr; } Array; void setlength(Array* pthis, size_t nlength) { Array aggr = { .length=pthis->length - nlength, .ptr=pthis->ptr + nlength }; for (size_t key = 0; key < aggr.length; key++) aggr.ptr[key] = 1; } void insertBack(Array* pthis) { setlength(pthis, pthis->length + 1); } --- Can even reproduce if I were to lower the loop in the same way that g++ does. --- void setlength(Array* pthis, size_t nlength) { Array aggr = {.length=pthis->length - nlength, .ptr=pthis->ptr + nlength}; size_t key = 0; while (1) { if (aggr.length <= key) break; aggr.ptr[key] = 1; key++; } } --- Just like in PR57199, if I were to add the following inside the for body. --- if (__builtin_expect(aggr.length > key, 0)) __builtin_unreachable(); --- Then the warning goes away.
Oh, and also happens if I move the pointer + length adjustment into the loop. --- void setlength(Array* pthis, size_t nlength) { Array aggr = {.length=pthis->length, .ptr=pthis->ptr}; for (size_t key = 0; key < aggr.length - nlength; key++) aggr.ptr[key + nlength] = 1; } ---
Created attachment 38541 [details] Reduced test Attaching complete minimal test, as the mechanically reduced test perhaps removes a bit too much. Needs -O2 -finline-functions or -O3 in order to trigger.
I think the warning is correct and here is why: If we look at the final code which is produced on the tree level: _3 = pthis_2(D)->length; _4 = _3 + 1; if (_3 > _4) goto <bb 4>; else goto <bb 3>; If pthis_2(D)->length was UINT_MAX, then that UINT_MAX > UINT_MAX + 1 would be true while for all other cases, it is false.
(In reply to Andrew Pinski from comment #3) > I think the warning is correct and here is why: > If we look at the final code which is produced on the tree level: > > _3 = pthis_2(D)->length; > _4 = _3 + 1; > if (_3 > _4) > goto <bb 4>; > else > goto <bb 3>; > > If pthis_2(D)->length was UINT_MAX, then that UINT_MAX > UINT_MAX + 1 would > be true while for all other cases, it is false. Yeah, I can see overflow as a possibility, but it should never happen. I'd be surprised if there is a system out there can return success from a malloc((size_t) -1). :-) If it is correctly detecting overflow, should it warn that instead? Also, if the behaviour is correct, I'd expect this to trigger it also. --- if (nlength < pthis->length) { for (size_t key = 0; key < pthis->length - nlength; key++) pthis->ptr[key + nlength] = 0; } ---
And thinking more about it, I don't think it even explains why this triggers the warning: --- if (nlength < pthis->length) { Array aggr = {pthis->length - nlength, pthis->ptr + nlength}; for (size_t key = 0; key < aggr.length; key++) aggr.ptr[key] = 0; } --- But this works just fine. --- if (nlength < pthis->length) { if (pthis->length >= nlength) __builtin_unreachable(); Array aggr = {pthis->length - nlength, pthis->ptr + nlength}; for (size_t key = 0; key < aggr.length; key++) aggr.ptr[key] = 0; } --- Surely the optimizer must be able to deduce that if the first condition is true, then the second condition must always be a given?
Ugh, I've just realised that I still managed to get the condition guard wrong. So there is indeed a (very, very remote) chance that length would overflow. Though realloc will overflow and fail first under normal operations. I've instead gone and fixed it in the upstream library instead.