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

disable iff in assertion causes assertion to fail #1640

Closed
veripoolbot opened this issue Dec 15, 2019 · 9 comments
Closed

disable iff in assertion causes assertion to fail #1640

veripoolbot opened this issue Dec 15, 2019 · 9 comments

Comments

@veripoolbot
Copy link

@veripoolbot veripoolbot commented Dec 15, 2019


Author Name: Peter Monsson
Original Redmine Issue: 1640 from https://www.veripool.org

Original Assignee: Peter Monsson


The following assertion should never fail:

a_valid: assert property (
@(posedge clk) disable iff (1==1)
1==1
);

But unfortunately, adding this assertion to top.v in make_tracing_c causes the simulation to fail with:

[3] %Error: top.v:42: Assertion failed in TOP.top.a_valid: 'assert property' failed.
%Error: top.v:42: Verilog $stop
Aborting...
Makefile:59: recipe for target 'run' failed
make: *** [run] Aborted (core dumped)

I am open to fix this issue, but I would need some hand holding, f. x. where do I look for the error in the source code?

Best Regards
Peter

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Dec 15, 2019


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-12-15T19:46:38Z


If you could take a look that would be great.

Run with --debug and look at the tree file to check the expression, in verilog.y you'll see y_IFF ("iff") becomes an argument to AstPslClocked. This is then expanded in V3AssertPre, probably that's where the problem is.

In your patch please update a test_regress test to show the issue, add your name to docs/CONTRIBUTORS and use indicate your email address for the commit (or if you prefer point me to a github branch with the change).

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Dec 17, 2019


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-12-17T11:12:21Z


Note some assertion bug fixes were recently committed, so might want to pull from master when looking at this.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Dec 20, 2019


Original Redmine Comment
Author Name: Peter Monsson
Original Date: 2019-12-20T20:04:22Z


I have updated to the newest master and created a patch with testcases. I hope that this is sufficient. Please let me know what is missing.

I have run regression on the t_assert_property*.pl tests. Is there a way to have a small list of tests to run? The full regression is a bit too slow on my notebook.

Best Regards
Peter

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Dec 20, 2019


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-12-20T22:44:27Z


Seems reasonable, thanks for looking into a patch. The only test I see failing you should look at is t_assert_cover. It's possible the test was bad and needs updating.

Also please rename _fails_1.pl to _bad_1.pl (we use _bad for failing tests).

If you think you'll be making other changes, please consider making a github account and send your github username & associated email address so the commit log will be correct.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Dec 21, 2019


Original Redmine Comment
Author Name: Peter Monsson
Original Date: 2019-12-21T11:05:10Z


Ahh, I see.

         blockp = new AstAnd(nodep->disablep()->fileline(),
                             new AstNot(nodep->disablep()->fileline(),
                                        nodep->disablep()->unlinkFrBack()),
                             blockp);

Works for cover, but not assert.

         blockp = new AstOr(nodep->disablep()->fileline(),
                            nodep->disablep()->unlinkFrBack(),
                            blockp);

Works for assert, but not cover.

         blockp = new AstIf(nodep->disablep()->fileline(),
                            new AstLogNot(nodep->disablep()->fileline(),
                                          nodep->disablep()->unlinkFrBack()),
                            blockp);

Gives me a V3Clean.cpp:111 internal error.

I am not sure hoe to fix this. I can look at the parent type to see if it is a cover or assert?

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Dec 21, 2019


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-12-21T12:48:29Z


I can look at the parent type to see if it is a cover or assert?

It looks like "iff" disables the action, which means don't throw an assert, or don't collect coverage. Currently "true" in the code means assert is ok, or collect coverage, so yes, I think you'd need to look at the parent type.

I'll take that patch, though note I'm not sure if this is the right thing to do long term or invert the true/false terms for the assert.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Dec 22, 2019

An update got lost in the bug migration:

Issue #1640 has been updated by Peter Monsson. File issue_1640.patch added I have updated the patch to handle cover and assertion properties. The old behavior stays for cover properties. The new behavior is for everything else.

@wsnyder

This comment has been minimized.

Copy link
Member

@wsnyder wsnyder commented Dec 22, 2019

Thanks Peter, code pushed to git.

@wsnyder wsnyder closed this Dec 22, 2019
@PeterMonsson

This comment has been minimized.

Copy link
Contributor

@PeterMonsson PeterMonsson commented Dec 23, 2019

Thank you, for helping me out with my first bugfix. Happy to help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.