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

Fixed error reporting in the verifier #2031

Merged
merged 4 commits into from Sep 28, 2023
Merged

Fixed error reporting in the verifier #2031

merged 4 commits into from Sep 28, 2023

Conversation

cmnrd
Copy link
Collaborator

@cmnrd cmnrd commented Sep 27, 2023

This PR fixes several issues related to error reporting in the verifier.

  • The verifier was invoked in LFGenerator. This led to the problem that GeneratorBase cleared all reported errors after the verifier run (See this comment). This is fixed by moving the invocation of the verifier to GeneratorBase.
  • If something went wrong while running uclid, the verifier code would simply exit. This led to the problem that the verifier tests abort without any indication of why (See this CI run). The verifier now correctly reports the error using our message reporter.
  • The PR also includes several cleanups.

Copy link
Collaborator

@lsk567 lsk567 left a comment

Choose a reason for hiding this comment

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

Thanks for working on this, Christian. Looks good to me!

I can confirm this is working on my Mac machine, including when one of the dependencies is not installed.

@lsk567
Copy link
Collaborator

lsk567 commented Sep 27, 2023

One minor point is the time measurement doesn't seem working. The tests all take 0 time to complete.

    ------------------------------------------------------------------------------
    Covered: 13/13
    ------------------------------------------------------------------------------

    ------------------------------------------------------------------------------
    Passing: 12/13
    ------------------------------------------------------------------------------
    Passed: src/verifier/AircraftDoor.lf in 0.00 seconds
    Passed: src/verifier/Alarm.lf in 0.00 seconds
    Passed: src/verifier/CoopSchedule.lf in 0.00 seconds
    Passed: src/verifier/Election2.lf in 0.00 seconds
    Passed: src/verifier/PingPongVerifier.lf in 0.00 seconds
    Passed: src/verifier/ProcessMsg.lf in 0.00 seconds
    Passed: src/verifier/ProcessSync.lf in 0.00 seconds
    Passed: src/verifier/Ring.lf in 0.00 seconds
    Passed: src/verifier/SafeSend.lf in 0.00 seconds
    Passed: src/verifier/Thermostat.lf in 0.00 seconds
    Passed: src/verifier/TrainDoor.lf in 0.00 seconds
    Passed: src/verifier/UnsafeSend.lf in 0.00 seconds
    ------------------------------------------------------------------------------
    +---------------------------------------------------------------------------+
    Failed: src/verifier/ADASModel.lf in 0.00 seconds

The failure case is artificially injected. No need to worry about it.

@lhstrh
Copy link
Member

lhstrh commented Sep 27, 2023

Heads up: this will clash with my refactoring in #2008.

@lhstrh
Copy link
Member

lhstrh commented Sep 27, 2023

The failure case is artificially injected. No need to worry about it.

What do you mean? If it fails the whole test doesn't pass. Are you saying it should not be marked as failing?

@lsk567
Copy link
Collaborator

lsk567 commented Sep 27, 2023

The failure case is artificially injected. No need to worry about it.

What do you mean? If it fails the whole test doesn't pass. Are you saying it should not be marked as failing?

I meant I changed the test on my local branch to make it fail, so that I can observe whether the test harness can catch it. And it did successfully. The version on the remote is still passing.

@cmnrd cmnrd added this pull request to the merge queue Sep 28, 2023
@cmnrd
Copy link
Collaborator Author

cmnrd commented Sep 28, 2023

I think the reported times are execution times of the binary program. For the verifier tests, we only build and do not run the tests, so I guess this explains the numbers.

Merged via the queue into master with commit bee1d9f Sep 28, 2023
41 checks passed
@cmnrd cmnrd deleted the verifier-fix branch September 28, 2023 13:46
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.

None yet

3 participants