Do JS_REQUIRES_STACK analysis on function pointers

RESOLVED FIXED

Status

()

RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: benjamin, Assigned: benjamin)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

10 years ago
The JS_REQUIRES_STACK analysis pass currently ignores function pointers. This worries me, especially because JS uses function pointers extensively in jsclass/jsobjectops/etc...

Adding in the pointer analysis wasn't hard, but it has found some issues and I'm not sure how they ought to be fixed.
(Assignee)

Comment 1

10 years ago
Created attachment 376358 [details] [diff] [review]
Do JS_REQUIRES_STACK analysis on function pointers, rev. 1
Assignee: general → benjamin
Attachment #376358 - Flags: review?(jorendorff)
Attachment #376358 - Flags: review?(dmandelin)
(Assignee)

Comment 2

10 years ago
This found ../../../src/js/src/jsdbgapi.cpp: In function ‘JSBool (* js_WrapWatchedSetter(JSContext*, jsid, uintN, JSBool (*)(JSContext*, JSObject*, jsval, jsval*)))(JSContext*, JSObject*, jsval, jsval*)’:
../../../src/js/src/jsdbgapi.cpp:701: error: Assigning non-JS_REQUIRES_STACK function pointer from JS_REQUIRES_STACK function

Because js_watch_set is a JS_REQUIRES_STACK function. I don't know the debugger APIs well enough to say what the correct solution here is (whether this is a real bug or a lack of annotation somewhere along the way).
Attachment #376358 - Flags: review?(dmandelin) → review+
Comment on attachment 376358 [details] [diff] [review]
Do JS_REQUIRES_STACK analysis on function pointers, rev. 1

Looks OK. I have a few suggestions.

>+            else {
>+              let fntype = dehydra_convert(TREE_TYPE(TREE_TYPE(CALL_EXPR_FN(t))).tree_check(FUNCTION_TYPE));
>+              if (hasAttribute(fntype, RED)) {
>+                isn.redInfo = ["cannot call JS_REQUIRES_STACK function pointer",
>+                               getLocation(false)];
>+                self.hasRed = true;
>+              }
>+              else if (hasAttribute(fntype, TURN_RED)) {
>+                isn.turnRed = true;
>+              }
>+            }

I think the above could be implemented a bit more efficiently with 

  let fntype = TREE_TYPE(TREE_TYPE(CALL_EXPR_FN(t))).tree_check(FUNCTION_TYPE));

and then the isRed function to check for the attribute. 

I also suggest a comment indicating that this case covers function pointers, because at first I was confused as to why there wouldn't be a caller in a function call, and what all those TREE_TYPE unwrappings meant.

>+function FunctionPointerCheck(fndecl)

I suggest a comment explaining that this is a type-checker that verifies that red function pointers are not assigned to green function pointer variables.

>+{
>+  let cfg = function_decl_cfg(fndecl);
>+  for (let bb in cfg_bb_iterator(cfg)) {
>+    for (let isn in bb_isn_iterator(bb)) {
>+      walk_tree(isn, function(t, stack) {
>+        function getLocation(skiptop) {
>+          if (!skiptop) {
>+            let loc = location_of(t);
>+            if (loc !== undefined)
>+              return loc;
>+          }
>+          
>+          for (let i = stack.length - 1; i >= 0; --i) {
>+            let loc = location_of(stack[i]);
>+            if (loc !== undefined)
>+              return loc;
>+          }
>+          return location_of(DECL_SAVED_TREE(fndecl));
>+        }

This location-munging code seems to turn up frequently. We should think about putting it in the library.
                  
>+        switch (TREE_CODE(t)) {
>+        case GIMPLE_MODIFY_STMT:
>+          let [dest, source] = t.operands();
>+
>+          let destType = TREE_TYPE(dest);
>+          if (TREE_CODE(destType) == POINTER_TYPE &&
>+              TREE_CODE(TREE_TYPE(destType)) == FUNCTION_TYPE &&
>+              !hasUserAttribute(TREE_TYPE(destType), RED)) {

As above, does isRed work here?

>+            // we're assigning to a green function pointer
>+            if (TREE_CODE(source) == ADDR_EXPR) {
>+              let sourcefn = source.operands()[0].tree_check(FUNCTION_DECL);
>+              if (hasUserAttribute(sourcefn, RED))
>+                error("Assigning non-JS_REQUIRES_STACK function pointer from JS_REQUIRES_STACK function", getLocation());
>+            }
>+            else {
>+              let sourceType = TREE_TYPE(TREE_TYPE(source).tree_check(POINTER_TYPE)).tree_check(FUNCTION_TYPE);
>+              if (hasUserAttribute(sourceType, RED))
>+                error("Assigning non-JS_REQUIRES_STACK function pointer from JS_REQUIRES_STACK function pointer", getLocation());
>+            }

In the above, it would be nice if the second case could cover everything, because logically the type should carry around all that information. Does just the second case work, or do attributes get dropped in '&f' expressions?
(Assignee)

Comment 4

10 years ago
The problem is that when we're annotating actual functions, the attribute is attached to the decl, not the type. We've talked about adding separate "usertype" and "userdecl" attribute types (and GCC 4.5 will support arbitrary attribute names supplied by plugins) but in the meantime there are two codepaths checking for attributes on typedefed functions and function declarations.
(Assignee)

Comment 5

10 years ago
Created attachment 376773 [details] [diff] [review]
Do JS_REQUIRES_STACK analysis on function pointers, rev. 2
Attachment #376358 - Attachment is obsolete: true
Attachment #376773 - Flags: review?(jorendorff)
Attachment #376358 - Flags: review?(jorendorff)
(Assignee)

Comment 6

10 years ago
Created attachment 376774 [details] [diff] [review]
Fix the code to correct function-pointer behavior, rev. 1

The cast functions are a little unfortunate: I tried to write a generic templated cast function, but GCC strips typedef/attribute information from templates, so it wasn't easy.
Attachment #376774 - Flags: review?(jorendorff)
Comment on attachment 376773 [details] [diff] [review]
Do JS_REQUIRES_STACK analysis on function pointers, rev. 2

OK! This looks really good, almost elegant.

- I would appreciate a comment in each test program like "// ERROR, red functions must not pass as green" so it's obvious just looking at the .cpp file that it's supposed to fail.

- It seems like it would allow you to pass a (GreenFuncPtr *) to a function that expects a (RedFuncPtr *).  I don't care about this case, but it might be nice to put in a comment about it.

- Would the analysis be confused by the `const` in this?

  const RedFuncPtr r = &f;
  GreenFunc g = r; // ERROR

- Does the analysis detect initializers, like

  struct X {
      GreenFuncPtr g;
  };

  X gg = { &redfun };

- The magic numbers 1 and 3 in functionPointerCheck could get names, I guess.  Why do we loop backwards over the args?

- I guess by the time we get here default arguments have been inserted at the call site?  (If not, the loop could read off the end of ops.)
Attachment #376773 - Flags: review?(jorendorff) → review+
Btw "almost elegant" wasn't intended as faint praise for the patch.  It's more of a commentary on the nature of the problem!
Comment on attachment 376774 [details] [diff] [review]
Fix the code to correct function-pointer behavior, rev. 1

Ugh.  array_sort is JS_REQUIRES_STACK, so it is not really a JSFastNative.  This is a bug.  We should detect this when we take its address here,

    JS_FN("sort",               array_sort,         1,JSFUN_GENERIC_NATIVE),

but JS_FN does a C-style cast, essentially a reinterpret_cast, so the problem is masked.  I think there is a way around this: we can change the definition of JSFunctionSpec like so:

  struct JSFunctionSpec {
+ #if defined(__cplusplus) && defined(NS_STATIC_CHECKING)
+     JSFunctionSpec(){}
+     JSFunctionSpec(const char *_name, JSNative _call, etc.)
+         : name(_name), call(_call), etc. {}
+     JSFunctionSpec(const char *_name, JSFastNative _call, etc.)
+         : name(_name), call((JSNative) _call), etc. {}
+     JSFunctionSpec(const char *_name, ... JSTraceInfo *trcinfo)
+         : blahblah {}
+ #endif
+ 
      const char      *name;
      JSNative        call;
      uint16          nargs;
      ...
  };

...

+ #if defined(__cplusplus) && defined(NS_STATIC_CHECKING)
+   #define JS_FS JSFunctionSpec
+   #define JS_FN JSFunctionSpec
+ #else
...the existing definitions...
+ #endif

And in jsfun.h,

-    JS_FN(name, JS_DATA_TO_FUNC_PTR(JSNative, trcinfo), nargs,            \
+    JS_FN(name, JS_DATA_TO_FUNC_PTR(JSFastNative, trcinfo), nargs,        \

I write "&& defined(NS_STATIC_CHECKING)" because I think when you do this, g++ generates actual static initializer *code* rather than static data.  We don't want that for release builds, certainly.
Attachment #376774 - Flags: review?(jorendorff)
> I write "&& defined(NS_STATIC_CHECKING)" because I think when you do this, g++
> generates actual static initializer *code* rather than static data.

This is true for GCC 4.3.2 at least.

However, the change I proposed is unnecessarily drastic.  A better idea is to just change the cast in JS_FN a bit, to make it more type-safe:

+ #ifdef NS_STATIC_CHECKING
+   JS_INLINE JSNative
+   castFastNativeToNative(JSFastNative f) {
+       return (JSNative) f;
+   }
+ #else
+   #define castFastNativeToNative(f) ((JSNative)(f))
+ #endif

...

  #define JS_FN(name,fastcall,nargs,flags)                                  \
-     JS_FS(name, (JSNative)(fastcall), nargs,                              \
+     JS_FS(name, castFastNativeToNative(fastcall), nargs,                  \
            (flags) | JSFUN_FAST_NATIVE | JSFUN_STUB_GSOPS, 0)

Note to myself: check that we catch similar errors in JSClass and JSPropertySpec initializers (where there is no macro to hack).
Comment on attachment 376774 [details] [diff] [review]
Fix the code to correct function-pointer behavior, rev. 1

For the benefit of my future self: The thing about js_MergeSort is, it requires the JS stack if and only if you pass it a JS_REQUIRES_STACK comparator function.

I'm fairly sure the approach taken in this patch (specifically in regard to js_MergeSort) isn't exactly what we want, but on the whole these patches are an improvement, and they should land pronto.

I will file a separate bug about array_sort, and maybe bsmedberg will file one about checking static initializers.
Attachment #376774 - Flags: review+
(Assignee)

Comment 12

10 years ago
I went ahead and did the static variable and struct initializer thing here (it was two basically separate changes)... it cause the following additional errors:

../../../src/js/src/jsarray.cpp:3287: error: Assigning non-JS_REQUIRES_STACK function pointer from JS_REQUIRES_STACK function array_sort(JSContext*, uintN, jsval*)
../../../src/js/src/jsarray.cpp:3287: error: Assigning non-JS_REQUIRES_STACK function pointer from JS_REQUIRES_STACK function array_forEach(JSContext*, uintN, jsval*)
../../../src/js/src/jsarray.cpp:3287: error: Assigning non-JS_REQUIRES_STACK function pointer from JS_REQUIRES_STACK function array_map(JSContext*, uintN, jsval*)
../../../src/js/src/jsarray.cpp:3287: error: Assigning non-JS_REQUIRES_STACK function pointer from JS_REQUIRES_STACK function array_reduce(JSContext*, uintN, jsval*)
../../../src/js/src/jsarray.cpp:3287: error: Assigning non-JS_REQUIRES_STACK function pointer from JS_REQUIRES_STACK function array_reduceRight(JSContext*, uintN, jsval*)
../../../src/js/src/jsarray.cpp:3287: error: Assigning non-JS_REQUIRES_STACK function pointer from JS_REQUIRES_STACK function array_filter(JSContext*, uintN, jsval*)
../../../src/js/src/jsarray.cpp:3287: error: Assigning non-JS_REQUIRES_STACK function pointer from JS_REQUIRES_STACK function array_some(JSContext*, uintN, jsval*)
../../../src/js/src/jsarray.cpp:3287: error: Assigning non-JS_REQUIRES_STACK function pointer from JS_REQUIRES_STACK function array_every(JSContext*, uintN, jsval*)
../../../src/js/src/jsfun.cpp:1841: error: Assigning non-JS_REQUIRES_STACK function pointer from JS_REQUIRES_STACK function js_fun_apply(JSContext*, uintN, jsval*)
../../../src/js/src/jsfun.cpp:1841: error: Assigning non-JS_REQUIRES_STACK function pointer from JS_REQUIRES_STACK function js_fun_call(JSContext*, uintN, jsval*)
../../../src/js/src/jsstr.cpp:2408: error: Assigning non-JS_REQUIRES_STACK function pointer from JS_REQUIRES_STACK function str_replace(JSContext*, uintN, jsval*)

I'm going to take a quick poke at making all of these not JS_REQUIRES_STACK and see what falls apart.
(Assignee)

Comment 13

10 years ago
Created attachment 383638 [details] [diff] [review]
Do JS_REQUIRES_STACK analysis on function pointers, rev. 3

This is the analysis which does static variable initializers and struct initializers correctly. The patch for the problems it found is in bug 498398
Attachment #376773 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Depends on: 499019
(Assignee)

Updated

10 years ago
Depends on: 499971

Comment 15

10 years ago
http://hg.mozilla.org/mozilla-central/rev/6765e50e8b05
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.