Repository navigation
[OptimizeInstructions] Check sign-ext child type - #9321
Conversation
In `OptimizeInstructions`, refinalization is deferred until the end of the function walk, so replacing a child expression (such as a trapping `array.len` on a null reference) with `unreachable` leaves an enclosing unary sign-extension instruction (`i32.extend8_s`, etc.) with its original integer type while its operand has `Type::unreachable`. While the shift-pair branch of `Properties::getSignExtValue` already rejects `Type::unreachable` operands via `Abstract::Shl`, the unary branch returned `unary->value` without checking that its type is still an integer, causing `OptimizeInstructions::makeZeroExt` to construct an integer mask literal with `Type::unreachable` and abort. Check `unary->value->type.isInteger()` in `Properties::getSignExtValue`.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates getSignExtValue in src/ir/properties.h to ensure that the operand of a sign-extend operation is an integer by checking unary->value->type.isInteger() before returning it. This prevents treating newly-unreachable operands, which may have an unreachable type, as valid integer sign-extended values. A corresponding test case has been added to verify this behavior. There are no review comments, and I have no additional feedback to provide.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| case ExtendS16Int64: | ||
| case ExtendS32Int64: | ||
| return unary->value; | ||
| return unary->value->type.isInteger() ? unary->value : nullptr; |
There was a problem hiding this comment.
[HIGH | Confidence: 5/5 | BUG]
While checking unary->value->type.isInteger() correctly addresses unary sign-extension operations whose operand became newly unreachable before refinalization, the shift-and-mask sign-extension pattern in the same function (lines 174–183) has the exact same vulnerability.
In getSignExtValue:
| return unary->value->type.isInteger() ? unary->value : nullptr; | |
| if (matches(curr, | |
| binary(ShrS, | |
| binary(Shl, any(&extended), ival(&leftShift)), | |
| ival(&rightShift)))) { | |
| auto leftShifts = Bits::getEffectiveShifts(leftShift, curr->type); | |
| auto rightShifts = Bits::getEffectiveShifts(rightShift, curr->type); | |
| if (leftShifts != 0 && leftShifts == rightShifts) { | |
| return extended; | |
| } | |
| } |
If extended becomes unreachable prior to refinalization propagating unreachable types to the outer Shl and ShrS nodes (for instance, (i32.shr_s (i32.shl (array.len (ref.null array)) 24) 24)), curr->type.isInteger() is initially true, extended is bound to the unreachable expression, and extended is returned.
Callers such as makeZeroExt (which calls Abstract::getBinary(curr->type, Abstract::And)) or Properties::getSignExtBits (which asserts curr->type.isInteger()) will crash when passed this unreachable node.
Consider verifying extended->type.isInteger() for the shift pattern as well:
| return unary->value->type.isInteger() ? unary->value : nullptr; | |
| if (leftShifts != 0 && leftShifts == rightShifts && | |
| extended->type.isInteger()) { | |
| return extended; | |
| } |
There was a problem hiding this comment.
This is a false positive. If the left child of the inner shift is unreachable, then binary(Shl, any(&extended), ival(&leftShift) will not match.
| case ExtendS16Int64: | ||
| case ExtendS32Int64: | ||
| return unary->value; | ||
| return unary->value->type.isInteger() ? unary->value : nullptr; |
There was a problem hiding this comment.
Please add a comment here, as otherwise this looks like dead code.
In
OptimizeInstructions, refinalization is deferred until the end of the function walk, so replacing a child expression (such as a trappingarray.lenon a null reference) withunreachableleaves an enclosing unary sign-extension instruction (i32.extend8_s, etc.) with its original integer type while its operand hasType::unreachable. While the shift-pair branch ofProperties::getSignExtValuealready rejectsType::unreachableoperands viaAbstract::Shl, the unary branch returnedunary->valuewithout checking that its type is still an integer, causingOptimizeInstructions::makeZeroExtto construct an integer mask literal withType::unreachableand abort. Checkunary->value->type.isInteger()inProperties::getSignExtValue.