-
Notifications
You must be signed in to change notification settings - Fork 28.9k
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.
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
Tracking Issue: enable sanitizers on our CI #32380
Comments
If we have a machine with more memory, we could make it a self-hosted github actions runner |
I thought about it, but I'm worried about maintenance burden (it's already hard to keep Jenkins hosts healthy). It should still be worth investigating (not only for this use case). |
Some todo from my original PR #31902 :
cc @mmarchini |
If we using it, we also should take actions to avoid it being abused. |
Not sure we need the docker image: gcc and clang on the Actions host are fairly recent, and we don't use a docker image in any of the other build Actions. Also, instead of skipping tests, we could disable all ASAN checks and enable them one by one, that way we can fix everything from one check before moving to the other, and we're guarantee the ones we already fixed will be flagged on CI if they regress. |
Docker image will has stable dev env for everyone, same gcc, clang, this will help a lot we reproduce problem. Also with docker we can also have parallel env. Like gcc8 and gcc9 same time (create different image). Also it create usefully env for macOS and windows users. (Currently macOS asan is broken for Node.js) Also any thought on should check gcc and clang both, they have difference google/sanitizers/wiki/AddressSanitizerClangVsGCC-(6.0-vs-8.1) |
For ASAN, there's probably not much value on using an older compiler version (an older version won't have a check not present on a newer one). I'll read the differences tomorrow. We might have better coverage with both, or maybe just one of them is enough. I think V8 only uses clang. |
Going through the differences in AddressSanitizerClangVsGCC-(6.0-vs-8.1), we should be looking into using clang for ASAN. GCC doesn't provide anything extra that we can use (most things gcc has over clang are only useful for building the Linux kernel with ASAN), and gcc lacks some features. |
From: http://llvm.org/docs/CMake.html
Can we try gold linker instead of ld ? |
Or pass extra flags I can try it, but I don't know where do I add them ? cc @bnoordhuis |
Somehow github action can build asan again: we have 30 test case failed: [40:39|% 100|+ 2803|- 30]: Done Full log: https://github.com/gengjiawen/node-github-workflow/runs/551124593?check_suite_focus=true |
I did some testing with clang instead of gcc and the results were promising, was able to build with ASAN (not in Debug mode) using a max of 2Gb memory. I'll open a PR to re-enable ASAN on Actions soon. there's one pending PR to get all cctests passing on ASAN, after that we can start working on getting JS tests passing as well. |
@mmarchini I've hit a bit of a wall trying to backport source-map functionality to the v14.x-staging release line, due to ASAN tests in #37717. Bisecting, the exceptions being raised appear to be caused by modifications to Any thoughts? |
What are sanitizers?
Sanitizers are build options which can be used to identify programming errors in C++. They add overhead to the program, which is why it's not recommended to use in releases, but they can be valuable to find memory leaks and other potential crashes on our tests. V8 has ASAN buildbots which run on every CL, and they are working on making their code base UBSan compatible as well.
Cool, what do we need to make it happen?
Well, first thing, asan has to work on Node.js. We have
--enable-asan
onconfigure
today, and the build will work on most computers (it requires a considerable amount of memory). But most of our tests will fail with it, because our code base is not ASAN clean. We might be also missing some ASAN flags: looking at V8 source code and build dependencies, they do one thing or two different.There's also the memory issue: as of right now, we need a big machine to build with ASAN. GitHub Actions can't run an ASAN build because it "only" has 7Gb. Hopefully there's something we can tune in the build to use less memory. Otherwise, we'll need a custom machine for it (probably on Jenkins).
As for other sanitizers (like UBSan), upstream projects need to support those first, so we are blocked for now.
To Do List (ASAN)
nodejs/
orgTo Do List (UBSan)
We can list current failing tests here once we get rid of the most noisy ones.
The text was updated successfully, but these errors were encountered: