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

Add test with ASAN enabled. #2313

Open
wants to merge 29 commits into
base: next
Choose a base branch
from
Open

Add test with ASAN enabled. #2313

wants to merge 29 commits into from

Conversation

Rot127
Copy link
Collaborator

@Rot127 Rot127 commented Apr 7, 2024

Your checklist for this pull request

  • I've documented or updated the documentation of every function and struct this PR changes. If not so I've explained why.
  • I've added tests that prove my fix is effective or that my feature works (if possible)

Detailed description

Add ASAN builds

It does a few things

  • Fixes the CAPSTONE_DIET build. Though it is not tested, since we cannot do it with the current test design, which relies on printing everything.
  • Fix a lot of mem leaks,
  • Fix the build of cstest via CMakeLists.txt. This allows now also to enable ASAN for the cstest. Also it is much easier to build cstest now.
  • Fixes cstest-report.py to work also when cstest is installed and in PATH.
  • Make run_invalid_cstool.sh more verbose to help spotting possible ASAN errors.
  • Add and fix tests/cs_details/issue.cs to the CI.

Test plan

CI green (or, if everything fails horribly, mark it optoinal for now)

Closing issues

closes #2368

@Rot127 Rot127 added build & packaging Build system and packaging related Testing Test related issue labels Apr 7, 2024
@Rot127 Rot127 added this to the v6 milestone Apr 7, 2024
@Rot127 Rot127 force-pushed the ASAN branch 2 times, most recently from af12ea4 to 24616c6 Compare April 7, 2024 11:20
@github-actions github-actions bot added the CS-core-files auto-sync label Apr 17, 2024
@Rot127 Rot127 force-pushed the ASAN branch 8 times, most recently from 2719ec4 to 4ca2ce5 Compare April 22, 2024 09:52
@Rot127 Rot127 marked this pull request as ready for review April 22, 2024 11:00
@github-actions github-actions bot added the ARM Arch label Apr 22, 2024
@Rot127 Rot127 mentioned this pull request May 11, 2024
6 tasks
@Rot127 Rot127 force-pushed the ASAN branch 2 times, most recently from 6c97062 to 3b53ed9 Compare May 20, 2024 10:52
@Rot127 Rot127 mentioned this pull request May 26, 2024
2 tasks
@XVilka

This comment was marked as resolved.

@github-actions github-actions bot added PowerPC Arch HPPA Arch labels Jun 1, 2024
@Rot127
Copy link
Collaborator Author

Rot127 commented Jun 1, 2024

I remove the DIET changes again. Build is still fixed.
They are too complicated to fix for now and not testable. Because we can only test the details and decoding by printing it first, all the test cases of us fail.
So I think if we fix it it should be done once in a separated PR.
Also we can reduce the binary size even more.

Copy link
Contributor

@XVilka XVilka left a comment

Choose a reason for hiding this comment

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

Cool! In the future we also should enable UBSAN, I know there are many potential places that can trigger it. But that's a story for another PR...

@XVilka
Copy link
Contributor

XVilka commented Jun 1, 2024

@kabeor please take a look and lets merge this quickly, so new PRs will not introduce ASAN warnings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ARM Arch build & packaging Build system and packaging related CS-core-files auto-sync Documentation HPPA Arch PowerPC Arch Testing Test related issue
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

Add "DIET" CI job
2 participants