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

Container arguments with type infint are treated as type variables #3889

Merged
merged 1 commit into from
Feb 17, 2023

Conversation

mihaibudiu
Copy link
Contributor

@mihaibudiu mihaibudiu commented Feb 14, 2023

Fixes #3886

The problem is the following: given a constructor like control C()(int x), the type of x is unified with the argument in every invocation of the constructor. But not all invocations must have the exact same argument type.

In other parts of the compiler type 'int' (Type_InfInt) is treated like a type variable - and unification is used to decide the actual concrete type of an int expression. Here we do the same: when the control C is instantiated, we treat the type of x as a fresh type variable; in this way, the arguments of two separate instantiations of C do not need to unify with each other (via the common constructor type).

C(5) c1;
C(4) c2;

We do not require 4 and 5 to have the exact same type, they are different int types.

@mihaibudiu
Copy link
Contributor Author

@fruffy this also fixes some problems with the README, please review.

@mihaibudiu
Copy link
Contributor Author

This should supersede #3884

Copy link
Collaborator

@fruffy fruffy left a comment

Choose a reason for hiding this comment

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

Updating the libbpf dependency might cause issues with the psa-ebpf or ebpf-kernel repository? Does that happen? I guess the XDP one is deprecated.

@@ -240,7 +238,7 @@ use them, but YMMV.

- Google Protocol Buffers 3.0 or higher for control plane API generation

- C++ boost library (minimally used)
- C++ boost library
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should probably reduce our use of boost now that we have C++17 support.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@ChrisDodd ChrisDodd left a comment

Choose a reason for hiding this comment

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

Looks good, and fixes my testcases. If you check this in, I'll prep a change adding just the test.

@@ -240,7 +238,7 @@ use them, but YMMV.

- Google Protocol Buffers 3.0 or higher for control plane API generation

- C++ boost library (minimally used)
- C++ boost library
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@ChrisDodd
Copy link
Contributor

Updating the libbpf dependency might cause issues with the psa-ebpf or ebpf-kernel repository? Does that happen? I guess the XDP one is deprecated.

Looks like this is actually an unrelated change that crept into this PR?

@mihaibudiu
Copy link
Contributor Author

I have squashed the commits and added tests.

Signed-off-by: Mihai Budiu <mbudiu@vmware.com>
@mihaibudiu
Copy link
Contributor Author

Rebased on main to pickup the macos fixes so we can merge.

@mihaibudiu
Copy link
Contributor Author

@fruffy this fails with an assertion failure in the testing module.

What can we do to merge this PR?

 734/1528 Test #3373: testgen-p4c-bmv2-ptf/issue2345.p4 .......................................***Failed    2.79 sec
============ Program seed 1000 =============

warning: Ingress parser exception handler not fully implemented
============ Program trace for Test 0 ============

p: [Parser] p
[State] start
[Event]: Extract: Succeeded
[Extract] hdr.eth_hdr.dst_addr; = 0x0000_0000_0000
[Extract] hdr.eth_hdr.src_addr; = 0x0000_0000_0000
[Extract] hdr.eth_hdr.eth_type; = 0x0000
[Event]: Extract Condition: p4t*zombie.const.0.*packetLen_bits >= 112; | Extract Size: 112
[State] accept
[Event]: Control vrfy start
[Event]: Control ingress start
[Expression]: If Statement false: 2 == 1; = 0;
[Expression]: If Statement false: 0; = 0;
[Expression]: If Statement true: 1; = 1;
[Expression]: If Statement true: 1; = 1;
[Expression]: If Statement false: 0 != 0; = 0;
[Event]: Control egress start
[Event]: Control update start
[Event]: Control deparser start
[Emit] h.eth_hdr.dst_addr; = 0x0000_0000_0002
[Emit] h.eth_hdr.src_addr; = 0x0000_0000_0001
[Emit] h.eth_hdr.eth_type; = 0x0002
[Emit] h.eth_hdr.*valid; = 1;
[Event]: Prepending the emit buffer to the program packet.
[Expression]: If Statement false: 511 == 0; = 0;
=======================================
============ Input packet for Test 0 ============
0000000000000000000000000000BECFA09165F0C14F4A89BFD7B2AB382F0171AB9B222042AEBC844E0B3EA00501BB6D03921316266DF88E289E02AAFF1F9A05DA00E7797EBB054E157BE295737132E0F4AB8DD8A95CD7A84357C403CCF495285090A841C4070EBD5064DAE5191ADCC8511508FCB332D45FF7B45CA3F9A092090F4BB4E973B43D9F400B56F27ECCD0FAAE45FB66273D7A00DE0E076B579EA897888A6A59AEDD3E99A4D5EE5F6A9CDAFE22D2F3E080CD15FAA8F4E2A5759ED13A879C11D0219EE7AE8B72BE589958056D14E92EA85D1E0F7169F264643694E81A40E4BE127BA9C5C2BD3A3503592A9306A9EFD77C51EDDA0C1ABC3BAE8D242D0EA0DB65AE895C582017690A6BF228037B0E2D365B6F59D998DE80DF5A7DBD
=======================================
Input packet size: 2288
============ Ports for Test 0 ============
Input port: 1
=======================================
============ Output packet for Test 0 ============
0000000000020000000000010002BECFA09165F0C14F4A89BFD7B2AB382F0171AB9B222042AEBC844E0B3EA00501BB6D03921316266DF88E289E02AAFF1F9A05DA00E7797EBB054E157BE295737132E0F4AB8DD8A95CD7A84357C403CCF495285090A841C4070EBD5064DAE5191ADCC8511508FCB332D45FF7B45CA3F9A092090F4BB4E973B43D9F400B56F27ECCD0FAAE45FB66273D7A00DE0E076B579EA897888A6A59AEDD3E99A4D5EE5F6A9CDAFE22D2F3E080CD15FAA8F4E2A5759ED13A879C11D0219EE7AE8B72BE589958056D14E92EA85D1E0F7169F264643694E81A40E4BE127BA9C5C2BD3A3503592A9306A9EFD77C51EDDA0C1ABC3BAE8D242D0EA0DB65AE895C582017690A6BF228037B0E2D365B6F59D998DE80DF5A7DBD
=======================================
Output packet size: 2288 
=======================================
============ Output mask Test 0 ============
FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF
=======================================
Output port: 0

=======================================
============ Test 0: Statements covered: 0.888889 (8/9) ============
============ End Test 0 ============

@fruffy
Copy link
Collaborator

fruffy commented Feb 17, 2023

This could be non-determinism, let me rerun first.

@fruffy
Copy link
Collaborator

fruffy commented Feb 17, 2023

The test is passing now. Looks like a fluke because I do not understand how this line can fail.

@mihaibudiu mihaibudiu merged commit 0c82a42 into p4lang:main Feb 17, 2023
@mihaibudiu mihaibudiu deleted the issue3884 branch February 17, 2023 02:01
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.

Problems passing int types through constructors to other constructors.
3 participants