Issue 5131 - [emscripten] Clean separation of bindings code and runtime code

compiled/bindings/generator.cpp

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2017 eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 #include "generator.h"
 
 #include <cstdio>
 
 namespace
 {
   std::vector<bindings_internal::ClassInfo> classes;

[Wladimir Palant, 2017/04/12 14:17:00]

I found two bad assumptions here which went wrong after this refactoring.

1) This was originally a map and I assumed that iterating over it I would get
the members in the order in which they were added. They are actually sorted by
type ID which isn't necessarily incrementing in the order we want. So this is a
vector now, searching it isn't an impact on performance we care about for
bindings generation.

2) ClassInfo::baseClass member was a pointer referring to an instance within
this map. This was a bad idea as pointers to map/vector buffer aren't guaranteed
to stay stable as items are added. So ClassInfo::baseClass is a TYPEID now and
looked up when needed.

[sergei, 2017/04/18 10:46:09]

That's correct, pointers and iterators are invalidated by almost all operations
modifying std::map and vector. One can use iterators of std::list but here I
think having TYPEID value is even better.

-  std::vector<bindings_internal::CustomGenerator> customGenerators;
 }
 
 namespace bindings_internal
 {
   FunctionInfo::FunctionInfo()
   {
     name[0] = '\0';
   }
 
   FunctionInfo::FunctionInfo(TypeCategory returnType, TYPEID pointerType,
       std::initializer_list<TypeCategory> argTypes, bool instance_function,
       void* function)
       : returnType(returnType), pointerType(pointerType),
         instance_function(instance_function)
   {
     name[0] = '\0';
 
     // The function parameter is a pointer to the function pointer.
     // Emscripten's "function pointers" are actually integers indicating the
     // position in the call table. 0 represents nullptr.
     if (!*reinterpret_cast<int*>(function))
       return;
 
     std::string signature;
-    if (returnType == TypeCategory::DEPENDENT_STRING ||
-        returnType == TypeCategory::OWNED_STRING)
-    {
-      // Objects aren't really returned but passed as parameter. Note that
-      // this pointer might come before it but we don't care because both
-      // are integers (pointers) as far as Emscripten is concerned.
-      signature += "vi";
-    }
-    else if (returnType == TypeCategory::VOID)
-      signature += 'v';
-    else if (returnType == TypeCategory::FLOAT)
-      signature += 'd';
-    else if (returnType == TypeCategory::INT ||
-        returnType == TypeCategory::INT64 ||
-        returnType == TypeCategory::STRING_REF ||
-        returnType == TypeCategory::CLASS_PTR)
-    {
+
+    // Add return type to the signature. Similar logic in Emscripten:
+    // https://github.com/kripken/emscripten/blob/1.37.3/src/modules.js#L46
+    switch (returnType)
+    {
+      case TypeCategory::DEPENDENT_STRING:
+      case TypeCategory::OWNED_STRING:
+        // Technically, objects aren't really returned with clang. The caller
+        // instead adds the reference to the resulting object as an implicit
+        // parameter.
+        signature += "vi";
+        break;
+      case TypeCategory::VOID:
+        signature += 'v';
+        break;
+      case TypeCategory::FLOAT:
+        signature += 'f';
+        break;
+      case TypeCategory::DOUBLE:
+        signature += 'd';
+        break;
+      case TypeCategory::INT:
+      case TypeCategory::INT64:
+      case TypeCategory::STRING_REF:
+      case TypeCategory::CLASS_PTR:
+        signature += 'i';
+        break;
+      default:
+        throw std::runtime_error("Unexpected function return type");
+    }
+
+    // `this` pointer is an implicit parameter with clang and should be added
+    // to the signature.
+    if (instance_function)
       signature += 'i';
-    }
-    else
-      throw std::runtime_error("Unexpected function return type");
-
-    if (instance_function)
-    {
-      // this pointer is an implicit parameter
-      signature += 'i';
-    }
-
-    for (const auto& item : argTypes)
-    {
-      if (item == TypeCategory::INT || item == TypeCategory::STRING_REF ||
-          item == TypeCategory::CLASS_PTR)
-      {
-        signature += 'i';
-      }
-      else if (item == TypeCategory::INT64)
-        signature += "ii";
-      else if (item == TypeCategory::FLOAT)
-        signature += 'd';
-      else
-        throw std::runtime_error("Unexpected function argument type");
-      args.push_back(item);
+
+    // Add explicit parameters to the signature, Similar logic in Emscripten:
+    // https://github.com/kripken/emscripten/blob/1.37.3/src/modules.js#L67
+    for (const auto& type : argTypes)
+    {
+      switch (type)
+      {
+        case TypeCategory::INT:
+        case TypeCategory::STRING_REF:
+        case TypeCategory::CLASS_PTR:
+          signature += 'i';
+          break;
+        case TypeCategory::INT64:
+          // See https://github.com/kripken/emscripten/blob/1.37.3/src/modules.js#L73,
+          // numerical types larger than 32-bit are split into multiple
+          // 32-bit parameters.
+          signature += "ii";
+          break;
+        case TypeCategory::FLOAT:
+          signature += 'f';
+          break;
+        case TypeCategory::DOUBLE:
+          signature += 'd';
+          break;
+        default:
+          throw std::runtime_error("Unexpected function argument type");
+      }
+      args.push_back(type);
     }
 
     get_function_name(function, signature.c_str());
   }
 
   bool FunctionInfo::empty() const
   {
     return name[0] == '\0';
   }
 
@@ skipping 143 matching lines @@
     std::string call_str(call.name);
     call_str += "(";
     for (int i = 0; i < params.size(); i++)
     {
       if (i > 0)
         call_str += ", ";
       call_str += params[i];
     }
     call_str += ")";
 
-    if (call.returnType == TypeCategory::VOID)
-      return "  " + call_str + ";\n";
-    else if (call.returnType == TypeCategory::INT ||
-          call.returnType == TypeCategory::FLOAT)
-    {
-      return "  var result = " + call_str + ";\n";
-    }
-    else if (call.returnType == TypeCategory::INT64)
-    {
-      // Emscripten saves the high bits of a 64-bit return value in a special
-      // variable called tempRet0. We cannot use bit operators to combine the
-      // values because JavaScript operates on 32-bit integers.
-      return "  var result = (" + call_str + " >>> 0)" +
-             "             + (Runtime.getTempRet0() >>> 0) * 0x100000000;\n";
-    }
-    else if (call.returnType == TypeCategory::DEPENDENT_STRING ||
-        call.returnType == TypeCategory::OWNED_STRING)
-    {
-      std::string result;
-      result += "  var string = createString();\n";
-      result += "  " + call_str + ";\n";
-      result += "  var result = readString(string);\n";
-      if (call.returnType == TypeCategory::OWNED_STRING)
-        result += "  Module._DestroyString(string);\n";
-      return result;
-    }
-    else if (call.returnType == TypeCategory::STRING_REF)
-    {
-      return "  var result = readString(" + call_str + ");\n";
-    }
-    else if (call.returnType == TypeCategory::CLASS_PTR)
-    {
-      std::string result;
-      result += "  var result = " + call_str + ";\n";
-      result += "  if (result)\n";
-      result += "  {\n";
-
-      const ClassInfo* cls = find_class(call.pointerType);
-      if (!cls)
-        throw std::runtime_error("Function " + std::string(call.name) + " returns pointer to unknown class");
-
-      auto offset = cls->subclass_differentiator.offset;
-      if (offset == SIZE_MAX)
-        result += "    result = exports." + cls->name + "(result);\n";
-      else
-        result += "    result = exports." + cls->name + ".fromPointer(result);\n";
-
-      result += "  }\n";
-      result += "  else\n";
-      result += "    result = null;\n";
-      return result;
-    }
-    else
-      throw std::runtime_error("Unexpected return type for " + std::string(call.name));
+    switch (call.returnType)
+    {
+      case TypeCategory::VOID:
+        return "  " + call_str + ";\n";
+      case TypeCategory::INT:
+      case TypeCategory::FLOAT:
+      case TypeCategory::DOUBLE:
+        return "  var result = " + call_str + ";\n";
+      case TypeCategory::INT64:
+        return "  var result = Runtime.makeBigInt(" + call_str + ", " +
+                                                  "Runtime.getTempRet0(), " +
+                                                  "true);\n";
+      case TypeCategory::DEPENDENT_STRING:
+      case TypeCategory::OWNED_STRING:
+      {
+        std::string result;
+        result += "  var string = createString();\n";
+        result += "  " + call_str + ";\n";
+        result += "  var result = readString(string);\n";
+        if (call.returnType == TypeCategory::OWNED_STRING)
+          result += "  Module._DestroyString(string);\n";
+        return result;
+      }
+      case TypeCategory::STRING_REF:
+        return "  var result = readString(" + call_str + ");\n";
+      case TypeCategory::CLASS_PTR:
+      {
+        std::string result;
+        result += "  var result = " + call_str + ";\n";
+        result += "  if (result)\n";
+        result += "  {\n";
+
+        const ClassInfo* cls = find_class(call.pointerType);
+        if (!cls)
+          throw std::runtime_error("Function " + std::string(call.name) + " returns pointer to unknown class");
+
+        auto offset = cls->subclass_differentiator.offset;
+        if (offset == SIZE_MAX)
+          result += "    result = exports." + cls->name + "(result);\n";
+        else
+          result += "    result = exports." + cls->name + ".fromPointer(result);\n";
+
+        result += "  }\n";
+        result += "  else\n";
+        result += "    result = null;\n";
+        return result;
+      }
+      default:
+        throw std::runtime_error("Unexpected return type for " + std::string(call.name));
+    }
   }
 
   const std::string wrapCall(const FunctionInfo& call)
   {
     bool hasStringArgs = false;
     std::vector<std::string> params;
     std::string prefix = "function(";
     for (int i = 0; i < call.args.size(); i++)
     {
       std::string argName("arg" + std::to_string(i));
@@ skipping 148 matching lines @@
     }
   }
 }
 
 void printBindings()
 {
   bindings_internal::printHelpers();
 
   for (const auto& item : classes)
     bindings_internal::printClass(item);
-
-  for (const auto& item : customGenerators)
-    item();
 }
-
-void custom_generator(bindings_internal::CustomGenerator generator)
-{
-  customGenerators.push_back(generator);
-}