-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
build: use clang over g++ for ubsan #52169
Conversation
Review requested:
|
Need #52152 otherwise ubsan doesn't run on the PR |
It landed so you can rebase on main now |
f150ab8
to
5efaa76
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM if it works.
Run without this PR for comparison: https://github.com/nodejs/node/actions/runs/8372351086/job/22923148679 |
It took less time to compile, but it took much longer to test and it broke. I don't have much knowledge on this topic, so do you think there's something we can do to try and fix it, or does |
@lemire maybe has some knowledge on this |
To my knowledge, ubsan works with both clang and gcc. |
@H4ad Does it work locally for you, or does it only fail in CI? |
Here's part of the error log:
|
@targos One of those warnings is legitimate:
I don't think it is a bug per se because no compiler would do the wrong thing here. But it seems to indicate that the tool is doing its work. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang doesn't accept specific operators for suppression files. That's why I switched to g++. Are the actions running out of memory?
I just compiled locally but didn't try to run the tests for
No, I just thought we could use That being the case, I will close this PR, thanks for the feedback! |
Use
clang
instead ofg++
as we do fortest-asan
since it appears to use less memory, and probably should build faster too, refs: #32776