ScanDominatorsForDefs(MBasicBlock*) uses parameter block instead of loop variable i, reducing scan to a single-block check
Categories
(Core :: JavaScript Engine: JIT, defect, P2)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox152 | --- | fixed |
People
(Reporter: zchengchen0411, Assigned: zchengchen0411)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/147.0.0.0 Safari/537.36
Steps to reproduce:
Code audit of js/src/jit/ValueNumbering.cpp, specifically the single-argument overload of ScanDominatorsForDefs at lines 218–231.
Actual results:
The loop body uses the parameter block instead of the loop variable i:
static bool ScanDominatorsForDefs(MBasicBlock* block) {
for (MBasicBlock* i = block;;) {
if (BlockHasInterestingDefs(block)) { // <-- should be `i`
return true;
}
MBasicBlock* immediateDominator = i->immediateDominator();
if (immediateDominator == i) {
break;
}
i = immediateDominator;
}
return false;
}
Although i is updated on each iteration, it is never used in the loop body.
As a result, the function is effectively equivalent to:
return BlockHasInterestingDefs(block);
The intended dominator-chain traversal does not actually occur.
Expected results:
The loop should call BlockHasInterestingDefs(i), so that it correctly walks up
the dominator tree and checks each ancestor block.
This matches both the function comment:
"Walk up the dominator tree from |block| to the root and test for any defs"
and the behavior of the related two-argument overload.
Potential impact:
This can suppress a necessary GVN rerun.
The path is reachable from IsDominatorRefined (around lines 265–266):
if (block == old) {
return block != now && ScanDominatorsForDefs(now);
}
When a self-dominating block (e.g., a normal + OSR merge point that is not a
loop header) has a predecessor removed and ComputeNewDominator returns a new
dominator now != block, the scan is expected to walk up the dominator tree.
If now itself has no interesting definitions but one of its dominator
ancestors does, the buggy implementation returns false, incorrectly skipping
the GVN rerun.
Impact is limited to missed optimizations. There is no miscompilation or
memory safety issue. Triggering requires a specific CFG topology, so practical
impact is likely low.
Fix suggestion:
- if (BlockHasInterestingDefs(block)) {
+ if (BlockHasInterestingDefs(i)) {
The single-argument overload of ScanDominatorsForDefs was intended to walk
up the dominator tree from |block| to the root, checking each block for
defs interesting to GVN. However, while the loop variable |i| is updated
on each iteration, the call to BlockHasInterestingDefs mistakenly uses
|block| instead of |i|. As a result, every iteration checks the same
starting block and the function effectively reduces to:
BlockHasInterestingDefs(block)
This path is reachable from IsDominatorRefined when |block == old| and
ComputeNewDominator returns a new dominator distinct from |block|. In
such cases, if the new dominator itself has no interesting defs but one
of its ancestors does, the buggy implementation returns false and
incorrectly suppresses a GVN rerun.
The impact is limited to missed optimization (a GVN pass not being
triggered). There is no correctness or memory safety issue.
Fix by using the loop variable |i|, matching the two-argument overload
and the function's documentation.
|
||
Updated•6 days ago
|
|
|
||
Updated•6 days ago
|
|
||
Comment 5•5 days ago
|
||
| bugherder | ||
Description
•