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

improve Valgrind integration, and figure out where it fits in #3

Closed
edef1c opened this issue Apr 16, 2015 · 5 comments
Closed

improve Valgrind integration, and figure out where it fits in #3

edef1c opened this issue Apr 16, 2015 · 5 comments

Comments

@edef1c
Copy link
Owner

edef1c commented Apr 16, 2015

Currently, os::Stack uses the native functions provided by platform.c to register the allocated stacks with Valgrind. We don't currently expose these functions.

Having them as extern "C" functions instead of inline assembly in Rust is also suboptimal, but might save us some implementation complexity. Performing a Valgrind call while running natively only takes about 2ns.

Stacks should always be registered with Valgrind, even if a custom StackSource is being used — the cost is minimal, and this should work fine even in freestanding environments.

I'm not sure if this responsibility should lie with the Stack, or with the Context. Technically, it isn't stack memory until the context is invoked, and it only ceases to be stack memory when the context finishes execution. However, the Context's main function is intended to never return (this causes an intentional abort when the trampoline function returns), so the point for stack deregistration is unclear in this case.

edef1c added a commit that referenced this issue Apr 16, 2015
Now other Stack / StackSource implementations can use the same Valgrind
code. Ref #3.
@edef1c
Copy link
Owner Author

edef1c commented Apr 16, 2015

Either way, it seems like a good idea to expose a stack::Id type, as follows:

use valgrind;
pub struct Id(valgrind::stack_id_t);

impl Id {
  pub fn register(start: *const u8, end: *const u8) -> StackId {
    StackId(valgrind::stack_register(start, end))
  }
}

impl Drop for Id {
  fn drop(&mut self) {
    valgrind::stack_deregister(self.0)
  }
}

@edef1c
Copy link
Owner Author

edef1c commented Apr 16, 2015

Registration of stacks could be encouraged by adding fn get_id(&self) -> &Id to Stack, though that might mislead people into assuming this is a generally useful identifier, not just one that appears under Valgrind. That seems to be a concern with all of this, really.

@edef1c
Copy link
Owner Author

edef1c commented Apr 16, 2015

Additionally, the aforementioned stack::Id interface lacks a way to call valgrind::stack_change, though I'm not sure if fn change(&mut self, start: *const u8, end: *const u8) is a good idea.

@edef1c
Copy link
Owner Author

edef1c commented Apr 16, 2015

Focusing on Valgrind might be way too specific — there are many other tools that care about stacks, and there is much more information one might want to attach to them.
Perhaps a wrapper type is in order.

@edef1c
Copy link
Owner Author

edef1c commented Apr 16, 2015

I've decided that since segmented stacks are unsupported in regular Rust anyway, we'll leave stack resizing alone for now.
And since a stack is only really a stack when it has an associated context, we'll handle keep this stuff in Context.

@edef1c edef1c closed this as completed in 207ff11 Apr 16, 2015
edef1c added a commit that referenced this issue Apr 16, 2015
edef1c added a commit that referenced this issue Apr 16, 2015
edef1c added a commit that referenced this issue Apr 16, 2015
edef1c added a commit that referenced this issue Feb 25, 2017
Now other Stack / StackSource implementations can use the same Valgrind
code. Ref #3.
edef1c added a commit that referenced this issue Feb 25, 2017
fix #3
This takes all Valgrind functionality private again.
Valgrind stack registrations are now associated with a Context, not with
a Stack. This makes sense, since it's only actually a stack when a
Context is running on it. Perhaps Valgrind will even be able to detect
early stack frees now.
edef1c added a commit that referenced this issue Feb 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

No branches or pull requests

1 participant