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

Failure to build with non ansi port declarations #854

Closed
antonblanchard opened this issue Jan 5, 2023 · 4 comments
Closed

Failure to build with non ansi port declarations #854

antonblanchard opened this issue Jan 5, 2023 · 4 comments

Comments

@antonblanchard
Copy link

In efabless/caravel#402 an update of Icarus Verilog resulted in a compile error. A cut down test case:

module test (
  input i,
  output o
);

  wire i;
  reg o;

endmodule

The error reported by iverilog:

test.v:6: error: 'i' has already been declared in this scope.
test.v:2:      : It was declared here as a net.
test.v:7: error: 'o' has already been declared in this scope.
test.v:3:      : It was declared here as a net.

This was bisected to 6204b78

Assuming this syntax is "non-ANSI" (I don't deal with a lot of Verilog so I wasn't 100% sure), then it looks like an issue with the parser where an implicit net type ends up marking the port as SR_BOTH.

@larsclausen
Copy link
Collaborator

larsclausen commented Jan 5, 2023

LRM explicitly declares the code above as not valid

23.2.2.2 ANSI style list of port declarations

Each port declaration provides the complete information about the port. The port’s direction, width, net or
variable type, and signedness are completely described. The port identifier shall not be redeclared, in part or
in full, inside the module body.

ANSI style declaration is when your port list is in parenthesis. Non-ANSI when it is as separate statements.

// ANSI
module test(input i, ouput o);
...
endmodule

// Non-ANSI
module test;
input i;
output o;
...
endmodule

For the latter case it is allowed to also have a wire i in there, for the former not.

I'm a bit torn here. On one hand this is breaking existing code, so it might make sense to have a compiler option to allow this. On the other hand it is clearly not allowed by the standard and not just a gray area. And other tools also will report an error.

@antonblanchard
Copy link
Author

@larsclausen Thanks for the clarification. I just checked Verilator and it fails too, so it seems like the caravel verilog should be fixed.

@antonblanchard
Copy link
Author

I created a PR to fix the caravel issues: efabless/caravel#403

@martinwhitaker
Copy link
Collaborator

As caravel has been fixed, closing as invalid. We can reconsider if we get a lot more complaints.

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

No branches or pull requests

3 participants