fix for passing structs by value, and warning for unfixable case of passing structs by value between C and C++ calling conventions

4 files changed, 111 insertions, 6 deletions

diff --git a/src/jsifier.js b/src/jsifier.js
index 92668595..67221c97 100644
--- a/src/jsifier.js
+++ b/src/jsifier.js
@@ -449,6 +449,16 @@ function JSify(data, functionsOnly, givenFunctions, givenGlobalVariables) {
 
       func.JS += '  ' + RuntimeGenerator.stackEnter(func.initialStack) + ';\n';
 
+      // Make copies of by-value params
+      func.params.forEach(function(param) {
+        if (param.byVal) {
+          var type = removePointing(param.type);
+          var typeInfo = Types.types[type];
+          func.JS += '  var tempParam = ' + param.ident + '; ' + param.ident + ' = ' + RuntimeGenerator.stackAlloc(typeInfo.flatSize) + ';' +
+                     makeCopyValues(param.ident, 'tempParam', typeInfo.flatSize, 'null') + ';\n';
+        }
+      });
+
       if (LABEL_DEBUG) func.JS += "  print(INDENT + ' Entering: " + func.ident + "'); INDENT += '  ';\n";
 
       if (true) { // TODO: optimize away when not needed
diff --git a/src/parseTools.js b/src/parseTools.js
index 98987229..acb5c9cd 100644
--- a/src/parseTools.js
+++ b/src/parseTools.js
@@ -165,6 +165,12 @@ function isType(type) { // TODO!
   return isVoidType(type) || Runtime.isNumberType(type) || isStructType(type) || isPointerType(type) || isFunctionType(type);
 }
 
+function isPossiblyFunctionType(type) {
+  // A quick but unreliable way to see if something is a function type. Yes is just 'maybe', no is definite.
+  var suffix = ')*';
+  return type.substr(-suffix.length) == suffix;
+}
+
 function isVarArgsFunctionType(type) {
   // assumes this is known to be a function type already
   var varArgsSuffix = '...)*';
@@ -173,7 +179,7 @@ function isVarArgsFunctionType(type) {
 
 function countNormalArgs(type) {
   var out = {};
-  assert(isFunctionType(type, out));
+  if (!isFunctionType(type, out)) return -1;
   if (isVarArgsFunctionType(type)) out.numArgs--;
   return out.numArgs;
 }
@@ -307,8 +313,10 @@ function parseParamTokens(params) {
     var segment = params.slice(0, i);
     params = params.slice(i+1);
     segment = cleanSegment(segment);
+    var byVal = false;
     if (segment[1] && segment[1].text === 'byval') {
       // handle 'byval' and 'byval align X'
+      byVal = true;
       segment.splice(1, 1);
       if (segment[1] && segment[1].text === 'align') {
         segment.splice(1, 2);
@@ -353,6 +361,7 @@ function parseParamTokens(params) {
       //          } else {
       //            throw "what is this params token? " + JSON.stringify(segment);
     }
+    ret[ret.length-1].byVal = byVal;
   }
   return ret;
 }
@@ -1290,6 +1299,32 @@ function finalizeLLVMFunctionCall(item, noIndexizeFunctions) {
     case 'getelementptr': // TODO finalizeLLVMParameter on the ident and the indexes?
       return makePointer(makeGetSlabs(item.ident, item.type)[0], getGetElementPtrIndexes(item), null, item.type);
     case 'bitcast':
+      // Warn about some types of casts, then fall through to the handling code below
+      var oldType = item.params[0].type;
+      var newType = item.type;
+      if (isPossiblyFunctionType(oldType) && isPossiblyFunctionType(newType) &&
+          countNormalArgs(oldType) != countNormalArgs(newType)) {
+        warn('Casting a function pointer type to another with a different number of arguments. See more info in the source (grep for this text). ' +
+             oldType + ' ==> ' + newType);
+        // This may be dangerous as clang generates different code for C and C++ calling conventions. The only problem
+        // case appears to be passing a structure by value, C will have (field1, field2) as function args, and the
+        // function will internally create a structure with that data, while C++ will have (struct* byVal) and it
+        // will create a copy before calling the function, then call it with a pointer to the copy. Mixing the two
+        // first of all leads to two copies being made, so this is a bad idea even regardless of Emscripten. But,
+        // what is a problem for Emscripten is that mixing these two calling conventions (say, calling a C one from
+        // C++) will then assume that (struct* byVal) is actually the same as (field1, field2). In native code, this
+        // is easily possible, you place the two fields on the stack and call the function (you know to place the
+        // values since there is 'byVal'). In Emscripten, though, this means we would need to always do one or the
+        // other of the two possibilities, for example, always passing by-value structs as (field1, field2). This
+        // would slow down everything, just to handle this corner case. (Which, just to point out how much of a
+        // corner case it is, does not appear to happen with nested structures!)
+        //
+        // The recommended solution for this problem is not to mix C and C++ calling conventions when passing structs
+        // by value. Either always pass structs by value within C code or C++ code, but not mixing the two by
+        // defining a function in one and calling it from the other (so, just changing .c to .cpp, or moving code
+        // from one file to another, would be enough to fix this), or, do not pass structs by value (which in general
+        // is inefficient, and worth avoiding if you can).
+      }
     case 'inttoptr':
     case 'ptrtoint':
       return finalizeLLVMParameter(item.params[0], noIndexizeFunctions);
diff --git a/src/utility.js b/src/utility.js
index f0439ea7..737d515a 100644
--- a/src/utility.js
+++ b/src/utility.js
@@ -64,6 +64,10 @@ function assertTrue(a, msg) {
 assert = assertTrue;
 
 function warn(a, msg) {
+  if (!msg) {
+    msg = a;
+    a = false;
+  }
   if (!a) {
     dprint('Warning: ' + msg);
   }
diff --git a/tests/runner.py b/tests/runner.py
index 52d1d0b0..2ff97c2c 100644
--- a/tests/runner.py
+++ b/tests/runner.py
@@ -1772,7 +1772,50 @@ if 'benchmark' not in str(sys.argv):
           '''
         self.do_run(src, '*cheez: 0+24*\n*cheez: 0+24*\n*albeit*\n*albeit*\nQ85*\nmaxxi:21*\nmaxxD:22.10*\n*vfp:22,199*\n*vfp:22,199*\n')
 
-    def test_mix_c_cpp(self):
+    def test_structbyval(self):
+        # part 1: make sure that normally, passing structs by value works
+
+        src = r'''
+          #include <stdio.h>
+
+          struct point
+          {
+            int x, y;
+          };
+
+          void dump(struct point p) {
+            p.x++; // should not modify
+            p.y++; // anything in the caller!
+            printf("dump: %d,%d\n", p.x, p.y);
+          }
+
+          void dumpmod(struct point *p) {
+            p->x++; // should not modify
+            p->y++; // anything in the caller!
+            printf("dump: %d,%d\n", p->x, p->y);
+          }
+
+          int main( int argc, const char *argv[] ) {
+            point p = { 54, 2 };
+            printf("pre:  %d,%d\n", p.x, p.y);
+            dump(p);
+            void (*dp)(point p) = dump; // And, as a function pointer
+            dp(p);
+            printf("post: %d,%d\n", p.x, p.y);
+            dumpmod(&p);
+            dumpmod(&p);
+            printf("last: %d,%d\n", p.x, p.y);
+            return 0;
+          }
+        '''
+        self.do_run(src, 'pre:  54,2\ndump: 55,3\ndump: 55,3\npost: 54,2\ndump: 55,3\ndump: 56,4\nlast: 56,4')
+
+        # Check for lack of warning in the generated code (they should appear in part 2)
+        generated = open(os.path.join(self.get_dir(), 'src.cpp.o.js')).read()
+        assert 'Casting a function pointer type to another with a different number of arguments.' not in generated, 'Unexpected warning'
+
+        # part 2: make sure we warn about mixing c and c++ calling conventions here
+
         header = r'''
           struct point
           {
@@ -1799,15 +1842,19 @@ if 'benchmark' not in str(sys.argv):
           #include <stdio.h>
           #include "header.h"
 
+          #ifdef __cplusplus
           extern "C" {
-            void dump(point p);
+          #endif
+            void dump(struct point p);
+          #ifdef __cplusplus
           }
+          #endif
 
           int main( int argc, const char *argv[] ) {
-            point p = { 54, 2 };
+            struct point p = { 54, 2 };
             printf("pre:  %d,%d\n", p.x, p.y);
             dump(p);
-            void (*dp)(point p) = dump; // And, as a function pointer
+            void (*dp)(struct point p) = dump; // And, as a function pointer
             dp(p);
             printf("post: %d,%d\n", p.x, p.y);
             return 0;
@@ -1821,7 +1868,16 @@ if 'benchmark' not in str(sys.argv):
         all_name = os.path.join(self.get_dir(), 'all.bc')
         Building.link([supp_name + '.o', main_name + '.o'], all_name)
 
-        self.do_ll_run(all_name, 'pre:  54,2\ndump: 55,3\ndump: 55,3\npost: 54,2')
+        try:
+          # This will fail! See explanation near the warning we check for, in the compiler source code
+          self.do_ll_run(all_name, 'pre:  54,2\ndump: 55,3\ndump: 55,3\npost: 54,2')
+        except Exception, e:
+          # Check for warning in the generated code
+          generated = open(os.path.join(self.get_dir(), 'src.cpp.o.js')).read()
+          assert 'Casting a function pointer type to another with a different number of arguments.' in generated, 'Missing expected warning'
+          assert 'void (i32, i32)* ==> void (%struct.point.0*)*' in generated, 'Missing expected warning details'
+          return
+        raise Exception('We should not have gotten to here!')
 
     def test_stdlibs(self):
         if Settings.USE_TYPED_ARRAYS == 2: