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

Split Painless AST into a "user" tree and an "ir" tree #51278

Merged
merged 25 commits into from
Jan 24, 2020
Merged
Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
ceafd7c
copy nodes to split ir and ast
jdconrad Dec 15, 2019
bf21ab1
converted some ast nodes to ir nodes
jdconrad Dec 15, 2019
71add6c
converted more nodes
jdconrad Dec 15, 2019
b6ccc78
checkpoint
jdconrad Dec 15, 2019
754d2d9
converted more nodes
jdconrad Dec 16, 2019
fd96614
completion of expression node conversion
jdconrad Dec 16, 2019
b6c7d8e
convert all prefix nodes
jdconrad Dec 17, 2019
f68872a
partially changed node data to user getters/setters
jdconrad Dec 18, 2019
c18a182
partially changed data to be mutable
jdconrad Dec 18, 2019
0bcc0c7
converted more nodes to ir
jdconrad Dec 19, 2019
58f8e6d
completeion of first pass of splitting nodes
jdconrad Dec 19, 2019
6349c67
fixes
jdconrad Dec 19, 2019
ab969e4
fix subtle issue with unboxing def
jdconrad Dec 19, 2019
86dc9a7
move script root
jdconrad Dec 21, 2019
6149e43
remove class
jdconrad Dec 22, 2019
32b8b8b
add setters with covariant return types for all nodes
jdconrad Dec 24, 2019
a9118e7
build ir tree from ast
jdconrad Dec 24, 2019
058c416
fix bugs/tests
jdconrad Dec 25, 2019
e4fcce2
remove bad refactor of initializer to initializerNode
jdconrad Jan 21, 2020
0222ff5
remove pseudo builder setters from ir nodes
jdconrad Jan 22, 2020
fbe2745
modify user nodes to work with ir setters
jdconrad Jan 23, 2020
8a0a65c
fix instanceof missing node
jdconrad Jan 23, 2020
7f123a4
Merge branch 'master' into trees1
jdconrad Jan 23, 2020
1c03a51
remove all default constructors from ir nodes
jdconrad Jan 23, 2020
56a366a
added some comments for cat
jdconrad Jan 23, 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
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,11 @@

import org.elasticsearch.bootstrap.BootstrapInfo;
import org.elasticsearch.painless.antlr.Walker;
import org.elasticsearch.painless.ir.ClassNode;
import org.elasticsearch.painless.lookup.PainlessLookup;
import org.elasticsearch.painless.node.SClass;
import org.elasticsearch.painless.spi.Whitelist;
import org.elasticsearch.painless.symbol.ScriptRoot;
import org.objectweb.asm.util.Printer;

import java.lang.reflect.Method;
Expand Down Expand Up @@ -212,13 +214,14 @@ ScriptRoot compile(Loader loader, Set<String> extractedVariables, String name, S
SClass root = Walker.buildPainlessTree(scriptClassInfo, name, source, settings, painlessLookup, null);
root.extractVariables(extractedVariables);
ScriptRoot scriptRoot = root.analyze(painlessLookup, settings);
Map<String, Object> statics = root.write();
ClassNode classNode = root.writeClass();
Map<String, Object> statics = classNode.write();

try {
Class<? extends PainlessScript> clazz = loader.defineScript(CLASS_NAME, root.getBytes());
Class<? extends PainlessScript> clazz = loader.defineScript(CLASS_NAME, classNode.getBytes());
clazz.getField("$NAME").set(null, name);
clazz.getField("$SOURCE").set(null, source);
clazz.getField("$STATEMENTS").set(null, root.getStatements());
clazz.getField("$STATEMENTS").set(null, classNode.getStatements());
clazz.getField("$DEFINITION").set(null, painlessLookup);

for (Map.Entry<String, Object> statik : statics.entrySet()) {
Expand All @@ -244,8 +247,9 @@ byte[] compile(String name, String source, CompilerSettings settings, Printer de
debugStream);
root.extractVariables(new HashSet<>());
root.analyze(painlessLookup, settings);
root.write();
ClassNode classNode = root.writeClass();
classNode.write();

return root.getBytes();
return classNode.getBytes();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import org.elasticsearch.painless.lookup.PainlessLookup;
import org.elasticsearch.painless.lookup.PainlessLookupBuilder;
import org.elasticsearch.painless.spi.Whitelist;
import org.elasticsearch.painless.symbol.ScriptRoot;
import org.elasticsearch.script.ScriptContext;
import org.elasticsearch.script.ScriptEngine;
import org.elasticsearch.script.ScriptException;
Expand Down Expand Up @@ -340,7 +341,7 @@ private <T> T generateFactory(
GeneratorAdapter deterAdapter = new GeneratorAdapter(Opcodes.ASM5, isResultDeterministic,
writer.visitMethod(Opcodes.ACC_PUBLIC, methodName, isResultDeterministic.getDescriptor(), null, null));
deterAdapter.visitCode();
deterAdapter.push(scriptRoot.deterministic);
deterAdapter.push(scriptRoot.isDeterministic());
stu-elastic marked this conversation as resolved.
Show resolved Hide resolved
deterAdapter.returnValue();
deterAdapter.endMethod();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ class PainlessParser extends Parser {
RULE_lamtype = 31, RULE_funcref = 32;
public static final String[] ruleNames = {
"source", "function", "parameters", "statement", "rstatement", "dstatement",
"trailer", "block", "empty", "initializer", "afterthought", "declaration",
"trailer", "block", "empty", "initializer", "afterthought", "declaration",
"decltype", "declvar", "trap", "expression", "unary", "chain", "primary",
"postfix", "postdot", "callinvoke", "fieldaccess", "braceaccess", "arrayinitializer",
"listinitializer", "mapinitializer", "maptoken", "arguments", "argument",
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
/*
* Licensed to Elasticsearch under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

package org.elasticsearch.painless.ir;

import java.util.ArrayList;
import java.util.List;

public abstract class ArgumentsNode extends ExpressionNode {

/* ---- begin tree structure ---- */

private final List<ExpressionNode> argumentNodes = new ArrayList<>();
Copy link
Contributor

Choose a reason for hiding this comment

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

These are the actual arguments, correct? If so, consider renaming to arguments. It's a bit confusing to have an ArgumentsNode type with field argumentNodes.

Copy link
Contributor Author

@jdconrad jdconrad Jan 23, 2020

Choose a reason for hiding this comment

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

I'm open to suggestions here. I know the names are a bit confusing and clash, but they do both describe accurately what each item is. I have divided the data in the "ir" nodes with two types of data - tree structure and local data. All tree structure members end with Node to help differentiate this from typical data. This also creates a general consistency between all nodes.


public void addArgumentNode(ExpressionNode argumentNode) {
argumentNodes.add(argumentNode);
}

public List<ExpressionNode> getArgumentNodes() {
return argumentNodes;
}

/* ---- end tree structure */

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,213 @@
/*
* Licensed to Elasticsearch under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

package org.elasticsearch.painless.ir;


import org.elasticsearch.painless.ClassWriter;
import org.elasticsearch.painless.DefBootstrap;
import org.elasticsearch.painless.Globals;
import org.elasticsearch.painless.MethodWriter;
import org.elasticsearch.painless.Operation;
import org.elasticsearch.painless.lookup.PainlessCast;
import org.elasticsearch.painless.lookup.PainlessLookupUtility;
import org.elasticsearch.painless.lookup.def;

public class AssignmentNode extends BinaryNode {

/* ---- begin node data ---- */

private boolean pre;
private boolean post;
private Operation operation;
private boolean read;
private boolean cat;
Copy link
Contributor

Choose a reason for hiding this comment

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

What is a cat here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cat is short for concatenation for String types. These names are copied directly from their equivalent "user" node, so I would prefer to leave this for now as mechanical, but renaming should be a future change.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's clear when reading the code further down, but a word or two as a comment would be nice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a comment here and in BinaryMathNode that also does concatenations.

private Class<?> compoundType;
private PainlessCast there;
private PainlessCast back;

public void setPre(boolean pre) {
this.pre = pre;
}

public boolean getPre() {
return pre;
}

public void setPost(boolean post) {
this.post = post;
}

public boolean getPost() {
return post;
}

public void setOperation(Operation operation) {
this.operation = operation;
}

public Operation getOperation() {
return operation;
}

public void setRead(boolean read) {
this.read = read;
}

public boolean getRead() {
return read;
}

public void setCat(boolean cat) {
this.cat = cat;
}

public boolean getCat() {
return cat;
}

public void setCompoundType(Class<?> compoundType) {
this.compoundType = compoundType;
}

public Class<?> getCompoundType() {
return compoundType;
}

public String getCompoundCanonicalTypeName() {
return PainlessLookupUtility.typeToCanonicalTypeName(compoundType);
}

public void setThere(PainlessCast there) {
this.there = there;
}

public PainlessCast getThere() {
return there;
}

public void setBack(PainlessCast back) {
this.back = back;
}

public PainlessCast getBack() {
return back;
}

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

@Override
protected void write(ClassWriter classWriter, MethodWriter methodWriter, Globals globals) {
methodWriter.writeDebugInfo(location);

// For the case where the assignment represents a String concatenation
// we must, depending on the Java version, write a StringBuilder or
// track types going onto the stack. This must be done before the
// lhs is read because we need the StringBuilder to be placed on the
// stack ahead of any potential concatenation arguments.
int catElementStackSize = 0;

if (cat) {
catElementStackSize = methodWriter.writeNewStrings();
}

getLeftNode().setup(classWriter, methodWriter, globals); // call the setup method on the lhs to prepare for a load/store operation

if (cat) {
// Handle the case where we are doing a compound assignment
// representing a String concatenation.

methodWriter.writeDup(getLeftNode().accessElementCount(), catElementStackSize); // dup the top element and insert it
// before concat helper on stack
getLeftNode().load(classWriter, methodWriter, globals); // read the current lhs's value
methodWriter.writeAppendStrings(getLeftNode().getExpressionType()); // append the lhs's value using the StringBuilder

getRightNode().write(classWriter, methodWriter, globals); // write the bytecode for the rhs

// check to see if the rhs has already done a concatenation
if (getRightNode() instanceof BinaryMathNode == false || ((BinaryMathNode)getRightNode()).getCat() == false) {
// append the rhs's value since it's hasn't already
methodWriter.writeAppendStrings(getRightNode().getExpressionType());
}

methodWriter.writeToStrings(); // put the value for string concat onto the stack
methodWriter.writeCast(back); // if necessary, cast the String to the lhs actual type

if (read) {
// if this lhs is also read from dup the value onto the stack
methodWriter.writeDup(MethodWriter.getType(
getLeftNode().getExpressionType()).getSize(), getLeftNode().accessElementCount());
}

// store the lhs's value from the stack in its respective variable/field/array
getLeftNode().store(classWriter, methodWriter, globals);
} else if (operation != null) {
// Handle the case where we are doing a compound assignment that
// does not represent a String concatenation.

methodWriter.writeDup(getLeftNode().accessElementCount(), 0); // if necessary, dup the previous lhs's value
// to be both loaded from and stored to
getLeftNode().load(classWriter, methodWriter, globals); // load the current lhs's value

if (read && post) {
// dup the value if the lhs is also read from and is a post increment
methodWriter.writeDup(MethodWriter.getType(
getLeftNode().getExpressionType()).getSize(), getLeftNode().accessElementCount());
}

methodWriter.writeCast(there); // if necessary cast the current lhs's value
// to the promotion type between the lhs and rhs types
getRightNode().write(classWriter, methodWriter, globals); // write the bytecode for the rhs

// XXX: fix these types, but first we need def compound assignment tests.
// its tricky here as there are possibly explicit casts, too.
// write the operation instruction for compound assignment
if (compoundType == def.class) {
methodWriter.writeDynamicBinaryInstruction(
location, compoundType, def.class, def.class, operation, DefBootstrap.OPERATOR_COMPOUND_ASSIGNMENT);
} else {
methodWriter.writeBinaryInstruction(location, compoundType, operation);
}

methodWriter.writeCast(back); // if necessary cast the promotion type value back to the lhs's type

if (read && !post) {
// dup the value if the lhs is also read from and is not a post increment
methodWriter.writeDup(MethodWriter.getType(
getLeftNode().getExpressionType()).getSize(), getLeftNode().accessElementCount());
}

// store the lhs's value from the stack in its respective variable/field/array
getLeftNode().store(classWriter, methodWriter, globals);
} else {
// Handle the case for a simple write.

getRightNode().write(classWriter, methodWriter, globals); // write the bytecode for the rhs rhs

if (read) {
// dup the value if the lhs is also read from
methodWriter.writeDup(MethodWriter.getType(
getLeftNode().getExpressionType()).getSize(), getLeftNode().accessElementCount());
}

// store the lhs's value from the stack in its respective variable/field/array
getLeftNode().store(classWriter, methodWriter, globals);
}
}
}
Loading