Default-initialized arguments inserted by `DefaultArguments` pass are not cloned

[kfcripps]

extern void __e(in bit<1> x = 1w1);

control C() {
    apply {
        __e();
        __e();
    }
}

control proto();
package top(proto p);

top(C()) main;

The tmp-0002-DefaultArguments_1_DoDefaultArguments.p4 output of p4test --dump tmp --top4 DefaultArguments tmp.p4 -v contains:

...
control C() {
    apply /*
    <BlockStatement>(1942) */
    {
        /*
      <MethodCallStatement>(1945)
        <MethodCallExpression>(1946)
          <PathExpression>(738)
            __e
          <Vector<Type>>(44), size=0
          <Vector<Argument>>(1951), size=1
            <Argument>(1954)
              <Constant>(721) 1
                <Type_Bits>(462) */
        __e(/*
            <Argument>(1954)
              <Constant>(721) 1
                <Type_Bits>(462) */
x = 1w1);
        /*
      <MethodCallStatement>(1955)
        <MethodCallExpression>(1956)
          <PathExpression>(744)
            __e
          <Vector<Type>>(50), size=0
          <Vector<Argument>>(1961), size=1
            <Argument>(1964)
              <Constant>(721) 1
                <Type_Bits>(462) */
        __e(/*
            <Argument>(1964)
              <Constant>(721) 1
                <Type_Bits>(462) */
x = 1w1);
    }
}
...

Note that the 1w1 constant inserted for the x argument to both __e() calls has a node id of 721. This can cause unexpected problems in later passes that expect each of these method calls' argument's expressions to be visited. Since each __e() method call's argument expression points to the same IR::Node, only the expression of the x argument belonging to the first __e() method call will be visited.

I believe the DefaultArguments pass should clone the 1w1 constant instead of each __e() call's argument pointing to the same IR::Constant expression.

[kfcripps]

This seems to fix the problem, but I am not sure if this is the best place to do the cloning:

diff --git a/frontends/p4/defaultArguments.cpp b/frontends/p4/defaultArguments.cpp
index 93ad1339bdc..4d75fee19f2 100644
--- a/frontends/p4/defaultArguments.cpp
+++ b/frontends/p4/defaultArguments.cpp
@@ -66,7 +66,7 @@ static const IR::Vector<IR::Argument> *fillDefaults(const TypeMap *typeMap,
             const IR::Expression *value = param->defaultValue;
             TypeNameSubstitutionVisitor tsvv(tsv, typeMap);
             value = param->defaultValue->apply(tsvv);
-            arg = new IR::Argument(param->srcInfo, param->name, value);
+            arg = new IR::Argument(param->srcInfo, param->name, value->clone());
             changed = true;
         }
         args->push_back(arg);

[kfcripps]

Seems like #2608 fixed a similar problem (but where the parameter's default value did not have an explicit type) (#2599)

[ChrisDodd]

Ideally passes should not depend on identical nodes being distinct, so the clones should not be necessary, but unfortunately several passes are broken like that.

[kfcripps]

@ChrisDodd Hmm, I just notice that there is the visitDagOnce flag that can be set to false to allow nodes to be visited more than once, and setting it to false in my custom backend pass instead of making the change from #5473 (comment) seems to also fix my problem. If you think that not cloning parameter default values when inserting default-initialized arguments into method call expressions will not cause any problems with any of the p4c frontend passes, then I will close this issue and not merge #5474. If you think it might still be useful, then I will merge it.

[ChrisDodd]

When problems are discovered with passses where they don't handle DAGs properly are found, I would prefer to fix them in that pass (so definitely add the visitDagOnce = false flag to that pass). However, if you can't track down what is going wrong in the other pass, adding extra clones to ensure the IR is a pure tree is not unacceptable.

[kfcripps]

Closing this but leaving #5474 open in case anyone else finds a real need for it