[gjs/wip/gcampax/70-arg-cache: 8/11] arg-cache: Limit GI arguments to 253 and use 8-bit storage

[Philip Chimento <pchimento src gnome org>]

commit 08a56d5516c1fdcbc6d8c45c6181d473aeb728e4
Author: Philip Chimento <philip chimento gmail com>
Date:   Sat Jul 18 11:27:47 2020 -0700

    arg-cache: Limit GI arguments to 253 and use 8-bit storage
    
    It's wasteful to use a signed int to store the number of function
    arguments. We do need two special values to indicate return value and
    instance parameter, but these can be the two highest unsigned values
    instead of -1 and -2.
    
    It seems reasonable to use 8 bits for the number of arguments. Throw an
    exception when invoking a function with too many arguments, and use
    assertions in GjsArgumentCache for internal checks.

 gi/arg-cache.cpp | 61 +++++++++++++++++++++++++-------------------------------
 gi/arg-cache.h   | 59 ++++++++++++++++++++++++++++++++++++++++++++++++------
 gi/function.cpp  |  5 +++++
 3 files changed, 85 insertions(+), 40 deletions(-)
---
diff --git a/gi/arg-cache.cpp b/gi/arg-cache.cpp
index 0d204ae6..7e25204b 100644
--- a/gi/arg-cache.cpp
+++ b/gi/arg-cache.cpp
@@ -216,10 +216,11 @@ GJS_JSAPI_RETURN_CONVENTION
 static bool gjs_marshal_generic_in_in(JSContext* cx, GjsArgumentCache* self,
                                       GjsFunctionCallState*, GIArgument* arg,
                                       JS::HandleValue value) {
-    return gjs_value_to_g_argument(
-        cx, value, &self->type_info, self->arg_name,
-        self->is_return ? GJS_ARGUMENT_RETURN_VALUE : GJS_ARGUMENT_ARGUMENT,
-        self->transfer, self->nullable, arg);
+    return gjs_value_to_g_argument(cx, value, &self->type_info, self->arg_name,
+                                   self->is_return_value()
+                                       ? GJS_ARGUMENT_RETURN_VALUE
+                                       : GJS_ARGUMENT_ARGUMENT,
+                                   self->transfer, self->nullable, arg);
 }
 
 GJS_JSAPI_RETURN_CONVENTION
@@ -250,7 +251,7 @@ static bool gjs_marshal_explicit_array_in_in(JSContext* cx,
             self->transfer, self->nullable, &data, &length))
         return false;
 
-    int length_pos = self->contents.array.length_pos;
+    uint8_t length_pos = self->contents.array.length_pos;
     gjs_g_argument_set_array_length(self->contents.array.length_tag,
                                     &state->in_cvalues[length_pos], length);
     gjs_arg_set(arg, data);
@@ -266,8 +267,8 @@ static bool gjs_marshal_explicit_array_inout_in(JSContext* cx,
     if (!gjs_marshal_explicit_array_in_in(cx, self, state, arg, value))
         return false;
 
-    int length_pos = self->contents.array.length_pos;
-    int ix = self->arg_pos;
+    uint8_t length_pos = self->contents.array.length_pos;
+    uint8_t ix = self->arg_pos;
 
     if (!gjs_arg_get<void*>(arg)) {
         // Special case where we were given JS null to also pass null for
@@ -318,14 +319,15 @@ static bool gjs_marshal_callback_in(JSContext* cx, GjsArgumentCache* self,
         closure = trampoline->closure;
     }
 
-    int destroy_pos = self->contents.callback.destroy_pos;
-    if (destroy_pos >= 0) {
+    if (self->has_callback_destroy()) {
+        uint8_t destroy_pos = self->contents.callback.destroy_pos;
         gjs_arg_set(&state->in_cvalues[destroy_pos],
                     trampoline ? gjs_destroy_notify_callback : nullptr);
     }
-    int closure_pos = self->contents.callback.closure_pos;
-    if (closure_pos >= 0)
+    if (self->has_callback_closure()) {
+        uint8_t closure_pos = self->contents.callback.closure_pos;
         gjs_arg_set(&state->in_cvalues[closure_pos], trampoline);
+    }
 
     if (trampoline && self->contents.callback.scope != GI_SCOPE_TYPE_CALL) {
         // Add an extra reference that will be cleared when collecting async
@@ -763,7 +765,7 @@ static bool gjs_marshal_explicit_array_out_out(JSContext* cx,
                                                GjsFunctionCallState* state,
                                                GIArgument* arg,
                                                JS::MutableHandleValue value) {
-    int length_pos = self->contents.array.length_pos;
+    uint8_t length_pos = self->contents.array.length_pos;
     GIArgument* length_arg = &(state->out_cvalues[length_pos]);
     GITypeTag length_tag = self->contents.array.length_tag;
     size_t length = gjs_g_argument_get_array_length(length_tag, length_arg);
@@ -825,7 +827,7 @@ GJS_JSAPI_RETURN_CONVENTION
 static bool gjs_marshal_explicit_array_out_release(
     JSContext* cx, GjsArgumentCache* self, GjsFunctionCallState* state,
     GIArgument* in_arg G_GNUC_UNUSED, GIArgument* out_arg) {
-    int length_pos = self->contents.array.length_pos;
+    uint8_t length_pos = self->contents.array.length_pos;
     GIArgument* length_arg = &(state->out_cvalues[length_pos]);
     GITypeTag length_tag = self->contents.array.length_tag;
     size_t length = gjs_g_argument_get_array_length(length_tag, length_arg);
@@ -838,7 +840,7 @@ GJS_JSAPI_RETURN_CONVENTION
 static bool gjs_marshal_explicit_array_in_release(
     JSContext* cx, GjsArgumentCache* self, GjsFunctionCallState* state,
     GIArgument* in_arg, GIArgument* out_arg G_GNUC_UNUSED) {
-    int length_pos = self->contents.array.length_pos;
+    uint8_t length_pos = self->contents.array.length_pos;
     GIArgument* length_arg = &(state->in_cvalues[length_pos]);
     GITypeTag length_tag = self->contents.array.length_tag;
     size_t length = gjs_g_argument_get_array_length(length_tag, length_arg);
@@ -854,7 +856,7 @@ GJS_JSAPI_RETURN_CONVENTION
 static bool gjs_marshal_explicit_array_inout_release(
     JSContext* cx, GjsArgumentCache* self, GjsFunctionCallState* state,
     GIArgument* in_arg G_GNUC_UNUSED, GIArgument* out_arg) {
-    int length_pos = self->contents.array.length_pos;
+    uint8_t length_pos = self->contents.array.length_pos;
     GIArgument* length_arg = &(state->in_cvalues[length_pos]);
     GITypeTag length_tag = self->contents.array.length_tag;
     size_t length = gjs_g_argument_get_array_length(length_tag, length_arg);
@@ -971,11 +973,8 @@ bool gjs_arg_cache_build_return(JSContext*, GjsArgumentCache* self,
     }
 
     *inc_counter_out = true;
-    self->arg_pos = -1;
-    self->arg_name = "return value";
+    self->set_return_value();
     self->transfer = g_callable_info_get_caller_owns(callable);
-    self->nullable = false;  // We don't really care for return values
-    self->is_return = true;
 
     if (g_type_info_get_tag(&self->type_info) == GI_TYPE_TAG_ARRAY) {
         int length_pos = g_type_info_get_array_length(&self->type_info);
@@ -984,14 +983,14 @@ bool gjs_arg_cache_build_return(JSContext*, GjsArgumentCache* self,
 
             // Even if we skip the length argument most of the time, we need to
             // do some basic initialization here.
-            arguments[length_pos].arg_pos = length_pos;
+            arguments[length_pos].set_arg_pos(length_pos);
             arguments[length_pos].marshal_in = gjs_marshal_generic_out_in;
 
             self->marshal_in = gjs_marshal_generic_out_in;
             self->marshal_out = gjs_marshal_explicit_array_out_out;
             self->release = gjs_marshal_explicit_array_out_release;
 
-            self->contents.array.length_pos = length_pos;
+            self->set_array_length_pos(length_pos);
 
             GIArgInfo length_arg;
             g_callable_info_load_arg(callable, length_pos, &length_arg);
@@ -1278,13 +1277,8 @@ bool gjs_arg_cache_build_instance(JSContext* cx, GjsArgumentCache* self,
                                   GICallableInfo* callable) {
     GIBaseInfo* interface_info = g_base_info_get_container(callable);  // !owned
 
-    self->arg_pos = -2;
-    self->arg_name = "instance parameter";
+    self->set_instance_parameter();
     self->transfer = g_callable_info_get_instance_ownership_transfer(callable);
-    // Some calls accept null for the instance, but generally in an object
-    // oriented language it's wrong to call a method on null
-    self->nullable = false;
-    self->skip_out = true;
 
     // These cases could be covered by the generic marshaller, except that
     // there's no way to get GITypeInfo for a method's instance parameter.
@@ -1317,17 +1311,16 @@ bool gjs_arg_cache_build_instance(JSContext* cx, GjsArgumentCache* self,
 }
 
 bool gjs_arg_cache_build_arg(JSContext* cx, GjsArgumentCache* self,
-                             GjsArgumentCache* arguments, int gi_index,
+                             GjsArgumentCache* arguments, uint8_t gi_index,
                              GIDirection direction, GIArgInfo* arg,
                              GICallableInfo* callable, bool* inc_counter_out) {
     g_assert(inc_counter_out && "forgot out parameter");
 
-    self->arg_pos = gi_index;
+    self->set_arg_pos(gi_index);
     self->arg_name = g_base_info_get_name(arg);
     g_arg_info_load_type(arg, &self->type_info);
     self->transfer = g_arg_info_get_ownership_transfer(arg);
     self->nullable = g_arg_info_may_be_null(arg);
-    self->is_return = false;
 
     if (direction == GI_DIRECTION_IN)
         self->skip_out = true;
@@ -1414,8 +1407,8 @@ bool gjs_arg_cache_build_arg(JSContext* cx, GjsArgumentCache* self,
                 }
 
                 self->contents.callback.scope = g_arg_info_get_scope(arg);
-                self->contents.callback.destroy_pos = destroy_pos;
-                self->contents.callback.closure_pos = closure_pos;
+                self->set_callback_destroy_pos(destroy_pos);
+                self->set_callback_closure_pos(closure_pos);
             }
 
             return true;
@@ -1440,7 +1433,7 @@ bool gjs_arg_cache_build_arg(JSContext* cx, GjsArgumentCache* self,
             } else {
                 // Even if we skip the length argument most of time, we need to
                 // do some basic initialization here.
-                arguments[length_pos].arg_pos = length_pos;
+                arguments[length_pos].set_arg_pos(length_pos);
                 arguments[length_pos].marshal_in = gjs_marshal_generic_out_in;
 
                 self->marshal_in = gjs_marshal_generic_out_in;
@@ -1448,7 +1441,7 @@ bool gjs_arg_cache_build_arg(JSContext* cx, GjsArgumentCache* self,
                 self->release = gjs_marshal_explicit_array_out_release;
             }
 
-            self->contents.array.length_pos = length_pos;
+            self->set_array_length_pos(length_pos);
 
             GIArgInfo length_arg;
             g_callable_info_load_arg(callable, length_pos, &length_arg);
diff --git a/gi/arg-cache.h b/gi/arg-cache.h
index 2a43a2be..50618d3f 100644
--- a/gi/arg-cache.h
+++ b/gi/arg-cache.h
@@ -31,6 +31,7 @@
 
 #include <girepository.h>
 #include <glib-object.h>
+#include <glib.h>  // for g_assert
 
 #include <js/RootingAPI.h>
 #include <js/TypeDecls.h>
@@ -54,23 +55,22 @@ struct GjsArgumentCache {
     const char* arg_name;
     GITypeInfo type_info;
 
-    int arg_pos;
+    uint8_t arg_pos;
     bool skip_in : 1;
     bool skip_out : 1;
     GITransfer transfer : 2;
     bool nullable : 1;
-    bool is_return : 1;
 
     union {
         // for explicit array only
         struct {
-            int length_pos;
+            uint8_t length_pos;
             GITypeTag length_tag : 5;
         } array;
 
         struct {
-            int closure_pos;
-            int destroy_pos;
+            uint8_t closure_pos;
+            uint8_t destroy_pos;
             GIScopeType scope : 2;
         } callback;
 
@@ -104,6 +104,53 @@ struct GjsArgumentCache {
 
     GJS_JSAPI_RETURN_CONVENTION
     bool handle_nullable(JSContext* cx, GIArgument* arg);
+
+    // Introspected functions can have up to 253 arguments. 255 is a placeholder
+    // for the return value and 254 for the instance parameter. The callback
+    // closure or destroy notify parameter may have a value of 255 to indicate
+    // that it is absent.
+    static constexpr uint8_t MAX_ARGS = 253;
+    static constexpr uint8_t INSTANCE_PARAM = 254;
+    static constexpr uint8_t RETURN_VALUE = 255;
+    static constexpr uint8_t ABSENT = 255;
+    void set_arg_pos(int pos) {
+        g_assert(pos <= MAX_ARGS && "No more than 253 arguments allowed");
+        arg_pos = pos;
+    }
+    void set_array_length_pos(int pos) {
+        g_assert(pos <= MAX_ARGS && "No more than 253 arguments allowed");
+        contents.array.length_pos = pos;
+    }
+    void set_callback_destroy_pos(int pos) {
+        g_assert(pos <= MAX_ARGS && "No more than 253 arguments allowed");
+        contents.callback.destroy_pos = pos < 0 ? ABSENT : pos;
+    }
+    GJS_USE bool has_callback_destroy() {
+        return contents.callback.destroy_pos != ABSENT;
+    }
+    void set_callback_closure_pos(int pos) {
+        g_assert(pos <= MAX_ARGS && "No more than 253 arguments allowed");
+        contents.callback.closure_pos = pos < 0 ? ABSENT : pos;
+    }
+    GJS_USE bool has_callback_closure() {
+        return contents.callback.closure_pos != ABSENT;
+    }
+
+    void set_instance_parameter() {
+        arg_pos = INSTANCE_PARAM;
+        arg_name = "instance parameter";
+        // Some calls accept null for the instance, but generally in an object
+        // oriented language it's wrong to call a method on null
+        nullable = false;
+        skip_out = true;
+    }
+
+    void set_return_value() {
+        arg_pos = RETURN_VALUE;
+        arg_name = "return value";
+        nullable = false;  // We don't really care for return values
+    }
+    GJS_USE bool is_return_value() { return arg_pos == RETURN_VALUE; }
 };
 
 // This is a trick to print out the sizes of the structs at compile time, in
@@ -122,7 +169,7 @@ static_assert(sizeof(GjsArgumentCache) <= 136,
 
 GJS_JSAPI_RETURN_CONVENTION
 bool gjs_arg_cache_build_arg(JSContext* cx, GjsArgumentCache* self,
-                             GjsArgumentCache* arguments, int gi_index,
+                             GjsArgumentCache* arguments, uint8_t gi_index,
                              GIDirection direction, GIArgInfo* arg,
                              GICallableInfo* callable, bool* inc_counter_out);
 
diff --git a/gi/function.cpp b/gi/function.cpp
index 50c08352..5f8a6819 100644
--- a/gi/function.cpp
+++ b/gi/function.cpp
@@ -708,6 +708,11 @@ static bool gjs_invoke_c_function(JSContext* context, Function* function,
 
     unsigned ffi_argc = function->invoker.cif.nargs;
     gi_argc = g_callable_info_get_n_args( (GICallableInfo*) function->info);
+    if (gi_argc > GjsArgumentCache::MAX_ARGS) {
+        GjsAutoChar name = format_function_name(function);
+        gjs_throw(context, "Function %s has too many arguments", name.get());
+        return false;
+    }
 
     // ffi_argc is the number of arguments that the underlying C function takes.
     // gi_argc is the number of arguments the GICallableInfo describes (which