Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 3 commits into from May 5, 2021
Merged

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

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3cc464f
fix(ruby): Fix various exceptions in Ruby on 64-bit Windows.
dazuma May 5, 2021
3b7e106
Removed extraneous code, and used VALUE instead of intptr_t
dazuma May 5, 2021
f3fec6f
Call the C function for new object instance rather than evaling a Rub…
dazuma May 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 10 additions & 2 deletions ruby/ext/google/protobuf_c/protobuf.c
View file
Open in desktop
Expand Up @@ -238,8 +238,16 @@ void Arena_register(VALUE module) {
// We use WeakMap for the cache. For Ruby <2.7 we also need a secondary Hash
// to store WeakMap keys because Ruby <2.7 WeakMap doesn't allow non-finalizable
// keys.

#if RUBY_API_VERSION_CODE >= 20700
//
// We also need the secondary Hash if sizeof(long) < sizeof(VALUE), because this
// means it may not be possible to fit a pointer into a Fixnum. Keys are
// pointers, and if they fit into a Fixnum, Ruby doesn't collect them, but if
// they overflow and require allocating a Bignum, they could get collected
// prematurely, thus removing the cache entry. This happens on 64-bit Windows,
// on which pointers are 64 bits but longs are 32 bits. In this case, we enable
// the secondary Hash to hold the keys and prevent them from being collected.

#if RUBY_API_VERSION_CODE >= 20700 && SIZEOF_LONG >= SIZEOF_VALUE
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@dazuma dazuma May 5, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

#define USE_SECONDARY_MAP 0
#else
#define USE_SECONDARY_MAP 1
Expand Down