Issue 4127 - [emscripten] Convert subscription classes to C++ - Part 1

compiled/bindings.ipp

 /*
  * 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
@@ skipping 25 matching lines @@
 namespace bindings_internal
 {
   typedef void* TYPEID;
 
   enum class TypeCategory
   {
     UNKNOWN,
     VOID,
     INT,
     INT64,
     FLOAT,

[sergei, 2017/04/12 12:04:32]

I would rather call it DOUBLE because emscripten uses double (actually JS
number).

[Wladimir Palant, 2017/04/13 13:04:40]

I was under the impression that Emscripten doesn't really distinguish between
single and double precision floating point numbers. Turned out I was wrong - see
https://github.com/kripken/emscripten/blob/1.37.3/src/modules.js#L46. So we
should have both FLOAT and DOUBLE here.

+    DOUBLE,
     DEPENDENT_STRING,
     OWNED_STRING,
     STRING_REF,
     CLASS_PTR
   };
 
   template<typename T>
   struct TypeInfo
   {
     /*
@@ skipping 14 matching lines @@
     {
       if (std::is_void<T>())
         return TypeCategory::VOID;
 
       if (std::is_same<T, uint64_t>())
         return TypeCategory::INT64;
 
       if (std::is_integral<T>() || std::is_enum<T>())
         return TypeCategory::INT;
 
-      if (std::is_floating_point<T>())
+      if (std::is_same<T, float>())
         return TypeCategory::FLOAT;
+
+      if (std::is_same<T, double>())
+        return TypeCategory::DOUBLE;
 
       if (std::is_same<DependentString, T>() || std::is_same<const DependentString, T>())
         return TypeCategory::DEPENDENT_STRING;
 
       if (std::is_same<OwnedString, T>() || std::is_same<const OwnedString, T>())
         return TypeCategory::OWNED_STRING;
 
       if (std::is_same<String&, T>() || std::is_same<const String&, T>() ||
         std::is_same<DependentString&, T>())
       {
@@ skipping 41 matching lines @@
     {
       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)
+
+      // 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)
         {
-          signature += 'i';
+          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");
         }
-        else if (item == TypeCategory::INT64)
-          signature += "ii";

[sergei, 2017/04/12 12:04:32]

It would be good to have a link (maybe better to some code, commit/file:line),
from which one can understand that we are passing correct values to
Runtime.dynCall.

[Wladimir Palant, 2017/04/13 13:04:39]

Ok, I managed to find the correct code line, linked to it.

-        else if (item == TypeCategory::FLOAT)
-          signature += 'd';
-        else
-          throw std::runtime_error("Unexpected function argument type");
-        args.push_back(item);
+        args.push_back(type);
       }
 
       get_function_name(function, signature.c_str());
     }
 
     template<typename ReturnType, typename... Args>
     FunctionInfo(ReturnType (*function)(Args...))
         : FunctionInfo(TypeInfo<ReturnType>(),
           TypeInfo<ReturnType>().pointer_type(), { TypeInfo<Args>()... }, false,
           &function)
@@ skipping 212 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.

[sergei, 2017/04/12 12:04:32]

I would replace JavaScript by something more specific, like "bitwise and shift
operators".

[Wladimir Palant, 2017/04/13 13:04:40]

This comment is gone - I found that Emscripten exposes a function to combine two
i64 parts, doing exactly the same thing that I have here. So we don't need to
reinvent the wheel. Sadly, the reverse transformation is only used internally
and not exposed as a function.

-      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";
-
-      auto it = classes.find(call.pointerType);
-      if (it == classes.end())
-        throw std::runtime_error("Function " + std::string(call.name) + " returns pointer to unknown class");
-
-      const ClassInfo& cls = it->second;
-      auto offset = cls.subclass_differentiator.offset;
-      if (offset == SIZE_MAX)
-        result += "    result = " + cls.name + "(result);\n";
-      else
-      {
-        result += "    var type = HEAP32[result + " + std::to_string(offset)+ " >> 2];\n";
-        result += "    if (type in " + cls.name + "_mapping)\n";
-        result += "      result = new (exports[" + cls.name + "_mapping[type]])(result);\n";
-        result += "    else\n";
-        result += "      throw new Error('Unexpected " + cls.name + " type: ' + type);\n";
-      }
-
-      result += "  }\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";
+
+        auto it = classes.find(call.pointerType);
+        if (it == classes.end())
+          throw std::runtime_error("Function " + std::string(call.name) + " returns pointer to unknown class");
+
+        const ClassInfo& cls = it->second;
+        auto offset = cls.subclass_differentiator.offset;
+        if (offset == SIZE_MAX)
+          result += "    result = " + cls.name + "(result);\n";
+        else
+        {
+          result += "    var type = HEAP32[result + " + std::to_string(offset)+ " >> 2];\n";
+          result += "    if (type in " + cls.name + "_mapping)\n";
+          result += "      result = new (exports[" + cls.name + "_mapping[type]])(result);\n";
+          result += "    else\n";
+          result += "      throw new Error('Unexpected " + cls.name + " type: ' + type);\n";
+        }
+
+        result += "  }\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));
       if (i > 0)
         prefix += ", ";
       prefix += argName;
 
       if (call.args[i] == TypeCategory::STRING_REF)
       {
         hasStringArgs = true;
         params.push_back(std::string("createString(") + argName + ")");
       }
       else if (call.args[i] == TypeCategory::CLASS_PTR)
         params.push_back(argName + "._pointer");
       else if (call.args[i] == TypeCategory::INT64)
       {
         // 64-bit integers are passed as two integer parameters
         params.push_back(argName + " >>> 0");
         params.push_back(argName + " / 0x100000000 >>> 0");

[sergei, 2017/04/12 12:04:32]

It would be very useful to have few tests for that because it can work for some
values but break for bigger or smaller values which are not common and difficult
to catch.

[Wladimir Palant, 2017/04/13 13:04:40]

We have a few tests in FilterNotifier further up my patch queue but I've added
better values to subscriptionClasses tests as well now.

       }
       else
         params.push_back(argName);
     }
     prefix += ")\n{\n";
 
     std::string suffix = "}";
     if (call.returnType != TypeCategory::VOID)
       suffix = "  return result;\n" + suffix;
 
@@ skipping 259 matching lines @@
 
     std::vector<std::pair<int, std::string>> mapping;
     for (const auto& item : list)
       mapping.emplace_back(item.first, item.second);
 
     bindings_internal::register_differentiator(
         bindings_internal::TypeInfo<ClassType>(), offset, mapping);
     return *this;
   }
 };