From 539177988d3eb8120180371797948dfc95138d78 Mon Sep 17 00:00:00 2001 From: Charles Oliver Nutter Date: Mon, 14 Oct 2024 15:05:51 -0500 Subject: [PATCH 1/7] Add ChilledString to dumper --- core/src/main/java/org/jruby/ir/persistence/IRDumper.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/core/src/main/java/org/jruby/ir/persistence/IRDumper.java b/core/src/main/java/org/jruby/ir/persistence/IRDumper.java index 40c55ea9062..ef4ddcea10f 100644 --- a/core/src/main/java/org/jruby/ir/persistence/IRDumper.java +++ b/core/src/main/java/org/jruby/ir/persistence/IRDumper.java @@ -18,6 +18,7 @@ import org.jruby.ir.operands.Array; import org.jruby.ir.operands.Bignum; import org.jruby.ir.operands.BuiltinClass; +import org.jruby.ir.operands.ChilledString; import org.jruby.ir.operands.ClosureLocalVariable; import org.jruby.ir.operands.Complex; import org.jruby.ir.operands.CurrentScope; @@ -309,6 +310,7 @@ public void Array(Array array) { public void Boolean(org.jruby.ir.operands.Boolean bool) { print(bool.isTrue() ? "t" : "f"); } public void BuiltinClass(BuiltinClass builtinClass) { } // FIXME: need to print enum public void UnboxedBoolean(UnboxedBoolean bool) { print(bool.isTrue() ? "t" : "f"); } + public void ChilledString(ChilledString chilled) { print(chilled.getByteList()); } public void ClosureLocalVariable(ClosureLocalVariable closurelocalvariable) { LocalVariable(closurelocalvariable); } public void CurrentScope(CurrentScope currentscope) { } public void Complex(Complex complex) { visit(complex.getNumber()); } From f275ac93d8e7f25500033e3d93c625d95d9f4229 Mon Sep 17 00:00:00 2001 From: Charles Oliver Nutter Date: Mon, 14 Oct 2024 15:06:24 -0500 Subject: [PATCH 2/7] Chilled strings are not marked as frozen --- core/src/main/java/org/jruby/RubyString.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/core/src/main/java/org/jruby/RubyString.java b/core/src/main/java/org/jruby/RubyString.java index ca11a5be9fe..1a3e44fda3a 100644 --- a/core/src/main/java/org/jruby/RubyString.java +++ b/core/src/main/java/org/jruby/RubyString.java @@ -986,7 +986,6 @@ protected void frozenCheck() { private void mutateChilledString() { getRuntime().getWarnings().warn("literal string will be frozen in the future"); - setFrozen(false); flags &= ~CHILLED_F; } @@ -1314,7 +1313,7 @@ public final IRubyObject minus_at(ThreadContext context) { @JRubyMethod(name = "+@") // +'foo' returns modifiable string public final IRubyObject plus_at() { - return isFrozen() ? this.dup() : this; + return isFrozen() | isChilled() ? this.dup() : this; } @Deprecated @@ -6797,7 +6796,6 @@ public IRubyObject freeze(ThreadContext context) { public RubyString chill() { flags |= CHILLED_F; - setFrozen(true); return this; } From 7400d1be0413500f8989001e33e3cef272cefd9c Mon Sep 17 00:00:00 2001 From: Charles Oliver Nutter Date: Mon, 14 Oct 2024 15:06:50 -0500 Subject: [PATCH 3/7] All callers pass in cached ByteList, so shared --- core/src/main/java/org/jruby/RubyString.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/java/org/jruby/RubyString.java b/core/src/main/java/org/jruby/RubyString.java index 1a3e44fda3a..b287b126063 100644 --- a/core/src/main/java/org/jruby/RubyString.java +++ b/core/src/main/java/org/jruby/RubyString.java @@ -536,7 +536,7 @@ public static RubyString newString(Ruby runtime, ByteList bytes, int coderange) } public static RubyString newChilledString(Ruby runtime, ByteList bytes, int coderange) { - return newString(runtime, bytes, coderange).chill(); + return newStringShared(runtime, bytes, coderange).chill(); } public static RubyString newString(Ruby runtime, ByteList bytes, Encoding encoding) { From e4e096743d97dcdd3b468223733135732f2517dd Mon Sep 17 00:00:00 2001 From: Charles Oliver Nutter Date: Mon, 14 Oct 2024 15:07:26 -0500 Subject: [PATCH 4/7] Override appropriate frozen checks in String We have too many ways to check for frozen status and error etc, and they all need to be chilled-aware, so for now just override them in RubyString. In the future we'll want to sort this out to one master method that subclasses can override for specialized frozen checks. --- core/src/main/java/org/jruby/RubyBasicObject.java | 11 ++--------- core/src/main/java/org/jruby/RubyString.java | 10 ++++++++++ 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/core/src/main/java/org/jruby/RubyBasicObject.java b/core/src/main/java/org/jruby/RubyBasicObject.java index 5d7701773d3..ecfc8ca16e3 100644 --- a/core/src/main/java/org/jruby/RubyBasicObject.java +++ b/core/src/main/java/org/jruby/RubyBasicObject.java @@ -1606,15 +1606,8 @@ public void copyInstanceVariablesInto(final InstanceVariables other) { * including information about whether this object is frozen. * Will throw a suitable exception in that case. */ - public final void ensureInstanceVariablesSettable() { - if (!isFrozen()) { - return; - } else if (this instanceof RubyString string) { - // We put this second to reduce overhead since most objects will not be frozen and we do not want this instanceof all the time. - string.frozenCheck(); - } else { - raiseFrozenError(); - } + public void ensureInstanceVariablesSettable() { + checkFrozen(); } @Override diff --git a/core/src/main/java/org/jruby/RubyString.java b/core/src/main/java/org/jruby/RubyString.java index b287b126063..b69060a07c7 100644 --- a/core/src/main/java/org/jruby/RubyString.java +++ b/core/src/main/java/org/jruby/RubyString.java @@ -984,6 +984,16 @@ protected void frozenCheck() { } } + @Override + public void checkFrozen() { + frozenCheck(); + } + + @Override + public final void ensureInstanceVariablesSettable() { + frozenCheck(); + } + private void mutateChilledString() { getRuntime().getWarnings().warn("literal string will be frozen in the future"); flags &= ~CHILLED_F; From 01216ab390d6d06f7fe3ab3751424fa741db8397 Mon Sep 17 00:00:00 2001 From: Charles Oliver Nutter Date: Mon, 14 Oct 2024 15:08:29 -0500 Subject: [PATCH 5/7] Hook up indy ChilledString logic --- .../java/org/jruby/ir/targets/indy/IndyValueCompiler.java | 2 +- .../java/org/jruby/ir/targets/indy/StringBootstrap.java | 6 +++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/org/jruby/ir/targets/indy/IndyValueCompiler.java b/core/src/main/java/org/jruby/ir/targets/indy/IndyValueCompiler.java index 43bef6c3c3e..ef47841cbd7 100644 --- a/core/src/main/java/org/jruby/ir/targets/indy/IndyValueCompiler.java +++ b/core/src/main/java/org/jruby/ir/targets/indy/IndyValueCompiler.java @@ -89,7 +89,7 @@ public void pushString(ByteList bl, int cr) { public void pushChilledString(ByteList bl, int cr) { compiler.loadContext(); - compiler.adapter.invokedynamic("string", sig(RubyString.class, ThreadContext.class), StringBootstrap.CSTRING_BOOTSTRAP, RubyEncoding.decodeRaw(bl), bl.getEncoding().toString(), cr); + compiler.adapter.invokedynamic("chilled", sig(RubyString.class, ThreadContext.class), StringBootstrap.CSTRING_BOOTSTRAP, RubyEncoding.decodeRaw(bl), bl.getEncoding().toString(), cr); } public void pushFrozenString(ByteList bl, int cr, String file, int line) { diff --git a/core/src/main/java/org/jruby/ir/targets/indy/StringBootstrap.java b/core/src/main/java/org/jruby/ir/targets/indy/StringBootstrap.java index 929a3e701f4..d9bef898dfe 100644 --- a/core/src/main/java/org/jruby/ir/targets/indy/StringBootstrap.java +++ b/core/src/main/java/org/jruby/ir/targets/indy/StringBootstrap.java @@ -80,7 +80,7 @@ public class StringBootstrap { private static final MethodHandle CSTRING_HANDLE = Binder .from(RubyString.class, ThreadContext.class, ByteList.class, int.class) - .invokeStaticQuiet(LOOKUP, StringBootstrap.class, "string"); + .invokeStaticQuiet(LOOKUP, StringBootstrap.class, "chilledString"); private static final MethodHandle FSTRING_HANDLE = Binder @@ -136,6 +136,10 @@ public static RubyString string(ThreadContext context, ByteList value, int cr) { return RubyString.newStringShared(context.runtime, value, cr); } + public static RubyString chilledString(ThreadContext context, ByteList value, int cr) { + return RubyString.newChilledString(context.runtime, value, cr); + } + public static RubyString bufferString(ThreadContext context, Encoding encoding, int size, int cr) { return RubyString.newString(context.runtime, new ByteList(size, encoding), cr); } From 357da0adc23d0e0678b9a47c5591760f2bc28c28 Mon Sep 17 00:00:00 2001 From: Charles Oliver Nutter Date: Mon, 14 Oct 2024 15:22:54 -0500 Subject: [PATCH 6/7] Opt out of ObjectSpace for FString ObjectSpace for string is already problematic, but in this case we have two specialized types of strings that are born frozen before they are fully initialized. Adding these into ObjectSpace requires installing a hidden variable, which triggers frozen checks, which fire causing a FrozenError to try to inspect an incompletely- initialized object. It's simpler to just say that FString (and the Debug version) never go into ObjectSpace. --- core/src/main/java/org/jruby/RubyString.java | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/org/jruby/RubyString.java b/core/src/main/java/org/jruby/RubyString.java index b69060a07c7..f90b94e79ed 100644 --- a/core/src/main/java/org/jruby/RubyString.java +++ b/core/src/main/java/org/jruby/RubyString.java @@ -451,6 +451,11 @@ protected RubyString(Ruby runtime, RubyClass rubyClass, ByteList value, int cr) flags |= cr; } + protected RubyString(Ruby runtime, RubyClass rubyClass, ByteList value, int cr, boolean objectspace) { + this(runtime, rubyClass, value, objectspace); + flags |= cr; + } + // Deprecated String construction routines @Deprecated @@ -1095,7 +1100,7 @@ static class DebugFrozenString extends RubyString { private final int line; protected DebugFrozenString(Ruby runtime, RubyClass rubyClass, ByteList value, int cr, String file, int line) { - super(runtime, rubyClass, value, cr); + super(runtime, rubyClass, value, cr, false); this.file = file; this.line = line; @@ -1132,7 +1137,7 @@ public static class FString extends RubyString { private IRubyObject converted; protected FString(Ruby runtime, RubyClass rubyClass, ByteList value, int cr) { - super(runtime, rubyClass, value, cr); + super(runtime, rubyClass, value, cr, false); // set flag for code that does not use isFrozen setFrozen(true); From 6ff1e2fc6473522ec9787ef6c66624cc4b6d0d7a Mon Sep 17 00:00:00 2001 From: Charles Oliver Nutter Date: Mon, 14 Oct 2024 15:49:19 -0500 Subject: [PATCH 7/7] Restore isFrozen short circuit for ObjectSpace ObjectSpace wants to be able to assign a hidden variable, but calling checkFrozen here leads to child class implementations being called that may have other side effects. This is another place where the frozen checks need to be unified and cleaned up, but for now I restore the !isFrozen short circuit and revert the checkFrozen call to avoid this unintended behavior. --- core/src/main/java/org/jruby/RubyBasicObject.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/org/jruby/RubyBasicObject.java b/core/src/main/java/org/jruby/RubyBasicObject.java index ecfc8ca16e3..8ff11f9d39b 100644 --- a/core/src/main/java/org/jruby/RubyBasicObject.java +++ b/core/src/main/java/org/jruby/RubyBasicObject.java @@ -1607,7 +1607,9 @@ public void copyInstanceVariablesInto(final InstanceVariables other) { * Will throw a suitable exception in that case. */ public void ensureInstanceVariablesSettable() { - checkFrozen(); + if (isFrozen()) { + raiseFrozenError(); + } } @Override