Skip to content
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

Include UndefinedBehavior Sanitizer to our CI #46062

Closed
RafaelGSS opened this issue Jan 2, 2023 · 6 comments · Fixed by #46297
Closed

Include UndefinedBehavior Sanitizer to our CI #46062

RafaelGSS opened this issue Jan 2, 2023 · 6 comments · Fixed by #46297

Comments

@RafaelGSS
Copy link
Member

RafaelGSS commented Jan 2, 2023

Hi!

Are we interested in including an Undefined Behavior Sanitizer (pretty similar to our test-asan) in our CI workflow?

I know that currently, our CI takes a quite long time to conclude (considering flaky tests), but, undefined behavior is very often harmful to compiler assumptions and eventually, makes a branch slower.

I made a test including this flag only on macOS (just to test a particular case), but in case we concur on having this as an option I can pursue this work. See https://github.com/nodejs/node/compare/main...RafaelGSS:node:build/incldue-ub-build?expand=1

Interested in hearing your thoughts @nodejs/build @richardlau @addaleax

@bnoordhuis
Copy link
Member

undefined behavior is very often harmful to compiler assumptions and eventually, makes a branch slower

It's the other way around: UB gives the compiler a free pass to completely optimize away buggy code. :-)

Something to investigate is if V8 is ubsan-clean nowadays. It didn't used to be (v8:3770) but I see there's an ubsan buildbot now and a -fsanitize=undefined switch in BUILD.gn so... maybe?

@RafaelGSS
Copy link
Member Author

From my local tests, only small-icu isn't ubsan-clean.

@bnoordhuis
Copy link
Member

I've opened a tracking issue for libuv: libuv/libuv#3869

I believe libuv is in reasonably good shape already but we'll see.

@bnoordhuis
Copy link
Member

libuv/libuv#3870 - libuv has a ubsan CI bot now (and it's clean.)

@RafaelGSS
Copy link
Member Author

Excellent. I think we could skip the small-icu for now, what do you think? In the case of affirmative, I can open the PR to include the workflow.

@bnoordhuis
Copy link
Member

Sounds good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants