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

Rakudo Issue 1588: Create getsignals op for MoarVM / JVM / JS #430

Merged
merged 8 commits into from May 30, 2018

Conversation

jstuder-gh
Copy link
Contributor

This op will retrieve the signals available on the host system. Also
removing the signal-related nqp const values as they are no long valid
with this change.

See rakudo/rakudo#1588

@jstuder-gh
Copy link
Contributor Author

Corresponding Rakudo PR: rakudo/rakudo#1601

@jstuder-gh
Copy link
Contributor Author

Relies on PR MoarVM/MoarVM#814

This op will retrieve the signals available on the host system. Also
removing the signal-related nqp const values as they are no long valid
with this change.
Create the get signals op and associated method within the runtime code.
This method queries the kill command to determine the signals available
on the host system (even though JVM doesn't support all of them; they
could be useful in conjunction with NativeCall).
We need some way to identify which signals are unsupported by a given
backend. In the case of the JVM, only SIGINT and SIGKILL are supported.
We can differentiate between supported and unsupported by negating the
signums for unsupported signals. niner++ for the suggestion.
@jstuder-gh
Copy link
Contributor Author

I'm hoping to bring attention back to this PR (and its corresponding Rakudo component) as it's been in the queue for a while and may have been forgotten. However, I still feel that it is useful and solves a real issue.

What this op does is provide a hash of the signals that are available on the system, which ultimately, can be used to populate the Signal enum values. Signals that are not available have a value of 0 and signals that are not supported by the backend being used are negative.

The MoarVM op has been merged, so now this PR and the Rakudo one need merging in order to complete the issue for the that backend. The Travis tests that failed when I opened this PR would succeed now.

Since the JVM implementation of the slice op wasn't without issue, I'm hoping for a review of the JVM implementation of this op from an expert. @usev6, when you get the time could you please take a look and let me know if there is anything I can do better?

@jstuder-gh jstuder-gh requested review from usev6 and lizmat May 25, 2018 01:21
Copy link
Contributor

@lizmat lizmat left a comment

Choose a reason for hiding this comment

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

Can't really judge the code quality on the MoarVM/JVM backends, but it looks sane to me and nice to have.

On the other backends, these three signal values are the only ones
defined for WIN32. For consistency's sake, make sure that this is the
same on the JS backend as well.
Since they are no longer accurate.
Modified the JVM backend's retrieval of signal values. The problem was
that the '/bin/kill -l'  was inconsistent among different
implementations and producing incorrect results in some. For example, on
Fedora 27, '/bin/kill -l' produces:

HUP INT QUIT ILL TRAP ABRT IOT BUS FPE KILL USR1 SEGV USR2 PIPE ALRM
TERM STKFLT CHLD CLD CONT STOP TSTP TTIN TTOU URG XCPU XFSZ VTALRM PROF
WINCH IO POLL PWR UNUSED SYS

Clearly, this method of determining the signum by sequence in the
'/bin/kill -l' output is flawed. This could be accounted for if this
output was consistent across platforms, but it is not.

Using the Bourne shell (sh) kill built-in as that seems to be the most
consistent and portable across all of the POSIX-complient systems tried.
Using the following form to retrieve the name given a signal value:

    sh -c "kill -l $signum"

While this spawns more processes, it allows us to be sure that the
signals names and values match and does not have the same potential for
error that the previous method had.
@jstuder-gh
Copy link
Contributor Author

I've just added a getsignals op for the JS backend as well.

Also, I've updated the JVM implementation to address an issue I noticed recently. See the commit message for e9c9ca7 for more details, but I think this is a positive change. I've also broken up the logic previously within the getsignals method into several methods within a SigProcess subclass, which I also believe to be helpful.

@jstuder-gh jstuder-gh changed the title Rakudo Issue 1588: Create getsignals op for MoarVM / JVM Rakudo Issue 1588: Create getsignals op for MoarVM / JVM / JS May 29, 2018
@jstuder-gh
Copy link
Contributor Author

Also wanted to point out that with the new commits all Travis builds pass. The previous failures were due to the fact that, at the time it was originally submitted, the MoarVM getsignals op did not exist. The MVM PR has since been merged and now these tests pass as a result.

@niner niner merged commit df910c9 into Raku:master May 30, 2018
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