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

Compile iseq_collector.c logic only on CRuby #1039

Merged
merged 4 commits into from
Dec 5, 2023

Conversation

eregon
Copy link
Member

@eregon eregon commented Nov 28, 2023

Description

Compile iseq_collector.c logic only on CRuby

  • iseq_collector.c relies on many CRuby internals, there is no point to compile it on other Rubies.
  • It now fails to compile on TruffleRuby:
  ../../../../ext/debug/iseq_collector.c: In function ‘imemo_type’:
  ../../../../ext/debug/iseq_collector.c:18:13: error: implicit declaration of function ‘RBASIC’ [-Werror=implicit-function-declaration]
    18 |     return (RBASIC(imemo)->flags >> FL_USHIFT) & imemo_mask;
        |            ^~~~~~

See for example this GH action run: https://github.com/ruby/irb/actions/runs/7017641177/job/19091354167?pr=790#step:3:87

I also added a workflow to ensure the extension keeps building fine on TruffleRuby, and cleaned up a bit existing workflows.

@eregon
Copy link
Member Author

eregon commented Nov 28, 2023

Unfortunately there is a rubygems.org issue currently which means stringio 3.1.0 might not be picked up in CI: rubygems/rubygems.org#4247

@eregon eregon force-pushed the iseq-colletor-cruby-only branch from 5bce4d9 to 11a74ec Compare November 28, 2023 12:46
@eregon
Copy link
Member Author

eregon commented Nov 28, 2023

I have pushed a small fix trying to get the gem to load on TruffleRuby, to figure out more CRuby-only code paths.
The error after this PR is:

$ ruby -rdebug -e 'p DEBUGGER__'                                
<internal:core> core/tracepoint.rb:24:in `block in initialize': unknown event: call (ArgumentError)
	from <internal:core> core/tracepoint.rb:19:in `each'
	from <internal:core> core/tracepoint.rb:19:in `initialize'
	from /home/eregon/.rubies/truffleruby-dev/lib/gems/gems/debug-1.8.0/lib/debug/session.rb:1834:in `create_method_added_tracker'
	from /home/eregon/.rubies/truffleruby-dev/lib/gems/gems/debug-1.8.0/lib/debug/session.rb:1854:in `<class:Session>'
	from /home/eregon/.rubies/truffleruby-dev/lib/gems/gems/debug-1.8.0/lib/debug/session.rb:93:in `<module:DEBUGGER__>'
	from /home/eregon/.rubies/truffleruby-dev/lib/gems/gems/debug-1.8.0/lib/debug/session.rb:87:in `<top (required)>'
	from <internal:core> core/kernel.rb:297:in `require_relative'
	from /home/eregon/.rubies/truffleruby-dev/lib/gems/gems/debug-1.8.0/lib/debug.rb:6:in `<top (required)>'
	from <internal:core> core/kernel.rb:234:in `gem_original_require'
	from <internal:/home/eregon/.rubies/truffleruby-dev/lib/mri/rubygems/core_ext/kernel_require.rb>:159:in `require'
	from <internal:core> core/unbound_method.rb:18:in `bind_call'
	from <internal:core> core/kernel.rb:272:in `require'
<internal:core> core/kernel.rb:236:in `gem_original_require': cannot load such file -- debug (LoadError)
	from <internal:/home/eregon/.rubies/truffleruby-dev/lib/mri/rubygems/core_ext/kernel_require.rb>:85:in `require'
	from <internal:core> core/unbound_method.rb:18:in `bind_call'
	from <internal:core> core/kernel.rb:272:in `require'

Which means TruffleRuby needs to implement more TracePoint events.

@eregon eregon force-pushed the iseq-colletor-cruby-only branch from 11a74ec to 4a5f01a Compare November 28, 2023 12:55
@eregon
Copy link
Member Author

eregon commented Nov 28, 2023

The Protocol workflow seems to randomly fail, already before:
https://github.com/ruby/debug/actions/workflows/protocol.yml?query=branch%3Amaster

@eregon eregon force-pushed the iseq-colletor-cruby-only branch from 4a5f01a to 1b538ff Compare November 28, 2023 12:58
@eregon
Copy link
Member Author

eregon commented Nov 28, 2023

Unfortunately there is a rubygems.org issue currently which means stringio 3.1.0 might not be picked up in CI: rubygems/rubygems.org#4247

That's fixed now, and the added truffleruby workflow passes.

ext/debug/extconf.rb Outdated Show resolved Hide resolved
@ko1
Copy link
Collaborator

ko1 commented Nov 29, 2023

BTW now debug.gem doesn't support TruffleRuby and why should we merge it?

@eregon
Copy link
Member Author

eregon commented Nov 29, 2023

BTW now debug.gem doesn't support TruffleRuby and why should we merge it?

Because it breaks irb's CI: https://github.com/ruby/irb/actions/runs/7017641177/job/19091354167?pr=790#step:3:87

@eregon
Copy link
Member Author

eregon commented Nov 29, 2023

And also it's important that Gemfile can just install the debug gem on TruffleRuby too.

@eregon
Copy link
Member Author

eregon commented Nov 29, 2023

@ko1 Let me give you some context.

  • gem install debug works on TruffleRuby 23.1.1
  • But it does not work on truffleruby master, because truffleruby no longer (and basically cannot) support RBASIC(obj)->flags since it changed its C extension implementation: Running C extensions natively oracle/truffleruby#3118 (comment).
  • C extension should not use those flags anyway: https://bugs.ruby-lang.org/issues/18059
  • https://github.com/ruby/debug/blob/master/ext/debug/iseq_collector.c is clearly CRuby-specific, it seems good to guard it anyway. It would probably segfault or other bad error on non-CRuby (if it compiled fine).
  • https://github.com/ruby/debug/blob/master/ext/debug/debug.c OTOH is more portable and in fact TruffleRuby implements rb_debug_inspector_open().
  • It's important the debug gem builds and gem-installs fine on TruffleRuby because it might be part of a Gemfile, or a gem dependency and also debug is a bundled gem since 3.1, so it is also built and shipped as part of truffleruby.
  • require 'debug' fails (and on 23.1.1 too), that's fine for now, the debug gem does not support truffleruby yet. And in fact the next thing that breaks is TruffleRuby does not implement :call TracePoint yet (so up to TruffleRuby to implement that). + require 'debug' starts the debugger so it's only required when running the debugger not when loading the application.
  • Also it means this code is now tested in CI.

* iseq_collector.c relies on many CRuby internals, there is no point to compile it on other Rubies.
* It now fails to compile on TruffleRuby:
  ../../../../ext/debug/iseq_collector.c: In function ‘imemo_type’:
  ../../../../ext/debug/iseq_collector.c:18:13: error: implicit declaration of function ‘RBASIC’ [-Werror=implicit-function-declaration]
    18 |     return (RBASIC(imemo)->flags >> FL_USHIFT) & imemo_mask;
        |            ^~~~~~
@eregon
Copy link
Member Author

eregon commented Dec 5, 2023

@ko1 Could you take another look at this PR?
I don't think it's controversial to compile CRuby-specific code only on CRuby, is it?

@ko1
Copy link
Collaborator

ko1 commented Dec 5, 2023

Ok I'll merge it.

Note that, this is repeating, I don't provide TruffleRuby support yet and I'll remove .github/workflows/truffleruby.yml if new feature breaks the test and I don't have any idea to fix in future with informing you.

@ko1 ko1 merged commit f1042b7 into ruby:master Dec 5, 2023
21 checks passed
@eregon
Copy link
Member Author

eregon commented Dec 6, 2023

Thank you. Yes, that's fine.
I'm happy to help if you @-me on GitHub if there is any issue, but yes feel free to disable (e.g. if: false) or remove that workflow if not.

xjunior referenced this pull request in powerhome/audiences Dec 11, 2023
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [debug](https://togithub.com/ruby/debug) | `1.8.0` -> `1.9.0` |
[![age](https://developer.mend.io/api/mc/badges/age/rubygems/debug/1.9.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/rubygems/debug/1.9.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/rubygems/debug/1.8.0/1.9.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/rubygems/debug/1.8.0/1.9.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>ruby/debug (debug)</summary>

### [`v1.9.0`](https://togithub.com/ruby/debug/releases/tag/v1.9.0)

[Compare
Source](https://togithub.com/ruby/debug/compare/v1.8.0...v1.9.0)

#### What's Changed

-   Configuration
- `session_name` config by [@&#8203;ko1](https://togithub.com/ko1) in
[https://github.com/ruby/debug/pull/1036](https://togithub.com/ruby/debug/pull/1036)
-   CLI
- Require Reline 0.3.8+ to avoid frozen issue by
[@&#8203;st0012](https://togithub.com/st0012) in
[https://github.com/ruby/debug/pull/1020](https://togithub.com/ruby/debug/pull/1020)
- Fix prompt list size and colorized code line size to match input line
size passed to Reline by [@&#8203;tompng](https://togithub.com/tompng)
in
[https://github.com/ruby/debug/pull/1010](https://togithub.com/ruby/debug/pull/1010)
- Fix broken command history when executing debugger on irb by
[@&#8203;takatea](https://togithub.com/takatea) in
[https://github.com/ruby/debug/pull/997](https://togithub.com/ruby/debug/pull/997)
- Drop patch for Reline 0.2.7 by
[@&#8203;st0012](https://togithub.com/st0012) in
[https://github.com/ruby/debug/pull/1022](https://togithub.com/ruby/debug/pull/1022)
- Support IRB console by [@&#8203;st0012](https://togithub.com/st0012)
in
[https://github.com/ruby/debug/pull/1024](https://togithub.com/ruby/debug/pull/1024)
-   Remote
- Allow TracePoint reentry during DAP's evaluation by
[@&#8203;st0012](https://togithub.com/st0012) in
[https://github.com/ruby/debug/pull/1026](https://togithub.com/ruby/debug/pull/1026)
- CDP: Add debuggerId field in the RETURN OBJECT of "Debugger.enable" by
[@&#8203;ono-max](https://togithub.com/ono-max) in
[https://github.com/ruby/debug/pull/1028](https://togithub.com/ruby/debug/pull/1028)
- CDP: disable JavaScript engine based autocompletion by
[@&#8203;ono-max](https://togithub.com/ono-max) in
[https://github.com/ruby/debug/pull/1029](https://togithub.com/ruby/debug/pull/1029)
- Do not use HEAD request if 1 port by
[@&#8203;ko1](https://togithub.com/ko1) in
[https://github.com/ruby/debug/pull/1035](https://togithub.com/ruby/debug/pull/1035)
- Show session_name on connection by
[@&#8203;ko1](https://togithub.com/ko1) in
[https://github.com/ruby/debug/pull/1037](https://togithub.com/ruby/debug/pull/1037)
-   Internal
- Stop assuming Integer#times is written in C by
[@&#8203;k0kubun](https://togithub.com/k0kubun) in
[https://github.com/ruby/debug/pull/1015](https://togithub.com/ruby/debug/pull/1015)
- Disable cloned breakpoints trace point events by
[@&#8203;vinistock](https://togithub.com/vinistock) in
[https://github.com/ruby/debug/pull/1008](https://togithub.com/ruby/debug/pull/1008)
- Unfreeze threads for some object-evaluating commands by
[@&#8203;st0012](https://togithub.com/st0012) in
[https://github.com/ruby/debug/pull/1030](https://togithub.com/ruby/debug/pull/1030)
- Prevent backtrace from hanging if objects in the backtrace use Thread
in inspect by [@&#8203;vinistock](https://togithub.com/vinistock) in
[https://github.com/ruby/debug/pull/1038](https://togithub.com/ruby/debug/pull/1038)
- Compile iseq_collector.c logic only on CRuby by
[@&#8203;eregon](https://togithub.com/eregon) in
[https://github.com/ruby/debug/pull/1039](https://togithub.com/ruby/debug/pull/1039)
- Fix compatibility with Fiber Scheduler. by
[@&#8203;ioquatix](https://togithub.com/ioquatix) in
[https://github.com/ruby/debug/pull/987](https://togithub.com/ruby/debug/pull/987)
- Do not make a Fiber for commands by
[@&#8203;ko1](https://togithub.com/ko1) in
[https://github.com/ruby/debug/pull/1044](https://togithub.com/ruby/debug/pull/1044)
- support Ruby 3.3 by [@&#8203;ko1](https://togithub.com/ko1) in
[https://github.com/ruby/debug/pull/1045](https://togithub.com/ruby/debug/pull/1045)
-   Misc/Doc
- Fix ruby warnings by [@&#8203;y-yagi](https://togithub.com/y-yagi) in
[https://github.com/ruby/debug/pull/993](https://togithub.com/ruby/debug/pull/993)
- Fix a typo by [@&#8203;makenowjust](https://togithub.com/makenowjust)
in
[https://github.com/ruby/debug/pull/988](https://togithub.com/ruby/debug/pull/988)
- Update `TrapInterceptor` to avoid assigning to an unused variable by
[@&#8203;DavidZivk](https://togithub.com/DavidZivk) in
[https://github.com/ruby/debug/pull/985](https://togithub.com/ruby/debug/pull/985)
- remove debug print by [@&#8203;ko1](https://togithub.com/ko1) in
[https://github.com/ruby/debug/pull/1043](https://togithub.com/ruby/debug/pull/1043)
- Minor punctuation and grammar fixes by
[@&#8203;ahangarha](https://togithub.com/ahangarha) in
[https://github.com/ruby/debug/pull/1041](https://togithub.com/ruby/debug/pull/1041)
-   Tests
- Bump actions/checkout from 3 to 4 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[https://github.com/ruby/debug/pull/1014](https://togithub.com/ruby/debug/pull/1014)

#### New Contributors

- [@&#8203;dependabot](https://togithub.com/dependabot) made their first
contribution in
[https://github.com/ruby/debug/pull/1014](https://togithub.com/ruby/debug/pull/1014)
- [@&#8203;tompng](https://togithub.com/tompng) made their first
contribution in
[https://github.com/ruby/debug/pull/1010](https://togithub.com/ruby/debug/pull/1010)
- [@&#8203;takatea](https://togithub.com/takatea) made their first
contribution in
[https://github.com/ruby/debug/pull/997](https://togithub.com/ruby/debug/pull/997)
- [@&#8203;y-yagi](https://togithub.com/y-yagi) made their first
contribution in
[https://github.com/ruby/debug/pull/993](https://togithub.com/ruby/debug/pull/993)
- [@&#8203;DavidZivk](https://togithub.com/DavidZivk) made their first
contribution in
[https://github.com/ruby/debug/pull/985](https://togithub.com/ruby/debug/pull/985)
- [@&#8203;eregon](https://togithub.com/eregon) made their first
contribution in
[https://github.com/ruby/debug/pull/1039](https://togithub.com/ruby/debug/pull/1039)
- [@&#8203;ahangarha](https://togithub.com/ahangarha) made their first
contribution in
[https://github.com/ruby/debug/pull/1041](https://togithub.com/ruby/debug/pull/1041)
- [@&#8203;ioquatix](https://togithub.com/ioquatix) made their first
contribution in
[https://github.com/ruby/debug/pull/987](https://togithub.com/ruby/debug/pull/987)

**Full Changelog**:
ruby/debug@v1.8.0...v1.9.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/powerhome/audiences).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy44Ny4yIiwidXBkYXRlZEluVmVyIjoiMzcuODcuMiIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants