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

Allow olly to attach to an external process #45

Closed
wants to merge 3 commits into from

Conversation

eutro
Copy link
Contributor

@eutro eutro commented Apr 19, 2024

This PR allows olly to attach to an external process by directory and PID. That is, it implements #41.

I understand that @tmcgilchrist mentioned here that he has already started working on an implementation, I hope I am not stepping on toes with this.


The primary non-obvious difference between this and the existing code in launch.ml is that Unix.wait cannot be used to determine if the process is still alive (you can only wait for children), and so Unix.kill pid 0 is used instead. According to the POSIX specification of kill:

If sig is 0 (the null signal), error checking is performed but no signal is actually sent. The null signal can be used to check the validity of pid.

and

The kill() function shall fail if:
[EINVAL]
The value of the sig argument is an invalid or unsupported signal number.
[EPERM]
The process does not have permission to send the signal to any receiving process.
[ESRCH]
No process or process group can be found corresponding to that specified by pid.

So

let alive () =
  try Unix.kill pid 0; true
  with Unix.Unix_error (Unix.ESRCH, _, _) -> false

should work portably on Unix.

Olly currently already doesn't work on Windows due to how Unix handles PIDs (badly). I leave figuring out what to do with that for a later PR.


The second major change is how to actually handle this at the command line. Ideally, as mentioned here, we could do something like olly gc-stats --attach pid, while still allowing olly gc-stats /path/to/program.

Cmdliner has no clean way to express this, so I just specify --attach as a normal option and fail (still with a command line parsing error) if none/both are specified.

I also made EXECUTABLE be a sequence of arguments rather than just one space-split one, this makes expressing the above slightly easier. It is also in-line with existing tools like perf record or gdb --args, and allows olly to pass arguments (and execute programs) containing spaces. This part is technically backwards-incompatible, if that's a problem.

- The command line changes are nontrivial in that they cannot be
expressed with usual `Cmdliner` combinators. Attaching is done with an
`--attach` option that is incompatible with the normal `EXECUTABLE`
arguments

- `EXECUTABLE` is now a sequence of arguments, rather than a single
space-separated one. This makes the above mutual exclusion slightly
easier to represent, and is more in-line with existing tools (`perf`,
`gdb --args`).

- `Unix.kill pid 0` is used to check if the process is alive, (since
`wait` can only be used for child processes), this is valid and
intended usage according to the POSIX specification.
@eutro eutro marked this pull request as draft April 19, 2024 21:42
@tmcgilchrist tmcgilchrist mentioned this pull request Aug 8, 2024
@tmcgilchrist
Copy link
Collaborator

@eutro thank you for making this work. I've added minor changes to get CI working and added 5.2 builds.
Merging this work under #50

tmcgilchrist added a commit that referenced this pull request Aug 8, 2024
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