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

enable use of ocamldebug #811

Open
wants to merge 1 commit into
base: sail2
Choose a base branch
from
Open

Conversation

lexbailey
Copy link
Contributor

This introduces a few changes:

  1. the dune config is changed to generate bytecode objects for all modules (in addition to the regular native objects)
  2. an option is added that makes sail load bytecode plugins instead of native plugins (ocamldebug requires them to be bytecode)
  3. adds a wrapper called debug_sail which runs ocamldebug with the right options and environment to make it work

Some things that are worth detailed review:
I had to change Dynlink.loadfile_private to Dynlink.loadfile. I'm not entirely sure the consequences of this, but it didn't work with the _private version. Is this expected? (scr/bin/sail.ml:151)

In src/bin/sail.ml:588 there is some special-case parsing of command line arguments, because the full parsing normally happens after the plugins are loaded, but this particular option needs parsing before and plugins can load. Is there a better way to do this?

I'm not very familiar with dune, so I'm not sure if the changes I've made to the config are best-practice.

@lexbailey
Copy link
Contributor Author

ah, the CI is unhappy 😅 , it's late now, so I'll fix that tomorrow

Copy link

github-actions bot commented Dec 4, 2024

Test Results

   12 files  ±0     24 suites  ±0   0s ⏱️ ±0s
  727 tests ±0    727 ✅ ±0  0 💤 ±0  0 ❌ ±0 
2 447 runs  ±0  2 446 ✅ ±0  1 💤 ±0  0 ❌ ±0 

Results for commit 3c5e993. ± Comparison against base commit 9fd690d.

♻️ This comment has been updated with latest results.

@Alasdair
Copy link
Collaborator

Alasdair commented Dec 4, 2024

I am wondering if there is any way to force dune to generate bytecode if you do (modes (best plugin)), which would default to native but also allow compiling on architectures ocamlopt doesn't support.

I think you could then do something like

(files (glob_files plugin.cm{a,xs}))

in the install stanza, which would install whatever is generated.

I'm not sure why loadfile_private shouldn't work. There is also a function Dynlink.adapt_filename which is intended to automatically adapt library names between cma and cmxs. You can also use Sys.backend_type to identify whether we are in native or bytecode mode at runtime without a flag.

@lexbailey
Copy link
Contributor Author

Aha, yeah that's a much neater solution. I have switched to using Sys.backend_type for detecting btyecode mode.
I quite like the adapt_filename function, but because sail is enumerating all the plugins in a directory, I think it would make the code much less obvious.

I've pushed these changes to the PR just to get the tests running to see if that fixes any of the errors introduced. I'll look at the dune docs while I wait for that to run, to see if there's a way to choose between bytecode and native

@lexbailey lexbailey force-pushed the sail2 branch 2 times, most recently from a94eabb to 0978e43 Compare December 5, 2024 13:19
@lexbailey
Copy link
Contributor Author

looks like loadfile_private is required to make native plugins work, but loadfile is required for the debugger to work. I'm not sure why, but I've made it choose between them based on the backend and now the tests pass.

I couldn't find a way to make dune build native by default and have an option to use bytecode, so I've left the dune config as-is. There appears to be no significant impact on build time, so it's probably easiest just to leave it like that?

This is probably ready for another look.

This introduces a few changes:
1. the dune config is changed to generate bytecode objects for all modules
2. sail can now load bytecode plugins, and will do so when the main sail program is running from bytecode
3. adds a wrapper called debug_sail which runs ocamldebug with the right environment
@Alasdair
Copy link
Collaborator

I think this looks good. I'll probably give it a try myself then merge it tomorrow.

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.

2 participants