1745949 - Differential testing: edgecase analysis miscomputes positive/negative

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P2)

Description

[lukas.bernhard]

Steps to reproduce:

During differential fuzzing, I encountered a miscomputation regarding negative 0.
The attached sample computes different values, depending on whether edgecase analysis is enabled or disable.
spidermonkey git commit: f465d324513f

function main() {
let v33;
v28 = 1n;

let v31 = -65536;
do {
    v33 = v31 * -1.0;
    v33 += v33;
    v31++;
} while (v31 < v28);

print(Object.is(v33, -0)); // true without edgecase analysis, false with edgecase analysis

}
main();

Actual results:

Running the sample with edgecase analysis disable (obj-x86_64-pc-linux-gnu/dist/bin/js --no-threads --cpu-count=1 --ion-offthread-compile=off --fuzzing-safe --differential-testing --ion-edgecase-analysis=off diff.js) the computation in v33 is negative 0.

Running the sample with edgecase analysis enabled (obj-x86_64-pc-linux-gnu/dist/bin/js --no-threads --cpu-count=1 --ion-offthread-compile=off --fuzzing-safe --differential-testing --ion-edgecase-analysis=on diff.js) the computation in v33 is positive 0.

Comment 1

[Nicolas B. Pierron [:nbp]]

Iain, can you have a look at this issue?

Comment 2

[Iain Ireland [:iain]]

This might be the straw that breaks the camel's back for edgecase analysis, or it might be a bug in range analysis.

The root of the problem is these two lines:

v33 = v31 * -1;
v33 += v33;

In range analysis, we see that v33+v33 does not have any consumers other than resume points. (We did OSR into the loop before reaching the print statement below, so the use of v33 in the call to Object.is is not visible.) We decide to truncate it. Because it is used in resume points, we create a cloned copy, which will be recovered on bailout.

In RangeAnalysis::truncate, we call needTruncation (which counter-intuitively sets the truncation kind for MAdd) before we clone, which means that the cloned node is also truncated.

The original MAdd is eliminated by DCE. The cloned copy survives until codegen. In edgecase analysis, to decide whether we need a negative zero check, we examine the consumers of v31*-1. The only consumer is the cloned MAdd. Because it is truncated, we decide not to generate a negative zero check for the multiplication.

nbp, do you know whether it's intended behaviour for the cloned MAdd in this case to be truncated? This comment from the bug that added cloning seems to imply that the cloned copy should not be truncated, which I think would fix this bug, but I might be missing something.

I've always found it confusing that calling needTruncation modifies the instruction. I wonder if we can/should move all mutation into the truncate() implementation instead. I wrote up a quick patch: it fixes this testcase, and doesn't break anything obvious, but I haven't checked performance yet.

Comment 3

[Nicolas B. Pierron [:nbp]]

(In reply to Iain Ireland [:iain] from comment #2)

nbp, do you know whether it's intended behaviour for the cloned MAdd in this case to be truncated? This comment from the bug that added cloning seems to imply that the cloned copy should not be truncated, which I think would fix this bug, but I might be missing something.

I confirm this previous comment, the goal of Recover instruction is to capture the non-truncated (raw) result.

I've always found it confusing that calling needTruncation modifies the instruction. I wonder if we can/should move all mutation into the truncate() implementation instead. I wrote up a quick patch: it fixes this testcase, and doesn't break anything obvious, but I haven't checked performance yet.

The reasoning for splitting the annotation and the mutation is that it is easier to reason about before applying mutations. Also, I admit this logic pre-date the introduction of Recover Instructions.

[eddited after re-looking at the code]

Comment 4

[Iain Ireland [:iain]]

Attachment: Bug 1745949: Clone before truncating r=jandem,nbp

Before truncating an instruction, RangeAnalysis sometimes creates a cloned copy for use in the bailout path. The original design intent was that the cloned copy would not be truncated. However, we currently call needTruncation to decide whether to truncate a node, then clone it, then call truncate. Somewhat counterintuitively, most implementations of needTruncation modify the truncation kind for the node. Because this happens before we clone, the cloned copy ends up being truncated.

In this particular fuzzbug, edgecase analysis sees that the only consumer of a multiplication by -1 is a truncated addition, and removes the negative zero check. If we clone the MAdd before truncating it, then edgecase analysis does the right thing.

To accomplish this, for each MIR instruction that implements needTruncation/truncate, I made needTruncation a const function and moved any mutation into truncate itself. This ensures that the cloned node will be an unmodified copy of the original.

I took a quick look at Octane, and this doesn't appear to have affected our score.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4a3fbe0d0746
Clone before truncating r=jandem,nbp

Comment 6

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/4a3fbe0d0746