about summary refs log tree commit diff
diff options
context:
space:
mode:
authorbors <bors@rust-lang.org>2021-10-29 21:50:46 +0000
committerbors <bors@rust-lang.org>2021-10-29 21:50:46 +0000
commit6d42707cdef2b08ddb8549723585ffa6980535b4 (patch)
tree476dad2a7216130331c1fd91e6f6ede6b0c7fab8
parente99963c554e4d12010c2dea9f46d7bbd41dc1271 (diff)
parent68a4460b614bc925a12231d5bba04dd66cb78c6e (diff)
Auto merge of #90346 - ferrocene:pa-short-circuit, r=oli-obk
Replace some operators in libcore with their short-circuiting equivalents

In libcore there are a few occurrences of bitwise operators used in boolean expressions instead of their short-circuiting equivalents. This makes it harder to perform some kinds of source code analysis over libcore, for example [MC/DC] code coverage (a requirement in safety-critical environments).

This PR aims to remove as many bitwise operators in boolean expressions from libcore as possible, without any performance regression and without other changes. This means not all bitwise operators are removed, only the ones that don't have any difference with their short-circuiting counterparts. This already simplifies achieving MC/DC coverage, and the other functions can be changed in future PRs.

The PR is best reviewed commit-by-commit, and each commit has the resulting assembly in the message.

## Checked integer methods

These methods recently switched to bitwise operators in PRs https://github.com/rust-lang/rust/pull/89459 and https://github.com/rust-lang/rust/pull/89351. I confirmed bitwise operators are needed in most of the functions, except these two:

* `{integer}::checked_div` ([Godbolt link (nightly)](https://rust.godbolt.org/z/17efh5jPc))
* `{integer}::checked_rem` ([Godbolt link (nightly)](https://rust.godbolt.org/z/85qGWc94K))

`@tspiteri` already mentioned this was the case in https://github.com/rust-lang/rust/pull/89459#issuecomment-932728384, but opted to also switch those two to bitwise operators for consistency. As that makes MC/DC analysis harder this PR proposes switching those two back to short-circuiting operators.

## `{unsigned_ints}::carrying_add`

[Godbolt link (1.56.0)](https://rust.godbolt.org/z/vG9vx8x48)

In this instance replacing the `|` with `||` produces the exact same assembly when optimizations are enabled, so switching to the short-circuiting operator shouldn't have any impact.

## `{unsigned_ints}::borrowing_sub`

[Godbolt link (1.56.0)](https://rust.godbolt.org/z/asEfKaGE4)

In this instance replacing the `|` with `||` produces the exact same assembly when optimizations are enabled, so switching to the short-circuiting operator shouldn't have any impact.

## String UTF-8 validation

[Godbolt link (1.56.0)](https://rust.godbolt.org/z/a4rEbTvvx)

In this instance replacing the `|` with `||` produces practically the same assembly, with the two operands for the "or" swapped:

```asm
; Old
mov  rax, qword ptr [rdi + rdx + 8]
or   rax, qword ptr [rdi + rdx]
test rax, r9
je   .LBB0_7

; New
mov  rax, qword ptr [rdi + rdx]
or   rax, qword ptr [rdi + rdx + 8]
test rax, r8
je   .LBB0_7
```

[MC/DC]: https://en.wikipedia.org/wiki/Modified_condition/decision_coverage
-rw-r--r--library/core/src/num/int_macros.rs6
-rw-r--r--library/core/src/num/uint_macros.rs4
-rw-r--r--library/core/src/str/validations.rs2
3 files changed, 5 insertions, 7 deletions
diff --git a/library/core/src/num/int_macros.rs b/library/core/src/num/int_macros.rs
index 5ef2558a5fe..052e1a21b32 100644
--- a/library/core/src/num/int_macros.rs
+++ b/library/core/src/num/int_macros.rs
@@ -608,8 +608,7 @@ macro_rules! int_impl {
                       without modifying the original"]
         #[inline]
         pub const fn checked_div(self, rhs: Self) -> Option<Self> {
-            // Using `&` helps LLVM see that it is the same check made in division.
-            if unlikely!(rhs == 0 || ((self == Self::MIN) & (rhs == -1))) {
+            if unlikely!(rhs == 0 || ((self == Self::MIN) && (rhs == -1))) {
                 None
             } else {
                 // SAFETY: div by zero and by INT_MIN have been checked above
@@ -662,8 +661,7 @@ macro_rules! int_impl {
                       without modifying the original"]
         #[inline]
         pub const fn checked_rem(self, rhs: Self) -> Option<Self> {
-            // Using `&` helps LLVM see that it is the same check made in division.
-            if unlikely!(rhs == 0 || ((self == Self::MIN) & (rhs == -1))) {
+            if unlikely!(rhs == 0 || ((self == Self::MIN) && (rhs == -1))) {
                 None
             } else {
                 // SAFETY: div by zero and by INT_MIN have been checked above
diff --git a/library/core/src/num/uint_macros.rs b/library/core/src/num/uint_macros.rs
index 507ff516a8f..691d0891b14 100644
--- a/library/core/src/num/uint_macros.rs
+++ b/library/core/src/num/uint_macros.rs
@@ -1525,7 +1525,7 @@ macro_rules! uint_impl {
             //   to generate optimal code for now, and LLVM doesn't have an equivalent intrinsic
             let (a, b) = self.overflowing_add(rhs);
             let (c, d) = a.overflowing_add(carry as $SelfT);
-            (c, b | d)
+            (c, b || d)
         }
 
         /// Calculates `self` + `rhs` with a signed `rhs`
@@ -1606,7 +1606,7 @@ macro_rules! uint_impl {
             //   to generate optimal code for now, and LLVM doesn't have an equivalent intrinsic
             let (a, b) = self.overflowing_sub(rhs);
             let (c, d) = a.overflowing_sub(borrow as $SelfT);
-            (c, b | d)
+            (c, b || d)
         }
 
         /// Computes the absolute difference between `self` and `other`.
diff --git a/library/core/src/str/validations.rs b/library/core/src/str/validations.rs
index 093c9c37b60..f2d1c737808 100644
--- a/library/core/src/str/validations.rs
+++ b/library/core/src/str/validations.rs
@@ -210,7 +210,7 @@ pub(super) fn run_utf8_validation(v: &[u8]) -> Result<(), Utf8Error> {
                         // break if there is a nonascii byte
                         let zu = contains_nonascii(*block);
                         let zv = contains_nonascii(*block.offset(1));
-                        if zu | zv {
+                        if zu || zv {
                             break;
                         }
                     }