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

Does current table_executor consider default action reachability? #54

Closed
RabbitWhite1 opened this issue Feb 22, 2024 · 5 comments · Fixed by #56
Closed

Does current table_executor consider default action reachability? #54

RabbitWhite1 opened this issue Feb 22, 2024 · 5 comments · Fixed by #56

Comments

@RabbitWhite1
Copy link
Collaborator

RabbitWhite1 commented Feb 22, 2024

The current implementation of table_executor adds reachability for each action here. But the expression actionHitCondition is constructed with new IR::LAnd(hitCondition, actionChoice), where hitCondition is for key matching and actionChoice is for action name.

But for default actions, shall we add something like "if none of the actions matched, the default action will be reached"?

The reason I am thinking of this is that I noticed the removing for tna_operations.p4 looks incorrect. For example, if a packet misses on table forward, it should be dropped by executing default action miss. But since forward.apply() is removed, it won't be dropped.

@fruffy
Copy link
Owner

fruffy commented Feb 22, 2024

This is a bug in the dead code eliminator, less so in the analysis. It used to be the case that only NoAction was removed, but we should instead substitute the apply call with the default action instead if a table is removed.

@RabbitWhite1
Copy link
Collaborator Author

RabbitWhite1 commented Feb 22, 2024

I see. I found that defaultonly actions are filtered out in the dead code eliminator. Can we simply transform the apply call to the default action call right before here if the default action for this table is specified?

@fruffy
Copy link
Owner

fruffy commented Feb 22, 2024

Yes, we basically retrieve the default action from the table. We can check whether the body is empty, too.

@RabbitWhite1
Copy link
Collaborator Author

Great. I will submit a pr to fix this.

@RabbitWhite1
Copy link
Collaborator Author

RabbitWhite1 commented Feb 23, 2024

Fixed by #56. Also updated reference files. PTAL @fruffy

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 a pull request may close this issue.

2 participants