From 257f78fb671151f1db06dcd8e35cf4cc736f735e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20Barri=C3=A9?= Date: Mon, 14 Oct 2024 11:28:59 +0200 Subject: [PATCH] Show where mutated chilled strings were allocated [Feature #20205] The warning now suggests running with --debug-frozen-string-literal: ``` test.rb:3: warning: literal string will be frozen in the future (run with --debug-frozen-string-literal for more information) ``` When using --debug-frozen-string-literal, the location where the string was created is shown: ``` test.rb:3: warning: literal string will be frozen in the future test.rb:1: info: the string was created here ``` When resurrecting strings and debug mode is not enabled, the overhead is a simple FL_TEST_RAW. When mutating chilled strings and deprecation warnings are not enabled, the overhead is a simple warning category enabled check. Co-authored-by: Jean Boussier Co-authored-by: Nobuyoshi Nakada Co-authored-by: Jean Boussier --- bootstraptest/test_yjit.rb | 4 +- compile.c | 23 ++-- error.c | 57 ++++++++-- internal/string.h | 6 +- prism_compile.c | 9 +- spec/ruby/command_line/fixtures/debug_info.rb | 1 - spec/ruby/command_line/frozen_strings_spec.rb | 13 ++- string.c | 103 +++++++++++------- test/ruby/test_rubyoptions.rb | 5 +- 9 files changed, 144 insertions(+), 77 deletions(-) diff --git a/bootstraptest/test_yjit.rb b/bootstraptest/test_yjit.rb index 545243a0e4c6b4..d647992986f14c 100644 --- a/bootstraptest/test_yjit.rb +++ b/bootstraptest/test_yjit.rb @@ -4726,7 +4726,7 @@ def test(klass, args) } # Chilled string setivar trigger warning -assert_equal 'literal string will be frozen in the future', %q{ +assert_match(/literal string will be frozen in the future/, %q{ Warning[:deprecated] = true $VERBOSE = true $warning = "no-warning" @@ -4754,7 +4754,7 @@ def setivar!(str) setivar!("chilled") # Emit warning $warning -} +}) # arity=-2 cfuncs assert_equal '["", "1/2", [0, [:ok, 1]]]', %q{ diff --git a/compile.c b/compile.c index 5bf496ed596d75..b89d97bdf8ad90 100644 --- a/compile.c +++ b/compile.c @@ -4873,10 +4873,8 @@ static_literal_value(const NODE *node, rb_iseq_t *iseq) case NODE_FILE: case NODE_STR: if (ISEQ_COMPILE_DATA(iseq)->option->debug_frozen_string_literal || RTEST(ruby_debug)) { - VALUE debug_info = rb_ary_new_from_args(2, rb_iseq_path(iseq), INT2FIX((int)nd_line(node))); - VALUE lit = rb_str_dup(get_string_value(node)); - rb_ivar_set(lit, id_debug_created_info, rb_ary_freeze(debug_info)); - return rb_str_freeze(lit); + VALUE lit = get_string_value(node); + return rb_str_with_debug_created_info(lit, rb_iseq_path(iseq), (int)nd_line(node)); } else { return get_string_value(node); @@ -10927,28 +10925,25 @@ iseq_compile_each0(rb_iseq_t *iseq, LINK_ANCHOR *const ret, const NODE *const no debugp_param("nd_lit", get_string_value(node)); if (!popped) { VALUE lit = get_string_value(node); - switch (ISEQ_COMPILE_DATA(iseq)->option->frozen_string_literal) { + const rb_compile_option_t *option = ISEQ_COMPILE_DATA(iseq)->option; + if ((option->debug_frozen_string_literal || RTEST(ruby_debug)) && + option->frozen_string_literal != ISEQ_FROZEN_STRING_LITERAL_DISABLED) { + lit = rb_str_with_debug_created_info(lit, rb_iseq_path(iseq), line); + } + switch (option->frozen_string_literal) { case ISEQ_FROZEN_STRING_LITERAL_UNSET: ADD_INSN1(ret, node, putchilledstring, lit); - RB_OBJ_WRITTEN(iseq, Qundef, lit); break; case ISEQ_FROZEN_STRING_LITERAL_DISABLED: ADD_INSN1(ret, node, putstring, lit); - RB_OBJ_WRITTEN(iseq, Qundef, lit); break; case ISEQ_FROZEN_STRING_LITERAL_ENABLED: - if (ISEQ_COMPILE_DATA(iseq)->option->debug_frozen_string_literal || RTEST(ruby_debug)) { - VALUE debug_info = rb_ary_new_from_args(2, rb_iseq_path(iseq), INT2FIX(line)); - lit = rb_str_dup(lit); - rb_ivar_set(lit, id_debug_created_info, rb_ary_freeze(debug_info)); - lit = rb_str_freeze(lit); - } ADD_INSN1(ret, node, putobject, lit); - RB_OBJ_WRITTEN(iseq, Qundef, lit); break; default: rb_bug("invalid frozen_string_literal"); } + RB_OBJ_WRITTEN(iseq, Qundef, lit); } break; } diff --git a/error.c b/error.c index d266f8fd9b2245..b6ed5d22d13ca2 100644 --- a/error.c +++ b/error.c @@ -3973,13 +3973,25 @@ inspect_frozen_obj(VALUE obj, VALUE mesg, int recur) return mesg; } +static VALUE +get_created_info(VALUE obj, int *pline) +{ + VALUE info = rb_attr_get(obj, id_debug_created_info); + + if (NIL_P(info)) return Qnil; + + VALUE path = rb_ary_entry(info, 0); + VALUE line = rb_ary_entry(info, 1); + if (NIL_P(path)) return Qnil; + *pline = NUM2INT(line); + return StringValue(path); +} + void rb_error_frozen_object(VALUE frozen_obj) { rb_yjit_lazy_push_frame(GET_EC()->cfp->pc); - VALUE debug_info; - const ID created_info = id_debug_created_info; VALUE mesg = rb_sprintf("can't modify frozen %"PRIsVALUE": ", CLASS_OF(frozen_obj)); VALUE exc = rb_exc_new_str(rb_eFrozenError, mesg); @@ -3987,15 +3999,46 @@ rb_error_frozen_object(VALUE frozen_obj) rb_ivar_set(exc, id_recv, frozen_obj); rb_exec_recursive(inspect_frozen_obj, frozen_obj, mesg); - if (!NIL_P(debug_info = rb_attr_get(frozen_obj, created_info))) { - VALUE path = rb_ary_entry(debug_info, 0); - VALUE line = rb_ary_entry(debug_info, 1); - - rb_str_catf(mesg, ", created at %"PRIsVALUE":%"PRIsVALUE, path, line); + int created_line; + VALUE created_path = get_created_info(frozen_obj, &created_line); + if (!NIL_P(created_path)) { + rb_str_catf(mesg, ", created at %"PRIsVALUE":%d", created_path, created_line); } rb_exc_raise(exc); } +void +rb_warn_unchilled(VALUE obj) +{ + rb_warning_category_t category = RB_WARN_CATEGORY_DEPRECATED; + if (!NIL_P(ruby_verbose) && rb_warning_category_enabled_p(category)) { + int line; + VALUE file = rb_source_location(&line); + VALUE mesg = NIL_P(file) ? rb_str_new(0, 0) : rb_str_dup(file); + + if (!NIL_P(file)) { + if (line) rb_str_catf(mesg, ":%d", line); + rb_str_cat2(mesg, ": "); + } + rb_str_cat2(mesg, "warning: literal string will be frozen in the future"); + + VALUE str = obj; + if (STR_SHARED_P(str)) { + str = RSTRING(obj)->as.heap.aux.shared; + } + VALUE created = get_created_info(str, &line); + if (NIL_P(created)) { + rb_str_cat2(mesg, " (run with --debug-frozen-string-literal for more information)\n"); + } else { + rb_str_cat2(mesg, "\n"); + rb_str_append(mesg, created); + if (line) rb_str_catf(mesg, ":%d", line); + rb_str_cat2(mesg, ": info: the string was created here\n"); + } + rb_warn_category(mesg, rb_warning_category_to_name(category)); + } +} + #undef rb_check_frozen void rb_check_frozen(VALUE obj) diff --git a/internal/string.h b/internal/string.h index 10879bd1d98b48..81e94ed6b57aee 100644 --- a/internal/string.h +++ b/internal/string.h @@ -57,6 +57,10 @@ VALUE rb_str_upto_each(VALUE, VALUE, int, int (*each)(VALUE, VALUE), VALUE); size_t rb_str_size_as_embedded(VALUE); bool rb_str_reembeddable_p(VALUE); VALUE rb_str_upto_endless_each(VALUE, int (*each)(VALUE, VALUE), VALUE); +VALUE rb_str_with_debug_created_info(VALUE, VALUE, int); + +/* error.c */ +void rb_warn_unchilled(VALUE str); static inline bool STR_EMBED_P(VALUE str); static inline bool STR_SHARED_P(VALUE str); @@ -124,7 +128,7 @@ static inline void CHILLED_STRING_MUTATED(VALUE str) { FL_UNSET_RAW(str, STR_CHILLED); - rb_category_warn(RB_WARN_CATEGORY_DEPRECATED, "literal string will be frozen in the future"); + rb_warn_unchilled(str); } static inline void diff --git a/prism_compile.c b/prism_compile.c index d5b764cd890636..46ffccaaa11a2c 100644 --- a/prism_compile.c +++ b/prism_compile.c @@ -319,10 +319,7 @@ parse_static_literal_string(rb_iseq_t *iseq, const pm_scope_node_t *scope_node, if (ISEQ_COMPILE_DATA(iseq)->option->debug_frozen_string_literal || RTEST(ruby_debug)) { int line_number = pm_node_line_number(scope_node->parser, node); - VALUE debug_info = rb_ary_new_from_args(2, rb_iseq_path(iseq), INT2FIX(line_number)); - value = rb_str_dup(value); - rb_ivar_set(value, id_debug_created_info, rb_ary_freeze(debug_info)); - rb_str_freeze(value); + value = rb_str_with_debug_created_info(value, rb_iseq_path(iseq), line_number); } return value; @@ -726,9 +723,7 @@ static VALUE pm_static_literal_string(rb_iseq_t *iseq, VALUE string, int line_number) { if (ISEQ_COMPILE_DATA(iseq)->option->debug_frozen_string_literal || RTEST(ruby_debug)) { - VALUE debug_info = rb_ary_new_from_args(2, rb_iseq_path(iseq), INT2FIX(line_number)); - rb_ivar_set(string, id_debug_created_info, rb_ary_freeze(debug_info)); - return rb_str_freeze(string); + return rb_str_with_debug_created_info(string, rb_iseq_path(iseq), line_number); } else { return rb_fstring(string); diff --git a/spec/ruby/command_line/fixtures/debug_info.rb b/spec/ruby/command_line/fixtures/debug_info.rb index ee607910c0d8f4..f02b0419208c34 100644 --- a/spec/ruby/command_line/fixtures/debug_info.rb +++ b/spec/ruby/command_line/fixtures/debug_info.rb @@ -1,4 +1,3 @@ -# frozen_string_literal: true a = 'string' b = a c = b diff --git a/spec/ruby/command_line/frozen_strings_spec.rb b/spec/ruby/command_line/frozen_strings_spec.rb index 06889755d254cf..014153e0b4afbe 100644 --- a/spec/ruby/command_line/frozen_strings_spec.rb +++ b/spec/ruby/command_line/frozen_strings_spec.rb @@ -57,9 +57,18 @@ describe "The --debug flag produces" do it "debugging info on attempted frozen string modification" do - error_str = ruby_exe(fixture(__FILE__, 'debug_info.rb'), options: '--debug', args: "2>&1") + error_str = ruby_exe(fixture(__FILE__, 'debug_info.rb'), options: '--enable-frozen-string-literal --debug', args: "2>&1") error_str.should include("can't modify frozen String") error_str.should include("created at") - error_str.should include("command_line/fixtures/debug_info.rb:2") + error_str.should include("command_line/fixtures/debug_info.rb:1") + end + + guard -> { ruby_version_is "3.4" and !"test".frozen? } do + it "debugging info on mutating chilled string" do + error_str = ruby_exe(fixture(__FILE__, 'debug_info.rb'), options: '-w --debug', args: "2>&1") + error_str.should include("literal string will be frozen in the future") + error_str.should include("the string was created here") + error_str.should include("command_line/fixtures/debug_info.rb:1") + end end end diff --git a/string.c b/string.c index e193539f4faa81..b8213ea0850c86 100644 --- a/string.c +++ b/string.c @@ -1811,41 +1811,9 @@ ec_str_alloc_heap(struct rb_execution_context_struct *ec, VALUE klass) } static inline VALUE -str_duplicate_setup(VALUE klass, VALUE str, VALUE dup) +str_duplicate_setup_encoding(VALUE str, VALUE dup, VALUE flags) { - const VALUE flag_mask = - ENC_CODERANGE_MASK | ENCODING_MASK | - FL_FREEZE - ; - VALUE flags = FL_TEST_RAW(str, flag_mask); int encidx = 0; - if (STR_EMBED_P(str)) { - long len = RSTRING_LEN(str); - - RUBY_ASSERT(STR_EMBED_P(dup)); - RUBY_ASSERT(str_embed_capa(dup) >= len + 1); - MEMCPY(RSTRING(dup)->as.embed.ary, RSTRING(str)->as.embed.ary, char, len + 1); - } - else { - VALUE root = str; - if (FL_TEST_RAW(str, STR_SHARED)) { - root = RSTRING(str)->as.heap.aux.shared; - } - else if (UNLIKELY(!(flags & FL_FREEZE))) { - root = str = str_new_frozen(klass, str); - flags = FL_TEST_RAW(str, flag_mask); - } - RUBY_ASSERT(!STR_SHARED_P(root)); - RUBY_ASSERT(RB_OBJ_FROZEN_RAW(root)); - - RSTRING(dup)->as.heap.ptr = RSTRING_PTR(str); - FL_SET(root, STR_SHARED_ROOT); - RB_OBJ_WRITE(dup, &RSTRING(dup)->as.heap.aux.shared, root); - flags |= RSTRING_NOEMBED | STR_SHARED; - } - - STR_SET_LEN(dup, RSTRING_LEN(str)); - if ((flags & ENCODING_MASK) == (ENCODING_INLINE_MAX<= len + 1); + MEMCPY(RSTRING(dup)->as.embed.ary, RSTRING(str)->as.embed.ary, char, len + 1); + STR_SET_LEN(dup, RSTRING_LEN(str)); + return str_duplicate_setup_encoding(str, dup, flags); +} + +static inline VALUE +str_duplicate_setup_heap(VALUE klass, VALUE str, VALUE dup) +{ + VALUE flags = FL_TEST_RAW(str, flag_mask); + VALUE root = str; + if (FL_TEST_RAW(str, STR_SHARED)) { + root = RSTRING(str)->as.heap.aux.shared; + } + else if (UNLIKELY(!(flags & FL_FREEZE))) { + root = str = str_new_frozen(klass, str); + flags = FL_TEST_RAW(str, flag_mask); + } + RUBY_ASSERT(!STR_SHARED_P(root)); + RUBY_ASSERT(RB_OBJ_FROZEN_RAW(root)); + + RSTRING(dup)->as.heap.ptr = RSTRING_PTR(str); + FL_SET(root, STR_SHARED_ROOT); + RB_OBJ_WRITE(dup, &RSTRING(dup)->as.heap.aux.shared, root); + flags |= RSTRING_NOEMBED | STR_SHARED; + + STR_SET_LEN(dup, RSTRING_LEN(str)); + return str_duplicate_setup_encoding(str, dup, flags); +} + +static inline VALUE +str_duplicate_setup(VALUE klass, VALUE str, VALUE dup) { - VALUE dup; if (STR_EMBED_P(str)) { - dup = ec_str_alloc_embed(ec, klass, RSTRING_LEN(str) + TERM_LEN(str)); + return str_duplicate_setup_embed(klass, str, dup); } else { - dup = ec_str_alloc_heap(ec, klass); + return str_duplicate_setup_heap(klass, str, dup); } - - return str_duplicate_setup(klass, str, dup); } static inline VALUE @@ -1912,13 +1916,32 @@ VALUE rb_ec_str_resurrect(struct rb_execution_context_struct *ec, VALUE str, bool chilled) { RUBY_DTRACE_CREATE_HOOK(STRING, RSTRING_LEN(str)); - VALUE new_str = ec_str_duplicate(ec, rb_cString, str); + VALUE new_str, klass = rb_cString; + + if (!(chilled && RTEST(rb_ivar_defined(str, id_debug_created_info))) && STR_EMBED_P(str)) { + new_str = ec_str_alloc_embed(ec, klass, RSTRING_LEN(str) + TERM_LEN(str)); + str_duplicate_setup_embed(klass, str, new_str); + } + else { + new_str = ec_str_alloc_heap(ec, klass); + str_duplicate_setup_heap(klass, str, new_str); + } if (chilled) { STR_CHILL_RAW(new_str); } return new_str; } +VALUE +rb_str_with_debug_created_info(VALUE str, VALUE path, int line) +{ + VALUE debug_info = rb_ary_new_from_args(2, path, INT2FIX(line)); + if (OBJ_FROZEN_RAW(str)) str = rb_str_dup(str); + rb_ivar_set(str, id_debug_created_info, rb_ary_freeze(debug_info)); + STR_CHILL_RAW(str); + return rb_str_freeze(str); +} + /* * * call-seq: diff --git a/test/ruby/test_rubyoptions.rb b/test/ruby/test_rubyoptions.rb index f82861b8ced83b..f9a0d6055b331f 100644 --- a/test/ruby/test_rubyoptions.rb +++ b/test/ruby/test_rubyoptions.rb @@ -1262,9 +1262,8 @@ def test_frozen_string_literal_debug_chilled_strings code = <<~RUBY "foo" << "bar" RUBY - warning = ["-:1: warning: literal string will be frozen in the future"] - assert_in_out_err(["-W:deprecated"], code, [], warning) - assert_in_out_err(["-W:deprecated", "--debug-frozen-string-literal"], code, [], warning) + assert_in_out_err(["-W:deprecated"], code, [], ["-:1: warning: literal string will be frozen in the future (run with --debug-frozen-string-literal for more information)"]) + assert_in_out_err(["-W:deprecated", "--debug-frozen-string-literal"], code, [], ["-:1: warning: literal string will be frozen in the future", "-:1: info: the string was created here"]) assert_in_out_err(["-W:deprecated", "--disable-frozen-string-literal", "--debug-frozen-string-literal"], code, [], []) assert_in_out_err(["-W:deprecated", "--enable-frozen-string-literal", "--debug-frozen-string-literal"], code, [], ["-:1:in '
': can't modify frozen String: \"foo\", created at -:1 (FrozenError)"]) end