Skip to content

Commit 9b6c3a5

Browse files
ghadishaybanstuarthalloway
ghadishayban
authored and
stuarthalloway
committed
Clear reference to 'this' on all tail calls
Removes calls to emitClearLocals(), which is a no-op. When the context is RETURN (indicating a tail call), and the operation is an InvokeExpr, StaticMethodExpr, or InstanceMethodExpr, clear the reference to 'this' which is in slot 0 of the locals. Edge-case: Inside the body of a try block, we cannot clear 'this' at the tail position as we might need to keep refs around for use in the catch or finally clauses. Introduces another truthy dynamic binding var to track position being inside a try block. In a try block with no catch or finally, use enclosing context and return a regular BodyExpr. Adds two helpers to emitClearThis and inTailCall. Adds test for the original reducer case and some try/catch cases. Signed-off-by: Stuart Halloway <[email protected]>
1 parent 6f4a49b commit 9b6c3a5

File tree

3 files changed

+81
-43
lines changed

3 files changed

+81
-43
lines changed

‎src/jvm/clojure/lang/Compiler.java‎

Lines changed: 59 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -223,6 +223,7 @@ public class Compiler implements Opcodes{
223223
static final public Var METHOD = Var.create(null).setDynamic();
224224

225225
//null or not
226+
static final public Var IN_TRY_BLOCK = Var.create(null).setDynamic();
226227
static final public Var IN_CATCH_FINALLY = Var.create(null).setDynamic();
227228

228229
static final public Var NO_RECUR = Var.create(null).setDynamic();
@@ -371,6 +372,10 @@ static boolean isSpecial(Object sym){
371372
return specials.containsKey(sym);
372373
}
373374

375+
static boolean inTailCall(C context) {
376+
return (context == C.RETURN) && (IN_TRY_BLOCK.deref() == null);
377+
}
378+
374379
static Symbol resolveSymbol(Symbol sym){
375380
//already qualified or classname?
376381
if(sym.name.indexOf('.') > 0)
@@ -1002,12 +1007,13 @@ else if(instance != null && instance.hasJavaClass() && instance.getJavaClass() !
10021007
Symbol sym = (Symbol) RT.first(call);
10031008
Symbol tag = tagOf(form);
10041009
PersistentVector args = PersistentVector.EMPTY;
1010+
boolean tailPosition = inTailCall(context);
10051011
for(ISeq s = RT.next(call); s != null; s = s.next())
10061012
args = args.cons(analyze(context == C.EVAL ? context : C.EXPRESSION, s.first()));
10071013
if(c != null)
1008-
return new StaticMethodExpr(source, line, column, tag, c, munge(sym.name), args);
1014+
return new StaticMethodExpr(source, line, column, tag, c, munge(sym.name), args, tailPosition);
10091015
else
1010-
return new InstanceMethodExpr(source, line, column, tag, instance, munge(sym.name), args);
1016+
return new InstanceMethodExpr(source, line, column, tag, instance, munge(sym.name), args, tailPosition);
10111017
}
10121018
}
10131019
}
@@ -1444,13 +1450,15 @@ static class InstanceMethodExpr extends MethodExpr{
14441450
public final int line;
14451451
public final int column;
14461452
public final Symbol tag;
1453+
public final boolean tailPosition;
14471454
public final java.lang.reflect.Method method;
14481455

14491456
final static Method invokeInstanceMethodMethod =
14501457
Method.getMethod("Object invokeInstanceMethod(Object,String,Object[])");
14511458

14521459

1453-
public InstanceMethodExpr(String source, int line, int column, Symbol tag, Expr target, String methodName, IPersistentVector args)
1460+
public InstanceMethodExpr(String source, int line, int column, Symbol tag, Expr target,
1461+
String methodName, IPersistentVector args, boolean tailPosition)
14541462
{
14551463
this.source = source;
14561464
this.line = line;
@@ -1459,6 +1467,7 @@ public InstanceMethodExpr(String source, int line, int column, Symbol tag, Expr
14591467
this.methodName = methodName;
14601468
this.target = target;
14611469
this.tag = tag;
1470+
this.tailPosition = tailPosition;
14621471
if(target.hasJavaClass() && target.getJavaClass() != null)
14631472
{
14641473
List methods = Reflector.getMethods(target.getJavaClass(), args.count(), methodName, false);
@@ -1552,10 +1561,10 @@ public void emitUnboxed(C context, ObjExpr objx, GeneratorAdapter gen){
15521561
gen.checkCast(type);
15531562
MethodExpr.emitTypedArgs(objx, gen, method.getParameterTypes(), args);
15541563
gen.visitLineNumber(line, gen.mark());
1555-
if(context == C.RETURN)
1564+
if(tailPosition)
15561565
{
15571566
ObjMethod method = (ObjMethod) METHOD.deref();
1558-
method.emitClearLocals(gen);
1567+
method.emitClearThis(gen);
15591568
}
15601569
Method m = new Method(methodName, Type.getReturnType(method), Type.getArgumentTypes(method));
15611570
if(method.getDeclaringClass().isInterface())
@@ -1626,12 +1635,14 @@ static class StaticMethodExpr extends MethodExpr{
16261635
public final int column;
16271636
public final java.lang.reflect.Method method;
16281637
public final Symbol tag;
1638+
public final boolean tailPosition;
16291639
final static Method forNameMethod = Method.getMethod("Class classForName(String)");
16301640
final static Method invokeStaticMethodMethod =
16311641
Method.getMethod("Object invokeStaticMethod(Class,String,Object[])");
16321642
final static Keyword warnOnBoxedKeyword = Keyword.intern("warn-on-boxed");
16331643

1634-
public StaticMethodExpr(String source, int line, int column, Symbol tag, Class c, String methodName, IPersistentVector args)
1644+
public StaticMethodExpr(String source, int line, int column, Symbol tag, Class c,
1645+
String methodName, IPersistentVector args, boolean tailPosition)
16351646
{
16361647
this.c = c;
16371648
this.methodName = methodName;
@@ -1640,6 +1651,7 @@ public StaticMethodExpr(String source, int line, int column, Symbol tag, Class c
16401651
this.line = line;
16411652
this.column = column;
16421653
this.tag = tag;
1654+
this.tailPosition = tailPosition;
16431655

16441656
List methods = Reflector.getMethods(c, args.count(), methodName, true);
16451657
if(methods.isEmpty())
@@ -1778,10 +1790,10 @@ public void emit(C context, ObjExpr objx, GeneratorAdapter gen){
17781790
MethodExpr.emitTypedArgs(objx, gen, method.getParameterTypes(), args);
17791791
gen.visitLineNumber(line, gen.mark());
17801792
//Type type = Type.getObjectType(className.replace('.', '/'));
1781-
if(context == C.RETURN)
1793+
if(tailPosition)
17821794
{
17831795
ObjMethod method = (ObjMethod) METHOD.deref();
1784-
method.emitClearLocals(gen);
1796+
method.emitClearThis(gen);
17851797
}
17861798
Type type = Type.getType(c);
17871799
Method m = new Method(methodName, Type.getReturnType(method), Type.getArgumentTypes(method));
@@ -2265,13 +2277,14 @@ public Expr parse(C context, Object frm) {
22652277
}
22662278
else
22672279
{
2268-
if(bodyExpr == null)
2269-
try {
2270-
Var.pushThreadBindings(RT.map(NO_RECUR, true));
2271-
bodyExpr = (new BodyExpr.Parser()).parse(context, RT.seq(body));
2272-
} finally {
2273-
Var.popThreadBindings();
2274-
}
2280+
if(bodyExpr == null)
2281+
try {
2282+
Var.pushThreadBindings(RT.map(NO_RECUR, true, IN_TRY_BLOCK, RT.T));
2283+
bodyExpr = (new BodyExpr.Parser()).parse(context, RT.seq(body));
2284+
} finally {
2285+
Var.popThreadBindings();
2286+
}
2287+
22752288
if(Util.equals(op, CATCH))
22762289
{
22772290
Class c = HostExpr.maybeClass(RT.second(f), false);
@@ -2319,17 +2332,21 @@ public Expr parse(C context, Object frm) {
23192332
}
23202333
}
23212334
}
2322-
if(bodyExpr == null) {
2323-
try
2324-
{
2325-
Var.pushThreadBindings(RT.map(NO_RECUR, true));
2326-
bodyExpr = (new BodyExpr.Parser()).parse(C.EXPRESSION, RT.seq(body));
2327-
}
2328-
finally
2329-
{
2330-
Var.popThreadBindings();
2331-
}
2332-
}
2335+
if(bodyExpr == null)
2336+
{
2337+
// this codepath is hit when there is neither catch or finally, e.g. (try (expr))
2338+
// return a body expr directly
2339+
try
2340+
{
2341+
Var.pushThreadBindings(RT.map(NO_RECUR, true));
2342+
bodyExpr = (new BodyExpr.Parser()).parse(context, RT.seq(body));
2343+
}
2344+
finally
2345+
{
2346+
Var.popThreadBindings();
2347+
}
2348+
return bodyExpr;
2349+
}
23332350

23342351
return new TryExpr(bodyExpr, catches, finallyExpr, retLocal,
23352352
finallyLocal);
@@ -2577,23 +2594,13 @@ public void emit(C context, ObjExpr objx, GeneratorAdapter gen){
25772594
gen.newInstance(type);
25782595
gen.dup();
25792596
MethodExpr.emitTypedArgs(objx, gen, ctor.getParameterTypes(), args);
2580-
if(context == C.RETURN)
2581-
{
2582-
ObjMethod method = (ObjMethod) METHOD.deref();
2583-
method.emitClearLocals(gen);
2584-
}
25852597
gen.invokeConstructor(type, new Method("<init>", Type.getConstructorDescriptor(ctor)));
25862598
}
25872599
else
25882600
{
25892601
gen.push(destubClassName(c.getName()));
25902602
gen.invokeStatic(RT_TYPE, forNameMethod);
25912603
MethodExpr.emitArgsAsArray(args, objx, gen);
2592-
if(context == C.RETURN)
2593-
{
2594-
ObjMethod method = (ObjMethod) METHOD.deref();
2595-
method.emitClearLocals(gen);
2596-
}
25972604
gen.invokeStatic(REFLECTOR_TYPE, invokeConstructorMethod);
25982605
}
25992606
if(context == C.STATEMENT)
@@ -3570,6 +3577,7 @@ static class InvokeExpr implements Expr{
35703577
public final IPersistentVector args;
35713578
public final int line;
35723579
public final int column;
3580+
public final boolean tailPosition;
35733581
public final String source;
35743582
public boolean isProtocol = false;
35753583
public boolean isDirect = false;
@@ -3579,12 +3587,14 @@ static class InvokeExpr implements Expr{
35793587
static Keyword onKey = Keyword.intern("on");
35803588
static Keyword methodMapKey = Keyword.intern("method-map");
35813589

3582-
public InvokeExpr(String source, int line, int column, Symbol tag, Expr fexpr, IPersistentVector args) {
3590+
public InvokeExpr(String source, int line, int column, Symbol tag, Expr fexpr, IPersistentVector args, boolean tailPosition) {
35833591
this.source = source;
35843592
this.fexpr = fexpr;
35853593
this.args = args;
35863594
this.line = line;
35873595
this.column = column;
3596+
this.tailPosition = tailPosition;
3597+
35883598
if(fexpr instanceof VarExpr)
35893599
{
35903600
Var fvar = ((VarExpr)fexpr).var;
@@ -3736,10 +3746,10 @@ void emitArgsAndCall(int firstArgToEmit, C context, ObjExpr objx, GeneratorAdapt
37363746
}
37373747
gen.visitLineNumber(line, gen.mark());
37383748

3739-
if(context == C.RETURN)
3749+
if(tailPosition)
37403750
{
37413751
ObjMethod method = (ObjMethod) METHOD.deref();
3742-
method.emitClearLocals(gen);
3752+
method.emitClearThis(gen);
37433753
}
37443754

37453755
gen.invokeInterface(IFN_TYPE, new Method("invoke", OBJECT_TYPE, ARG_TYPES[Math.min(MAX_POSITIONAL_ARITY + 1,
@@ -3755,6 +3765,7 @@ public Class getJavaClass() {
37553765
}
37563766

37573767
static public Expr parse(C context, ISeq form) {
3768+
boolean tailPosition = inTailCall(context);
37583769
if(context != C.EVAL)
37593770
context = C.EXPRESSION;
37603771
Expr fexpr = analyze(context, form.first());
@@ -3817,7 +3828,7 @@ static public Expr parse(C context, ISeq form) {
38173828
// throw new IllegalArgumentException(
38183829
// String.format("No more than %d args supported", MAX_POSITIONAL_ARITY));
38193830

3820-
return new InvokeExpr((String) SOURCE.deref(), lineDeref(), columnDeref(), tagOf(form), fexpr, args);
3831+
return new InvokeExpr((String) SOURCE.deref(), lineDeref(), columnDeref(), tagOf(form), fexpr, args, tailPosition);
38213832
}
38223833
}
38233834

@@ -5758,6 +5769,11 @@ void emitClearLocalsOld(GeneratorAdapter gen){
57585769
}
57595770
}
57605771
}
5772+
5773+
void emitClearThis(GeneratorAdapter gen) {
5774+
gen.visitInsn(Opcodes.ACONST_NULL);
5775+
gen.visitVarInsn(Opcodes.ASTORE, 0);
5776+
}
57615777
}
57625778

57635779
public static class LocalBinding{
@@ -6183,14 +6199,14 @@ public Expr parse(C context, Object frm) {
61836199
{
61846200
if(recurMismatches != null && RT.booleanCast(recurMismatches.nth(i/2)))
61856201
{
6186-
init = new StaticMethodExpr("", 0, 0, null, RT.class, "box", RT.vector(init));
6202+
init = new StaticMethodExpr("", 0, 0, null, RT.class, "box", RT.vector(init), false);
61876203
if(RT.booleanCast(RT.WARN_ON_REFLECTION.deref()))
61886204
RT.errPrintWriter().println("Auto-boxing loop arg: " + sym);
61896205
}
61906206
else if(maybePrimitiveType(init) == int.class)
6191-
init = new StaticMethodExpr("", 0, 0, null, RT.class, "longCast", RT.vector(init));
6207+
init = new StaticMethodExpr("", 0, 0, null, RT.class, "longCast", RT.vector(init), false);
61926208
else if(maybePrimitiveType(init) == float.class)
6193-
init = new StaticMethodExpr("", 0, 0, null, RT.class, "doubleCast", RT.vector(init));
6209+
init = new StaticMethodExpr("", 0, 0, null, RT.class, "doubleCast", RT.vector(init), false);
61946210
}
61956211
//sequential enhancement of env (like Lisp let*)
61966212
try

‎test/clojure/test_clojure/compilation.clj‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -342,3 +342,21 @@
342342
(deftest clj-1399
343343
;; throws an exception on failure
344344
(is (eval `(fn [] ~(CLJ1399. 1)))))
345+
346+
(deftest CLJ-1250-this-clearing
347+
(let [closed-over-in-catch (let [x :foo]
348+
(fn []
349+
(try
350+
(throw (Exception. "boom"))
351+
(catch Exception e
352+
x)))) ;; x should remain accessible to the fn
353+
354+
a (atom nil)
355+
closed-over-in-finally (fn []
356+
(try
357+
:ret
358+
(finally
359+
(reset! a :run))))]
360+
(is (= :foo (closed-over-in-catch)))
361+
(is (= :ret (closed-over-in-finally)))
362+
(is (= :run @a))))

‎test/clojure/test_clojure/reducers.clj‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,3 +89,7 @@
8989
([ret k v] (when (= k k-fail)
9090
(throw (IndexOutOfBoundsException.)))))
9191
(zipmap (range test-map-count) (repeat :dummy)))))))
92+
93+
(deftest test-closed-over-clearing
94+
;; this will throw OutOfMemory without proper reference clearing
95+
(is (number? (reduce + 0 (r/map identity (range 1e8))))))

0 commit comments

Comments
 (0)