Bug #9226

Getting method `inspect' called on unexpected T_NODE object (0x000000025ddea8 flags=0x109089c klass=0x0) (NotImplementedError) from Hash#inspect

Added by Myron Marston about 1 year ago. Updated about 1 year ago.

[ruby-core:58948]
Status:Closed
Priority:High
Assignee:Koichi Sasada
ruby -v:2.1.0.preview2 Backport:1.9.3: UNKNOWN, 2.0.0: UNKNOWN

Description

We're trying to get a green RSpec build against ruby 2.1.0.preview2, and we're getting this very odd failure on travis:

https://travis-ci.org/rspec/rspec-core/jobs/15066502#L122

The line where it's failing is here:

https://github.com/rspec/rspec-core/blob/2e77a83d92eb1e661398f00359fc784da019401a/lib/rspec/core/filter_manager.rb#L75

It's calling inspect on a Hash and blowing up with that confusing error. The hash that's causing this failure (if that helps) is here:

https://github.com/rspec/rspec-core/blob/2e77a83d92eb1e661398f00359fc784da019401a/spec/spec_helper.rb#L149-L158

It's the { :ruby => lambda { } } hash being passed to filter_run_excluding.

Associated revisions

Revision 44060
Added by Aman Gupta about 1 year ago

hash.c: fix WB miss issue in Hash#replace

  • hash.c (rb_hash_replace): add a write barrier to fix GC mark miss on hashes using Hash#replace [Bug #9226]

Revision 44060
Added by Aman Gupta about 1 year ago

hash.c: fix WB miss issue in Hash#replace

  • hash.c (rb_hash_replace): add a write barrier to fix GC mark miss on hashes using Hash#replace [Bug #9226]

Revision 44109
Added by Aman Gupta about 1 year ago

gc.c: build complete object graph for RGENGC_CHECK_MODE

* gc.c (reflist_add): return 0 if reference already exists
* gc.c (allrefs_add): return 1 on newly added references
* gc.c (allrefs_i): follow references to construct complete object
  graph. before this patch, RGENGC_CHECK could fail to verify some WB
  miss issues. [Bug #9226] 

Revision 44109
Added by Aman Gupta about 1 year ago

gc.c: build complete object graph for RGENGC_CHECK_MODE

* gc.c (reflist_add): return 0 if reference already exists
* gc.c (allrefs_add): return 1 on newly added references
* gc.c (allrefs_i): follow references to construct complete object
  graph. before this patch, RGENGC_CHECK could fail to verify some WB
  miss issues. [Bug #9226] 

History

#1 Updated by Aman Gupta about 1 year ago

I was able to reproduce this on trunk with the following patch to rspec:

--- a/lib/rspec/core/command_line.rb
+++ b/lib/rspec/core/command_line.rb
@@ -20,6 +20,7 @@ module RSpec
@configuration.output_stream = out if @configuration.output_stream == $stdout
@options.configure(@configuration)
@configuration.load_spec_files
+ GC.stress=true
@world.announce_filters

     @configuration.reporter.report(@world.example_count) do |reporter|

With the patch, you can see different results during script/rspec_with_simplecov spec -b --format progress boot due to object re-use:

exclude {:ruby=>#Proc:./spec/spec_helper.rb:149}
exclude {:ruby=>RSpec::ExampleGroups::RSpecCoreConfiguration::SeedUsed}
exclude {:ruby=>#RubyVM::Env:0x007ff5ec199198}

I noticed that RSpec::Core::FilterManager was using Hash#merge, and after some debugging I confirmed that the following patch fixes this issue on trunk:

--- a/hash.c
+++ b/hash.c
@@ -1429,6 +1429,7 @@ rb_hash_replace(VALUE hash, VALUE hash2)
st_table *old_table = RHASH(hash)->ntbl;
if (old_table) st_free_table(old_table);
RHASH(hash)->ntbl = st_copy(table2);
+ OBJ_WB_UNPROTECT(hash);
}

 return hash;

#2 Updated by Charlie Somerville about 1 year ago

Instead of shading the hash, we could probably do something like this:

diff --git hash.c hash.c
index a52e02f..e7a505e 100644
--- hash.c
+++ hash.c
@@ -1414,22 +1414,9 @@ rb_hash_replace(VALUE hash, VALUE hash2)

 table2 = RHASH(hash2)->ntbl;
  • if (RHASH_EMPTY_P(hash2)) {
  • rb_hash_clear(hash);
  • if (table2) hash_tbl(hash)->type = table2->type;
  • return hash;

- }

  • if (RHASH_ITER_LEV(hash) > 0) {
  • rb_hash_clear(hash);
  • hash_tbl(hash)->type = table2->type;
  • rb_hash_foreach(hash2, replace_i, hash);
  • }
  • else {
  • st_table *old_table = RHASH(hash)->ntbl;
  • if (old_table) st_free_table(old_table);
  • RHASH(hash)->ntbl = st_copy(table2);
  • }
  • rb_hash_clear(hash);
  • hash_tbl(hash)->type = table2->type;
  • rb_hash_foreach(hash2, replace_i, hash);

    return hash;
    }

#3 Updated by Aman Gupta about 1 year ago

  • Category set to core
  • Assignee set to Koichi Sasada
  • Priority changed from Normal to High

#4 Updated by Koichi Sasada about 1 year ago

I like charliesome's approach.
Could you commit it?

#5 Updated by Aman Gupta about 1 year ago

  • Status changed from Open to Closed
  • % Done changed from 0 to 100

This issue was solved with changeset r44060.
Myron, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.


hash.c: fix WB miss issue in Hash#replace

  • hash.c (rb_hash_replace): add a write barrier to fix GC mark miss on hashes using Hash#replace [Bug #9226]

#6 Updated by Aman Gupta about 1 year ago

Committed.

I was surprised but RGENGC_CHECK did not find this issue.

#7 Updated by Koichi Sasada about 1 year ago

  • Status changed from Closed to Open
  • % Done changed from 100 to 0

OMG That is another issue....

#8 Updated by Myron Marston about 1 year ago

Wow, thanks for the quick fix :).

#9 Updated by Aman Gupta about 1 year ago

With r44059, I can reproduce this issue with the following ruby code. I confirmed RGENGC_CHECK=2 does not complain.

def create_oldgen_hash
@h1 = {}
GC.start
end

def replace_with_young_hash
h2 = {"a"=>"b"}
@h1.replace(h2)
h2 = nil
end

create_oldgen_hash
replace_with_young_hash
GC.start(full_mark:false,immediate_sweep:false)
p @h1

#10 Updated by Aman Gupta about 1 year ago

  • Status changed from Open to Closed
  • % Done changed from 0 to 100

This issue was solved with changeset r44109.
Myron, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.


gc.c: build complete object graph for RGENGC_CHECK_MODE

* gc.c (reflist_add): return 0 if reference already exists
* gc.c (allrefs_add): return 1 on newly added references
* gc.c (allrefs_i): follow references to construct complete object
  graph. before this patch, RGENGC_CHECK could fail to verify some WB
  miss issues. [Bug #9226] 

#11 Updated by Aman Gupta about 1 year ago

After r44109, RGENGC_CHECK_MODE is able to detect the original Hash#replace miss issue:

$ ./miniruby test1.rb
gc_marks_check_i: WB miss 0x7fb33b053bb0 (T_HASH) -> 0x7fb33b033f90 (T_STRING)
test1.rb:15: [BUG] before_marks: GC has problem.

Also available in: Atom PDF