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

Correctly infer argument types for constructor calls #3686

Merged
merged 4 commits into from
Nov 15, 2022

Conversation

mihaibudiu
Copy link
Contributor

Signed-off-by: Mihai Budiu mbudiu@vmware.com
This PR is extracted from #3520.
It fixes a couple of bugs which were discovered using the list tests, but it really has nothing to do with lists.
#3520 should be rebased on top of this one.
The attached test used to fail before this fix.

Copy link

@apinski-cavium apinski-cavium left a comment

Choose a reason for hiding this comment

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

Just some simplifications because of C++17. structured binding and initialization of pairs via {} syntax.

arguments = newArgs;
auto objectType = new IR::Type_Extern(ext->srcInfo, ext->name, methodType->typeParameters,
ext->methods);
return std::pair<const IR::Type*, const IR::Vector<IR::Argument>*>(objectType, arguments);

Choose a reason for hiding this comment

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

Since p4c is now compiling as C++17 you can just do:
return {objectType, arguments};
here.

expression->arguments);
auto contType = typeAndArgs.first;
auto newArgs = typeAndArgs.second;
if (newArgs == nullptr)

Choose a reason for hiding this comment

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

auto [contType, newArgs] = checkExternConstructor(expression, simpleType->to<IR::Type_Extern>(),
                                                  expression->arguments);

(github did save my comment previously).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The bazel build system complains about your proposal:

frontends/p4/typeChecking/typeChecker.cpp:1130:14: warning: decomposition declaration only available with -std=c++1z or -std=gnu++1z
         auto [type, newArgs] = checkExternConstructor(decl, et, decl->arguments);
              ^

Anyone know where the language level is in the bazel scripts?

Copy link

@jfingerh jfingerh left a comment

Choose a reason for hiding this comment

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

The test case and expected output looks reasonable to me. I have not made any attempt to review the changes to the compiler internals, so treat this approval with that qualification.

Copy link
Contributor

@jafingerhut jafingerhut left a comment

Choose a reason for hiding this comment

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

Adding review from the account with write access, in case that helps make progress. Again, I am only able to knowledgeably review the test case and expected output files, which look good.

rst0git
rst0git previously approved these changes Nov 12, 2022
Copy link
Member

@rst0git rst0git left a comment

Choose a reason for hiding this comment

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

LGTM

@rst0git rst0git dismissed their stale review November 12, 2022 12:48

p4testdata/random.p4(55): [--Werror=type-error] error: AssignmentStatement

@rst0git
Copy link
Member

rst0git commented Nov 12, 2022

It looks like this change breaks the random.p4 test in the eBPF backend:

$ p4c-ebpf -Ibackends/ebpf/runtime/p4include --target kernel -o ptf_out/random.c p4testdata/random.p4 --Wdisable=unused --max-ternary-masks 3 -DPORT0=4 -DPORT1=5 -DPORT2=6 -DPORT3=7 -DPORT4=8 -DPORT5=9 -DPSA_RECIRC=2 --xdp2tc=cpumap --arch psa;
p4testdata/random.p4(55): [--Werror=type-error] error: AssignmentStatement
        a.rand.f3 = r3.read();
                  ^
  ---- Actual error:
  /usr/local/share/p4c/p4include/psa.p4(676): Cannot cast implicitly type 'T' to type 'bit<16>'
  extern Random<T> {
                ^
  ---- Originating from:
  p4testdata/random.p4(55): Source expression 'r3.read' produces a result of type 'T' which cannot be assigned to a left-value with type 'bit<16>'
          a.rand.f3 = r3.read();
                      ^^^^^^^^^
  /usr/local/share/p4c/p4include/psa.p4(676)
  extern Random<T> {
                ^
p4testdata/random.p4(64): [--Werror=type-error] error: cast: action argument must be a compile-time constant
        default_action = do_forward((PortId_t) 5);
                                    ^^^^^^^^^^^^
p4testdata/random.p4(70): [--Werror=type-error] error: AssignmentStatement
        a.rand.f2 = r1.read();
                  ^
  ---- Actual error:
  /usr/local/share/p4c/p4include/psa.p4(676): Cannot cast implicitly type 'T' to type 'bit<16>'
  extern Random<T> {
                ^
  ---- Originating from:
  p4testdata/random.p4(70): Source expression 'r1.read' produces a result of type 'T' which cannot be assigned to a left-value with type 'bit<16>'
          a.rand.f2 = r1.read();
                      ^^^^^^^^^
  /usr/local/share/p4c/p4include/psa.p4(676)
  extern Random<T> {
                ^

@mihaibudiu
Copy link
Contributor Author

@fruffy this fails in cpplint. However, 'make cpplint' no longer runs cpplint. Can we fix that so that is continues to work the same way it did? Also, from the current output logs I cannot understand where the failure is.

@mihaibudiu
Copy link
Contributor Author

I see the actual error, but it's not easy to search for it, since the word "error" does not appear close to the problem.

Mihai Budiu added 4 commits November 14, 2022 12:04
Signed-off-by: Mihai Budiu <mbudiu@vmware.com>
Signed-off-by: Mihai Budiu <mbudiu@vmware.com>
Signed-off-by: Mihai Budiu <mbudiu@vmware.com>
Signed-off-by: Mihai Budiu <mbudiu@vmware.com>
@fruffy
Copy link
Collaborator

fruffy commented Nov 14, 2022

I can add a CMake target. In the meantime, tools/run-cpplint.sh should do the same thing.

@jfingerh
Copy link

@mbudiu-vmw Are you hoping for any other reviews for this? If so, it might be nice to ping them explicitly.

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.

6 participants