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

NodeJs v8.0.0-v8.1.0, C4716: 'v8::ArrayBuffer::Allocator::Reserve': must return a value #13392

Closed
prepare opened this issue Jun 2, 2017 · 12 comments
Labels
build Issues and PRs related to build files or the CI. v8 engine Issues and PRs related to the V8 dependency. windows Issues and PRs related to the Windows platform.

Comments

@prepare
Copy link

prepare commented Jun 2, 2017

Do you forget to return the value ?

C4716: 'v8::ArrayBuffer::Allocator::Reserve': must return a value

nodejs
version v8.0.0
build from source (TODAY) from main web page (https://nodejs.org/en/download/current/)
platform: build with VS2015, x64, Win7


In main source from webpage.
This file, dep/v8/src/api.cc

node_1

this method (1) should return 'something' (2), it is not void.
so VS2015-> compile error -> semantic check error.


I compare the source from the original main web (3) (https://github.com/nodejs/node/blob/master/deps/v8/src/api.cc#L440)
and the source on github. It has a little diff.


@addaleax
Copy link
Member

addaleax commented Jun 2, 2017

UNIMPLEMENTED() just crashes, there’s no point in returning anything; can you show the actual error that you get from compiling?

@addaleax addaleax added build Issues and PRs related to build files or the CI. v8 engine Issues and PRs related to the V8 dependency. windows Issues and PRs related to the Windows platform. labels Jun 2, 2017
@prepare
Copy link
Author

prepare commented Jun 2, 2017

node_2

This is the only one error when I build it.

@prepare
Copy link
Author

prepare commented Jun 2, 2017

node_3

To pass that, I fix it by adding return nullptr;

@joaocgreis
Copy link
Member

I confirm this happens in debug builds.

@prepare
Copy link
Author

prepare commented Jun 9, 2017

Still not fixed in NodeJS v 8.1.0 (main source from webpage)
err01

@aqrln
Copy link
Contributor

aqrln commented Jun 9, 2017

As far as I understand, this is an issue with V8 sources, and thus should be reported in their issue tracker, shouldn't it?

@prepare
Copy link
Author

prepare commented Jun 9, 2017

@aqrln,

But the error is 'included' in nodejs's official source snapshot from nodejs's main page.

I think we should patch it before release to other node users (that build from src).

for me, I'm not hurry :)

But I've never see this (err) before in the official node source.

:)

@gibfahn
Copy link
Member

gibfahn commented Jun 9, 2017

But the error is 'included' in nodejs's official source snapshot from nodejs's main page.

We take everything in deps/ from the maintainers of those projects, and we try not to float patches unless necessary. So in this case nodejs/node deps/v8/src/api.cc#L436 corresponds to v8/v8 src/api.cc#L452.

So assuming that this is a V8 issue (and that it's not something special we're doing in Node) the way to get it fixed is to raise it upstream.

@addaleax
Copy link
Member

addaleax commented Jun 9, 2017

As far as I understand, this is an issue with V8 sources, and thus should be reported in their issue tracker, shouldn't it?

Just so you know, this is a temporary situation; these methods will become pure virtual at some point in the (possibly near) future.

Anyway, here’s an upstream patch: https://codereview.chromium.org/2929993003/

@bzoz
Copy link
Contributor

bzoz commented Jun 9, 2017

It happens only in Debug builds because in Release it is suppressed by whole program optimization. When building V8 by itself function level linking is enabled and it also suppress this.

@TimothyGu
Copy link
Member

Adding the exact error message so the issue is searchable: C4716: 'v8::ArrayBuffer::Allocator::Reserve': must return a value

@prepare prepare changed the title NodeJs v8.0.0 build error in v8/src/api.cc, Do you forget to return the value? NodeJs v8.0.0 build error in v8/src/api.cc, C4716: 'v8::ArrayBuffer::Allocator::Reserve': must return a value Jun 12, 2017
@prepare prepare changed the title NodeJs v8.0.0 build error in v8/src/api.cc, C4716: 'v8::ArrayBuffer::Allocator::Reserve': must return a value NodeJs v8.0.0-v8.1.0 build error in v8/src/api.cc, C4716: 'v8::ArrayBuffer::Allocator::Reserve': must return a value Jun 12, 2017
@prepare prepare changed the title NodeJs v8.0.0-v8.1.0 build error in v8/src/api.cc, C4716: 'v8::ArrayBuffer::Allocator::Reserve': must return a value NodeJs v8.0.0-v8.1.0, C4716: 'v8::ArrayBuffer::Allocator::Reserve': must return a value Jun 12, 2017
bzoz added a commit to JaneaSystems/node that referenced this issue Jun 12, 2017
Adds missing return which fixes debug builds on Windows

Fixes: nodejs#13392
addaleax pushed a commit that referenced this issue Jun 12, 2017
Adds missing return which fixes debug builds on Windows

Fixes: #13392
Ref: https://codereview.chromium.org/2929993003/
PR-URL: #13634
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
kisg pushed a commit to paul99/v8mips that referenced this issue Jun 13, 2017
Return `nullptr` from `ArrayBuffer::Allocator::Reserve` because
apparently not doing so results in compile errors for some people.

BUG=

Ref: nodejs/node#13392
Review-Url: https://codereview.chromium.org/2929993003
Cr-Commit-Position: refs/heads/master@{#45886}
@bzoz
Copy link
Contributor

bzoz commented Jun 13, 2017

Fixed in a0f8faa

@bzoz bzoz closed this as completed Jun 13, 2017
addaleax pushed a commit that referenced this issue Jun 17, 2017
Adds missing return which fixes debug builds on Windows

Fixes: #13392
Ref: https://codereview.chromium.org/2929993003/
PR-URL: #13634
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
addaleax pushed a commit that referenced this issue Jun 21, 2017
Adds missing return which fixes debug builds on Windows

Fixes: #13392
Ref: https://codereview.chromium.org/2929993003/
PR-URL: #13634
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
addaleax pushed a commit that referenced this issue Jun 24, 2017
Adds missing return which fixes debug builds on Windows

Fixes: #13392
Ref: https://codereview.chromium.org/2929993003/
PR-URL: #13634
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
rvagg pushed a commit that referenced this issue Jun 29, 2017
Adds missing return which fixes debug builds on Windows

Fixes: #13392
Ref: https://codereview.chromium.org/2929993003/
PR-URL: #13634
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
addaleax pushed a commit that referenced this issue Jul 11, 2017
Adds missing return which fixes debug builds on Windows

Fixes: #13392
Ref: https://codereview.chromium.org/2929993003/
PR-URL: #13634
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
kisg pushed a commit to paul99/v8mips that referenced this issue Jul 18, 2017
…atchset #1 id:1 of https://codereview.chromium.org/2929993003/ )

Reason for revert:
not needed any more, and contradicts our cleanup efforts: https://chromium-review.googlesource.com/c/507287/

Original issue's description:
> [api] Fix compilation error for `UNIMPLEMENTED()` method
>
> Return `nullptr` from `ArrayBuffer::Allocator::Reserve` because
> apparently not doing so results in compile errors for some people.
>
> BUG=
>
> Ref: nodejs/node#13392
> Review-Url: https://codereview.chromium.org/2929993003
> Cr-Commit-Position: refs/heads/master@{#45886}
> Committed: https://chromium.googlesource.com/v8/v8/+/f14d1b62313329f421f12cae429673c1b13018f9

[email protected],[email protected]

Review-Url: https://codereview.chromium.org/2946933002
Cr-Commit-Position: refs/heads/master@{#46739}
addaleax pushed a commit that referenced this issue Jul 18, 2017
Adds missing return which fixes debug builds on Windows

Fixes: #13392
Ref: https://codereview.chromium.org/2929993003/
PR-URL: #13634
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
targos pushed a commit to targos/node that referenced this issue Jul 21, 2017
Adds missing return which fixes debug builds on Windows

Fixes: nodejs#13392
Ref: https://codereview.chromium.org/2929993003/
PR-URL: nodejs#13634
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
addaleax pushed a commit that referenced this issue Jul 24, 2017
Adds missing return which fixes debug builds on Windows

Fixes: #13392
Ref: https://codereview.chromium.org/2929993003/
PR-URL: #13634
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit to MylesBorins/node that referenced this issue Aug 2, 2017
Adds missing return which fixes debug builds on Windows

Fixes: nodejs#13392
Ref: https://codereview.chromium.org/2929993003/
Refs: nodejs#13634

PR-URL: nodejs#14582
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. v8 engine Issues and PRs related to the V8 dependency. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

No branches or pull requests

8 participants