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

Use copy_nonoverlapping instead of slice::copy_from #1448

Merged
merged 1 commit into from
Jun 26, 2024

Conversation

jswrenn
Copy link
Collaborator

@jswrenn jswrenn commented Jun 21, 2024

In doing so, we eliminate a potential panic path. Although I was not able to observe panic paths emitted in toy examples, they might be emitted in complex examples in which the optimizer is low on gas. Regardless, I expect this change will ease our future adoption of call-graph analysis techniques of potential panic paths.

Ref #200 (comment)

@codecov-commenter
Copy link

codecov-commenter commented Jun 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (main@e12c1e9). Learn more about missing BASE report.
Report is 7 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1448   +/-   ##
=======================================
  Coverage        ?   87.45%           
=======================================
  Files           ?       15           
  Lines           ?     5166           
  Branches        ?        0           
=======================================
  Hits            ?     4518           
  Misses          ?      648           
  Partials        ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/lib.rs Outdated
Comment on lines 3984 to 3994
if bytes.len() == mem::size_of_val(self) {
// SAFETY: Within this branch of the conditional, we have ensured
// that `bytes.len()` (the size of the destination) is equal to
// `mem::size_of_val(self)` the size of the source value. Neither
// the size of the source nor the size of the destination change
// between the above size check and the invocation of
// `copy_unchecked`. The size of `self` (the source) cannot change,
// because `Self` is bounded as `Immutable`, and the size of `bytes`
// cannot change, because we hold an exclusive reference to it and
// do not change its size.
unsafe { util::copy_unchecked(self.as_bytes(), bytes) }
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if bytes.len() == mem::size_of_val(self) {
// SAFETY: Within this branch of the conditional, we have ensured
// that `bytes.len()` (the size of the destination) is equal to
// `mem::size_of_val(self)` the size of the source value. Neither
// the size of the source nor the size of the destination change
// between the above size check and the invocation of
// `copy_unchecked`. The size of `self` (the source) cannot change,
// because `Self` is bounded as `Immutable`, and the size of `bytes`
// cannot change, because we hold an exclusive reference to it and
// do not change its size.
unsafe { util::copy_unchecked(self.as_bytes(), bytes) }
let self_bytes = self.as_bytes();
if bytes.len() == self_bytes.len() {
// SAFETY: Within this branch of the conditional, we have ensured
// that `bytes.len()` (the size of the destination) is equal to
// `self_bytes.len()` (the size of the source). Neither
// the size of the source nor the size of the destination change
// between the above size check and the invocation of
// `copy_unchecked`.
unsafe { util::copy_unchecked(self_bytes, bytes) }

(And reflow the safety commnet)

This makes the safety argument simpler since we don't need to reason about the relationship between size_of_val(self) and self.as_bytes(). It is probably ever so slightly tougher on the optimizer, but I doubt it's enough to make a difference.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

src/lib.rs Outdated
// `Immutable`, and the size of `bytes` cannot change, because
// we hold an exclusive reference to it and do not change its
// size.
unsafe { util::copy_unchecked(self.as_bytes(), bytes) }
Copy link
Member

Choose a reason for hiding this comment

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

Same here as above - do self.as_bytes() first and bounds check using self_bytes.len().

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

src/lib.rs Outdated
// cannot change, because we hold an exclusive reference to it and do
// not change its size.
unsafe {
util::copy_unchecked(self.as_bytes(), bytes);
Copy link
Member

Choose a reason for hiding this comment

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

Same here as above - do self.as_bytes() first and bounds check using self_bytes.len().

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

src/util.rs Outdated
// SAFETY: This invocation satisfies the safety contract of
// copy_nonoverlapping [1]:
// - `src.as_ptr()` is trivially valid for reads of `src.len()` bytes
// - `dst.as_ptr()` is valid for writes of `dst.len.len()` bytes, because the
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// - `dst.as_ptr()` is valid for writes of `dst.len.len()` bytes, because the
// - `dst.as_ptr()` is valid for writes of `src.len()` bytes, because the

Typo?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In doing so, we eliminate a potential panic path. Although I was
not able to observe panic paths emitted in toy examples, they might
be emitted in complex examples in which the optimizer is low on
gas. Regardless, I expect this change will ease our future adoption
of call-graph analysis techniques of potential panic paths.

Ref #200 (comment)
@jswrenn jswrenn added this pull request to the merge queue Jun 26, 2024
Merged via the queue into main with commit 892cba5 Jun 26, 2024
78 checks passed
@jswrenn jswrenn deleted the copy_nonoverlapping branch June 26, 2024 21:28
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