Skip to content

miri: Add separate initializer for SOFT_ASSERTIONS #18137

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

Conversation

do-not-use-parker-timmerman
Copy link
Contributor

Motivation

This PR fixes a recognized bug.

  • Fixes MaterializeInc/database-issues#5316

Recently we began running Miri in CI, which allows us to catch some tricky bugs. Unfortunately Miri doesn't support global constructors (see rust-lang/miri#450) which we use when initializing the global SOFT_ASSERTIONS flag. This causes any test that uses soft assertions to fail.

This PR adds a new Miri specific initializer for SOFT_ASSERTIONS which is always true. A second approach we could take would be to use once_cell::Lazy instead of ctor to initialize SOFT_ASSERTIONS, but AFAIK that would introduce a small performance regression, as it would result in SOFT_ASSERTIONS getting initialized on first use, as opposed to when the program is loaded.

Checklist

  • This PR has adequate test coverage / QA involvement has been duly considered.
  • This PR has an associated up-to-date design doc, is a design doc (template), or is sufficiently small to not require a design.
  • This PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way) and therefore is tagged with a T-proto label.
  • If this PR will require changes to cloud orchestration, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label (example).
  • This PR includes the following user-facing behavior changes:

- add new initializer of SOFT_ASSERTIONS
- re-enable test in stash
Copy link
Contributor

@maddyblue maddyblue left a comment

Choose a reason for hiding this comment

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

Neat!

@do-not-use-parker-timmerman do-not-use-parker-timmerman merged commit eaeb3bc into MaterializeInc:main Mar 15, 2023
@do-not-use-parker-timmerman do-not-use-parker-timmerman deleted the miri/fix-test branch March 15, 2023 17:33
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.

4 participants