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: add sanitizers #3625

Merged
merged 3 commits into from
Nov 2, 2022
Merged

CI: add sanitizers #3625

merged 3 commits into from
Nov 2, 2022

Conversation

davidbolvansky
Copy link
Contributor

@davidbolvansky davidbolvansky commented Oct 26, 2022

Enable undefined behaviour sanitizer and address sanitizer.

@apinski-cavium
Copy link

Note GCC has a similar (same) sanitizers that can be run there too.
I found GCC's address sanitizer finding more use out of scope issues than clang's does find.

Also I noticed you had to disable the leak detection. I could find you can only disable it at runtime via the ASAN_OPTIONS=detect_leaks=0 env variable settings.

@davidbolvansky
Copy link
Contributor Author

And clang somehow crashes in CI.
So yes, I will switch to gcc build.

@davidbolvansky davidbolvansky changed the title CI: add sanitizers to clang build of p4c CI: add sanitizers Oct 30, 2022
@davidbolvansky davidbolvansky marked this pull request as ready for review October 30, 2022 19:34
@davidbolvansky
Copy link
Contributor Author

Ready for review.

@fruffy
Copy link
Collaborator

fruffy commented Oct 31, 2022

Do you want this as a nightly test by any chance? Or should we merge the sanitizer test with the normal test runner?

@davidbolvansky
Copy link
Contributor Author

As the normal one.

@fruffy
Copy link
Collaborator

fruffy commented Nov 1, 2022

Then let's merge build-linux and build-linux-sanitizers. build-linux-sanitizers seems like a superset to the other test to me and the additional sanitizer checks do not increase the compile time noticeably.

@davidbolvansky
Copy link
Contributor Author

Okay. In sanitizers build gcc fires additional “maybe-uninitialized” warnings but there are quite noisy and so often not real issues basically. So these noisy warnings + -Werror by default would be problematic so..

A) disable them completely via -Wno-maybe-uninitialized
B) just never show them as errors when -Werror is used. Flag: -Wno-error=maybe-uninitialized

WDYT?

@fruffy
Copy link
Collaborator

fruffy commented Nov 1, 2022

Are we actually looking at the “maybe-uninitialized” warnings? If not, I would go with the second option.

Otherwise, we can keep the duplicate CI run. I am guessing we have to run the tests to trigger some sanitizer assertions, right?

@davidbolvansky
Copy link
Contributor Author

davidbolvansky commented Nov 1, 2022

Are we actually looking at the “maybe-uninitialized” warnings? If not, I would go with the second option.

I look on them and they are highly unlikely to happen (be a real bug) in practice. What is worse, sometimes innocent code change triggers for example gcc's inliner and then gcc emits (from midend passes!) warnings randomly..

Second option sounds fine to me, so I will update this patch.

And yes to the last question.

@@ -32,9 +32,9 @@ jobs:
key: test-${{ matrix.unified }}-${{ runner.os }}-gcc
max-size: 1000M

- name: Build (Ubuntu Linux, GCC)
- name: Build (Ubuntu Linux, GCC, Sanitizers)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a nit, maybe also rename the top-level test to include this name. Then the label immediately shows in the action panel.

@davidbolvansky
Copy link
Contributor Author

Feel free to merge it if you are fine with the current version of this PR, @fruffy

@fruffy fruffy merged commit 7867db4 into p4lang:main Nov 2, 2022
@davidbolvansky
Copy link
Contributor Author

Thanks for the review and feedback, @fruffy

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