From 91455710f38f226e45591203b0d4cd8e392131fd Mon Sep 17 00:00:00 2001 From: Keno Fischer Date: Fri, 22 Mar 2024 05:37:18 -0400 Subject: [PATCH] Use jl_filename/jl_lineno less (#53799) I don't like `jl_filename`/`jl_lineno`. They are weird internal state, and they are also not thread safe, so if different threads are evaling different things at the same time, line numbers can get confused. This PR changes the core function `jl_toplevel_eval_flex` to keep track of its current file/line context on the stack, so at least there is no confusion within one call to this function. With this PR and #53797, the global `jl_filename`/`jl_lineno` are used for three purposes: 1. To initialize the filename/lineno used by lowering from `Core.eval`. 2. To give binding deprecation warnings. 3. For `jl_critical_error`. 4. By humans in the debugger. I think 3 and 4 are fine, they are exceptional cases. Case 2, I think could be changed to plumb through locations explicitly, but it's a bit annoying, so I didn't tackle it here. Case 1, I think can probably just be changed to consistently initialize, and if you want a proper line number, you need to put it in there explicitly. However, I didn't change that in this PR, because I think it could be slightly breaking, so should be pkgeval'd. --- src/julia_internal.h | 2 +- src/toplevel.c | 73 ++++++++++++++++++++++++++------------------ test/backtrace.jl | 9 ++++++ 3 files changed, 53 insertions(+), 31 deletions(-) diff --git a/src/julia_internal.h b/src/julia_internal.h index dc93e522b0c4d..7df996d888909 100644 --- a/src/julia_internal.h +++ b/src/julia_internal.h @@ -790,7 +790,7 @@ JL_DLLEXPORT int jl_datatype_isinlinealloc(jl_datatype_t *ty, int pointerfree); int jl_type_equality_is_identity(jl_value_t *t1, jl_value_t *t2) JL_NOTSAFEPOINT; void jl_eval_global_expr(jl_module_t *m, jl_expr_t *ex, int set_type); -jl_value_t *jl_toplevel_eval_flex(jl_module_t *m, jl_value_t *e, int fast, int expanded); +JL_DLLEXPORT jl_value_t *jl_toplevel_eval_flex(jl_module_t *m, jl_value_t *e, int fast, int expanded, const char **toplevel_filename, int *toplevel_lineno); jl_value_t *jl_eval_global_var(jl_module_t *m JL_PROPAGATES_ROOT, jl_sym_t *e); jl_value_t *jl_interpret_opaque_closure(jl_opaque_closure_t *clos, jl_value_t **args, size_t nargs); diff --git a/src/toplevel.c b/src/toplevel.c index 8715a955e7fb2..d7d4b04c3e67b 100644 --- a/src/toplevel.c +++ b/src/toplevel.c @@ -203,16 +203,16 @@ static jl_value_t *jl_eval_module_expr(jl_module_t *parent_module, jl_expr_t *ex } // add `eval` function form = jl_call_scm_on_ast_and_loc("module-default-defs", (jl_value_t*)name, newm, filename, lineno); - jl_toplevel_eval_flex(newm, form, 0, 1); + jl_toplevel_eval_flex(newm, form, 0, 1, &filename, &lineno); form = NULL; } for (int i = 0; i < jl_array_nrows(exprs); i++) { // process toplevel form ct->world_age = jl_atomic_load_acquire(&jl_world_counter); - form = jl_expand_stmt_with_loc(jl_array_ptr_ref(exprs, i), newm, jl_filename, jl_lineno); + form = jl_expand_stmt_with_loc(jl_array_ptr_ref(exprs, i), newm, filename, lineno); ct->world_age = jl_atomic_load_acquire(&jl_world_counter); - (void)jl_toplevel_eval_flex(newm, form, 1, 1); + (void)jl_toplevel_eval_flex(newm, form, 1, 1, &filename, &lineno); } ct->world_age = last_age; @@ -286,13 +286,13 @@ static jl_value_t *jl_eval_module_expr(jl_module_t *parent_module, jl_expr_t *ex return (jl_value_t*)newm; } -static jl_value_t *jl_eval_dot_expr(jl_module_t *m, jl_value_t *x, jl_value_t *f, int fast) +static jl_value_t *jl_eval_dot_expr(jl_module_t *m, jl_value_t *x, jl_value_t *f, int fast, const char **toplevel_filename, int *toplevel_lineno) { jl_task_t *ct = jl_current_task; jl_value_t **args; JL_GC_PUSHARGS(args, 3); - args[1] = jl_toplevel_eval_flex(m, x, fast, 0); - args[2] = jl_toplevel_eval_flex(m, f, fast, 0); + args[1] = jl_toplevel_eval_flex(m, x, fast, 0, toplevel_filename, toplevel_lineno); + args[2] = jl_toplevel_eval_flex(m, f, fast, 0, toplevel_filename, toplevel_lineno); if (jl_is_module(args[1])) { JL_TYPECHK(getglobal, symbol, args[2]); args[0] = jl_eval_global_var((jl_module_t*)args[1], (jl_sym_t*)args[2]); @@ -666,46 +666,49 @@ static void check_macro_rename(jl_sym_t *from, jl_sym_t *to, const char *keyword // Eval `throw(ErrorException(msg)))` in module `m`. // Used in `jl_toplevel_eval_flex` instead of `jl_throw` so that the error // location in julia code gets into the backtrace. -static void jl_eval_throw(jl_module_t *m, jl_value_t *exc) +static void jl_eval_throw(jl_module_t *m, jl_value_t *exc, const char *filename, int lineno) { jl_value_t *throw_ex = (jl_value_t*)jl_exprn(jl_call_sym, 2); JL_GC_PUSH1(&throw_ex); jl_exprargset(throw_ex, 0, jl_builtin_throw); jl_exprargset(throw_ex, 1, exc); - jl_toplevel_eval_flex(m, throw_ex, 0, 0); + jl_toplevel_eval_flex(m, throw_ex, 0, 0, &filename, &lineno); JL_GC_POP(); } // Format error message and call jl_eval -static void jl_eval_errorf(jl_module_t *m, const char* fmt, ...) +static void jl_eval_errorf(jl_module_t *m, const char *filename, int lineno, const char* fmt, ...) { va_list args; va_start(args, fmt); jl_value_t *exc = jl_vexceptionf(jl_errorexception_type, fmt, args); va_end(args); JL_GC_PUSH1(&exc); - jl_eval_throw(m, exc); + jl_eval_throw(m, exc, filename, lineno); JL_GC_POP(); } -jl_value_t *jl_toplevel_eval_flex(jl_module_t *JL_NONNULL m, jl_value_t *e, int fast, int expanded) +JL_DLLEXPORT jl_value_t *jl_toplevel_eval_flex(jl_module_t *JL_NONNULL m, jl_value_t *e, int fast, int expanded, const char **toplevel_filename, int *toplevel_lineno) { jl_task_t *ct = jl_current_task; if (!jl_is_expr(e)) { if (jl_is_linenode(e)) { - jl_lineno = jl_linenode_line(e); + *toplevel_lineno = jl_linenode_line(e); jl_value_t *file = jl_linenode_file(e); if (file != jl_nothing) { assert(jl_is_symbol(file)); - jl_filename = jl_symbol_name((jl_sym_t*)file); + *toplevel_filename = jl_symbol_name((jl_sym_t*)file); } + // Not thread safe. For debugging and last resort error messages (jl_critical_error) only. + jl_filename = *toplevel_filename; + jl_lineno = *toplevel_lineno; return jl_nothing; } if (jl_is_symbol(e)) { char *n = jl_symbol_name((jl_sym_t*)e), *n0 = n; while (*n == '_') ++n; if (*n == 0 && n > n0) - jl_eval_errorf(m, "all-underscore identifiers are write-only and their values cannot be used in expressions"); + jl_eval_errorf(m, *toplevel_filename, *toplevel_lineno, "all-underscore identifiers are write-only and their values cannot be used in expressions"); } return jl_interpret_toplevel_expr_in(m, e, NULL, NULL); } @@ -714,12 +717,12 @@ jl_value_t *jl_toplevel_eval_flex(jl_module_t *JL_NONNULL m, jl_value_t *e, int if (ex->head == jl_dot_sym && jl_expr_nargs(ex) != 1) { if (jl_expr_nargs(ex) != 2) - jl_eval_errorf(m, "syntax: malformed \".\" expression"); + jl_eval_errorf(m, *toplevel_filename, *toplevel_lineno, "syntax: malformed \".\" expression"); jl_value_t *lhs = jl_exprarg(ex, 0); jl_value_t *rhs = jl_exprarg(ex, 1); // only handle `a.b` syntax here, so qualified names can be eval'd in pure contexts if (jl_is_quotenode(rhs) && jl_is_symbol(jl_fieldref(rhs, 0))) { - return jl_eval_dot_expr(m, lhs, rhs, fast); + return jl_eval_dot_expr(m, lhs, rhs, fast, toplevel_filename, toplevel_lineno); } } @@ -734,7 +737,7 @@ jl_value_t *jl_toplevel_eval_flex(jl_module_t *JL_NONNULL m, jl_value_t *e, int size_t last_age = ct->world_age; if (!expanded && jl_needs_lowering(e)) { ct->world_age = jl_atomic_load_acquire(&jl_world_counter); - ex = (jl_expr_t*)jl_expand_with_loc_warn(e, m, jl_filename, jl_lineno); + ex = (jl_expr_t*)jl_expand_with_loc_warn(e, m, *toplevel_filename, *toplevel_lineno); ct->world_age = last_age; } jl_sym_t *head = jl_is_expr(ex) ? ex->head : NULL; @@ -766,7 +769,8 @@ jl_value_t *jl_toplevel_eval_flex(jl_module_t *JL_NONNULL m, jl_value_t *e, int } else { if (!jl_is_module(u)) - jl_eval_errorf(m, "invalid using path: \"%s\" does not name a module", + jl_eval_errorf(m, *toplevel_filename, *toplevel_lineno, + "invalid using path: \"%s\" does not name a module", jl_symbol_name(name)); // `using A` and `using A.B` syntax jl_module_using(m, u); @@ -792,7 +796,8 @@ jl_value_t *jl_toplevel_eval_flex(jl_module_t *JL_NONNULL m, jl_value_t *e, int continue; } } - jl_eval_errorf(m, "syntax: malformed \"using\" statement"); + jl_eval_errorf(m, *toplevel_filename, *toplevel_lineno, + "syntax: malformed \"using\" statement"); } JL_GC_POP(); return jl_nothing; @@ -839,7 +844,8 @@ jl_value_t *jl_toplevel_eval_flex(jl_module_t *JL_NONNULL m, jl_value_t *e, int continue; } } - jl_eval_errorf(m, "syntax: malformed \"import\" statement"); + jl_eval_errorf(m, *toplevel_filename, *toplevel_lineno, + "syntax: malformed \"import\" statement"); } JL_GC_POP(); return jl_nothing; @@ -849,8 +855,9 @@ jl_value_t *jl_toplevel_eval_flex(jl_module_t *JL_NONNULL m, jl_value_t *e, int for (size_t i = 0; i < jl_array_nrows(ex->args); i++) { jl_sym_t *name = (jl_sym_t*)jl_array_ptr_ref(ex->args, i); if (!jl_is_symbol(name)) - jl_eval_errorf(m, exp ? "syntax: malformed \"export\" statement" : - "syntax: malformed \"public\" statement"); + jl_eval_errorf(m, *toplevel_filename, *toplevel_lineno, + exp ? "syntax: malformed \"export\" statement" : + "syntax: malformed \"public\" statement"); jl_module_public(m, name, exp); } JL_GC_POP(); @@ -883,17 +890,19 @@ jl_value_t *jl_toplevel_eval_flex(jl_module_t *JL_NONNULL m, jl_value_t *e, int jl_value_t *res = jl_nothing; int i; for (i = 0; i < jl_array_nrows(ex->args); i++) { - res = jl_toplevel_eval_flex(m, jl_array_ptr_ref(ex->args, i), fast, 0); + res = jl_toplevel_eval_flex(m, jl_array_ptr_ref(ex->args, i), fast, 0, toplevel_filename, toplevel_lineno); } JL_GC_POP(); return res; } else if (head == jl_error_sym || head == jl_incomplete_sym) { if (jl_expr_nargs(ex) == 0) - jl_eval_errorf(m, "malformed \"%s\" expression", jl_symbol_name(head)); + jl_eval_errorf(m, *toplevel_filename, *toplevel_lineno, + "malformed \"%s\" expression", jl_symbol_name(head)); if (jl_is_string(jl_exprarg(ex, 0))) - jl_eval_errorf(m, "syntax: %s", jl_string_data(jl_exprarg(ex, 0))); - jl_eval_throw(m, jl_exprarg(ex, 0)); + jl_eval_errorf(m, *toplevel_filename, *toplevel_lineno, + "syntax: %s", jl_string_data(jl_exprarg(ex, 0))); + jl_eval_throw(m, jl_exprarg(ex, 0), *toplevel_filename, *toplevel_lineno); } else if (jl_is_symbol(ex)) { JL_GC_POP(); @@ -908,7 +917,8 @@ jl_value_t *jl_toplevel_eval_flex(jl_module_t *JL_NONNULL m, jl_value_t *e, int assert(head == jl_thunk_sym); thk = (jl_code_info_t*)jl_exprarg(ex, 0); if (!jl_is_code_info(thk) || !jl_typetagis(thk->code, jl_array_any_type)) { - jl_eval_errorf(m, "malformed \"thunk\" statement"); + jl_eval_errorf(m, *toplevel_filename, *toplevel_lineno, + "malformed \"thunk\" statement"); } body_attributes((jl_array_t*)thk->code, &has_ccall, &has_defs, &has_loops, &has_opaque, &forced_compile); @@ -949,7 +959,9 @@ jl_value_t *jl_toplevel_eval_flex(jl_module_t *JL_NONNULL m, jl_value_t *e, int JL_DLLEXPORT jl_value_t *jl_toplevel_eval(jl_module_t *m, jl_value_t *v) { - return jl_toplevel_eval_flex(m, v, 1, 0); + const char *filename = jl_filename; + int lieno = jl_lineno; + return jl_toplevel_eval_flex(m, v, 1, 0, &filename, &lieno); } // Check module `m` is open for `eval/include`, or throw an error. @@ -1049,7 +1061,8 @@ static jl_value_t *jl_parse_eval_all(jl_module_t *module, jl_value_t *text, size_t last_age = ct->world_age; int lineno = 0; jl_lineno = 0; - jl_filename = jl_string_data(filename); + const char *filename_str = jl_string_data(filename); + jl_filename = filename_str; int err = 0; JL_TRY { @@ -1064,7 +1077,7 @@ static jl_value_t *jl_parse_eval_all(jl_module_t *module, jl_value_t *text, expression = jl_expand_with_loc_warn(expression, module, jl_string_data(filename), lineno); ct->world_age = jl_atomic_load_acquire(&jl_world_counter); - result = jl_toplevel_eval_flex(module, expression, 1, 1); + result = jl_toplevel_eval_flex(module, expression, 1, 1, &filename_str, &lineno); } } JL_CATCH { diff --git a/test/backtrace.jl b/test/backtrace.jl index 50a50100488c4..aa10720316a5d 100644 --- a/test/backtrace.jl +++ b/test/backtrace.jl @@ -202,6 +202,15 @@ let trace = try end @test trace[1].func === Symbol("top-level scope") end +let trace = try + eval(Expr(:toplevel, LineNumberNode(3, :a_filename), Expr(:error, 1))) + catch + stacktrace(catch_backtrace()) + end + @test trace[1].func === Symbol("top-level scope") + @test trace[1].file === :a_filename + @test trace[1].line == 3 +end let trace = try include_string(@__MODULE__, """