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

Reorganize mrb_callinfo #5272

Merged
merged 5 commits into from Jan 12, 2021
Merged

Reorganize mrb_callinfo #5272

merged 5 commits into from Jan 12, 2021

Conversation

dearblue
Copy link
Contributor

Overview

  • mrb_callinfo::stack will now indicate the current call level.
    Previously, stackent saved the previous stack.
  • mrb_callinfo::pc will now indicate the current call level.
    Previously, this saved the previous one.
    This change reverts commit a0c1e07.
  • mrb_callinfo::argc and mrb_callinfo::acc are represented by int16_t.
  • Represents mrb_callinfo::env and mrb_callinfo::target_class with union.

This is because it is enough to express the range up to (-1..255) or (-3..255).
If there is `env`, `env->c` means `target_class`.
This enhances self-containment.

Previously `mrb_context::stack` had the current call level stack, but now it owns it.
The `mrb_context::stack` field, which is no longer needed, will be removed.
This enhances self-containment.

- Changed the `mrb_callinfo::pc` field to point to itself.
  Previously it indicated the return destination of the previous call level.
  `mrb_callinfo::pc` will now hold the address to its own `proc->body.irep->iseq`.
- Removed `mrb_callinfo::err` field.
  This is because `mrb_callinfo::pc - 1` is semantically the same as the previous `err`.
- The `pc0` and `pc_save` variables in `mrb_vm_exec()` are no longer needed and have been deleted.
- It removes the argument because `cipush()` doesn't need to save the previous `pc`.
…ruby#5261"

This reverts commit a0c1e07.

This is because the `mrb_callinfo::pc` has been reorganized, resulting in over-correction.
@@ -355,7 +355,6 @@ mrb_fiber_yield(mrb_state *mrb, mrb_int len, const mrb_value *a)
if (c->vmexec) {
c->vmexec = FALSE;
mrb->c->ci->acc = CI_ACC_RESUMED;
c->cibase->pc = c->ci->pc;
Copy link
Member

Choose a reason for hiding this comment

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

This line (saving the current execution point to cibase->pc) is an essential part of #5261 fix. Is it really OK to remove this line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it needs to be removed.
By this PR, in the case of child fiber, its cibase->pc already points to the address on cibase->proc->body.irep->iseq.

@matz matz merged commit bec4d30 into mruby:master Jan 12, 2021
@dearblue dearblue deleted the reorganize-ci branch January 12, 2021 14:16
dearblue added a commit to dearblue/mruby that referenced this pull request Jan 16, 2021
When I `#call` the "proc" object created by the `mrb_proc_new_cfunc()` function from Ruby space, the return value did not go into the correct stack position.
This can destroy the calling variable.

This issue is now caused by mruby#5272. sorry.
matz added a commit that referenced this pull request Jan 16, 2021
Fixed stack position of return value; ref #5272
@shuujii
Copy link
Contributor

shuujii commented Jan 17, 2021

It seems that mruby-bin-debugger can't be built.

/mruby/mrbgems/mruby-bin-debugger/tools/mrdb/mrdb.c:529:23: error: no member named 'target_class' in 'mrb_callinfo'
      c = mrb->c->ci->target_class->super;
          ~~~~~~~~~~  ^
1 error generated.

@dearblue
Copy link
Contributor Author

Thank you for your report. I will fix it.

dearblue added a commit to dearblue/mruby that referenced this pull request Jan 17, 2021
This is a missing change in mruby#5272.

This issue was reported by @shuujii.
mruby#5272 (comment)
dearblue added a commit to dearblue/mruby that referenced this pull request Jan 31, 2021
matz added a commit that referenced this pull request Feb 1, 2021
Remove unnecessary `ci0` variables; ref #5272
matz added a commit that referenced this pull request Sep 4, 2021
Otherwise `target_class` can be lost when it differs from `proc`'s
`target_class`, e.g. when called from `instance_eval`.

Also we should not pass `target_class` to `MRB_OBJ_ALLOC` since it
checks instance type from the class, and `target_class` may not have
proper information. ref #5272
matz added a commit that referenced this pull request Jun 22, 2022
Since #5272 target_class kept in a Proc may be NULL. It crashes
`iij/mruby-require` gem for example; close #5725
dearblue added a commit to dearblue/mruby that referenced this pull request Jun 25, 2022
If `ci->u.target_class` was `NULL`, the value of the argument was always ignored.
This problem is caused by mruby#5272.

ref. mruby#5725
dearblue added a commit to dearblue/mruby that referenced this pull request Jul 30, 2022
GC may occur in the `c->stbase = mrb_malloc()` part of the `fiber_init()` function.
The `SIGSEGV` happens because it references the `c->ci->stack` field without checking `c->ci`.
This is caused by mruby#5272.
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.

None yet

3 participants