src: asan build fix for clang #29383
Conversation
bnoordhuis
left a comment
There was a problem hiding this comment.
LGTM. A note on the commit log:
adding missing header for the leak check.
We (usually) use present tense and (always) capitalize: s/adding/Add/
I'd probably also s/leak check/leak checker/, s/leak check/leak sanitizer/ or just s/the leak check/LSAN/.
|
A little off the topic, how do I enable asan when building Node.js ? |
|
|
@nodejs/build Is it possible to add a job to test asan build works and prevent future regression ? |
|
I'm part of the Build WG these days so I suppose I can answer that. ^^ It's certainly possible to add a linux+asan job (this PR is about lsan though) but running the test suite is out of the question - sanitizers cause a 20-200x slowdown. That makes it less useful because the point of course is to catch runtime misbehavior. Maybe we could run a subset of tests but I don't immediately have good suggestions on what subset that should be. TBD whether to build with clang, gcc or both. Can you open a nodejs/build issue if you still think it's a good idea? |
|
An additional suggestion would be to use a subset of asan checks via ASAN_OPTIONS environment variable. |
Add missing header for LSAN.
Add missing header for LSAN. PR-URL: #29383 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>
|
Landed in 4d2856e |
Add missing header for LSAN. PR-URL: #29383 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>
adding missing header for the leak check.