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.

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

Use TracePoint.allow_reentry to support TracePoint events while eval #847

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

marshall-lee
Copy link

@marshall-lee marshall-lee commented Oct 11, 2023

  • Update Ruby and gems versions (needed to run a new test case using TracePoint API on new Rubies)

  • Undefine alloc funcs of Context and ThreadsTable. This is to suppress a warning of this kind on modern rubies:

    warning: undefining the allocator of T_DATA class Byebug::ThreadsTable
    warning: undefining the allocator of T_DATA class Byebug::Context
    
  • Use TracePoint.allow_reentry to support others' TracePoint events while evaling.

  • Temporarily shut Byebug's TracePoint event handlers in the thread that issued allowing_other_threads { ... } to avoid infinite recursion in event handlers. CTX_FL_IGNORE flag was useful here.

  • Add a test case using TracePoint.new(:class) hook inspired by @fxn's comment Incompatibility with Zeitwerk #564 (comment)

Fixes #564

This is to suppress a warning of the kind on modern rubies:

  warning: undefining the allocator of T_DATA class Byebug::ThreadsTable
  warning: undefining the allocator of T_DATA class Byebug::Context
Comment on lines 370 to 386
{
case HIT_COND_NONE:
return 1;
case HIT_COND_GE:
{
case HIT_COND_GE: {
if (breakpoint->hit_count >= breakpoint->hit_value)
return 1;
break;
}
case HIT_COND_EQ:
{
case HIT_COND_EQ: {
if (breakpoint->hit_count == breakpoint->hit_value)
return 1;
break;
}
case HIT_COND_MOD:
{
case HIT_COND_MOD: {
if (breakpoint->hit_count % breakpoint->hit_value == 0)
return 1;
break;
Copy link
Author

Choose a reason for hiding this comment

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

This is not me, it's clang-format.

@@ -658,6 +658,7 @@ void
Init_byebug_context(VALUE mByebug)
{
cContext = rb_define_class_under(mByebug, "Context", rb_cObject);
rb_undef_alloc_func(cContext);
Copy link
Author

Choose a reason for hiding this comment

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

Wasn't really needed but the warning is annoying.

@d4rky-pl
Copy link

I have checked this PR in our codebase on some simple examples where Zeitwerk was previously failing and it seems to be working fine.

@deivid-rodriguez I know this gem has been essentially unmaintained since 2021 but is there any chance you could take a look? 🙏

@douglas
Copy link

douglas commented Dec 20, 2023

Hello @deivid-rodriguez ! Just to echo @d4rky-pl comment above, could you take a look at this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incompatibility with Zeitwerk
3 participants