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

Don't consider 7th bit of signal to calculate signal for Linux platform. #549

Closed
wants to merge 1 commit into from

Conversation

chaosagent
Copy link
Contributor

It's used for PTRACE_O_SYSGOOD, and creates a bad signal if on.

It's used for PTRACE_O_SYSGOOD, and creates a bad signal if on.
@chaosagent
Copy link
Contributor Author

We need a way to return whether the bit is set though

@@ -81,7 +81,7 @@ mod status {
}

pub fn stop_signal(status: i32) -> Signal {
Signal::from_c_int((status & 0xFF00) >> 8).unwrap()
Signal::from_c_int((status & 0x7F00) >> 8).unwrap()
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a comment explaining the mask?

@kamalmarhubi
Copy link
Member

Need to double check the cross-platform-ness of this fix, but it seems reasonable. Could you add something to the changelog for this?

@chaosagent
Copy link
Contributor Author

I'll add the comments/changelog entry when I get a chance (soon).

I think the presence of a set 7th bit should be returned in the WaitStatus somehow, so that the caller can read it. What do you think would be the best way to represent this? I feel like adding it as a PtraceEvent would be the most straightforward to implement in a platform-specific way (both are exclusive to Linux). The architecture availability of this could be managed through enabling/disabling a particular value of the event enum used in PtraceEvent

@kamalmarhubi
Copy link
Member

I'm not sure yet, as I'm not really familiar with ptrace. I'm quite happy to take this fix now and we can file an issue to discuss how to expose it. Does that work?

@Susurrus
Copy link
Contributor

@chaosagent We'd like to close this issue in favor of #566. Can you see if that PR works for your needs as well and seems like the correct path forward?

@Susurrus
Copy link
Contributor

I believe this was addressed by #566, so I'm closing this one. Please reopen or file a new one if not.

@Susurrus Susurrus closed this Jul 25, 2017
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.

3 participants