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

compiled/bindings.ipp

Index: compiled/bindings.ipp
===================================================================
--- a/compiled/bindings.ipp
+++ b/compiled/bindings.ipp
@@ -37,16 +37,18 @@ 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.

     DEPENDENT_STRING,
     OWNED_STRING,
     STRING_REF,
     CLASS_PTR
   };
 
   template<typename T>
   struct TypeInfo
@@ -65,19 +67,25 @@ namespace bindings_internal
       return &s_typeIDHelper;
     }
 
     constexpr operator TypeCategory() const
     {
       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>())
+        return TypeCategory::FLOAT;
+
       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>())
@@ -127,49 +135,62 @@ namespace bindings_internal
       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)
+      {
+        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)
+        if (item == TypeCategory::INT || item == TypeCategory::STRING_REF ||
+            item == TypeCategory::CLASS_PTR)
         {
+          signature += 'i';
+        }
+        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);
       }
 
-      if (returnType != TypeCategory::VOID && returnType != TypeCategory::INT &&
-          returnType != TypeCategory::DEPENDENT_STRING &&
-          returnType != TypeCategory::OWNED_STRING &&
-          returnType != TypeCategory::STRING_REF &&
-          returnType != TypeCategory::CLASS_PTR)
-      {
-        throw std::runtime_error("Unexpected function return type");
-      }
-
-      effectiveArgs = args.size();
-      effectiveReturnType = returnType;
-      if (instance_function)
-        effectiveArgs++;
-
-      if (returnType == TypeCategory::DEPENDENT_STRING ||
-          returnType == TypeCategory::OWNED_STRING)
-      {
-        effectiveArgs++;
-        effectiveReturnType = TypeCategory::VOID;
-      }
-
-      get_function_name(function, effectiveArgs,
-          effectiveReturnType == TypeCategory::VOID);
+      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)
     {
@@ -191,35 +212,32 @@ namespace bindings_internal
     {
     }
 
     bool empty() const
     {
       return name[0] == '\0';
     }
 
-    void get_function_name(void* ptr, int numArgs, bool voidResult)
+    void get_function_name(void* ptr, const char* signature)
     {
       // This is a hack, C++ won't let us get the mangled function name.
       // JavaScript is more dynamic so we pass the pointer to our function
       // there. With that and the function signature we can call the function -
       // with a full stack so that we will cause it to abort. Sometimes the
       // function we are calling will also be missing from the build. The result
       // is the same: abort() is called which in turn calls stackTrace(). By
       // replacing stackTrace() we get access to the call stack and search it
       // for the name of our function.
 
       EM_ASM_ARGS({
-        var signature = $3 ? "v" : "i";
+        var signature = AsciiToString($2);
         var args = [];
-        for (var i = 0; i < $2; i++)
-        {
-          signature += "i";
+        for (var i = 1; i < signature.length; i++)
           args.push(0);
-        }
 
         var oldPrint = Module.print;
         var oldPrintErr = Module.printErr;
         var oldStackTrace = stackTrace;
         var sp = Runtime.stackSave();
         Module.print = function(){};
         Module.printErr = function(){};
         stackTrace = function()
@@ -254,17 +272,17 @@ namespace bindings_internal
         }
         finally
         {
           Runtime.stackRestore(sp);
           Module.print = oldPrint;
           Module.printErr = oldPrintErr;
           stackTrace = oldStackTrace;
         }
-      }, name, ptr, numArgs, voidResult);
+      }, name, ptr, signature);
     }
   };
 
   class NoBaseClass
   {
   };
 
   struct PropertyInfo
@@ -394,18 +412,29 @@ namespace bindings_internal
       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)
+    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)
@@ -459,16 +488,24 @@ namespace bindings_internal
         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;