Graal: DSL flat layout issue with custom abstract execute method: Incorrect cast

[smarr]

The following class leads to incorrectly generated code:

@DSLOptions(defaultGenerator = DSLGenerator.FLAT)
public abstract class FooBar extends Node {
  @Children protected final FooBar[] args;

  protected FooBar() { args = new FooBar[0]; }

  public abstract Object execute(VirtualFrame frame, Object[] args);

  @Specialization
  public Object test(final Object[] args) {
    return null;
  }
}

It results in the following execute(.) method. The last line has the incorrect (Object) cast.

    @Override
    public Object execute(VirtualFrame frameValue, Object[] arg0Value) {
        int state = state_;
        if ((state & 0b10) != 0 /* is-active test(Object[]) */) {
            return test(arg0Value);
        }
        CompilerDirectives.transferToInterpreterAndInvalidate();
        return executeAndSpecialize((Object) arg0Value);
    }

[chumer]

This should just cause a warning, right?

[smarr]

Warning? No, that's non-compiling code.
I think I forgot to post this:

private Object executeAndSpecialize(Object[] arg0Value) {...}

executeAndSpecialize is actually typed correctly with the first argument being Object[]. So, the issue is the incorrect cast to (Object).

Didn't see casts in other similar generated code, so perhaps checking whether a cast is necessary doesn't get it quite right with arrays? I am just guessing.

[chumer]

Ah. you are right. my bad. The problem is around varargs support in the dsl. sometimes a Object cast is necessary. It somehow triggers at the wrong place here.

[smarr]

@chumer any advise on where I could look to fix this, or how to add a test that I could debug?

[smarr]

A test class that will break the build:

diff --git a/truffle/com.oracle.truffle.api.dsl.test/src/com/oracle/truffle/api/dsl/test/ExecuteMethodTest.java b/truffle/com.oracle.truffle.api.dsl.test/src/com/oracle/truffle/api/dsl/test/ExecuteMethodTest.java
index 2a9dd8ed..3a5a8006 100644
--- a/truffle/com.oracle.truffle.api.dsl.test/src/com/oracle/truffle/api/dsl/test/ExecuteMethodTest.java
+++ b/truffle/com.oracle.truffle.api.dsl.test/src/com/oracle/truffle/api/dsl/test/ExecuteMethodTest.java
@@ -436,4 +436,24 @@ public class ExecuteMethodTest {
         }
     }

+    @TypeSystem
+    @DSLOptions(defaultGenerator = DSLOptions.DSLGenerator.FLAT)
+    static class ObjectArrayTypes {
+    }
+
+    @TypeSystemReference(ObjectArrayTypes.class)
+    abstract static class ObjectArrayAsArg extends Node {
+        @Children protected final ObjectArrayAsArg[] args;
+
+        ObjectArrayAsArg() {
+            args = new ObjectArrayAsArg[0];
+        }
+
+        abstract Object execute(VirtualFrame frame, Object[] args);
+
+        @Specialization
+        Object test(Object[] a) {
+            return null;
+        }
+    }
 }

[smarr]

For reference, to debug this, I did the following:

• download ecj (as shown in .travis.yml): wget http://archive.eclipse.org/eclipse/downloads/drops4/R-4.5.2-201602121500/ecj-4.5.2.jar
• make sure it is used compilation with export JDT=ecj-4.5.2.jar
• build dsl test project like this: ../mx/mx -v -d build --only com.oracle.truffle.api.dsl.test -c --no-daemon
• put a breakpoint in NodeCodeGenerator.generateNodes(..) checking that the type element has the name ObjectArrayAsArg

Now I see that FlatNodeGenerator.FrameState.addReferencesTo(..) has a hardcoded check like this:

if (executions.size() == 1 && ElementUtils.typeEquals(var.getTypeMirror(), factory.getType(Object[].class))) {
    // if the current type is Object[] do not use varargs for a single argument
    builder.string("(Object) ");
}

This triggers for my case. So, it looks like I need the type of the called method to check that we don't actually have a Object[] argument.
In FlatNodeGenerator.createCallExecuteAndSpecialize(..) I got the ExecutableTypeData forType, which seems to have that information. However, in wrapInAMethod(..) I don't seem to have the same type info. Perhaps it is in parentMethod. So, I wonder whether passing in the type info is the correct thing to do, and whether checking for the specific type, before applying a (Object) cast is the right thing to do.

[chumer]

The fix is in the pipeline.

[chumer]

fixed

[smarr]

Thanks