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

Remove Clone requirement for argument types #353

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

Conversation

bnaecker
Copy link
Collaborator

  • Rework construction of the internal type-checking function to remove the clone requirement. Construct and call the type-check-function right at the site we already expand the probe argument closure, rather than outside. This means we don't need to expand things twice, and don't need to clone the closure, and so don't need the argument types to be cloneable either.
  • Fixup tests for new method, add some comments.
  • Update trybuild tests
  • Closes Remove unnecessary Clone requirement #136

@bnaecker bnaecker force-pushed the no-clone-closure branch 7 times, most recently from 9f35399 to 43b7c86 Compare December 18, 2024 23:35
- Rework construction of the internal type-checking function to remove
  the clone requirement. Construct and call the type-check-function
  right at the site we already expand the probe argument closure, rather
  than outside. This means we don't need to expand things twice, and
  don't need to clone the closure, and so don't need the argument types
  to be cloneable either.
- Fixup tests for new method, add some comments.
- Update trybuild tests
- Closes #136
@bnaecker bnaecker requested a review from ahl December 19, 2024 00:25
Copy link
Collaborator

@ahl ahl left a comment

Choose a reason for hiding this comment

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

looks good; you might try building crucible or something with this just to make sure nothing broke.

unsafe {
::std::arch::asm!(
"990: nop",
#probe_rec,
#in_regs
options(nomem, nostack, preserves_flags)
options(nostack)
Copy link
Collaborator

Choose a reason for hiding this comment

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

my read of nomem and preserves_flags is that they're appropriate here; is there a reason you removed them?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was being cautious. The flags might be preserved, I think, but I'm not sure about memory accesses. The raw assembly we've written doesn't read or write any. But let's suppose there's a char * in one of the registers, because we're passing a string as a probe argument. DTrace will need to access that memory, but I don't really know if that counts here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was being cautious. The flags might be preserved, I think, [...]

Yeah. It's really the nop or the mov so flags are preserved.

but I'm not sure about memory accesses. The raw assembly we've written doesn't read or write any. But let's suppose there's a char * in one of the registers, because we're passing a string as a probe argument. DTrace will need to access that memory, but I don't really know if that counts here.

I don't think that's going to count. Say a debugger attaches to this process when the %rip is on this specific instruction; it then does loads and stores. That's fine.

The asm! block does not read from or write to any memory accessible outside of the asm! block. This allows the compiler to cache the values of modified global variables in registers across the asm! block since it knows that they are not read or written to by the asm!. The compiler also assumes that this asm! block does not perform any kind of synchronization with other threads, e.g. via fences.

Yeah, we're not going to screw up cached loads for example...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, I'll put these flags back.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If I'm understanding correctly, then the nomem option is also fine in the first ASM block too, right? That's the clr %rax bit, which does potentially touch the flags, but does not access any memory at all.

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.

Remove unnecessary Clone requirement
2 participants