v8: backport 45e2628d and d6fb6de7 from upstream #6384
Conversation
Original commit message:
Add a separate scope for switch
The ES2015 specification for switch statements 13.12.11 specifies that
they get their own lexical scope. This patch introduces such a scope
through a complex desugaring in terms of blocks, done so that Crankshaft
does not have to be updated to support multiple constructs providing
scopes.
Recommitting this patch after a bug fix in Crankshaft to allow a
desugaring with certain elements missing a source location:
https://codereview.chromium.org/1313443002
BUG=v8:4377
LOG=Y
R=adamk
Review URL: https://codereview.chromium.org/1309163003
Cr-Commit-Position: refs/heads/master@{nodejs#30340}
Original commit message:
Ensure hole checks take place in switch statement scopes
Switch statements introduce their own scope for cases, but this scope
is not necessarily executed in order, as the following function shows:
switch (x) {
case 1:
let y = 1;
case 2:
y = 2;
case 3:
print(y);
}
If x = 2 or x = 3, the code should throw a ReferenceError. However,
FullCodeGen's hole check elimination used the simple algorithm of
assuming that if the initializer was in the same scope, then it was
reached before the use, and therefore the hole check could be
eliminated.
This patch adds an extra bit to scopes, to track if they may
nonlinearly. The parser marks the scope that switch introduces as
nonlinear. FullCodeGen does not eliminate the hole check from
a scope which is nonlinear. This patch refactors FullCodeGen to
put the hole check elimination in one place, rather than in each
backend.
BUG=v8:3926
LOG=Y
R=adamk
Review URL: https://codereview.chromium.org/1312613003
Cr-Commit-Position: refs/heads/master@{nodejs#30453}
|
took the liberty to update the PR title to be more descriptive of the changed proposed. |
|
@nodejs/release Are we still doing v5 releases now that v6 is out? If not, there is not much point in reviewing and/or landing this. |
|
It's possible, yes. We've been saying that v5 will be supported for up to two months after v6, largely to give people time to transition just in case any significant regressions or security issues come up. Really, tho, it's going to depend on whether anyone steps up to backport stuff and drive the release. |
|
If you guys aren't cutting anymore v5 updates it's moot. I wasn't aware how close we were to v6, though this affects v4 as well. Like I mentioned in the comment above I can port this to v4 since I'm already familiar with this code and the test cases. Let me know if you want me to move forward on that front. |
|
Backporting to v4 sounds like a good idea. @mhdawson How much work is it to extend the test matrix for V8's test suite beyond x64? |
|
As covered in nodejs/build#387 it depends on what we want to extend it to. For some platforms the issue having the right hardware available. From the discussion it sounds like you'd want ia32. @jbergstroem is there a machine with 8G memory that is ia32 ? |
|
@mhdawson nope; it most often doesn't make sense to run 32-bit installs with more than 4G ram but I reckon kernels with PAE would handle it. Should I look at setting this up? Are google testing this? |
And ARM + optionally PPC. |
|
PPC is already running, ARM I tried but the hardware was just not reliable/capable enough |
|
Google does test 32 bit - https://build.chromium.org/p/client.v8/console. I believe the builds under "linux" would be 32 bit. I doubt that they test on 32 bit with more than 4G though, so its possible that for 32 bit the tests run differently so if you point me at one that has 4G I'll add it and see what happens. |
|
As v5.x is now eol I am going to close this. Please feel free to let me know if this was inappropriate |
Checklist
Affected core subsystem(s)
v8
Description of change
Backports two commits from v8 upstream, 45e2628d + d6fb6de7. The first commit is not strictly required to fix the issue, but large changes were made to the switch block parser and a clean merge is very desirable in cases like these.
This fixes an issue with scoping in switch blocks which will cause a full server crash. An example of code which can trigger this bug follows:
This affects both the v4.x and v5.x branches. master is unaffected since its v8 version already includes those changes. I'm not 100% sure I'm PR'ing to the right branch, so I held off on doing v4.x. If y'all give me the go ahead I can get a PR out for v4.x as well.