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

ZIR-232: Add Assert and Abort externs #69

Merged
merged 4 commits into from
Nov 13, 2024
Merged

ZIR-232: Add Assert and Abort externs #69

merged 4 commits into from
Nov 13, 2024

Conversation

jacobdweightman
Copy link
Contributor

@jacobdweightman jacobdweightman commented Nov 12, 2024

Resolves ZIR-185

  • Added extra mutable argument to interpreter externs to signal if proving should stop
  • Added extern Assert(x: Val, message: String); which stops proving if x is not zero
  • Added extern Abort(); which unconditionally stops proving

@github-actions github-actions bot changed the title Add Assert and Abort externs ZIR-232: Add Assert and Abort externs Nov 12, 2024
@@ -44,7 +44,8 @@ template <typename T> void formatFieldElem(const InterpVal* interpVal, llvm::raw
std::vector<uint64_t> ExternHandler::doExtern(llvm::StringRef name,
llvm::StringRef extra,
llvm::ArrayRef<const InterpVal*> args,
size_t outCount) {
size_t outCount,
bool* failed) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The use of pointer-to-bool in this API leaves me feeling sketchy, both because of the vagueness about lifetimes associated with bare pointers, and because the caller can easily (perhaps inadvertently) ignore the failure of a call. Would it work to return std::optional<std::vector<uint64_t>> instead, using the absence of a result to indicate failure? Or, if that's too heavy, perhaps use a bool& instead?

Perhaps these ideas have been considered and rejected - happy to accept this if there's reason to do it this way - but it'd be good to know.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was reluctant because there were a lot of places where existing extern handlers use return {};, which would then mean std::nullopt instead of an empty array. I got over it and fixed all of these, because you're right that it's better to do it that way.

Copy link

linear bot commented Nov 12, 2024

@jacobdweightman jacobdweightman enabled auto-merge (squash) November 13, 2024 19:08
@jacobdweightman jacobdweightman merged commit 0da40e9 into main Nov 13, 2024
8 checks passed
@jacobdweightman jacobdweightman deleted the jacob/assert branch November 13, 2024 19:26
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