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.

Sign up for GitHub

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

Jump to bottom

build: disable libstdc++ debug containers globally #30147

Closed
wants to merge 1 commit into from
Closed

build: disable libstdc++ debug containers globally #30147

wants to merge 1 commit into from

Conversation

bnoordhuis
Copy link
Member

Make sure _GLIBCXX_DEBUG is (un)defined consistently by turning the
disable_glibcxx_debug in common.gypi into a global variable that
overrides the definition in toolchain.gypi.

Different parts of the debug build were using differently sized
std::vectors and that ended about as well as you would expect.

Fixes: #30056

@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Oct 27, 2019
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@Fishrock123 Fishrock123 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 19, 2019
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
mhdawson
mhdawson approved these changes Dec 6, 2019
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@BridgeAR
Copy link
Member

@bnoordhuis this failure https://ci.nodejs.org/job/node-test-commit-linux-containered/16694/nodes=ubuntu1804_sharedlibs_debug_x64/console seems to be related.

@BridgeAR BridgeAR removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 10, 2019
@BridgeAR
Copy link
Member

Ping @bnoordhuis

@BridgeAR
Copy link
Member

BridgeAR commented Jan 2, 2020

@bnoordhuis PTAL at the mentioned failure.

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@bnoordhuis
build: disable libstdc++ debug containers globally
8bb84d6 
Different parts of the debug build were using differently sized
std::vectors due to `_GLIBCXX_DEBUG` sometimes being defined and
sometimes not. That ended about as well as you would expect.

Remove the flag.

Fixes: nodejs#30056
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/28158/

@bnoordhuis
Copy link
Member Author

Thanks for the ping, Ruben, and sorry for the delay. I decided to simply remove the setting since we want it disabled anyway.

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 12, 2020
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/28381/

@BridgeAR
Copy link
Member

It would be good to get a confirmation of some of the LGs, since this has changed since the PR was opened.

richardlau
richardlau reviewed Jan 12, 2020
View reviewed changes
common.gypi
@@ -30,10 +30,6 @@
'openssl_fips%': '',
'openssl_no_asm%': 0,

# Some STL containers (e.g. std::vector) do not preserve ABI compatibility
# between debug and non-debug mode.
'disable_glibcxx_debug': 1,
Copy link
Member

Choose a reason for hiding this comment

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

So this may not have had the desired effect (being overridden by the value in toolchain.gypi) but I'm a little hesitant about removing it completely from common.gypi from the release lines as that's visible to addon authors using node-gyp.

Copy link
Member Author

Choose a reason for hiding this comment

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

I know it's not an exact science but GH search doesn't turn up any binding.gyp files nor any JS or TS source files that contain the string disable_glibcxx_debug.

I think it's a pretty safe bet it's unused because it was non-functional anyway.

@BridgeAR BridgeAR removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 12, 2020
@bnoordhuis bnoordhuis added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 9, 2020
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/29037/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/29641/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/29732/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/29776/

addaleax pushed a commit that referenced this pull request Mar 12, 2020
@bnoordhuis @addaleax
build: disable libstdc++ debug containers globally
7f44d2c 
Different parts of the debug build were using differently sized
std::vectors due to `_GLIBCXX_DEBUG` sometimes being defined and
sometimes not. That ended about as well as you would expect.

Remove the flag.

Fixes: #30056

PR-URL: #30147
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@addaleax
Copy link
Member

Landed in 7f44d2c

@addaleax addaleax closed this Mar 12, 2020
BridgeAR pushed a commit that referenced this pull request Mar 17, 2020
@bnoordhuis @BridgeAR
build: disable libstdc++ debug containers globally
2428afb 
Different parts of the debug build were using differently sized
std::vectors due to `_GLIBCXX_DEBUG` sometimes being defined and
sometimes not. That ended about as well as you would expect.

Remove the flag.

Fixes: #30056

PR-URL: #30147
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorins MylesBorins mentioned this pull request Mar 19, 2020
Merged
MylesBorins pushed a commit that referenced this pull request Mar 24, 2020
@bnoordhuis @MylesBorins
build: disable libstdc++ debug containers globally
2069c4e 
Different parts of the debug build were using differently sized
std::vectors due to `_GLIBCXX_DEBUG` sometimes being defined and
sometimes not. That ended about as well as you would expect.

Remove the flag.

Fixes: #30056

PR-URL: #30147
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
targos pushed a commit that referenced this pull request Apr 22, 2020
@bnoordhuis @targos
build: disable libstdc++ debug containers globally
c8541a7 
Different parts of the debug build were using differently sized
std::vectors due to `_GLIBCXX_DEBUG` sometimes being defined and
sometimes not. That ended about as well as you would expect.

Remove the flag.

Fixes: #30056

PR-URL: #30147
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@targos targos mentioned this pull request Apr 22, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richardlau richardlau richardlau approved these changes

@mhdawson mhdawson mhdawson approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@devnexen devnexen devnexen approved these changes

@cjihrig cjihrig cjihrig approved these changes

@addaleax addaleax addaleax approved these changes

@Trott Trott Trott approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Trap hidden in debug gyp settings