Fix undefined behaviour warnings in ir_fold.h

[nielsdos]

See php/php-src#17430

I changed the adds, subs, muls to use unsigned arithmetic because on 2-complement systems that's the same as signed arithmetic but without potential UB warnings. This essentially makes the wrapping behaviour defined. I only did this for 32 and 64 bit types because for 8 and 16 bit the operations will do integer promotion, avoiding the issue.

There is also val.i64 < 0 && val.i64 - 1 < 0 that I changed. This is because the second condition would be removed by the compiler because val.i64 < 0 and signed wrapping is undefined.

[dstogov]

The fix looks right. LLVM and HotSpot also use unsigned math at least for add/sub/mul.

[dstogov]

Could you please add a comment: e.g. "Here and below we use unsigned math to prevent undefined signed overflow behavior".

[dstogov]

Unfortunately, the fix is incomplete. I see 6 new tests failures on C test suite from MIR project (https://github.com/vnmakarov/mir/tree/master/c-tests). I'll take care.

[dstogov]

The problem is in this rule

IR_FOLD(SUB(C_I32, C_I32))
{
	IR_ASSERT(IR_OPT_TYPE(opt) == op1_insn->type);
	IR_FOLD_CONST_I(op1_insn->val.u32 - op2_insn->val.u32);
}

With op1 == 0 and op2 == 1 now it starts produce 0xffffffff instead of expected -1.

[dstogov]

@nielsdos I reverted this patch.
Could you please take another look. I guess, i32 should be changed to u64 instead of u32.

[dstogov]

No. Actually we need to treat the result of unsigned operation as signed to make the proper sign extension.

IR_FOLD(SUB(C_I32, C_I32))
{
	IR_ASSERT(IR_OPT_TYPE(opt) == op1_insn->type);
	IR_FOLD_CONST_I((int32_t)(op1_insn->val.u32 - op2_insn->val.u32));
}

[dstogov]

I committed the fixed version 4c3389b

[nielsdos]

Thanks for handling this.
I looked at the commit, it looks good. There is one place where there is both the int32_t change and change from i32 to u64, that could be u32 instead but probably won't make real harm: 4c3389b#diff-974027a6039dc7d081f523d2b3c139e47b209fca874e31ad964b797921f15284R397

[dstogov]

I looked at the commit, it looks good. There is one place where there is both the int32_t change and change from i32 to u64, that could be u32 instead but probably won't make real harm: 4c3389b#diff-974027a6039dc7d081f523d2b3c139e47b209fca874e31ad964b797921f15284R397

Thanks for catching this. This is definitely my mistake. I'll commit s/u64/u32/.