[Interpreter] Ensure that implicit return undefined is generated.

[Interpreter] Ensure that implicit return undefined is generated.

When there is no explicit return we need to generate an implicit
return undefined.

BUG=v8:4280
LOG=N

Committed: https://crrev.com/c9f0368adda0436569076065367e59320327ca00
Cr-Commit-Position: refs/heads/master@{#30639}

[rmcilroy, 2015-09-07 23:25:06]

The CQ bit was checked by rmcilroy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-07 23:25:15]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1308693014/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1308693014/1

[commit-bot: I haz the power, 2015-09-07 23:50:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-07 23:50:29]

Dry run: This issue passed the CQ dry run.

[rmcilroy, 2015-09-08 08:19:24]

rmcilroy@chromium.org changed reviewers:
+ mstarzinger@chromium.org, oth@chromium.org

[rmcilroy, 2015-09-08 08:19:24]

Orion, please have a look, thanks.

Michi for owner stamp (comments welcome)

[Michael Starzinger, 2015-09-08 08:25:10]

I am not an owner of anything in this CL, but I am happy to offer my thoughts
...

https://codereview.chromium.org/1308693014/diff/1/src/interpreter/bytecode-ge...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1308693014/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:50: if (!builder_.HasExplicitReturn()) {
This looks dangerous. Depending on how control flow is modeled there could very
well be a case where the last instruction in the byte-code stream is a return
statement, but it is not guaranteed post-dominate all executions paths. Consider
the following ...

function f(a) {
  if (a) { return 23; }
}

[rmcilroy, 2015-09-08 09:41:59]

> I am not an owner of anything in this CL, but I am happy to 
> offer my thoughts
> ...

Sure, but Orion is not a committer yet and I need a committer stamp :). Your
thoughts are much appreciated!

https://codereview.chromium.org/1308693014/diff/1/src/interpreter/bytecode-ge...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1308693014/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:50: if (!builder_.HasExplicitReturn()) {
On 2015/09/08 08:25:10, Michael Starzinger wrote:
> This looks dangerous. Depending on how control flow is modeled there could
very
> well be a case where the last instruction in the byte-code stream is a return
> statement, but it is not guaranteed post-dominate all executions paths.
Consider
> the following ...
> 
> function f(a) {
>   if (a) { return 23; }
> }

Good point, we need to take this into account, but we would need to take this
into account once we start adding control flow in any case since the if block
will need to jump to an actual bytecode, and the bytecode array builder will
need to back-patch the jump instruction with the target of the jump.

The last thing the bytecode-generator would do in the graph above would be to
try and back-patch the if jump to target the next bytecode which is to be
emitted. Since that bytecode doesn't exist yet we could easily spot in the
bytecode-array-builder that there is a jump instruction which is patched to a
non-existent bytecode (e.g., just having a field tracking the greatest bytecode
indice which was patched into a jump instruction and check if that is
greater-than/equal than bytecodes_.size()), and add that to the
HasExplicitReturn conditions? WDYT?

[oth, 2015-09-08 10:38:59]

On 2015/09/08 09:41:59, rmcilroy wrote:
> > I am not an owner of anything in this CL, but I am happy to 
> > offer my thoughts
> > ...
> 
> Sure, but Orion is not a committer yet and I need a committer stamp :). Your
> thoughts are much appreciated!
> 
>
https://codereview.chromium.org/1308693014/diff/1/src/interpreter/bytecode-ge...
> File src/interpreter/bytecode-generator.cc (right):
> 
>
https://codereview.chromium.org/1308693014/diff/1/src/interpreter/bytecode-ge...
> src/interpreter/bytecode-generator.cc:50: if (!builder_.HasExplicitReturn()) {
> On 2015/09/08 08:25:10, Michael Starzinger wrote:
> > This looks dangerous. Depending on how control flow is modeled there could
> very
> > well be a case where the last instruction in the byte-code stream is a
return
> > statement, but it is not guaranteed post-dominate all executions paths.
> Consider
> > the following ...
> > 
> > function f(a) {
> >   if (a) { return 23; }
> > }
> 
> Good point, we need to take this into account, but we would need to take this
> into account once we start adding control flow in any case since the if block
> will need to jump to an actual bytecode, and the bytecode array builder will
> need to back-patch the jump instruction with the target of the jump.
> 
> The last thing the bytecode-generator would do in the graph above would be to
> try and back-patch the if jump to target the next bytecode which is to be
> emitted. Since that bytecode doesn't exist yet we could easily spot in the
> bytecode-array-builder that there is a jump instruction which is patched to a
> non-existent bytecode (e.g., just having a field tracking the greatest
bytecode
> indice which was patched into a jump instruction and check if that is
> greater-than/equal than bytecodes_.size()), and add that to the
> HasExplicitReturn conditions? WDYT?

An alternative would be to append "Ldar undefined; return" in ToBytecodeArray
and add a comment that this needs to be addressed in the soon to start flow
control activity.

As long as we have a clear comment either way, in the near term, then lgtm.

[Michael Starzinger, 2015-09-08 12:42:07]

LGTM (rubber-stamped).

[rmcilroy, 2015-09-08 14:14:46]

> An alternative would be to append "Ldar undefined; return" in ToBytecodeArray
> and add a comment that this needs to be addressed in the soon to start flow
> control activity.
>
> As long as we have a clear comment either way, in the near term, then lgtm.

Added a comment in HasExplicitReturn. Thanks for the reviews!

[rmcilroy, 2015-09-08 14:15:17]

The CQ bit was checked by rmcilroy@chromium.org

[rmcilroy, 2015-09-08 14:15:17]

The patchset sent to the CQ was uploaded after l-g-t-m from oth@chromium.org,
mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/1308693014/#ps20001
(title: "Add comment")

[commit-bot: I haz the power, 2015-09-08 14:15:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1308693014/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1308693014/20001

[commit-bot: I haz the power, 2015-09-08 15:02:52]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-09-08 15:03:07]

Patchset 2 (id:??) landed as
https://crrev.com/c9f0368adda0436569076065367e59320327ca00
Cr-Commit-Position: refs/heads/master@{#30639}