Bug #9226

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

Added by Myron Marston over 1 year ago. Updated over 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 over 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 over 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 over 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 over 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 over 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 over 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 over 1 year ago

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

#4 Updated by Koichi Sasada over 1 year ago

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

#5 Updated by Aman Gupta over 1 year ago

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

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 over 1 year ago

Committed.

I was surprised but RGENGC_CHECK did not find this issue.

#7 Updated by Koichi Sasada over 1 year ago

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

OMG That is another issue....

#8 Updated by Myron Marston over 1 year ago

Wow, thanks for the quick fix :).

#9 Updated by Aman Gupta over 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 over 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 over 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