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.

Sign up for GitHub

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

Remove custom "execute" method from SClass user node in Painless #51776

Merged
merged 8 commits into from
Feb 4, 2020
Merged

Remove custom "execute" method from SClass user node in Painless #51776

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
e42292c
add data to functions required to move execute method out of class
jdconrad Dec 27, 2019
27f21d0
fix tests
jdconrad Dec 27, 2019
111edd4
remove dead code
jdconrad Dec 27, 2019
eb03654
fixes
jdconrad Dec 27, 2019
53217b6
fixes
jdconrad Jan 31, 2020
b12215f
add another todo
jdconrad Jan 31, 2020
b724725
Merge branch 'master' into trees5
jdconrad Feb 3, 2020
3479ef0
response to PR comments.
jdconrad Feb 4, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion modules/lang-painless/src/main/java/org/elasticsearch/painless/MethodWriter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,8 @@ public void writeStatementOffset(Location location) {
int offset = location.getOffset();
// ensure we don't have duplicate stuff going in here. can catch bugs
// (e.g. nodes get assigned wrong offsets by antlr walker)
assert statements.get(offset) == false;
// TODO: introduce a way to ignore internal statements so this assert is not triggered
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider cutting an issue for TODOs like this so they don't get lost, then ref the issue in the comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done: #51836

Copy link
Contributor

Choose a reason for hiding this comment

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

Pls reference from TODO.

//assert statements.get(offset) == false;
statements.set(offset);
}

Expand Down
19 changes: 17 additions & 2 deletions modules/lang-painless/src/main/java/org/elasticsearch/painless/antlr/Walker.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@
import org.elasticsearch.painless.antlr.PainlessParser.VariableContext;
import org.elasticsearch.painless.antlr.PainlessParser.WhileContext;
import org.elasticsearch.painless.lookup.PainlessLookup;
import org.elasticsearch.painless.lookup.PainlessLookupUtility;
import org.elasticsearch.painless.node.AExpression;
import org.elasticsearch.painless.node.ANode;
import org.elasticsearch.painless.node.AStatement;
Expand Down Expand Up @@ -251,7 +252,20 @@ public ANode visitSource(SourceContext ctx) {
statements.add((AStatement)visit(statement));
}

return new SClass(scriptClassInfo, sourceName, sourceText, debugStream, location(ctx), functions, statements);
String returnCanonicalTypeName = PainlessLookupUtility.typeToCanonicalTypeName(scriptClassInfo.getExecuteMethodReturnType());
List<String> paramTypes = new ArrayList<>();
stu-elastic marked this conversation as resolved.
Show resolved Hide resolved
List<String> paramNames = new ArrayList<>();

for (ScriptClassInfo.MethodArgument argument : scriptClassInfo.getExecuteArguments()) {
paramTypes.add(PainlessLookupUtility.typeToCanonicalTypeName(argument.getClazz()));
paramNames.add(argument.getName());
}

SFunction execute = new SFunction(location(ctx), returnCanonicalTypeName, "execute", paramTypes, paramNames, new SBlock(
location(ctx), statements), true, false, false, true);
stu-elastic marked this conversation as resolved.
Show resolved Hide resolved
functions.add(execute);

return new SClass(scriptClassInfo, sourceName, sourceText, debugStream, location(ctx), functions);
}

@Override
Expand All @@ -278,7 +292,8 @@ public ANode visitFunction(FunctionContext ctx) {
statements.add((AStatement)visit(ctx.block().dstatement()));
}

return new SFunction(location(ctx), rtnType, name, paramTypes, paramNames, new SBlock(location(ctx), statements), false);
return new SFunction(
location(ctx), rtnType, name, paramTypes, paramNames, new SBlock(location(ctx), statements), false, true, false, false);
}

@Override
Expand Down
149 changes: 0 additions & 149 deletions modules/lang-painless/src/main/java/org/elasticsearch/painless/ir/ClassNode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,10 @@
import org.elasticsearch.painless.ScriptClassInfo;
import org.elasticsearch.painless.WriterConstants;
import org.elasticsearch.painless.symbol.ScopeTable;
import org.elasticsearch.painless.symbol.ScopeTable.Variable;
import org.elasticsearch.painless.symbol.ScriptRoot;
import org.objectweb.asm.ClassVisitor;
import org.objectweb.asm.Label;
import org.objectweb.asm.Opcodes;
import org.objectweb.asm.Type;
import org.objectweb.asm.commons.Method;
import org.objectweb.asm.util.Printer;

import java.lang.invoke.MethodType;
Expand All @@ -46,25 +43,15 @@

import static org.elasticsearch.painless.WriterConstants.BASE_INTERFACE_TYPE;
import static org.elasticsearch.painless.WriterConstants.BITSET_TYPE;
import static org.elasticsearch.painless.WriterConstants.BOOTSTRAP_METHOD_ERROR_TYPE;
import static org.elasticsearch.painless.WriterConstants.CLASS_TYPE;
import static org.elasticsearch.painless.WriterConstants.COLLECTIONS_TYPE;
import static org.elasticsearch.painless.WriterConstants.CONVERT_TO_SCRIPT_EXCEPTION_METHOD;
import static org.elasticsearch.painless.WriterConstants.DEFINITION_TYPE;
import static org.elasticsearch.painless.WriterConstants.DEF_BOOTSTRAP_DELEGATE_METHOD;
import static org.elasticsearch.painless.WriterConstants.DEF_BOOTSTRAP_DELEGATE_TYPE;
import static org.elasticsearch.painless.WriterConstants.DEF_BOOTSTRAP_METHOD;
import static org.elasticsearch.painless.WriterConstants.EMPTY_MAP_METHOD;
import static org.elasticsearch.painless.WriterConstants.EXCEPTION_TYPE;
import static org.elasticsearch.painless.WriterConstants.FUNCTION_TABLE_TYPE;
import static org.elasticsearch.painless.WriterConstants.GET_NAME_METHOD;
import static org.elasticsearch.painless.WriterConstants.GET_SOURCE_METHOD;
import static org.elasticsearch.painless.WriterConstants.GET_STATEMENTS_METHOD;
import static org.elasticsearch.painless.WriterConstants.OUT_OF_MEMORY_ERROR_TYPE;
import static org.elasticsearch.painless.WriterConstants.PAINLESS_ERROR_TYPE;
import static org.elasticsearch.painless.WriterConstants.PAINLESS_EXPLAIN_ERROR_GET_HEADERS_METHOD;
import static org.elasticsearch.painless.WriterConstants.PAINLESS_EXPLAIN_ERROR_TYPE;
import static org.elasticsearch.painless.WriterConstants.STACK_OVERFLOW_ERROR_TYPE;
import static org.elasticsearch.painless.WriterConstants.STRING_TYPE;

public class ClassNode extends IRNode {
Expand All @@ -73,7 +60,6 @@ public class ClassNode extends IRNode {

private final List<FieldNode> fieldNodes = new ArrayList<>();
private final List<FunctionNode> functionNodes = new ArrayList<>();
private final List<StatementNode> statementNodes = new ArrayList<>();

public void addFieldNode(FieldNode fieldNode) {
fieldNodes.add(fieldNode);
Expand All @@ -90,14 +76,6 @@ public void addFunctionNode(FunctionNode functionNode) {
public List<FunctionNode> getFunctionsNodes() {
return functionNodes;
}

public void addStatementNode(StatementNode statementNode) {
statementNodes.add(statementNode);
}

public List<StatementNode> getStatementsNodes() {
return statementNodes;
}

/* ---- end tree structure, begin node data ---- */

Expand All @@ -106,7 +84,6 @@ public List<StatementNode> getStatementsNodes() {
private String sourceText;
private Printer debugStream;
private ScriptRoot scriptRoot;
private boolean doesMethodEscape;

public void setScriptClassInfo(ScriptClassInfo scriptClassInfo) {
this.scriptClassInfo = scriptClassInfo;
Expand Down Expand Up @@ -148,14 +125,6 @@ public ScriptRoot getScriptRoot() {
return scriptRoot;
}

public void setMethodEscape(boolean doesMethodEscape) {
this.doesMethodEscape = doesMethodEscape;
}

public boolean doesMethodEscape() {
return doesMethodEscape;
}

/* ---- end node data ---- */

protected Globals globals;
Expand Down Expand Up @@ -245,12 +214,6 @@ public Map<String, Object> write() {
statementsMethod.returnValue();
statementsMethod.endMethod();

// Write the method defined in the interface:
MethodWriter executeMethod = classWriter.newMethodWriter(Opcodes.ACC_PUBLIC, scriptClassInfo.getExecuteMethod());
executeMethod.visitCode();
write(classWriter, executeMethod, globals, new ScopeTable());
executeMethod.endMethod();

// Write all fields:
for (FieldNode fieldNode : fieldNodes) {
fieldNode.write(classWriter, null, null, null);
Expand Down Expand Up @@ -305,116 +268,4 @@ public Map<String, Object> write() {

return statics;
}

@Override
protected void write(ClassWriter classWriter, MethodWriter methodWriter, Globals globals, ScopeTable scopeTable) {
// We wrap the whole method in a few try/catches to handle and/or convert other exceptions to ScriptException
Label startTry = new Label();
Label endTry = new Label();
Label startExplainCatch = new Label();
Label startOtherCatch = new Label();
Label endCatch = new Label();
methodWriter.mark(startTry);

scopeTable.defineInternalVariable(Object.class, "this");

// Method arguments
for (ScriptClassInfo.MethodArgument arg : scriptClassInfo.getExecuteArguments()) {
scopeTable.defineVariable(arg.getClazz(), arg.getName());
}

if (scriptRoot.getCompilerSettings().getMaxLoopCounter() > 0) {
// if there is infinite loop protection, we do this once:
// int #loop = settings.getMaxLoopCounter()

Variable loop = scopeTable.defineInternalVariable(int.class, "loop");

methodWriter.push(scriptRoot.getCompilerSettings().getMaxLoopCounter());
methodWriter.visitVarInsn(Opcodes.ISTORE, loop.getSlot());
}

for (int getMethodIndex = 0; getMethodIndex < scriptClassInfo.getGetMethods().size(); ++getMethodIndex) {
Method method = scriptClassInfo.getGetMethods().get(getMethodIndex);
Class<?> returnType = scriptClassInfo.getGetReturns().get(getMethodIndex);

String name = method.getName().substring(3);
name = Character.toLowerCase(name.charAt(0)) + name.substring(1);

if (scriptRoot.getUsedVariables().contains(name)) {
Variable variable = scopeTable.defineVariable(returnType, name);

methodWriter.loadThis();
methodWriter.invokeVirtual(Type.getType(scriptClassInfo.getBaseClass()), method);
methodWriter.visitVarInsn(method.getReturnType().getOpcode(Opcodes.ISTORE), variable.getSlot());
}
}

for (StatementNode statementNode : statementNodes) {
statementNode.write(classWriter, methodWriter, globals, scopeTable);
}

if (doesMethodEscape == false) {
switch (scriptClassInfo.getExecuteMethod().getReturnType().getSort()) {
case org.objectweb.asm.Type.VOID:
break;
case org.objectweb.asm.Type.BOOLEAN:
methodWriter.push(false);
break;
case org.objectweb.asm.Type.BYTE:
methodWriter.push(0);
break;
case org.objectweb.asm.Type.SHORT:
methodWriter.push(0);
break;
case org.objectweb.asm.Type.INT:
methodWriter.push(0);
break;
case org.objectweb.asm.Type.LONG:
methodWriter.push(0L);
break;
case org.objectweb.asm.Type.FLOAT:
methodWriter.push(0f);
break;
case org.objectweb.asm.Type.DOUBLE:
methodWriter.push(0d);
break;
default:
methodWriter.visitInsn(Opcodes.ACONST_NULL);
}
methodWriter.returnValue();
}

methodWriter.mark(endTry);
methodWriter.goTo(endCatch);
// This looks like:
// } catch (PainlessExplainError e) {
// throw this.convertToScriptException(e, e.getHeaders($DEFINITION))
// }
methodWriter.visitTryCatchBlock(startTry, endTry, startExplainCatch, PAINLESS_EXPLAIN_ERROR_TYPE.getInternalName());
methodWriter.mark(startExplainCatch);
methodWriter.loadThis();
methodWriter.swap();
methodWriter.dup();
methodWriter.getStatic(CLASS_TYPE, "$DEFINITION", DEFINITION_TYPE);
methodWriter.invokeVirtual(PAINLESS_EXPLAIN_ERROR_TYPE, PAINLESS_EXPLAIN_ERROR_GET_HEADERS_METHOD);
methodWriter.invokeInterface(BASE_INTERFACE_TYPE, CONVERT_TO_SCRIPT_EXCEPTION_METHOD);
methodWriter.throwException();
// This looks like:
// } catch (PainlessError | BootstrapMethodError | OutOfMemoryError | StackOverflowError | Exception e) {
// throw this.convertToScriptException(e, e.getHeaders())
// }
// We *think* it is ok to catch OutOfMemoryError and StackOverflowError because Painless is stateless
methodWriter.visitTryCatchBlock(startTry, endTry, startOtherCatch, PAINLESS_ERROR_TYPE.getInternalName());
methodWriter.visitTryCatchBlock(startTry, endTry, startOtherCatch, BOOTSTRAP_METHOD_ERROR_TYPE.getInternalName());
methodWriter.visitTryCatchBlock(startTry, endTry, startOtherCatch, OUT_OF_MEMORY_ERROR_TYPE.getInternalName());
methodWriter.visitTryCatchBlock(startTry, endTry, startOtherCatch, STACK_OVERFLOW_ERROR_TYPE.getInternalName());
methodWriter.visitTryCatchBlock(startTry, endTry, startOtherCatch, EXCEPTION_TYPE.getInternalName());
methodWriter.mark(startOtherCatch);
methodWriter.loadThis();
methodWriter.swap();
methodWriter.invokeStatic(COLLECTIONS_TYPE, EMPTY_MAP_METHOD);
methodWriter.invokeInterface(BASE_INTERFACE_TYPE, CONVERT_TO_SCRIPT_EXCEPTION_METHOD);
methodWriter.throwException();
methodWriter.mark(endCatch);
}
}
Loading