Make the new flag be -prim-hash-check

This allows us to use the flag in CI and check that the hashes are correct.

Signed-off-by: Stefan Marr <[email protected]>

Author: smarr

.github/workflows/ci.yml

@@ -65,6 +65,7 @@ jobs:
         run: |
           cd cmake-build
           ./unittests -cp ../Smalltalk:../TestSuite/BasicInterpreterTests ../Examples/Hello.som
+          ./SOM++ -prim-hash-check -cp ../Smalltalk ../Examples/Benchmarks/BenchmarkHarness.som VectorBenchmark 1 1
 
       # - name: Run Integration Tests
       #   run: |

.gitlab-ci.yml

@@ -79,6 +79,7 @@ build_and_test:
     - make -j
     - ./SOM++ -cfg -cp ../Smalltalk ../TestSuite/TestHarness.som
     - ./unittests -cfg -cp ../Smalltalk:../TestSuite/BasicInterpreterTests ../Examples/Hello.som
+    - ./SOM++ -prim-hash-check -cp ../Smalltalk ../Examples/Benchmarks/BenchmarkHarness.som VectorBenchmark 1 1
     - cd ..
 
     - cd release

src/primitivesCore/PrimitiveContainer.cpp

@@ -33,7 +33,9 @@
 #include <string>
 #include <utility>
 
+#include "../vm/Print.h"
 #include "../vm/Symbols.h"
+#include "../vm/Universe.h"
 #include "../vmobjects/VMClass.h"
 #include "../vmobjects/VMInvokable.h"
 #include "../vmobjects/VMPrimitive.h"
@@ -133,10 +135,12 @@ void PrimitiveContainer::Add(const char* name, bool classSide,
 }
 
 template <class PrimT>
-void PrimitiveContainer::installPrimitives(
+bool PrimitiveContainer::installPrimitives(
     bool classSide, bool showWarning, VMClass* clazz,
     std::map<std::string, std::pair<PrimT, PrimT>>& prims,
     VMInvokable* (*makePrimFn)(VMSymbol* sig, PrimT)) {
+    bool hasHashMismatch = false;
+
     for (auto const& p : prims) {
         PrimT prim1 = std::get<0>(p.second);
         PrimT prim2 = std::get<1>(p.second);
@@ -150,27 +154,55 @@ void PrimitiveContainer::installPrimitives(
 
         PrimInstallResult const result = clazz->InstallPrimitive(
             makePrimFn(sig, prim1), prim1.bytecodeHash, !prim2.IsValid());
+        if (!prim2.IsValid()) {
+            hasHashMismatch =
+                hasHashMismatch || result == PrimInstallResult::HASH_MISMATCH;
+        }
+
         if (result == PrimInstallResult::INSTALLED_ADDED && showWarning) {
             cout << "Warn: Primitive " << p.first
                  << " is not in class definition for class "
                  << clazz->GetName()->GetStdString() << '\n';
         } else if (result == PrimInstallResult::HASH_MISMATCH &&
                    prim2.IsValid()) {
             assert(prim1.isClassSide == prim2.isClassSide);
-            clazz->InstallPrimitive(makePrimFn(sig, prim2), prim2.bytecodeHash,
-                                    true);
+            PrimInstallResult const result2 = clazz->InstallPrimitive(
+                makePrimFn(sig, prim2), prim2.bytecodeHash, true);
+            hasHashMismatch =
+                hasHashMismatch || result2 == PrimInstallResult::HASH_MISMATCH;
         }
     }
+
+    return hasHashMismatch;
 }
 
 void PrimitiveContainer::InstallPrimitives(VMClass* clazz, bool classSide,
                                            bool showWarning) {
-    installPrimitives(classSide, showWarning, clazz, unaryPrims,
-                      VMSafePrimitive::GetSafeUnary);
-    installPrimitives(classSide, showWarning, clazz, binaryPrims,
-                      VMSafePrimitive::GetSafeBinary);
-    installPrimitives(classSide, showWarning, clazz, ternaryPrims,
-                      VMSafePrimitive::GetSafeTernary);
-    installPrimitives(classSide, showWarning, clazz, framePrims,
-                      VMPrimitive::GetFramePrim);
+    bool hasHashMismatch = false;
+    hasHashMismatch =
+        hasHashMismatch ||
+        installPrimitives(classSide, showWarning, clazz, unaryPrims,
+                          VMSafePrimitive::GetSafeUnary);
+    hasHashMismatch =
+        hasHashMismatch ||
+        installPrimitives(classSide, showWarning, clazz, binaryPrims,
+                          VMSafePrimitive::GetSafeBinary);
+    hasHashMismatch =
+        hasHashMismatch ||
+        installPrimitives(classSide, showWarning, clazz, ternaryPrims,
+                          VMSafePrimitive::GetSafeTernary);
+    hasHashMismatch = hasHashMismatch ||
+                      installPrimitives(classSide, showWarning, clazz,
+                                        framePrims, VMPrimitive::GetFramePrim);
+
+    if (abortOnCoreLibHashMismatch && hasHashMismatch) {
+        ErrorPrint("The implementation of methods in " +
+                   clazz->GetName()->GetStdString() +
+                   " seem to have changed.\n");
+        ErrorPrint(
+            "The primitive implementation in the matching VM class may need to "
+            "be changed. See for instance _Vector::_Vector() in "
+            "primitives/Vector.cpp\n");
+        Quit(1);
+    }
 }

src/primitivesCore/PrimitiveContainer.h

@@ -81,7 +81,7 @@ class PrimitiveContainer {
     std::map<std::string, std::pair<TernaryPrim, TernaryPrim>> ternaryPrims;
 
     template <class PrimT>
-    void installPrimitives(
+    bool installPrimitives(
         bool classSide, bool showWarning, VMClass* clazz,
         std::map<std::string, std::pair<PrimT, PrimT>>& prims,
         VMInvokable* (*makePrimFn)(VMSymbol* sig, PrimT));

src/vm/Universe.cpp

@@ -74,7 +74,7 @@ static gc_oop_t prebuildInts[INT_CACHE_MAX_VALUE - INT_CACHE_MIN_VALUE + 1];
 
 uint8_t dumpBytecodes;
 uint8_t gcVerbosity;
-bool printCoreLibMethodHashes = false;
+bool abortOnCoreLibHashMismatch = false;
 
 static std::string bm_name;
 
@@ -212,8 +212,8 @@ vector<std::string> Universe::handleArguments(int32_t argc, char** argv) {
         } else if ((strncmp(argv[i], "-h", 2) == 0) ||
                    (strncmp(argv[i], "--help", 6) == 0)) {
             printUsageAndExit(argv[0]);
-        } else if ((strncmp(argv[i], "-prim-hashes", 12)) == 0) {
-            printCoreLibMethodHashes = true;
+        } else if ((strncmp(argv[i], "-prim-hash-check", 16)) == 0) {
+            abortOnCoreLibHashMismatch = true;
         } else {
             vector<std::string> extPathTokens = vector<std::string>(2);
             std::string const tmpString = std::string(argv[i]);
@@ -283,10 +283,10 @@ void Universe::printUsageAndExit(char* executable) {
     cout << "         set search path for application classes\n";
     cout << "    -d   enable disassembling (twice for tracing)\n";
     cout << "    -cfg print VM configuration\n";
-    cout << "    -prim-hashes  print hashes of core-lib methods\n";
-    cout << "                  that have primitive replacements and exit with "
-            "error\n";
-    cout << "                  when they do not match expected results\n";
+    cout << "    -prim-hash-check check that method replacements have expected "
+            "hash.\n";
+    cout
+        << "                     Exit with error when a hash does not match.\n";
     cout
         << "    -g   enable garbage collection details:\n"
         << "         1x - print statistics when VM shuts down\n"

src/vm/Universe.h

@@ -41,7 +41,7 @@ class SourcecodeCompiler;
 // for runtime debug
 extern uint8_t dumpBytecodes;
 extern uint8_t gcVerbosity;
-extern bool printCoreLibMethodHashes;
+extern bool abortOnCoreLibHashMismatch;
 
 using namespace std;
 class Universe {

src/vmobjects/VMClass.cpp

@@ -38,7 +38,6 @@
 #include "../vm/Globals.h"
 #include "../vm/IsValidObject.h"
 #include "../vm/Print.h"
-#include "../vm/Universe.h"
 #include "ObjectFormats.h"
 #include "VMArray.h"
 #include "VMInvokable.h"
@@ -109,11 +108,6 @@ PrimInstallResult VMClass::InstallPrimitive(VMInvokable* invokable,
                     hashMismatch = true;
                 } else {
                     seenHash = method->GetBytecodeHash();
-                    if (printCoreLibMethodHashes) {
-                        cout << "Method: "
-                             << method->GetSignature()->AsDebugString()
-                             << " has hash: " << seenHash << '\n';
-                    }
                     if (seenHash == bytecodeHash) {
                         result = PrimInstallResult::INSTALLED_REPLACED;
                     } else {