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

Type check that RHS of shift is integral type #5099

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

Conversation

vlstill
Copy link
Contributor

@vlstill vlstill commented Jan 15, 2025

Fixes #5094. There was no check that RHS of shift is an integral type. In most targets, this was probably a backend error prior, but theoretically this could be a breaking change.

@vlstill vlstill added core Topics concerning the core segments of the compiler (frontend, midend, parser) p4-spec Topics related to the P4 specification (https://github.com/p4lang/p4-spec/). labels Jan 15, 2025
@vlstill vlstill self-assigned this Jan 15, 2025
Signed-off-by: Vladimír Štill <[email protected]>
Signed-off-by: Vladimír Štill <[email protected]>
Signed-off-by: Vladimír Štill <[email protected]>
@vlstill
Copy link
Contributor Author

vlstill commented Jan 15, 2025

OK, there is a problem, since the midend usually lowers type-introduced types back to their underlying type, this bug actually allowed shift by values of type introduced by type, e.g. the PortId_t in dpdk/psa.p4. By the spec (8.23. Operations on types introduced by type) this should not be allowed, but that would be far bigger breaking change than I originally thought. This probably needs more discussion on how to resolve it, I'm thinking of explicitly stripping type in the type checker for now to permit this behavior at least temporarily.

EDIT: --> #5100

@ChrisDodd
Copy link
Contributor

According to 8.11.2 in the spec, serializable enums may be implicitly converted to their underlying type, so this code probably needs to deal with that case too.

@vlstill
Copy link
Contributor Author

vlstill commented Jan 16, 2025

Right, let me check if I can reuse some existing check for implicit casts, we don't want these duplicated all around.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Topics concerning the core segments of the compiler (frontend, midend, parser) p4-spec Topics related to the P4 specification (https://github.com/p4lang/p4-spec/).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Shift by a string literal is not rejected by type checker
2 participants