From 0d1056dbed5f524bc2e0075f3029ff03d44a029f Mon Sep 17 00:00:00 2001 From: Matthew Flatt Date: Wed, 27 Jun 2012 09:16:38 -0600 Subject: [PATCH] fix potential bad interaction of module caching and JIT assumption The JIT wants to detect calls to functions bound to module-level variables that are always instantiated to a procedure of a particular arity, in which case it can avoid some checks. The problem is that bytecode and JITted code can be shared via the module cache across namespaces that use different modules to implement a particular module path or that enable the JIT differently. In particular, starting with one that has a procedure binding and then using one (in a different namespace) that has a non-procedure binding could lead to a crash. Defend against this possibility by never treating imported variables as constant in that sense. The JIT detects imported variables through a new import map in the prefix. This change may slow code. My guess is that it will have no performance impact in practice, due to cross-module inlining, although I can construct a microbenchmark that slows by 20%. --- collects/tests/racket/module.rktl | 58 +++++++++++++++++++++++++++++++ src/racket/src/eval.c | 33 +++++++++++++----- src/racket/src/jit.c | 50 +++++++++++++++++--------- src/racket/src/jit.h | 2 +- src/racket/src/jitcall.c | 34 ++++++++---------- src/racket/src/jitinline.c | 2 +- src/racket/src/mzclpf_post.inc | 1 + src/racket/src/mzmark_type.inc | 2 ++ src/racket/src/mzmarksrc.c | 1 + src/racket/src/schpriv.h | 1 + 10 files changed, 138 insertions(+), 46 deletions(-) diff --git a/collects/tests/racket/module.rktl b/collects/tests/racket/module.rktl index f9a0f68311..cb40296638 100644 --- a/collects/tests/racket/module.rktl +++ b/collects/tests/racket/module.rktl @@ -819,6 +819,64 @@ (compile-eval m1-expr) (compile-eval m2-expr))) +;; ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; +;; Check JIT treatement of seemingly constant imports + +(let () + (define (a-expr mut?) + `(module a racket/base + ,(if mut? + `(define a 5) + `(define (a x) + ;; long enough to not be inlined: + (list x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x))) + (provide a))) + (define b-expr + `(module b racket/base + (require 'a) + (define (b q) (a q)) + (provide b))) + + (define (compile-m e strs) + (parameterize ([current-namespace (make-base-namespace)]) + (for ([str (in-list strs)]) + (parameterize ([read-accept-compiled #t]) + (eval (read (open-input-bytes str))))) + (define o (open-output-bytes)) + (write (compile e) o) + (define s (get-output-bytes o)) + (define vlen (bytes-ref s 2)) + ;; Add a hash, so that loading this module in two contexts tries to + ;; use the same loaded bytecode and same JIT-generated code: + (bytes-copy! s (+ 4 vlen) + (subbytes + (bytes-append (string->bytes/utf-8 (format "~s" (bytes-length s))) + (make-bytes 20 0)) + 0 + 20)) + s)) + + (define a-s (compile-m (a-expr #f) '())) + (define am-s (compile-m (a-expr #t) '())) + (define b-s (compile-m b-expr (list a-s))) + + (define (go a-s) + (parameterize ([current-namespace (make-base-namespace)]) + (parameterize ([read-accept-compiled #t]) + (eval (read (open-input-bytes a-s))) + (define temp-dir (find-system-path 'temp-dir)) + (define dir (build-path temp-dir "compiled")) + (make-directory* dir) + (with-output-to-file (build-path dir "check-gen_rkt.zo") + #:exists 'truncate + (lambda () (write-bytes b-s))) + ((dynamic-require (build-path temp-dir "check-gen.rkt") 'b) 10) + (delete-file (build-path dir "check-gen_rkt.zo"))))) + ;; Triger JIT generation with constant function as `a': + (go a-s) + ;; Check that we don't crash when trying to use a different `a': + (err/rt-test (go am-s))) + ;; ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; (report-errs) diff --git a/src/racket/src/eval.c b/src/racket/src/eval.c index f4b9b387cf..474af6a8cf 100644 --- a/src/racket/src/eval.c +++ b/src/racket/src/eval.c @@ -786,7 +786,8 @@ static Scheme_Object *link_module_variable(Scheme_Object *modidx, int check_access, Scheme_Object *insp, int pos, int mod_phase, Scheme_Env *env, - Scheme_Object **exprs, int which) + Scheme_Object **exprs, int which, + char *import_map) { Scheme_Object *modname; Scheme_Env *menv; @@ -856,6 +857,9 @@ static Scheme_Object *link_module_variable(Scheme_Object *modidx, if (!(((Scheme_Bucket_With_Flags *)bkt)->flags & (GLOB_IS_IMMUTATED | GLOB_IS_LINKED))) ((Scheme_Bucket_With_Flags *)bkt)->flags |= GLOB_IS_LINKED; } + + if (!self && !(import_map[which >> 3] & (1 << (which & 0x7)))) + import_map[which >> 3] |= (1 << (which & 0x7)); return (Scheme_Object *)bkt; } @@ -863,7 +867,8 @@ static Scheme_Object *link_module_variable(Scheme_Object *modidx, static Scheme_Object *link_toplevel(Scheme_Object **exprs, int which, Scheme_Env *env, Scheme_Object *src_modidx, Scheme_Object *dest_modidx, - Scheme_Object *insp) + Scheme_Object *insp, + char *import_map) { Scheme_Object *expr = exprs[which]; @@ -896,14 +901,15 @@ static Scheme_Object *link_toplevel(Scheme_Object **exprs, int which, Scheme_Env if (SCHEME_PAIRP(modname)) { mod_phase = SCHEME_INT_VAL(SCHEME_CDR(modname)); modname = SCHEME_CAR(modname); - } + } } return link_module_variable(modname, varname, 0, NULL, -1, mod_phase, env, - NULL, 0); + NULL, 0, + import_map); } else if (SAME_TYPE(SCHEME_TYPE(expr), scheme_variable_type)) { Scheme_Bucket *b = (Scheme_Bucket *)expr; Scheme_Env *home; @@ -918,7 +924,8 @@ static Scheme_Object *link_toplevel(Scheme_Object **exprs, int which, Scheme_Env 1, home->module->insp, -1, home->mod_phase, env, - exprs, which); + exprs, which, + import_map); } else { Module_Variable *mv = (Module_Variable *)expr; @@ -931,7 +938,8 @@ static Scheme_Object *link_toplevel(Scheme_Object **exprs, int which, Scheme_Env mv->sym, 1, (mv->insp ? mv->insp : insp), mv->pos, mv->mod_phase, env, - exprs, which); + exprs, which, + import_map); } } @@ -5437,7 +5445,8 @@ Scheme_Object **scheme_push_prefix(Scheme_Env *genv, Resolve_Prefix *rp, { Scheme_Object **rs_save, **rs, *v; Scheme_Prefix *pf; - int i, j, tl_map_len; + char *import_map; + int i, j, tl_map_len, import_map_len; rs_save = rs = MZ_RUNSTACK; @@ -5458,6 +5467,13 @@ Scheme_Object **scheme_push_prefix(Scheme_Env *genv, Resolve_Prefix *rp, i += rp->num_lifts; tl_map_len = ((rp->num_toplevels + rp->num_lifts) + 31) / 32; + import_map_len = (rp->num_toplevels + 7) / 8; + + if (import_map_len) { + import_map = GC_malloc_atomic(import_map_len); + memset(import_map, 0, import_map_len); + } else + import_map = NULL; pf = scheme_malloc_tagged(sizeof(Scheme_Prefix) + ((i-mzFLEX_DELTA) * sizeof(Scheme_Object *)) @@ -5466,6 +5482,7 @@ Scheme_Object **scheme_push_prefix(Scheme_Env *genv, Resolve_Prefix *rp, pf->num_slots = i; pf->num_toplevels = rp->num_toplevels; pf->num_stxes = rp->num_stxes; + pf->import_map = import_map; --rs; MZ_RUNSTACK = rs; rs[0] = (Scheme_Object *)pf; @@ -5473,7 +5490,7 @@ Scheme_Object **scheme_push_prefix(Scheme_Env *genv, Resolve_Prefix *rp, for (i = 0; i < rp->num_toplevels; i++) { v = rp->toplevels[i]; if (genv || SCHEME_FALSEP(v)) - v = link_toplevel(rp->toplevels, i, genv ? genv : dummy_env, src_modidx, now_modidx, insp); + v = link_toplevel(rp->toplevels, i, genv ? genv : dummy_env, src_modidx, now_modidx, insp, import_map); pf->a[i] = v; } diff --git a/src/racket/src/jit.c b/src/racket/src/jit.c index 08e6c1d5f9..155473d206 100644 --- a/src/racket/src/jit.c +++ b/src/racket/src/jit.c @@ -458,14 +458,28 @@ static int no_sync_change(Scheme_Object *obj, int fuel) } } -Scheme_Object *scheme_extract_global(Scheme_Object *o, Scheme_Native_Closure *nc) +Scheme_Object *scheme_extract_global(Scheme_Object *o, Scheme_Native_Closure *nc, int local_only) { /* GLOBAL ASSUMPTION: we assume that globals are the last thing in the closure; grep for "GLOBAL ASSUMPTION" in fun.c. */ Scheme_Prefix *globs; + int pos; globs = (Scheme_Prefix *)nc->vals[nc->code->u2.orig_code->closure_size - 1]; - return globs->a[SCHEME_TOPLEVEL_POS(o)]; + pos = SCHEME_TOPLEVEL_POS(o); + + if (local_only) { + /* Usually, we look for local bindings only, because module caching means + that JIT-generated code can be linked to different other modules that + may have different bindings, even though we expect them binding to be + consistent. */ + if (pos < globs->num_toplevels) { + if (globs->import_map[pos >> 3] & (1 << (pos & 7))) + return NULL; + } + } + + return globs->a[pos]; } Scheme_Object *scheme_extract_closure_local(Scheme_Object *obj, mz_jit_state *jitter, int extra_push) @@ -511,17 +525,19 @@ int scheme_is_noncm(Scheme_Object *a, mz_jit_state *jitter, int depth, int stack && SAME_TYPE(SCHEME_TYPE(a), scheme_toplevel_type) && ((SCHEME_TOPLEVEL_FLAGS(a) & SCHEME_TOPLEVEL_FLAGS_MASK) >= SCHEME_TOPLEVEL_CONST)) { Scheme_Object *p; - p = scheme_extract_global(a, jitter->nc); - p = ((Scheme_Bucket *)p)->val; - if (p && SAME_TYPE(SCHEME_TYPE(p), scheme_native_closure_type)) { - Scheme_Native_Closure_Data *ndata = ((Scheme_Native_Closure *)p)->code; - if (ndata->closure_size >= 0) { /* not case-lambda */ - if (lambda_has_been_jitted(ndata)) { - if (SCHEME_NATIVE_CLOSURE_DATA_FLAGS(ndata) & NATIVE_PRESERVES_MARKS) - return 1; - } else { - if (SCHEME_CLOSURE_DATA_FLAGS(ndata->u2.orig_code) & CLOS_PRESERVES_MARKS) - return 1; + p = scheme_extract_global(a, jitter->nc, 1); + if (p) { + p = ((Scheme_Bucket *)p)->val; + if (p && SAME_TYPE(SCHEME_TYPE(p), scheme_native_closure_type)) { + Scheme_Native_Closure_Data *ndata = ((Scheme_Native_Closure *)p)->code; + if (ndata->closure_size >= 0) { /* not case-lambda */ + if (lambda_has_been_jitted(ndata)) { + if (SCHEME_NATIVE_CLOSURE_DATA_FLAGS(ndata) & NATIVE_PRESERVES_MARKS) + return 1; + } else { + if (SCHEME_CLOSURE_DATA_FLAGS(ndata->u2.orig_code) & CLOS_PRESERVES_MARKS) + return 1; + } } } } @@ -731,9 +747,11 @@ static int is_a_procedure(Scheme_Object *v, mz_jit_state *jitter) if (jitter->nc) { Scheme_Object *p; - p = scheme_extract_global(v, jitter->nc); - p = ((Scheme_Bucket *)p)->val; - return SAME_TYPE(SCHEME_TYPE(p), scheme_native_closure_type); + p = scheme_extract_global(v, jitter->nc, 1); + if (p) { + p = ((Scheme_Bucket *)p)->val; + return SAME_TYPE(SCHEME_TYPE(p), scheme_native_closure_type); + } } } } diff --git a/src/racket/src/jit.h b/src/racket/src/jit.h index cc04bf7b7f..afdb06d142 100644 --- a/src/racket/src/jit.h +++ b/src/racket/src/jit.h @@ -1282,7 +1282,7 @@ int scheme_jit_check_closure_flonum_bit(Scheme_Closure_Data *data, int pos, int # define CLOSURE_CONTENT_IS_FLONUM(data, pos) scheme_jit_check_closure_flonum_bit(data, pos, data->num_params) #endif -Scheme_Object *scheme_extract_global(Scheme_Object *o, Scheme_Native_Closure *nc); +Scheme_Object *scheme_extract_global(Scheme_Object *o, Scheme_Native_Closure *nc, int local_only); Scheme_Object *scheme_extract_closure_local(Scheme_Object *obj, mz_jit_state *jitter, int extra_push); void scheme_jit_register_traversers(void); diff --git a/src/racket/src/jitcall.c b/src/racket/src/jitcall.c index 80af1d953f..db6988423e 100644 --- a/src/racket/src/jitcall.c +++ b/src/racket/src/jitcall.c @@ -1468,36 +1468,30 @@ int scheme_generate_app(Scheme_App_Rec *app, Scheme_Object **alt_rands, int num_ } } } else if (t == scheme_toplevel_type) { - if (0 && (SCHEME_TOPLEVEL_FLAGS(rator) & SCHEME_TOPLEVEL_FLAGS_MASK) >= SCHEME_TOPLEVEL_FIXED) { + if ((SCHEME_TOPLEVEL_FLAGS(rator) & SCHEME_TOPLEVEL_FLAGS_MASK) >= SCHEME_TOPLEVEL_FIXED) { /* We can re-order evaluation of the rator. */ reorder_ok = 1; - if (jitter->nc && 0 + if (jitter->nc && ((SCHEME_TOPLEVEL_FLAGS(rator) & SCHEME_TOPLEVEL_FLAGS_MASK) >= SCHEME_TOPLEVEL_CONST)) { Scheme_Object *p; - p = scheme_extract_global(rator, jitter->nc); - p = ((Scheme_Bucket *)p)->val; - if (can_direct_native(p, num_rands, &extract_case)) { - direct_native = 1; + p = scheme_extract_global(rator, jitter->nc, 1); + if (p) { + p = ((Scheme_Bucket *)p)->val; + if (can_direct_native(p, num_rands, &extract_case)) { + direct_native = 1; - if ((SCHEME_TOPLEVEL_POS(rator) == jitter->self_toplevel_pos) - && (num_rands < MAX_SHARED_CALL_RANDS)) { - if (is_tail) - direct_self = 1; - else if (jitter->self_nontail_code) - nontail_self = 1; + if ((SCHEME_TOPLEVEL_POS(rator) == jitter->self_toplevel_pos) + && (num_rands < MAX_SHARED_CALL_RANDS)) { + if (is_tail) + direct_self = 1; + else if (jitter->self_nontail_code) + nontail_self = 1; + } } } } - } else if (jitter->nc) { - Scheme_Object *p; - - p = scheme_extract_global(rator, jitter->nc); - if (((Scheme_Bucket_With_Flags *)p)->flags & GLOB_IS_CONSISTENT) { - if (can_direct_native(((Scheme_Bucket *)p)->val, num_rands, &extract_case)) - direct_native = 1; - } } } else if (SAME_TYPE(t, scheme_closure_type)) { Scheme_Closure_Data *data; diff --git a/src/racket/src/jitinline.c b/src/racket/src/jitinline.c index 382a961ea7..c14af07bec 100644 --- a/src/racket/src/jitinline.c +++ b/src/racket/src/jitinline.c @@ -147,7 +147,7 @@ static int inlineable_struct_prim(Scheme_Object *o, mz_jit_state *jitter, int ex if (jitter->nc) { if (SAME_TYPE(SCHEME_TYPE(o), scheme_toplevel_type)) { Scheme_Object *p; - p = scheme_extract_global(o, jitter->nc); + p = scheme_extract_global(o, jitter->nc, 0); p = ((Scheme_Bucket *)p)->val; return check_val_struct_prim(p, arity); } else if (SAME_TYPE(SCHEME_TYPE(o), scheme_local_type)) { diff --git a/src/racket/src/mzclpf_post.inc b/src/racket/src/mzclpf_post.inc index 55838e3a3c..6fa6dd2be8 100644 --- a/src/racket/src/mzclpf_post.inc +++ b/src/racket/src/mzclpf_post.inc @@ -18,6 +18,7 @@ if (!pf->next_final) { /* We're the first to look at this prefix... */ + gcMARK2(pf->import_map, gc); if (pf->num_stxes) { /* Mark all syntax-object references */ for (i = pf->num_stxes+1; i--;) { diff --git a/src/racket/src/mzmark_type.inc b/src/racket/src/mzmark_type.inc index 710375d3a7..ceae929b24 100644 --- a/src/racket/src/mzmark_type.inc +++ b/src/racket/src/mzmark_type.inc @@ -2356,6 +2356,7 @@ static int prefix_val_MARK(void *p, struct NewGC *gc) { int i; for (i = pf->num_slots; i--; ) gcMARK2(pf->a[i], gc); + gcMARK2(pf->import_map, gc); return gcBYTES_TO_WORDS((sizeof(Scheme_Prefix) + ((pf->num_slots-mzFLEX_DELTA) * sizeof(Scheme_Object *)) @@ -2368,6 +2369,7 @@ static int prefix_val_FIXUP(void *p, struct NewGC *gc) { int i; for (i = pf->num_slots; i--; ) gcFIXUP2(pf->a[i], gc); + gcFIXUP2(pf->import_map, gc); return gcBYTES_TO_WORDS((sizeof(Scheme_Prefix) + ((pf->num_slots-mzFLEX_DELTA) * sizeof(Scheme_Object *)) diff --git a/src/racket/src/mzmarksrc.c b/src/racket/src/mzmarksrc.c index cfc646784c..df23bf2f97 100644 --- a/src/racket/src/mzmarksrc.c +++ b/src/racket/src/mzmarksrc.c @@ -946,6 +946,7 @@ prefix_val { int i; for (i = pf->num_slots; i--; ) gcMARK2(pf->a[i], gc); + gcMARK2(pf->import_map, gc); size: gcBYTES_TO_WORDS((sizeof(Scheme_Prefix) + ((pf->num_slots-mzFLEX_DELTA) * sizeof(Scheme_Object *)) diff --git a/src/racket/src/schpriv.h b/src/racket/src/schpriv.h index 304108fbe2..2ef8bfb090 100644 --- a/src/racket/src/schpriv.h +++ b/src/racket/src/schpriv.h @@ -2127,6 +2127,7 @@ typedef struct Scheme_Prefix Scheme_Object so; /* scheme_prefix_type */ int num_slots, num_toplevels, num_stxes; struct Scheme_Prefix *next_final; /* for special GC handling */ + char *import_map; /* bitmap indicating which toplevels are imported */ Scheme_Object *a[mzFLEX_ARRAY_DECL]; /* array of objects */ /* followed by an array of `int's for tl_map uses */ } Scheme_Prefix;