Skip to content

make sanitize#85

Merged
springmeyer merged 6 commits intomasterfrom
local-sanitizers
Oct 31, 2017
Merged

make sanitize#85
springmeyer merged 6 commits intomasterfrom
local-sanitizers

Conversation

@springmeyer
Copy link
Copy Markdown
Contributor

@springmeyer springmeyer commented Oct 30, 2017 •
edited
Loading

Running with the clang++ sanitizers (-fsanitize=address,undefined,integer,leak) locally should be easy. It is now. Just run:

make sanitize

Which will build with all key sanitizers enabled and run the tests. This should work on both Linux and OS X. It handles these various gochas which otherwise make setting up the sanitizers locally a difficult and error prone task:

  • Apple clang++ from xcode does not support the sanitizers
    • This uses clang++ 5.x from mason which supports them (and supports leak checking on OS X as of >= 5.x)
  • The Leak Sanitizer is not enabled by default on OS X
    • It sets ASAN_OPTIONS=detect_leaks:1 to enable leak checking on OS X
  • For the sanitizers to work with nodejs that is not compiled with them they need to be loaded at runtime
    • This loads the asan libraries at runtime, and handles cross-platform gochas including:
      • The library paths are different on OS X and Linux
      • The command to preload libraries is also different LD_PRELOAD vs DYLD_INSERT_LIBRARIES
      • The npm test command on OS X will fail since DYLD_INSERT_LIBRARIES cannot be inherited by shells so we call node test/*.test.js directly to workaround this limitation
  • For the sanitzer error reports to contain line numbers we need to:
    • Build in debug mode
    • Tell clang++ where the llvm-symbolizer is located with the ASAN_SYMBOLIZER_PATH (done in ./scripts/setup.sh)

Builds upon #75.

/cc @GretaCB

GretaCB
GretaCB reviewed Oct 30, 2017
View reviewed changes
Comment thread README.md Outdated
@@ -58,10 +58,7 @@ WERROR=false make
Enable additional sanitizers to catch hard-to-find bugs, for example:
Copy link
Copy Markdown
Contributor

@GretaCB GretaCB Oct 30, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Perhaps this could say something like:

Run tests locally with compile flags enabled to catch hard-to-find bugs (see this PR for more info):

make sanitize

The sanitizers are also run for a variety builds via Travis.

Copy link
Copy Markdown
Contributor Author

@springmeyer springmeyer Oct 30, 2017

Choose a reason for hiding this comment

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

👍 How does e57770a look?

Copy link
Copy Markdown
Contributor

@GretaCB GretaCB Oct 31, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Sweet, added a comment inline. Also, I pulled the branch down to run make sanitize locally and got the following error. Looks like leak flag isnt supported by x86_64-apple-darwin16.5.0, or perhaps my version of clang?

Details

clang-4.0: error: unsupported option '-fsanitize=leak' for target 'x86_64-apple-darwin16.5.0'
make[2]: *** [Debug/obj.target/module/src/module.o] Error 1
gyp ERR! build error
gyp ERR! stack Error: make failed with exit code: 2
gyp ERR! stack at ChildProcess.onExit (/Users/carol/.nvm/versions/node/v4.4.5/lib/node_modules/npm/node_modules/node-gyp/lib/build.js:285:23)
gyp ERR! stack at emitTwo (events.js:87:13)
gyp ERR! stack at ChildProcess.emit (events.js:172:7)
gyp ERR! stack at Process.ChildProcess._handle.onexit (internal/child_process.js:200:12)
gyp ERR! System Darwin 16.5.0
gyp ERR! command "/Users/carol/.nvm/versions/node/v4.4.5/bin/node" "/Users/carol/.nvm/versions/node/v4.4.5/lib/node_modules/npm/node_modules/node-gyp/bin/node-gyp.js" "build" "--error_on_warnings=true" "--loglevel=error" "--debug" "--module=/Users/carol/dev/node-cpp-skel/lib/binding/module.node" "--module_name=module" "--module_path=/Users/carol/dev/node-cpp-skel/lib/binding"
gyp ERR! cwd /Users/carol/dev/node-cpp-skel
gyp ERR! node -v v4.4.5
gyp ERR! node-gyp -v v3.6.0
gyp ERR! not ok
node-pre-gyp ERR! build error
node-pre-gyp ERR! stack Error: Failed to execute '/Users/carol/.nvm/versions/node/v4.4.5/bin/node /Users/carol/.nvm/versions/node/v4.4.5/lib/node_modules/npm/node_modules/node-gyp/bin/node-gyp.js build --error_on_warnings=true --loglevel=error --debug --module=/Users/carol/dev/node-cpp-skel/lib/binding/module.node --module_name=module --module_path=/Users/carol/dev/node-cpp-skel/lib/binding' (1)
node-pre-gyp ERR! stack at ChildProcess. (/Users/carol/dev/node-cpp-skel/node_modules/node-pre-gyp/lib/util/compile.js:83:29)
node-pre-gyp ERR! stack at emitTwo (events.js:87:13)
node-pre-gyp ERR! stack at ChildProcess.emit (events.js:172:7)
node-pre-gyp ERR! stack at maybeClose (internal/child_process.js:827:16)
node-pre-gyp ERR! stack at Process.ChildProcess._handle.onexit (internal/child_process.js:211:5)
node-pre-gyp ERR! System Darwin 16.5.0
node-pre-gyp ERR! command "/Users/carol/.nvm/versions/node/v4.4.5/bin/node" "/Users/carol/dev/node-cpp-skel/node_modules/.bin/node-pre-gyp" "configure" "build" "--error_on_warnings=true" "--loglevel=error" "--debug"
node-pre-gyp ERR! cwd /Users/carol/dev/node-cpp-skel
node-pre-gyp ERR! node -v v4.4.5
node-pre-gyp ERR! node-pre-gyp -v v0.6.36
node-pre-gyp ERR! not ok
Failed to execute '/Users/carol/.nvm/versions/node/v4.4.5/bin/node /Users/carol/.nvm/versions/node/v4.4.5/lib/node_modules/npm/node_modules/node-gyp/bin/node-gyp.js build --error_on_warnings=true --loglevel=error --debug --module=/Users/carol/dev/node-cpp-skel/lib/binding/module.node --module_name=module --module_path=/Users/carol/dev/node-cpp-skel/lib/binding' (1)
make[1]: *** [debug] Error 1
make: *** [sanitize] Error 2

Copy link
Copy Markdown
Contributor

@GretaCB GretaCB Oct 31, 2017

Choose a reason for hiding this comment

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

make distclean cleared it up. Local sanitize looks 👍

GretaCB
GretaCB reviewed Oct 31, 2017
View reviewed changes
Comment thread README.md Outdated
make sanitize
```

The sanitizers [are part of the compiler](https://github.com/mapbox/cpp/blob/master/glossary.md#sanitizers) are also run in a specific job on Travis.
Copy link
Copy Markdown
Contributor

@GretaCB GretaCB Oct 31, 2017

Choose a reason for hiding this comment

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

"and are also run in a specific job on Travis"

Looks good, thanks @springmeyer !

Copy link
Copy Markdown
Contributor Author

@springmeyer springmeyer Oct 31, 2017

Choose a reason for hiding this comment

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

Thanks, fixed in 5b7be9d

@springmeyer springmeyer merged commit e54f258 into master Oct 31, 2017
@springmeyer springmeyer deleted the local-sanitizers branch October 31, 2017 15:54
@springmeyer springmeyer mentioned this pull request Mar 9, 2018
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@GretaCB GretaCB GretaCB approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@springmeyer @GretaCB