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

Connecting the wrong interface to a port does not cause an error #1294

Closed
veripoolbot opened this issue Mar 28, 2018 · 4 comments
Closed

Connecting the wrong interface to a port does not cause an error #1294

veripoolbot opened this issue Mar 28, 2018 · 4 comments
Assignees

Comments

@veripoolbot
Copy link

@veripoolbot veripoolbot commented Mar 28, 2018


Author Name: Todd Strader (@toddstrader)
Original Redmine Issue: 1294 from https://www.veripool.org

Original Assignee: Todd Strader (@toddstrader)


Please see the GitHub branch for a test and a not-quite-there-yet fix:
https://github.com/toddstrader/verilator-dev/tree/wrong_intf

The fix works fine for my simple example, but breaks a lot of other tests like so:

%Error: t/t_interface_gen.v:26: Expected ifc interface but got ifc interface instead

The problem seems to be that the point at which I've added the check, interfaces are being de-parameterized. This means that the port's interface reference is pointing at the general interface while the pin is pointing at the specialized interface.

I just tried changing the test to compare ifaceName() instead of ifacep(). This works (all tests pass), but with the additional string compare. Is this the preferable way to go, or can this be caught at some earlier stage (without reproducing a lot of V3Param logic) so we can avoid the string compare?

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Mar 28, 2018


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2018-03-28T22:59:21Z


Seems an OK spot, I'd still compare the pointers as you do to speed things up, but then add an "&& " check on the names being different

Nits: Rename the test t_interface_wrong_bad.pl/v, put single quotes around the error names, and call AstNode::prettyName (so that a \escaped name will print right).

I think something like this makes the error clearer:

                         pinp->v3error("Port '"<<pinp->prettyName()<<"' expects '"
                                       <<AstNode::prettyName(portIrefp->ifaceName())
                                       <<"' interface but pin connects '"
                                       <<AstNode::prettyName(pinIrefp->ifaceName())
                                       <<"' interface");

Also not sure why you need a return - that will just make it miss reporting other possible errors if there's more than one interface. Think just remove it.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Apr 2, 2018


Original Redmine Comment
Author Name: Todd Strader (@toddstrader)
Original Date: 2018-04-02T18:21:06Z


I think the latest commit takes care of everything:
https://github.com/toddstrader/verilator-dev/tree/wrong_intf

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Apr 5, 2018


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2018-04-05T01:04:16Z


Good stuff! Thanks.

Merged into git towards 3.924.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Jun 13, 2018


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2018-06-13T01:29:20Z


In 3.924.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.