Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

improve --reduce-test #3719

Merged
merged 1 commit into from
Feb 14, 2020
Merged

improve --reduce-test #3719

merged 1 commit into from
Feb 14, 2020

Conversation

alexlamsl
Copy link
Collaborator

  • cover missing cases when eliminating unreferenced labels

@kzc this is #3716 (comment)

@kzc
Copy link
Contributor

kzc commented Feb 13, 2020

Makes sense. I recognize that label body code that worked at one point. It may have broken after the big else if list of AST classes was made.

fyi, test/input/reduce/label.js only works with Node 10. Using latest commit without this PR:

$ node-v0.10.41 bin/uglifyjs test/input/reduce/label.js -c --reduce-test
// Can't reproduce test failure with minify options provided:
// {"compress":true,"mangle":false}
$ node-v0.12.9 bin/uglifyjs test/input/reduce/label.js -c --reduce-test
// Can't reproduce test failure with minify options provided:
// {"compress":true,"mangle":false}
$ node-v4.2.1 bin/uglifyjs test/input/reduce/label.js -c --reduce-test
// Can't reproduce test failure with minify options provided:
// {"compress":true,"mangle":false}
$ node-v6.9.0 bin/uglifyjs test/input/reduce/label.js -c --reduce-test
// Can't reproduce test failure with minify options provided:
// {"compress":true,"mangle":false}
$ node-v8.0.0 bin/uglifyjs test/input/reduce/label.js -c --reduce-test
// Can't reproduce test failure with minify options provided:
// {"compress":true,"mangle":false}
$ node-v10.4.1 bin/uglifyjs test/input/reduce/label.js -c --reduce-test
// reduce test pass 1, iteration 0: 135 bytes
// reduce test pass 1, iteration 25: 84 bytes
// reduce test pass 1, iteration 50: 84 bytes
// reduce test pass 1: 84 bytes
// reduce test pass 2: 84 bytes
// reduce test pass 3: 84 bytes
var a = 0, o = this;

for (var k in o) {
    var b = this[k];
    if (b) {
        L17060: {
            a++;
        }
    }
}

var c;

console.log(a);
// output: 3
// minify: 2
// options: {"compress":true,"mangle":false}
$ node-v12.13.0 bin/uglifyjs test/input/reduce/label.js -c --reduce-test
// Can't reproduce test failure with minify options provided:
// {"compress":true,"mangle":false}

@kzc
Copy link
Contributor

kzc commented Feb 13, 2020

Likewise, the original case produced different results with different versions of Node:

$ node-v0.10.41 bin/uglifyjs 2235.js -c --reduce-test
// Can't reproduce test failure with minify options provided:
// {"compress":true,"mangle":false}
$ node-v6.9.0 bin/uglifyjs 2235.js -c --reduce-test
// reduce test pass 1, iteration 0: 29449 bytes
// reduce test pass 1, iteration 25: 13929 bytes
// reduce test pass 1, iteration 50: 11878 bytes
// reduce test pass 1, iteration 75: 11772 bytes
// reduce test pass 1, iteration 100: 11621 bytes
// reduce test pass 1, iteration 125: 11606 bytes
// reduce test pass 1, iteration 150: 11406 bytes
// reduce test pass 1, iteration 175: 10900 bytes
// reduce test pass 1, iteration 200: 10370 bytes
// reduce test pass 1, iteration 225: 10334 bytes
// reduce test pass 1: 383 bytes
// reduce test pass 2: 140 bytes
// reduce test pass 3: 117 bytes
var expr1 = this;

var Infinity_2 = expr1;

var NaN = function f0() {
    var b_2 = Infinity_2.undefined >>= 0;
}();

console.log(undefined);
// output: 0
// minify: undefined
// options: {"compress":true,"mangle":false}
$ node-v10.4.1 bin/uglifyjs 2235.js -c --reduce-test
// reduce test pass 1, iteration 0: 29449 bytes
// reduce test pass 1, iteration 25: 13929 bytes
// reduce test pass 1, iteration 50: 13923 bytes
// reduce test pass 1, iteration 75: 11909 bytes
// reduce test pass 1, iteration 100: 11884 bytes
// reduce test pass 1, iteration 125: 11877 bytes
// reduce test pass 1, iteration 150: 11875 bytes
// reduce test pass 1, iteration 175: 10968 bytes
// reduce test pass 1, iteration 200: 10607 bytes
// reduce test pass 1, iteration 225: 10529 bytes
// reduce test pass 1, iteration 250: 10307 bytes
// reduce test pass 1, iteration 275: 9472 bytes
// reduce test pass 1, iteration 300: 8854 bytes
// reduce test pass 1, iteration 325: 8580 bytes
// reduce test pass 1, iteration 350: 8522 bytes
// reduce test pass 1, iteration 375: 8520 bytes
// reduce test pass 1, iteration 400: 8239 bytes
// reduce test pass 1, iteration 425: 6725 bytes
// reduce test pass 1, iteration 450: 1205 bytes
// reduce test pass 1, iteration 475: 592 bytes
// reduce test pass 1: 567 bytes
// reduce test pass 2: 175 bytes
// reduce test pass 3: 164 bytes
var b = 0;

var expr1 = this;

for (var key1 in expr1) {
    var Infinity_2 = expr1[key1];
    if (Infinity_2) {
        0;
    } else {
        switch (0) {
          case 0:
            var a_2 = --b;
        }
    }
}

while (0) {
    var brake106 = 0;
}

console.log(b);
// output: -2
// minify: -3
// options: {"compress":true,"mangle":false}
$ node-v12.13.0 bin/uglifyjs 2235.js -c --reduce-test
// Can't reproduce test failure with minify options provided:
// {"compress":true,"mangle":false}

@alexlamsl
Copy link
Collaborator Author

Yeah, I've totally forgotten about that - pushed a new test case that is universal across all Node.js versions.

@alexlamsl
Copy link
Collaborator Author

There's also this 💎

$ node -v
v8.17.0

$ node
> require('./test/sandbox').run_code('var a,b=0;for(b in this)console.log(b)')
'a\nb\n'
$ node -v
v10.18.1

$ node
> require('./test/sandbox').run_code('var a,b=0;for(b in this)console.log(b)')
'b\na\n'

@kzc
Copy link
Contributor

kzc commented Feb 14, 2020

Globals traversal is apparently undefined behavior. Although property ordering is well defined in JS as order of definition, it's up to the JS engine to determine how it optimizes certain code.

Side note: when I was testing the original reduce_test implementation on old ufuzz test cases with the commit rolled back accordingly I'd frequently get unrelated NaN and undefined redefinition reduced cases. Just something to be suspicious of when you see such a reduced case in ufuzz output - try it on a different Node version.

@alexlamsl
Copy link
Collaborator Author

That's most likely due to the missing toplevel flag to sandbox.run_code(), which is patched in #3709

44499a6#diff-28c42a2a2ff0ec81eda66ad80750958cL370

@kzc
Copy link
Contributor

kzc commented Feb 14, 2020

An example of a NaN/undefined reduced test case is seen above in latest master, with that toplevel patch in effect.

@kzc
Copy link
Contributor

kzc commented Feb 14, 2020

My point was that a false positive reduced test case may on occasion mask a genuine bug. You cannot pick what's a good reduced test case and what is not - the algorithm will converge on one or the other as influenced by its environment (node globals, timeouts, whatever).

@alexlamsl
Copy link
Collaborator Author

Indeed − currently unless it's a clear false positive I tend to use the reduced test case to figure out what to modify in the original test case, then run to verify that is the only site that makes a difference.

@alexlamsl
Copy link
Collaborator Author

Just discovered that we can't reliably use test/reduce.js on Node.js 0.10 due to their lack of support for timeout under vm.runInContext().

@kzc
Copy link
Contributor

kzc commented Feb 14, 2020

Any idea why I cannot reproduce the output of the non-minified reduced result of node-v6.9.0 bin/uglifyjs 2235.js -c --reduce-test above?

I put the reduced test case in a file named 0.js and ran it against Node 6:

$ cat 0.js
var expr1 = this;

var Infinity_2 = expr1;

var NaN = function f0() {
    var b_2 = Infinity_2.undefined >>= 0;
}();

console.log(undefined);
// output: 0
// minify: undefined
// options: {"compress":true,"mangle":false}
$ cat 0.js | node-v6.9.0 
undefined
$ node-v6.9.0 0.js
undefined

- cover missing cases when eliminating unreferenced labels
- format multi-line outputs correctly
@alexlamsl
Copy link
Collaborator Author

That is most bizarre indeed − vm behaviour changes back and forth across Node.js versions:

$ node
> process.version
'v0.10.48'
> vm.runInNewContext(fs.readFileSync("0.js", "utf8"), { console: console })
undefined
undefined
$ node
> process.version
'v0.12.18'
> vm.runInNewContext(fs.readFileSync("0.js", "utf8"), { console: console })
0
undefined
$ node
> process.version
'v4.9.1'
> vm.runInNewContext(fs.readFileSync("0.js", "utf8"), { console: console })
0
undefined
$ node
> process.version
'v6.17.1'
> vm.runInNewContext(fs.readFileSync("0.js", "utf8"), { console: console })
0
undefined
$ node
> process.version
'v8.17.0'
> vm.runInNewContext(fs.readFileSync("0.js", "utf8"), { console: console })
undefined
undefined
$ node
> process.version
'v10.18.1'
> vm.runInNewContext(fs.readFileSync("0.js", "utf8"), { console: console })
undefined
undefined
$ node
> process.version
'v12.14.1'
> vm.runInNewContext(fs.readFileSync("0.js", "utf8"), { console: console })
undefined
undefined

@alexlamsl
Copy link
Collaborator Author

No discrepancies between Node.js versions if you replace undefined with say A

@kzc
Copy link
Contributor

kzc commented Feb 14, 2020

So some NaN/undefined/Infinity false positives are actually false false positives.

But the real problem is stranger still - obviously the code for 0.js was generated from the same version of node v6.9.0 by --reduce-test. Why would the same version of Node produce a different result from within reduce_test and from the command line?

@alexlamsl
Copy link
Collaborator Author

Because even on the same Node.js version, 0.js would give different result running directly or within vm, the latter of which is what --reduce-test uses for comparison.

@alexlamsl
Copy link
Collaborator Author

Ideally vm results should match that of native execution, so from that point of view Node.js 0.12 through to 6 are buggy in that department.

@alexlamsl alexlamsl merged commit f01f580 into mishoo:master Feb 14, 2020
@alexlamsl alexlamsl deleted the reduce-test branch February 14, 2020 02:47
@kzc
Copy link
Contributor

kzc commented Feb 14, 2020

That may be the reason why I got so many NaN/undefined/Infinity redefinition reduced test cases when creating reduce_test.

@alexlamsl
Copy link
Collaborator Author

Just curious − why do you settle on Node.js 6?

@kzc
Copy link
Contributor

kzc commented Feb 14, 2020

No particular reason. It works. With more packages going native async I'll be forced to upgrade soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants