fix(ruby): Fix various exceptions in Ruby on 64-bit Windows

[dazuma]

Activates the secondary ObjectCache map on this platform, to prevent weak keys from being garbage collected. This happened on 64-bit Windows because pointers don't necessarily fit in a Fixnum, and were being represented as GC-able Bignums on that platform.

Fixes #8554

/attn @haberman

[haberman]

Our direct concern is that an intptr_t divided by 4 can fit in a fixnum. This seems like a slightly roundabout way of checking that.

Perhaps we could do:

#if RUBY_FIXNUM_MAX >= (INTPTR_MAX / 4)

I also notice there is some extraneous code left in the function below. And we should change to use INT2FIX() so we get a loud error when our check is incorrect. How about we do this:

static VALUE ObjectCache_GetKey(const void* key, bool create) {
  intptr_t key_int = (intptr_t)key;
  PBRUBY_ASSERT((key_int & 3) == 0);
  VALUE ret = INT2FIX(key_int >> 2);
#if USE_SECONDARY_MAP
  ret = SecondaryMap_Get(ret, create);
#endif
  return ret;
}

[dazuma]

Regarding the preprocessor check: I was actually doing this based on value.h which doesn't depend on intptr_t (and INTPTR_MAX) being defined, but conditionally defines VALUE (and its size) differently depending on what's available. Ruby has already defined VALUE as a proxy for a pointer, and SIZEOF_VALUE as the size thereof, and I think we should use the abstraction it provides. In fact, for better compatibility, I might argue that we also should avoid depending on intptr_t, and use VALUE instead:

static VALUE ObjectCache_GetKey(const void* key, bool create) {
  VALUE key_val = (VALUE)key;
  PBRUBY_ASSERT((key_val & 3) == 0);
  VALUE ret = LL2NUM(key_val >> 2);
#if USE_SECONDARY_MAP
  ret = SecondaryMap_Get(ret, create);
#endif
  return ret;
}

Regarding the two extraneous lines, I'll remove them.

Regarding using INT2FIX, I don't think we can do that. The whole point is that on Win64, it may not be a Fixnum, and we have to reinstate the secondary Hash to compensate.

Pushed an update with the above suggestions.

[haberman]

You are right re: INT2FIX. Using VALUE seems reasonable, as it is typedef'd to uintptr_t.

This is the check I should have suggested:

static VALUE ObjectCache_GetKey(const void* key, bool create) {
  VALUE key_val = (VALUE)key;
  PBRUBY_ASSERT((key_val & 3) == 0);
  VALUE ret = LL2NUM(key_val >> 2);
#if USE_SECONDARY_MAP
  ret = SecondaryMap_Get(ret, create);
#endif
  if (!RB_FIXNUM_P(ret)) rb_raise(rb_eRuntimeError, "Key must be fixed!");
  return ret;
}

In other words, after the secondary map step, the key must be a fixnum for the main object cache to be safe.

[dazuma]

I actually don't think we can do that either. In the case where the secondary map is in use, the key is an arbitrary object, not a Fixnum. (We know it's not going to get collected because the secondary map is holding on to it.)

Incidentally, that rb_eval_string("Object.new") should also be changed to something less heavyweight (i.e. not involving parsing Ruby code) since it's part of the inner loop, called every time the cache is accessed. I changed it to a call to rb_class_new_instance.

[haberman]

Ah, you're absolutely right. Thanks for the changes.