Fix FNV-collision symbol aliasing and memoize OP_SYMBOL

src/ChibiRuby.SourceGenerator/ChibiRubySourceGenerator.cs

@@ -227,32 +227,23 @@ public static bool TryFind(int hashCode, ReadOnlySpan<byte> name, out Symbol sym
                 stringBuilder.AppendLine($$"""
             case {{x.Key}}:
 """);
-                if (x.Count() == 1)
+                // Always verify the actual name: a user-defined symbol may collide with a
+                // known symbol's 32-bit hash, and must not alias it.
+                var branch = "if";
+                foreach (var xs in x)
                 {
-                    var singleValue = x.First();
                     stringBuilder.AppendLine($$"""
-                symbol = new Symbol({{singleValue.Index}});
-                return true;
-""");
-                }
-                else
-                {
-                    var branch = "if";
-                    foreach (var xs in x)
-                    {
-                        stringBuilder.AppendLine($$"""
                 {{branch}} (name.SequenceEqual("{{xs.SymbolName}}"u8))
                 {
                     symbol = new Symbol({{xs.Index}});
                     return true;
                 }
 """);
-                        branch = "else if";
-                    }
-                    stringBuilder.AppendLine($$"""
+                    branch = "else if";
+                }
+                stringBuilder.AppendLine($$"""
                 break;
 """);
-                }
             }
             stringBuilder.AppendLine($$"""
         }

src/ChibiRuby/Irep.cs

@@ -29,11 +29,24 @@ public readonly struct CatchHandler(CatchHandlerType handlerType, uint begin, ui
     public readonly uint Target = target;
 }
 
+/// <summary>
+/// Memoized results of interning this irep's pool strings (OP_SYMBOL operands).
+/// Owner id and the symbol array live in one immutable-shape object so that a
+/// state can never observe another state's symbols through a torn owner/array pair.
+/// </summary>
+sealed class IrepPoolSymbolCache(int ownerStateId, Symbol[] symbols)
+{
+    public readonly int OwnerStateId = ownerStateId;
+    public readonly Symbol[] Symbols = symbols;
+}
+
 /// <summary>
 /// Program data
 /// </summary>
 public class Irep
 {
+    internal IrepPoolSymbolCache? PoolSymbolCache;
+
     public byte Flags { get; init; }
     public ushort RegisterVariableCount { get; init; }
     public byte[] Sequence { get; init; } = [];

src/ChibiRuby/MRubyState.Init.cs

@@ -80,6 +80,15 @@ public static MRubyState Create()
 
     public RiteParser RiteParser => riteParser ??= new RiteParser(this);
 
+    static int stateIdSource;
+
+    /// <summary>
+    /// Process-wide unique id of this state. Symbols are interned per state, so
+    /// caches of interned symbols (e.g. <see cref="Irep.PoolSymbolCache"/>) are
+    /// tagged with this id to stay valid when an Irep is shared between states.
+    /// </summary>
+    internal readonly int StateId = System.Threading.Interlocked.Increment(ref stateIdSource);
+
     readonly SymbolTable symbolTable = new();
     readonly VariableTable globalVariables = new();
 

src/ChibiRuby/MRubyState.Vm.cs

@@ -492,6 +492,25 @@ internal MRubyValue EvalUnder(MRubyValue self, RProc block, RClass c)
     /// <summary>
     /// Execute irep assuming the Stack values are placed
     /// </summary>
+    /// <summary>
+    /// OP_SYMBOL slow path: intern the pool string and memoize it on the irep.
+    /// A cache owned by another state is replaced wholesale (owner id and symbol
+    /// array travel together), so shared ireps stay correct across states.
+    /// </summary>
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    static Symbol PoolSymbolMiss(MRubyState state, Irep irep, int poolIndex)
+    {
+        var symbol = state.Intern(irep.PoolValues[poolIndex].As<RString>());
+        var cache = irep.PoolSymbolCache;
+        if (cache == null || cache.OwnerStateId != state.StateId)
+        {
+            cache = new IrepPoolSymbolCache(state.StateId, new Symbol[irep.PoolValues.Length]);
+            irep.PoolSymbolCache = cache;
+        }
+        cache.Symbols[poolIndex] = symbol;
+        return symbol;
+    }
+
     internal MRubyValue Execute(Irep irep, int pc, int stackKeep, RException? injectedRaise = null)
     {
         Exception = null;
@@ -2444,8 +2463,15 @@ static void APostLong(MRubyState state, RArray array, int pre, int post, ref MRu
                     {
                         Markers.Symbol();
                         bb = OperandBB.Read(ref sequence, ref callInfo.ProgramCounter);
-                        //var name = irep.PoolValues[bb.B].As<RString>();
-                        Unsafe.Add(ref registers, bb.A) = Intern(irep.PoolValues[bb.B].As<RString>());
+                        Symbol sym = default;
+                        var poolSymbols = irep.PoolSymbolCache;
+                        if (poolSymbols == null ||
+                            poolSymbols.OwnerStateId != StateId ||
+                            (sym = poolSymbols.Symbols[bb.B]).Value == 0)
+                        {
+                            sym = PoolSymbolMiss(this, irep, bb.B);
+                        }
+                        Unsafe.Add(ref registers, bb.A) = sym;
                         goto Next;
                     }
                     case OpCode.String:

src/ChibiRuby/Symbol.cs

@@ -13,23 +13,30 @@ public readonly record struct Symbol(uint Value)
 
 class SymbolTable
 {
-    readonly record struct Key(int HashCode)
+    /// <summary>
+    /// One interned symbol per node, chained per FNV-1a hash bucket. Names sharing a
+    /// hash coexist on the chain and are distinguished by comparing the actual bytes,
+    /// so a 32-bit hash collision can never alias two different symbol names.
+    /// </summary>
+    sealed class Entry(Symbol symbol, byte[] name, Entry? next)
     {
-        const uint OffsetBasis = 2166136261u;
-        const uint FnvPrime = 16777619u;
+        public readonly Symbol Symbol = symbol;
+        public readonly byte[] Name = name;
+        public readonly Entry? Next = next;
+    }
+
+    const uint OffsetBasis = 2166136261u;
+    const uint FnvPrime = 16777619u;
 
-        public static Key Create(ReadOnlySpan<byte> symbolName)
+    static int HashOf(ReadOnlySpan<byte> symbolName)
+    {
+        var hash = OffsetBasis;
+        foreach (var b in symbolName)
         {
-            var hash = OffsetBasis;
-            foreach (var b in symbolName)
-            {
-                hash ^= b;
-                hash *= FnvPrime;
-            }
-            return new Key(unchecked((int)hash));
+            hash ^= b;
+            hash *= FnvPrime;
         }
-
-        public override int GetHashCode() => HashCode;
+        return unchecked((int)hash);
     }
 
     const int PackLengthMax = 5;
@@ -44,21 +51,15 @@ public static Key Create(ReadOnlySpan<byte> symbolName)
     static byte[] ThreadStaticBuffer() => nameBuffer ??= new byte[32];
 
     readonly Dictionary<Symbol, byte[]> names = new(64);
-    readonly Dictionary<Key, Symbol> symbols = new(64);
+    readonly Dictionary<int, Entry> symbols = new(64);
 
     public Symbol Intern(ReadOnlySpan<byte> utf8)
     {
         if (TryFind(utf8, out var symbol))
         {
             return symbol;
         }
-
-        symbol = new Symbol(++lastId);
-        var nameBuf = new byte[utf8.Length];
-        utf8.CopyTo(nameBuf);
-        names.Add(symbol, nameBuf);
-        symbols.Add(Key.Create(utf8), symbol);
-        return symbol;
+        return Add(utf8.ToArray());
     }
 
     public Symbol InternLiteral(byte[] utf8)
@@ -67,9 +68,16 @@ public Symbol InternLiteral(byte[] utf8)
         {
             return symbol;
         }
-        symbol = new Symbol(++lastId);
-        names.Add(symbol, utf8);
-        symbols.Add(Key.Create(utf8), symbol);
+        return Add(utf8);
+    }
+
+    Symbol Add(byte[] name)
+    {
+        var symbol = new Symbol(++lastId);
+        names.Add(symbol, name);
+        var hash = HashOf(name);
+        symbols.TryGetValue(hash, out var head);
+        symbols[hash] = new Entry(symbol, name, head);
         return symbol;
     }
 
@@ -92,9 +100,20 @@ public bool TryFind(ReadOnlySpan<byte> utf8, out Symbol symbol)
         // {
         //     return true;
         // }
-        var key = Key.Create(utf8);
-        return symbols.TryGetValue(key, out symbol) ||
-               Names.TryFind(key.HashCode, utf8, out symbol);
+        var hash = HashOf(utf8);
+        if (symbols.TryGetValue(hash, out var entry))
+        {
+            do
+            {
+                if (entry.Name.AsSpan().SequenceEqual(utf8))
+                {
+                    symbol = entry.Symbol;
+                    return true;
+                }
+                entry = entry.Next;
+            } while (entry != null);
+        }
+        return Names.TryFind(hash, utf8, out symbol);
     }
 
     [MethodImpl(MethodImplOptions.AggressiveInlining)]

tests/ChibiRuby.Tests/SymbolTest.cs

@@ -14,4 +14,64 @@ public void InlinePack()
         Assert.That(name.SequenceEqual("call"u8), Is.True);
         // Assert.That(symbolTable.Intern("call"u8), Is.EqualTo(Names.Call));
     }
+
+    [Test]
+    public void InternFnv32CollidingNames()
+    {
+        // "costarring" and "liquid" are a known FNV-1a 32-bit collision pair;
+        // they must intern to distinct symbols with round-tripping names.
+        var symbolTable = new SymbolTable();
+
+        var costarring = symbolTable.Intern("costarring"u8);
+        var liquid = symbolTable.Intern("liquid"u8);
+
+        Assert.That(costarring, Is.Not.EqualTo(liquid));
+        Assert.That(symbolTable.NameOf(costarring).SequenceEqual("costarring"u8), Is.True);
+        Assert.That(symbolTable.NameOf(liquid).SequenceEqual("liquid"u8), Is.True);
+
+        // both remain findable after the collision chain forms
+        Assert.That(symbolTable.Intern("costarring"u8), Is.EqualTo(costarring));
+        Assert.That(symbolTable.Intern("liquid"u8), Is.EqualTo(liquid));
+
+        // another documented pair, interned in reverse order
+        var zinke = symbolTable.Intern("zinke"u8);
+        var altarage = symbolTable.Intern("altarage"u8);
+        Assert.That(altarage, Is.Not.EqualTo(zinke));
+        Assert.That(symbolTable.NameOf(altarage).SequenceEqual("altarage"u8), Is.True);
+    }
+
+    [Test]
+    public void PoolSymbolMemoAcrossStates()
+    {
+        // OP_SYMBOL (emitted by other mruby toolchains; the bundled compiler
+        // prefers OP_LOADSYM) interns a pool string and memoizes the result on
+        // the Irep. Symbols are per-state, so a second state executing the same
+        // Irep instance must not observe the first state's memo.
+        using var state1 = MRubyState.Create();
+        using var state2 = MRubyState.Create();
+
+        // hand-assembled: SYMBOL R1, Pool[0]; RETURN R1; STOP
+        var irep = new Irep
+        {
+            RegisterVariableCount = 2,
+            Sequence =
+            [
+                (byte)OpCode.Symbol, 1, 0,
+                (byte)OpCode.Return, 1,
+                (byte)OpCode.Stop,
+            ],
+            PoolValues = [new MRubyValue(state1.NewString("pool_symbol_memo_test"u8))],
+        };
+
+        var r1a = state1.Execute(irep);
+        var r1b = state1.Execute(irep); // memoized in state1
+        var r2a = state2.Execute(irep); // state2 must re-intern, not reuse state1's memo
+        var r2b = state2.Execute(irep); // memoized in state2
+
+        Assert.That(r1a.IsSymbol, Is.True);
+        Assert.That(r1b, Is.EqualTo(r1a));
+        Assert.That(r2b, Is.EqualTo(r2a));
+        Assert.That(state1.NameOf(r1a.SymbolValue).ToString(), Is.EqualTo("pool_symbol_memo_test"));
+        Assert.That(state2.NameOf(r2a.SymbolValue).ToString(), Is.EqualTo("pool_symbol_memo_test"));
+    }
 }