v8: fix debug builds on Windows

[bzoz]

Adds missing return which fixes Node 8.x debug builds on Windows, while we wait for upstream patch to land.

Fixes: #13392

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• commit message follows commit guidelines

Affected core subsystem(s)

v8

[refack]

Code LGTM (confirmed fixes issue)

[gibfahn]

cc/ @nodejs/v8

[targos]

Is the master branch also affected? I so this should target it. I will then include the commit in #13631

[gibfahn]

Is the master branch also affected? I so this should target it. I will then include the commit in #13631

That makes sense, I think we should try to avoid v8.x/master differences where possible.

[addaleax]

Is the master branch also affected?

No, this came from c8be718 (which contains parts of v8/v8@18a26cfe174) as parts of the ABI compatibility patches. The breaking code is in V8 6.0, so we may need to re-apply it once we upgrade, but ideally we could do that over upstream merge requests.

This should be safe to fast-track, it’s just adding an unused return value.

[addaleax]

Landed in #13598 as a0f8faa

[refack]

Is the master branch also affected?

No

Confirmed (empirically).

[addaleax]

Comment by @hammacher on the upstream patch:

Compilers should be able to deduce that UNIMPLEMENTED() does not return, and
should not complain.
Can you check whether this CL would also have fixed the issue:
https://chromium-review.googlesource.com/529072
(background: we require C++11 now, and all C++11-capable compilers should honour
the [[noreturn]] annotation)

If yes, we could revert this one.

Can somebody here check that? It seems reasonable to me but I’m not a Windows person.

[bzoz]

I'll try to compile with that patch and get back to you.

[bzoz]

Yep, that CL also fixes the issue

[refack]

Just FYI issue does not happen on VS2017

[gibfahn]

Comment by @hammacher on the upstream patch:

Compilers should be able to deduce that UNIMPLEMENTED() does not return, and
should not complain.
Can you check whether this CL would also have fixed the issue:
https://chromium-review.googlesource.com/529072
(background: we require C++11 now, and all C++11-capable compilers should honour
the [[noreturn]] annotation)

If yes, we could revert this one.

Can somebody here check that? It seems reasonable to me but I’m not a Windows person.

@prepare ^^ FYI (as you opened #13392)