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

llvm: Fix validity predicate for memory reads with symbolic block numbers #1238

Merged
merged 1 commit into from
Aug 13, 2024

Conversation

langston-barrett
Copy link
Contributor

Previously, this case returned the concretely-true predicate. This is incorrect, as isAllocatedGeneric takes an argument inAlloc, which is supposed to be applied to the AllocInfo of the allocation with the matching block number, as it is in the other branch.

Mea culpa, looks like I introduced this bug long ago.

@langston-barrett
Copy link
Contributor Author

I tried to create a regression test, but I'm having trouble triggering this case. Here's what I tried:

(defun @main () Unit
  (start start:
    (let szbv (fresh (Bitvector 64)))
    (let sz (ptr 64 0 szbv))

    (let mg (resolve-global "memset"))
    (let mh (load-handle (Ptr 64) ((Ptr 64) (Ptr 32) (Ptr 64)) mg))

    (let asz (bv 64 1))
    (let p (alloca none asz))
    (let pblk (ptr-block 64 p))
    (let qblk (fresh Nat))
    (assume! (equal? qblk pblk) "equal block numbers")
    (let q (ptr 64 qblk (bv 64 0)))

    (let c (ptr 32 0 (bv 32 0)))
    (let _ (funcall mh q c sz))

    (return ())))

But it looks like this wasn't good enough to both (1) avoid the Just case of asNat but (2) still hit the Just case of asConstantPred.

@langston-barrett
Copy link
Contributor Author

For what it's worth, I do have a test-case for a downstream project that this fixes.

@langston-barrett langston-barrett marked this pull request as ready for review August 9, 2024 16:06
@RyanGlScott
Copy link
Contributor

If it makes you feel better, some of the uc-crux-llvm test cases are sensitive to this change (as seen in the crux-llvm CI failures), so there's that. I'll leave it to your judgment to audit these test failures to figure out which changes are benign and which are evidence of deeper issues.

…bers

Previously, this case returned the concretely-true predicate. This is
incorrect, as `isAllocatedGeneric` takes an argument `inAlloc`, which is
supposed to be applied to the `AllocInfo` of the allocation with the
matching block number, as it is in the other branch.

Mea culpa, looks like I introduced this bug long ago.

re: UC-Crux tests. It looks to me like what's happening is that UC-Crux
previously found certain errors in each of these test cases that were
adequately handled by its heuristics. Fixing this predicate introduces a
new kind of error, and the heuristics aren't sophisticated enough to
deal with this one. UC-Crux then gives up, saying it's not sure how to
avoid these errors with a new precondition (leaving them "unclassified").

This is unfortunate, but UC-Crux already can't abduce preconditions for
a wide variety of programs, so I don't consider this a blocker.
@langston-barrett
Copy link
Contributor Author

It looks to me like what's happening is that UC-Crux previously found certain errors in each of these test cases that were adequately handled by its heuristics. Fixing this predicate introduces a new kind of error, and the heuristics aren't sophisticated enough to deal with this one. UC-Crux then gives up, saying it's not sure how to avoid these errors with a new precondition (leaving them "unclassified").

This is unfortunate, but UC-Crux already can't abduce preconditions for a wide variety of programs, so I don't consider this a blocker.

Copy link
Contributor

@RyanGlScott RyanGlScott left a comment

Choose a reason for hiding this comment

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

I do wonder if this will cause other proof goals to shift in downstream projects (e.g., SAW). That being said, this code is clearly more correct than it was before, so I'm in support of landing it.

@langston-barrett langston-barrett merged commit 7514e90 into master Aug 13, 2024
32 checks passed
@langston-barrett langston-barrett deleted the lb/fix-symbolic-reads branch August 13, 2024 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants