Adding the GSL as a dep + POC of its usefulness by refack · Pull Request #23821 · nodejs/node

refack · 2018-10-22T19:27:53Z

`564123b` deps: add the GSL

Refs: https://github.com/Microsoft/GSL
Refs: http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#S-gsl
A headers only library to support following the C++ Core Guidelines. Most APIs are compile-time only (zero runtime cost) types such as:

span - which is size aware T[]
string_span - same but specialized for char[]
at() - bounds safe access to T[]
copy() - bounds safe memcpy for span and T[]
owner - declarative ownership for naked pointers
not_null - a wrapper for pointers that does not compile if assigned nullptr or not initialized
narrow_cast - a grapable static_cast for numeric types casts
narrow (has runtime cost) - a static_cast that throws on truncation.
finally - an arbitrary RAII lambda wrapper
polyfills for C++20's [[expects]] and [[ensures]]

`176b82d` src: example use of `gsl::narrow` to uncover a bug in `fs::LStat` on Windows

Using gsl::narrow uncovered a numeric truncation of stats.ino on Windows.

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

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

refack · 2018-10-22T19:29:59Z

/CC @nodejs/platform-windows @nodejs/fs

This https://ci.nodejs.org/job/node-test-commit-windows-fanned/21700/ job should uncover the bug.

nodejs-github-bot · 2018-10-22T20:39:50Z

@refack build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1337/pipeline

refack · 2018-10-31T02:26:04Z

25ef826 - hooking the GSL to our Abort and using it to uncover a bug:
https://ci.nodejs.org/job/node-test-pull-request/18251/

PTAL

joyeecheung · 2018-10-31T13:07:47Z

Can you update the tools/license-builder.sh?

refack · 2018-10-31T14:46:57Z

Can you update the tools/license-builder.sh?

Ack. (I did it before, lost it in this revision).

refack · 2018-10-31T14:55:30Z

BTW: does anyone have an idea what to do with:

https://github.com/nodejs/node/blob/25ef8264068cb57728fdd2f9ea5df80423cdf6b2/src/node_file.h#L208

 fields->SetValue(offset + 7, gsl::narrow<NativeT>(s->st_ino));

when NativeT == double we got overflows on Windows and macOS.

My two ideas are either (seeking more):

Document that that stats.ino is unreliable for high values on WIN32 and macOS.
Set to -1 for overflow cases.

If you feel like voting
😕 for (1)
👎 for (2)

/CC @nodejs/fs @nodejs/platform-windows @nodejs/platform-macos

joyeecheung · 2018-10-31T18:30:09Z

@refack I'd suggest we leave them overflowed and recommend using the bigint version if they want to use that stats.ino for any equality/indentity check

A headers only library to support following the C++ Core Guidelines. Rev: microsoft/GSL@c02ddae Refs: https://github.com/Microsoft/GSL Refs: http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#S-gsl

refack · 2018-11-15T18:42:00Z

~~https://ci.nodejs.org/job/node-test-pull-request/18657/~~
https://ci.nodejs.org/job/node-test-pull-request/18658/

Documented unreliability of non BigInt stat.ino on Windows
Refs: #12115

PTAL

addaleax · 2018-11-15T22:24:41Z

I’m feeling a bit uncomfortable with mixing in a valid documentation patch into a PR of this size?

I think the fact that this PR has not received approvals after 24 days also says something about people’s feelings about it … you can try to get more people’s input, but I’m personally not convinced that the value added by the GSL is worth the added complexity of an extra library for us?

refack · 2018-11-15T22:34:06Z

We get bugs uncovered, almost for free.
From 10 lines in our code we found this bug happens on 4 platforms (not just Windows)
I think I made my case 🤷‍♂️

https://github.com/nodejs/node/blob/5a95506e5b985d3dd32ac436e3c09c63dffd30f2/src/node_file.h#L194-L214

Real fail on smartOS:

[323670]: ../deps/GSL/gsl/gsl_assert:132:void gsl::details::throw_exception(Exception&&) [with Exception = gsl::narrowing_error]: Assertion `std::exception' failed.
16:25:40 1: d4d83e node::Abort() [/home/iojs/build/workspace/node-test-commit-smartos/nodes/smartos17-64/out/Release/node]
16:25:40 2: d4d8c1 node::Assert(char const* const (*) [4]) [/home/iojs/build/workspace/node-test-commit-smartos/nodes/smartos17-64/out/Release/node]
16:25:40 3: d612ae void gsl::details::throw_exception<gsl::narrowing_error>(gsl::narrowing_error&&) [/home/iojs/build/workspace/node-test-commit-smartos/nodes/smartos17-64/out/Release/node]
16:25:40 4: d61a0f void node::fs::FillStatsArray<double, v8::Float64Array>(node::AliasedBuffer<double, v8::Float64Array, std::enable_if<std::is_scalar<double>::value, void>::type>*, uv_stat_t const*, unsigned long) [/home/iojs/build/workspace/node-test-commit-smartos/nodes/smartos17-64/out/Release/node]
16:25:40 5: d61bd7 node::fs::FillGlobalStatsArray(node::Environment*, bool, uv_stat_t const*, bool) [/home/iojs/build/workspace/node-test-commit-smartos/nodes/smartos17-64/out/Release/node]
16:25:40 6: d5bcf3 node::fs::LStat(v8::FunctionCallbackInfo<v8::Value> const&) [/home/iojs/build/workspace/node-test-commit-smartos/nodes/smartos17-64/out/Release/node]
16:25:40 7: 13f4309 v8::internal::MaybeHandle<v8::internal::Object> v8::internal::(anonymous namespace)::HandleApiCallHelper<false>(v8::internal::Isolate*, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::FunctionTemplateInfo>, v8::internal::Handle<v8::internal::Object>, v8::internal::BuiltinArguments) [/home/iojs/build/workspace/node-test-commit-smartos/nodes/smartos17-64/out/Release/node]
16:25:40 8: 13f51f0 v8::internal::Builtin_HandleApiCall(int, v8::internal::Object**, v8::internal::Isolate*) [/home/iojs/build/workspace/node-test-commit-smartos/nodes/smartos17-64/out/Release/node]

Real fail on macOS

16:28:23 1: 0x100062ac5 node::Abort() [/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1011/out/Release/node]
16:28:23 2: 0x1000629f7 node::PrintErrorString(char const*, ...) [/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1011/out/Release/node]
16:28:23 3: 0x100073b29 void gsl::details::throw_exception<gsl::narrowing_error>(gsl::narrowing_error&&) [/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1011/out/Release/node]
16:28:23 4: 0x100073aaa void gsl::details::throw_exception<gsl::narrowing_error>(gsl::narrowing_error&&) [/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1011/out/Release/node]
16:28:23 5: 0x100064a5a node::fs::FSReqCallback::ResolveStat(uv_stat_t const*) [/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1011/out/Release/node]
16:28:23 6: 0x100064e24 node::fs::AfterStat(uv_fs_s*) [/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1011/out/Release/node]
16:28:23 7: 0x1009fd0c2 uv__work_done [/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1011/out/Release/node]
16:28:23 8: 0x100a006b3 uv__async_io [/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1011/out/Release/node]
16:28:23 9: 0x100a10088 uv__io_poll [/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1011/out/Release/node]
16:28:23 10: 0x100a00b3a uv_run [/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1011/out/Release/node]

Real fail on AIX

16:40:46 /home/iojs/build/workspace/node-test-commit-aix/nodes/aix61-ppc64/./node[15532078]: ../deps/GSL/gsl/gsl_assert:132:void gsl::details::throw_exception(Exception&&) [with Exception = gsl::narrowing_error]: Assertion `std::exception' failed.
16:40:46 gmake[1]: *** [tools/doc/node_modules] IOT/Abort trap

addaleax · 2018-11-15T22:39:19Z

@refack Uh… so what you’re saying is that this PR introduces hard crashes into Node? I was not aware of that – I don’t think we can change the current behaviour of the stat methods like that, even throwing JS exceptions would likely not be okay.

(i.e. The only solution I can currently see is documenting limitations, which I am totally on board with but don’t want to mix in here)

addaleax

Introducing new hard-crashes is a no-go, imo

refack · 2018-11-15T23:45:38Z

Introducing new hard-crashes is a no-go, imo

I'm sorry if I didn't make myself clear about the crashes being an indication of the latent bug. Just like CHECKs.
Since how to follow up on this was discussed in #23821 (comment), I followed the consensus and just documented this newly discovered limitation.

P.S. seems like it's not just stat.ino that overflows...
New CI:
~~https://ci.nodejs.org/job/node-test-pull-request/18663/~~
~~https://ci.nodejs.org/job/node-test-pull-request/18664/~~
https://ci.nodejs.org/job/node-test-pull-request/18665/

refack · 2018-11-16T03:17:58Z

Promoted to js tests for easier groking
/CC @nodejs/platform-aix @nodejs/platform-smartos how would you like to proceed? a comment similar to this?

node/doc/api/fs.md

Line 641 in 01c7de7

*Note*: The `number` version is unreliable on Windows as values often overflow.

smartOS:

AssertionError[ERR_ASSERTION]: bigintStats.rdev: 18446744073709551615isnotequaltonumStats.rdev: 18446744073709552000atverifyStats(/home/iojs/build/workspace/node-test-commit-smartos/nodes/smartos16-64/test/parallel/test-fs-stat-ino-overflow.js:31:12)atObject.<anonymous>(/home/iojs/build/workspace/node-test-commit-smartos/nodes/smartos16-64/test/parallel/test-fs-stat-ino-overflow.js:46:3)atModule._compile(internal/modules/cjs/loader.js:722:30)atObject.Module._extensions..js(internal/modules/cjs/loader.js:733:10)atModule.load(internal/modules/cjs/loader.js:620:32)attryModuleLoad(internal/modules/cjs/loader.js:560:12)atFunction.Module._load(internal/modules/cjs/loader.js:552:3)atFunction.Module.runMain(internal/modules/cjs/loader.js:775:12)atstartup(internal/bootstrap/node.js:300:19)atbootstrapNodeJSCore(internal/bootstrap/node.js:826:3)

AIX:

AssertionError[ERR_ASSERTION]: bigintStats.dev: 9223372191473598466isnotequaltonumStats.dev: 9223372191473598000atverifyStats(/home/iojs/build/workspace/node-test-commit-aix/nodes/aix61-ppc64/test/parallel/test-fs-stat-ino-overflow.js:31:12)atObject.<anonymous>(/home/iojs/build/workspace/node-test-commit-aix/nodes/aix61-ppc64/test/parallel/test-fs-stat-ino-overflow.js:46:3)atModule._compile(internal/modules/cjs/loader.js:722:30)atObject.Module._extensions..js(internal/modules/cjs/loader.js:733:10)atModule.load(internal/modules/cjs/loader.js:620:32)attryModuleLoad(internal/modules/cjs/loader.js:560:12)atFunction.Module._load(internal/modules/cjs/loader.js:552:3)atFunction.Module.runMain(internal/modules/cjs/loader.js:775:12)atstartup(internal/bootstrap/node.js:300:19)atbootstrapNodeJSCore(internal/bootstrap/node.js:826:3)

no longer applies

refack · 2018-11-16T22:02:03Z

~~CI: https://ci.nodejs.org/job/node-test-pull-request/18686/~~
CI: https://ci.nodejs.org/job/node-test-pull-request/18687/

I've refactored the commits to exemplify the story:

Add the GSL
Use gsl::narrow to both declare narrowing casts, and CHECK them.
Document discovery that stat.ino overflows on Windows.
Document discovery that stat.rdev and stat.dev overflows on AIX and smartOS.

Trott · 2018-11-20T21:55:16Z

Would be great to see some explicit 👍 and/or 👎 on this one so we can get an idea what to do with it...

rvagg · 2018-11-22T10:38:16Z

TBH it's hard for me to get past the addition of a new thing to deps/ that doesn't remove something else (at least llhttp is aiming to remove http_parser). A question I have is—if these are such good "guidelines", why did they not make it into the language? C++, and Stroustrup himself, have a strong record of throwing shiny stuff into the language and even celebrating doing so. ("Keep developers happy by adding new stuff" is a paraphrase of something I heard Stroustrup say once).
This is going to need some enthusiastic support from one or two heavy C++ contributors, or at least mild support of many for me to be happy with it.

refack · 2018-11-23T01:50:33Z

A question I have is—if these are such good "guidelines", why did they not make it into the language? C++

Most of the stuff in this library are polyfills for C++11 to give us features that got to the language only C++17 and C++20 (and they degrade when the features are available from the compiler). So I'd say think of it more like a portable compiler plugin, then a library.

As I understand it the language standard moves forward very slowly end rigorously, while this library is more agile. Also The standard committee is 1000% committed to backward compatibility, while the guidelines can say "this feature is a legacy footgun, prefer the alternative new safer feature". (e.g. T[n] vs std::array<T, n>, or const char* vs string_view)

The big difference between this vs something like Boost is that it's very minimal (4KLoc) and 95% of the features are compile time only, with 0 run-time cost.

Maybe the best analog is CHECKs and DCHECKs, but at a deeper semantic level.

joyeecheung · 2018-11-23T04:15:41Z

The big difference between this vs something like Boost is that it's very minimal (4KLoc) and 95% of the features are compile time only, with 0 run-time cost.

So...I don't generally disagree about this, but I'd like to remind that just because a feature is compile-time only doesn't mean it does not have an impact on the runtime performance, as there is always cache-friendliness and micro-optimizations involved. I used to not worry about the cost of templates until a coworker of mine ran into performance regressions introduced by templates because somehow the way the templates were written resulted in less cache-friendly code, even though it looked harmless and functionally equivalent...just food for thought. In the end we still need to benchmark if we are going to touch a very hot code path (although I don't think the POC here does)

refack · 2018-11-23T04:39:12Z

So...I don't generally disagree about this, but I'd like to remind that just because a feature is compile-time

But this library was specifically designed, implemented, and coordinated with compiler writers to guarantee 0 run-time cost.

just food for thought. In the end we still need to benchmark if we are going to touch a very hot code path (although I don't think the POC here does)

💯 agree!

Which reminds me, upgrading our compilers "should" give us some "free" performance improvements, or at least it's worth evaluating (nodejs/build#1543)

refack · 2019-01-30T14:02:52Z

Real issue popped up:

AssertionError [ERR_ASSERTION]: 1548817600661n !== 1548817600662n
key=atimeMs, val=1548817600662
at verifyStats (c:\workspace\node-test-binary-windows\test\parallel\test-fs-stat-bigint.js:66:14)
at c:\workspace\node-test-binary-windows\test\parallel\test-fs-stat-bigint.js:140:3

Trott · 2019-01-31T12:00:36Z


-if (!common.isWindows)
+if (!(common.isWindows || common.isSunOS || common.isAIX))
 assert.fail('Code should fail only on Windows.');


This line should be updated to include mention of the other operating systems.

Trott · 2019-01-31T12:01:39Z

Should a different PR be opened for the content of the last two commits in this PR? (Has it already?)

refack requested review from addaleax, joyeecheung and richardlau October 22, 2018 19:29

refack self-assigned this Oct 22, 2018

refack requested review from mmarchini and tniessen October 24, 2018 17:38

refack force-pushed the add-the-gsl branch from 176b82d to 25ef826Compare October 31, 2018 02:24

refack mentioned this pull request Nov 14, 2018
Node occasionally gives multiple files/folders the same inode #12115
Closed

deps: add the GSL
8b05c27
A headers only library to support following the C++ Core Guidelines. Rev: microsoft/GSL@c02ddae Refs: https://github.com/Microsoft/GSL Refs: http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#S-gsl

refack force-pushed the add-the-gsl branch from 25ef826 to 0393da1Compare November 15, 2018 18:40

refack force-pushed the add-the-gsl branch 2 times, most recently from 01c7de7 to 5a95506Compare November 15, 2018 20:09

refack requested a review from gabrielschulhof November 15, 2018 22:39

addaleax previously requested changes Nov 15, 2018
View reviewed changes

refack force-pushed the add-the-gsl branch from 5a95506 to 1d1d44bCompare November 15, 2018 23:38

refack force-pushed the add-the-gsl branch 3 times, most recently from ebc66ca to 5e69527Compare November 16, 2018 01:52

refack added windows Issues and PRs related to the Windows platform. macos Issues and PRs related to the macOS platform / OSX. smartos Issues and PRs related to the SmartOS platform. aix Issues and PRs related to the AIX platform. labels Nov 16, 2018

refack added 2 commits November 16, 2018 16:56

src: example use of gsl::narrow to uncover a bug
81fb6c0

fs: turn exposed overflow to known_issue
cfb7408

refack force-pushed the add-the-gsl branch from 25d8299 to c043240Compare November 16, 2018 21:59

fs: expand known issue to AIX and smartOS
c99e3d1

refack force-pushed the add-the-gsl branch from c043240 to c99e3d1Compare November 16, 2018 22:09

This was referenced Nov 25, 2018
Investigate flaky parallel/test-fs-stat-bigint #24565
Closed
Test /parallel/test-fs-stat-bigint fails 3 out of 100 times #24593
Closed

joyeecheung mentioned this pull request Dec 2, 2018
inspector: split the HostPort being used and the one parsed from CLI #24772
Closed
2 tasks

refack closed this Dec 21, 2018

refack deleted the add-the-gsl branch December 21, 2018 01:31

Trott reviewed Jan 31, 2019
View reviewed changes

refack removed their assignment Mar 11, 2019

Uh oh!

Uh oh!

Conversation

refack commented Oct 22, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

564123b deps: add the GSL

176b82d src: example use of gsl::narrow to uncover a bug in fs::LStat on Windows

Checklist

Uh oh!

refack commented Oct 22, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Oct 22, 2018

Uh oh!

refack commented Oct 31, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joyeecheung commented Oct 31, 2018

Uh oh!

refack commented Oct 31, 2018

Uh oh!

refack commented Oct 31, 2018

Uh oh!

joyeecheung commented Oct 31, 2018

Uh oh!

refack commented Nov 15, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented Nov 15, 2018

Uh oh!

refack commented Nov 15, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented Nov 15, 2018

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

refack commented Nov 15, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

refack commented Nov 16, 2018

Uh oh!

refack commented Nov 16, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott commented Nov 20, 2018

Uh oh!

rvagg commented Nov 22, 2018

Uh oh!

refack commented Nov 23, 2018

Uh oh!

joyeecheung commented Nov 23, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

refack commented Nov 23, 2018

Uh oh!

refack commented Jan 30, 2019

Uh oh!

TrottJan 31, 2019

Choose a reason for hiding this comment

Uh oh!

Trott commented Jan 31, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

refack commented Oct 22, 2018•
edited
Loading

`564123b` deps: add the GSL

`176b82d` src: example use of `gsl::narrow` to uncover a bug in `fs::LStat` on Windows

refack commented Oct 22, 2018•
edited
Loading

refack commented Oct 31, 2018•
edited
Loading

refack commented Nov 15, 2018•
edited
Loading

refack commented Nov 15, 2018•
edited
Loading

refack commented Nov 15, 2018•
edited
Loading

refack commented Nov 16, 2018•
edited
Loading

joyeecheung commented Nov 23, 2018•
edited
Loading