GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — v8: fix debug builds on Windows by bzoz · Pull Request #13634 · nodejs/node

Conversation

@bzoz

@bzoz bzoz commented Jun 12, 2017

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bzoz
v8: fix debug builds on Windows
2e2268f 
Adds missing return which fixes debug builds on Windows

Fixes: nodejs#13392
@bzoz bzoz added c++ Issues and PRs that require attention from people who are familiar with C++. v8 engine Issues and PRs related to the V8 dependency. v8.x windows Issues and PRs related to the Windows platform. labels Jun 12, 2017
@nodejs-github-bot nodejs-github-bot added v8 engine Issues and PRs related to the V8 dependency. v8.x labels Jun 12, 2017
refack
refack approved these changes Jun 12, 2017
View reviewed changes

@refack refack left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM (confirmed fixes issue)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@gibfahn

gibfahn commented Jun 12, 2017

Copy link
Copy Markdown
Member

cc/ @nodejs/v8
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@targos

targos commented Jun 12, 2017

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gibfahn

gibfahn commented Jun 12, 2017

Copy link
Copy Markdown
Member

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@addaleax

addaleax commented Jun 12, 2017

Copy link
Copy Markdown
Member

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
addaleax pushed a commit that referenced this pull request Jun 12, 2017
@bzoz @addaleax
v8: fix debug builds on Windows
a0f8faa 
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

addaleax commented Jun 12, 2017

Copy link
Copy Markdown
Member

Landed in #13598 as a0f8faa
bzoz and bricss reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@addaleax addaleax closed this Jun 12, 2017
@addaleax addaleax mentioned this pull request Jun 12, 2017
v8.1.1 proposal #13598
Merged
@refack

refack commented Jun 12, 2017

Copy link
Copy Markdown
Contributor

Is the master branch also affected?

No

Confirmed (empirically).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@addaleax

addaleax commented Jun 13, 2017

Copy link
Copy Markdown
Member

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bzoz

bzoz commented Jun 13, 2017

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bzoz

bzoz commented Jun 13, 2017

Copy link
Copy Markdown
Contributor Author

Yep, that CL also fixes the issue
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@refack

refack commented Jun 13, 2017

Copy link
Copy Markdown
Contributor

Just FYI issue does not happen on VS2017
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gibfahn

gibfahn commented Jun 13, 2017

Copy link
Copy Markdown
Member

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)
prepare reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
Auditing for 6.11.1 nodejs/Release#230
Closed
2 of 3 tasks
addaleax pushed a commit that referenced this pull request Jun 17, 2017
@bzoz @addaleax
v8: fix debug builds on Windows
d44020a 
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 addaleax mentioned this pull request Jun 17, 2017
v8.2.0 proposal #13744
Merged
addaleax pushed a commit that referenced this pull request Jun 21, 2017
@bzoz @addaleax
v8: fix debug builds on Windows
43f0765 
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 pull request Jun 24, 2017
@bzoz @addaleax
v8: fix debug builds on Windows
fc64bca 
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 pull request Jun 29, 2017
@bzoz @rvagg
v8: fix debug builds on Windows
6b37a1a 
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 addaleax added dont-land-on-v4.x labels Jul 10, 2017
addaleax pushed a commit that referenced this pull request Jul 11, 2017
@bzoz @addaleax
v8: fix debug builds on Windows
c7c3e62 
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 pull request Jul 18, 2017
@bzoz @addaleax
v8: fix debug builds on Windows
54a9b6c 
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 pull request Jul 21, 2017
@bzoz @targos
v8: fix debug builds on Windows
31142a7 
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 pull request Jul 24, 2017
@bzoz @addaleax
v8: fix debug builds on Windows
55a1231 
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 addaleax mentioned this pull request Jul 24, 2017
v8.3.0 proposal #14452
Closed
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Aug 2, 2017
@bzoz @MylesBorins
v8: fix debug builds on Windows
6afa220 
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

Reviewers

1 more reviewer
@refack refack refack approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. v8 engine Issues and PRs related to the V8 dependency. windows Issues and PRs related to the Windows platform.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@bzoz @gibfahn @targos @addaleax @refack @nodejs-github-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.