build, windows: ignore C4251 & C4275 #15570

refack · 2017-09-23T12:35:28Z

ignore C4251 "mismatched dll-interface"
ignore C4275 "bad inheritance dll-interface"

Many V8 methods do not comply with the requirements to be DLL-exportable,
which results in these warnings reported several 1000s times during build.

CI: https://ci.nodejs.org/job/node-test-commit/12552/

Checklist

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

Affected core subsystem(s)

build,windows

refack · 2017-09-23T12:35:52Z

/cc @nodejs/build @nodejs/platform-windows

seishun · 2017-09-23T12:38:24Z

common.gypi

+        #
+        # Disable "warning C4251: 'identifier' : class 'type' needs to have dll-interface to be used by clients of class 'type2'"
+        # Disable "warning C4275: non - DLL-interface classkey 'identifier' used as base for DLL-interface classkey 'identifier'"
+        # Many V8 method do not comply with the requirements to be dll exported,


seishun · 2017-09-23T12:39:01Z

common.gypi

+        # Disable "warning C4251: 'identifier' : class 'type' needs to have dll-interface to be used by clients of class 'type2'"
+        # Disable "warning C4275: non - DLL-interface classkey 'identifier' used as base for DLL-interface classkey 'identifier'"
+        # Many V8 method do not comply with the requirements to be dll exported,
+        # which result in these warnings reported several 1000s times.


which results in these warnings being reported several thousands of times

gibfahn · 2017-09-23T13:33:12Z

Not against doing it, although wouldn't the ideal solution be for the V8 methods to comply with said requirement? Not sure how feasible that is, I guess cc/ @nodejs/v8?

refack · 2017-09-23T13:41:15Z

Just a data point:
Pre PR Job#11948 - console log size 13534KB
Post PR Job#11949 - console log size 316KB

bnoordhuis

It would be better to fix the warnings rather than suppress them.

In fact, I suspect they are our own fault. Right now, we unconditionally set BUILDING_V8_SHARED=1 all throughout the build but we should probably define it only while building V8 and define USING_V8_SHARED=1 while building node/cctest/addons.

bnoordhuis · 2017-09-25T11:35:59Z

common.gypi

+        # Disable "warning C4251: 'identifier' : class 'type' needs to have dll-interface to be used by clients of class 'type2'"
+        # Disable "warning C4275: non - DLL-interface classkey 'identifier' used as base for DLL-interface classkey 'identifier'"
+        # Many V8 methods do not comply with the requirements to be DLL exportable,
+        # which results in these warnings reported thousands of times.


Long lines, try to keep them <= 80 columns.

jasnell

LGTM with @bnoordhuis' nits fixed

BridgeAR · 2017-09-28T05:57:27Z

@refack would you want to try to fix this with the suggestion from @bnoordhuis? I think it would be pretty nice to fix the issues instead of skipping those but I am also fine with this as a intermediate step if you do not have the time for that right now.

seishun

Either the warnings should be fixed as @bnoordhuis said, or the comment should explain why we can't (or don't want to) fix them.

seishun · 2017-09-28T16:58:10Z

@bnoordhuis

In fact, I suspect they are our own fault. Right now, we unconditionally set BUILDING_V8_SHARED=1 all throughout the build but we should probably define it only while building V8 and define USING_V8_SHARED=1 while building node/cctest/addons.

Can't use USING_V8_SHARED=1 when building node, since we're linking to V8 statically, not dynamically. The warnings are emitted when building V8 anyway.

Anyway, I decided to get to the root of this.

C4251 is emitted when an exported class has a non-static data member of a non-exported class type. The rationale, as I understand it, is that the user of the exported class could attempt to access (directly, or via an inline member function) a static data member or a non-inline member function of the data member, resulting in a linker error.

However, CL.exe's analysis is lacking. I've looked at a dozen instances of C4251 emitted when building node, and all of them were noise. Moreover, fixing them on V8's side would result in boilerplate and/or pointless exports. Examples:

node/deps/v8/include/v8.h

Line 4191 in dcb24e3

std::vector<Buffer> received_buffers_;

received_buffers_ is private. It is accessed in the constructor, but it's private. Fixing this would require explicitly instantiating and exporting std::vector<Buffer>.

node/deps/v8/src/zone/accounting-allocator.h

Line 77 in fca7e49

base::Mutex unused_segments_mutex_;

unused_segments_mutex_ is private and isn't accessed in any inline member functions. Fixing this would require exporting base::Mutex when BUILDING_V8_SHARED is defined (now it's exported when BUILDING_V8_BASE_SHARED is defined).

node/deps/v8/src/zone/accounting-allocator.h

Line 83 in fca7e49

base::AtomicValue<MemoryPressureLevel> memory_pressure_level_;

memory_pressure_level_ is private and isn't accessed in any inline member functions. Fixing this would require explicitly instantiating and exporting base::AtomicValue<MemoryPressureLevel>.

node/deps/v8/src/execution.h

Line 224 in fca7e49

ThreadLocal thread_local_;

thread_local_ is private. It's referenced in several public inline member functions, but only its inline member functions and non-static data members are accessed.

If we want to fix this at the root, then we should get Microsoft to stop emitting C4251 when the data member is private and none of its non-inline member functions or static data members are accessed from any public inline member functions. @refack since you seem to have connections with Microsoft, perhaps you could suggest a preferable route to submit this request?

Or if there are any C++ gurus here who can show that the warnings are actually meaningful, I'll be glad to be proven wrong.

refack · 2017-09-28T18:27:34Z

If we want to fix this at the root, then we should get Microsoft to stop emitting C4251 when the data member is private and none of its non-inline member functions or static data members are accessed from any public inline member functions. @refack since you seem to have connections with Microsoft, perhaps you could suggest a preferable route to submit this request?

Me? I just have a contact. We have actual MS employees here :)
/cc @mrkmarron @harishtejwani @aruneshchandra and also @AndrewPardoe can help.

solodon4 · 2017-10-03T01:47:03Z

Hi @refack !
We can definitely look into C4251 generating less false positives. Can you please extract for us a few self-contained (ideally minimal) examples of when you think C4251 can/should not be emitted and i'll submit a work item for myself for 15.6. You can send them to me directly at yuriysol at microsoft. Thank you!

d3x0r · 2017-11-18T18:03:12Z

re C4251 ... there's only one place that uses NODE_EXTERN in the class definiton, and no other classes use it. Probably the class definition just needs to be fixed? ie remove NODE_EXTERN?
#17114

BridgeAR · 2017-12-06T03:53:48Z

@solodon4 did you have the chance to look into this any further?

@refack @seishun @bnoordhuis even if this gets fixed in Visual Studio 2017 15.6 we should think about maybe suppressing the warnings for older versions. Not everyone is going to use the newest version. What do you think?

bnoordhuis · 2017-12-06T23:09:51Z

Not opposed.

seishun · 2017-12-06T23:17:28Z

even if this gets fixed in Visual Studio 2017 15.6 we should think about maybe suppressing the warnings for older versions. Not everyone is going to use the newest version.

It's reasonable to expect an up-to-date toolchain. Plus in this case it's just warnings, it doesn't prevent node from building.

d3x0r · 2017-12-07T00:02:06Z

Personally I wish the headers would just get fixed instead of hiding it with warning disables.
WrappedObject class previously generated the warning, and was fixed... the new addition of node::CallbackScope::try_catch_ shouldn't have NODE_EXTERN marked on it. NO other class has that notation. My issue isn't about building node though, it's about building node addons.

solodon4 · 2017-12-07T06:05:42Z

@BridgeAR it didn't get in with the main train as I was busy with getting the warning level for external headers feature in - I'll post a blog about it in the coming days. This new feature might help you guys with the suppression if you treat V8 headers where the warning happens as external/system.

That said - we are still trying to get a couple more bugs fixed in escrow mode for 15.6, so i'll try to sneak it in, but can't promise. Voting on feedback ticket would certainly help bringing it to attention of higher ups and would justify getting it in while in escrow.

seishun · 2017-12-07T09:13:33Z

@solodon4 Is there a feedback ticket for this?

BridgeAR · 2018-01-31T01:41:54Z

@solodon4 is there anything besides the references issue where feedback could be provided?

solodon4 · 2018-02-05T20:32:25Z

@BridgeAR, the issue is currently tracked by an internal ticket for 15.7. Unfortunately, it didn't get high-enough on the management's radar to be considered for late stages of 15.6. That said, if you'd like to be able to provide an additional information on the issue or be able to vote on it, we'd need a feedback item created on Developer Community's C++ section. Please post the link here if you create the feedback item yourself, otherwise I'll post it when I get around to creating it myself.

BridgeAR · 2018-02-07T02:59:11Z

@solodon4 I personally do not use Windows and can not provide any further feedback. I would like to upvote that issue nevertheless. So if you could open a feedback item, that would be great!

@seishun maybe you could provide some further details?

BridgeAR · 2018-03-02T03:43:36Z

I am closing this due to no further progress and because I doubt that we get to a conclusion. Let us hope this is going to be fixed in 15.7.

If others think this should stay open, please leave a comment to reopen / reopen directly.

Compiling node with vcbuild generates 10,000s of these warnings, originating from v8.h. This makes it impossible to read any other diagnostic messages. Refs: nodejs#15570 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> PR-URL: nodejs#20958

Compiling node with vcbuild generates 10,000s of these warnings, originating from v8.h. This makes it impossible to read any other diagnostic messages. PR-URL: #20958 Refs: #15570 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Sep 23, 2017

refack self-assigned this Sep 23, 2017

refack added the windows Issues and PRs related to the Windows platform. label Sep 23, 2017

seishun reviewed Sep 23, 2017

View reviewed changes

bnoordhuis reviewed Sep 25, 2017

View reviewed changes

jasnell approved these changes Sep 25, 2017

View reviewed changes

seishun suggested changes Sep 28, 2017

View reviewed changes

refack added 3 commits October 18, 2017 17:04

build: ignore C4251 "mismatched dll-interface"

eb12f6f

build: ignore C4275 "bad inheritance dll-interface"

5b77d94

[test balloon]

eeda0e5

refack force-pushed the ignore-dll-error branch from 3ac36e0 to eeda0e5 Compare October 18, 2017 21:07

seishun mentioned this pull request Nov 18, 2017

Visual Studio 2013 build warnings #17114

Closed

BridgeAR added the blocked PRs that are blocked by other issues or PRs. label Nov 22, 2017

bnoordhuis mentioned this pull request Nov 24, 2017

Can we disable the C4251 build warning on Windows? #17286

Closed

MylesBorins force-pushed the master branch from b7405ab to 7f086dd Compare December 8, 2017 16:37

fs-eire mentioned this pull request Jan 2, 2018

Build warning in latest node version (MSVC2017 + Node8.3.0) microsoft/napajs#42

Open

maclover7 force-pushed the master branch from bb5575a to 993b716 Compare January 26, 2018 22:02

cjihrig force-pushed the master branch from 993b716 to 082f952 Compare January 26, 2018 22:36

BridgeAR closed this Mar 2, 2018

d3x0r mentioned this pull request May 15, 2018

CallbackScope should not have NODE_EXTERN? #20758

Closed

BridgeAR mentioned this pull request May 25, 2018

build,win: disable DLL-interface warnings #20958

Closed

2 tasks

refack deleted the ignore-dll-error branch September 5, 2018 22:30

refack removed their assignment Oct 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build, windows: ignore C4251 & C4275 #15570

build, windows: ignore C4251 & C4275 #15570

refack commented Sep 23, 2017 •

edited

Loading

refack commented Sep 23, 2017

seishun Sep 23, 2017

seishun Sep 23, 2017 •

edited

Loading

gibfahn commented Sep 23, 2017

refack commented Sep 23, 2017

bnoordhuis left a comment

bnoordhuis Sep 25, 2017

jasnell left a comment

BridgeAR commented Sep 28, 2017

seishun left a comment •

edited

Loading

seishun commented Sep 28, 2017

refack commented Sep 28, 2017

solodon4 commented Oct 3, 2017

d3x0r commented Nov 18, 2017

BridgeAR commented Dec 6, 2017

bnoordhuis commented Dec 6, 2017

seishun commented Dec 6, 2017

d3x0r commented Dec 7, 2017

solodon4 commented Dec 7, 2017

seishun commented Dec 7, 2017

BridgeAR commented Jan 31, 2018

solodon4 commented Feb 5, 2018

BridgeAR commented Feb 7, 2018

BridgeAR commented Mar 2, 2018

build, windows: ignore C4251 & C4275 #15570

build, windows: ignore C4251 & C4275 #15570

Conversation

refack commented Sep 23, 2017 • edited Loading

Checklist

Affected core subsystem(s)

refack commented Sep 23, 2017

seishun Sep 23, 2017

Choose a reason for hiding this comment

seishun Sep 23, 2017 • edited Loading

Choose a reason for hiding this comment

gibfahn commented Sep 23, 2017

refack commented Sep 23, 2017

bnoordhuis left a comment

Choose a reason for hiding this comment

bnoordhuis Sep 25, 2017

Choose a reason for hiding this comment

jasnell left a comment

Choose a reason for hiding this comment

BridgeAR commented Sep 28, 2017

seishun left a comment • edited Loading

Choose a reason for hiding this comment

seishun commented Sep 28, 2017

refack commented Sep 28, 2017

solodon4 commented Oct 3, 2017

d3x0r commented Nov 18, 2017

BridgeAR commented Dec 6, 2017

bnoordhuis commented Dec 6, 2017

seishun commented Dec 6, 2017

d3x0r commented Dec 7, 2017

solodon4 commented Dec 7, 2017

seishun commented Dec 7, 2017

BridgeAR commented Jan 31, 2018

solodon4 commented Feb 5, 2018

BridgeAR commented Feb 7, 2018

BridgeAR commented Mar 2, 2018

refack commented Sep 23, 2017 •

edited

Loading

seishun Sep 23, 2017 •

edited

Loading

seishun left a comment •

edited

Loading