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

Enforce RTLIL Wire/Const limit size of 2^24 #3460

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

lethalbit
Copy link
Member

This commit adds asserts to the RTLIL parser to ensure that a wire definition is no more than 2^24 bits wide, as well as that RTLIL Consts can not have a negative width nor be wider than 2^24 bits.

This addresses #1206 and #3317

@Ravenslofty
Copy link
Collaborator

I think the correct thing to use here is log_error rather than throwing an exception. I doubt any code is going to catch it anyway.

@lethalbit
Copy link
Member Author

Possibly, I'll see how it feels with log_error, I also need to test to see how things like memories interact with the changes, although all the tests pass we don't have any that would directly exercise this code that I know of, so more work absolutely needs to be done on it.

@nakengelhardt
Copy link
Member

Looking at the surrounding code, +1 on log_error.

@nakengelhardt
Copy link
Member

We need to test where large consts may be created:

  • memory initialization
  • FSM transition tables
  • others?

@@ -187,7 +187,11 @@ wire_stmt:

wire_options:
wire_options TOK_WIDTH TOK_INT {
current_wire->width = $3;
if ($3 > 0x1000000) {
Copy link
Contributor

Choose a reason for hiding this comment

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

it maybe better to use a constexpr defined in rtlil.h instead of repeating the magic number everywhere; my 2-cents

@@ -214,6 +217,12 @@ RTLIL::Const::Const(const std::string &str)

RTLIL::Const::Const(int val, int width)
{
if (width < 0)
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be a log_assert rather than log_error since this case isn't supposed to ever happen.

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

Successfully merging this pull request may close these issues.

None yet

5 participants