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

CI: Sanitize for undefined behavior #4845

Merged
merged 2 commits into from
Feb 3, 2025
Merged

CI: Sanitize for undefined behavior #4845

merged 2 commits into from
Feb 3, 2025

Conversation

whitequark
Copy link
Member

@whitequark whitequark commented Jan 14, 2025

What are the reasons/motivation for this change?

Yosys includes some amount of buggy code that invokes undefined behavior, e.g. #4844. This should be fixed and stay fixed.

Explain how this is achieved.

By running the Undefined Behavior Sanitizer on each commit/PR. UBSan does not have false positives and has a slim impact on performance, so this is done unconditionally.

If applicable, please suggest to reviewers how they can test the change.

Add SANITIZER = undefined to your Makefile.conf, then run make clean && make && UBSAN_OPTIONS=halt_on_error=1 make test.

@whitequark whitequark requested a review from mmicko as a code owner January 14, 2025 06:14
@whitequark whitequark mentioned this pull request Jan 14, 2025
@whitequark
Copy link
Member Author

We should probably exclude the whole libs/* subtree from UBSan checking since it's not like we're going to fix those bugs anyway. I think we should use -fsanitize-ignorelist=FILE where FILE contains:

src:libs/*

or maybe

[alignment]
src:libs/fst/*

What should FILE be? I'm thinking .sanitize-ignorelist or something.

@whitequark
Copy link
Member Author

The CI does fail in an unfortunately opaque way (the ubsan output gets redirected to tests/simple/memory.err) but I've confirmed that this is indeed #4844:

kernel/celledges.cc:262:30: runtime error: shift exponent 32 is too large for 32-bit type 'int'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior kernel/celledges.cc:262:30 in 

@KrystalDelusion
Copy link
Member

There is a Report errors step in the tests yaml that runs find tests/**/*.err -print -exec cat {} \;. It doesn't always work, but in this case it does give the ubsan output you're looking for:
https://github.com/YosysHQ/yosys/actions/runs/12762313299/job/35571164274?pr=4845#step:12:4995

@whitequark
Copy link
Member Author

Ah, that's handy.

@KrystalDelusion KrystalDelusion mentioned this pull request Jan 30, 2025
2 tasks
@povik povik force-pushed the catherine/ci-ubsan branch from fad158b to 212d2a6 Compare February 3, 2025 10:34
@povik
Copy link
Member

povik commented Feb 3, 2025

The CI is green now, someone please review

@KrystalDelusion
Copy link
Member

I think

[alignment]
src:libs/fst/*

in .sanitize-ignorelist is good (with appropriate gating I guess?)

@povik
Copy link
Member

povik commented Feb 3, 2025

The one instance of UB has been easy to patch. I'd only start the exclusion list if issues with that library keep coming back.

@KrystalDelusion
Copy link
Member

Oh! I missed that you patched the library 😅 lgtm

@povik povik merged commit f0aaa5d into main Feb 3, 2025
61 checks passed
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 this pull request may close these issues.

3 participants