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

compiler-rt: re-enable memcpy runtime safety in tests #22875

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dweiller
Copy link
Contributor

The PR re-enables runtime safety for compiler-rt memcpy tests. This should allow detecting logic bugs more reliably with tests.

The diff looks messy due to indentation changes, but the only actual changes are the re-inclusion of 3 asserts that were removed in #22854 and making the implementation generic based on a comptime enable_safety: bool parameter. This allows the tests to test the logic with enable_safety == true while not causing recursive calls to memcpy, as the memcpy calls made by safety code will call the version with enable_safety == false.

Another option would have been to have enable_safety be a comptime argument to all the helper functions - this makes a cleaner diff, but I think it makes the code a bit harder to read.

@andrewrk
Copy link
Member

what's the problem statement?

@dweiller
Copy link
Contributor Author

dweiller commented Feb 23, 2025

The problem I see is that having safety disabled in tests means that there is a higher chance that bugs introduced to memcpy in the future may pass the tests. Perhaps it's not likely to actually happen, but I thought it's best to be safe.

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.

2 participants