bitv: Avoid undefined behavior in mask_for_bits #2

cuviper · 2014-12-10T20:19:17Z

It was relying on the fact that x86 shifts are effectively %BITS, so
shifting a u32 by 32 preserves all bits. But this is undefined behavior
in general. Add an explicit %BITS to be safe.

It was relying on the fact that x86 shifts are effectively %BITS, so shifting a u32 by 32 preserves all bits. But this is undefined behavior in general. Add an explicit %BITS to be safe.

Gankra · 2014-12-10T20:24:08Z

Thanks!

Gankra · 2014-12-10T20:24:32Z

I'll probably have to squash some of these, which might change the author. Is that fine with you?

Gankra · 2014-12-10T20:27:21Z

src/libcollections/bit.rs

@@ -177,7 +177,7 @@ fn blocks_for_bits(bits: uint) -> uint {
 /// Computes the bitmask for the final word of the vector
 fn mask_for_bits(bits: uint) -> u32 {
    // Note especially that a perfect multiple of u32::BITS should mask all 1s.
-    !0u32 >> (u32::BITS - bits % u32::BITS)
+    !0u32 >> (u32::BITS - bits % u32::BITS) % u32::BITS


Actually, is it not the case that the internal % can be removed completely because math?

That is (x - (y mod j)) mod j == (x - y) mod (some multiple of j, because uints wrap) mod j

Yeah, as long as uint wrapping is defined behavior, it could even be (-bits) % u32::BITS. I expect the optimizer will already figure that out, but I can compare.

Ah yes, that works too! Nice.

All of our primitive integer types overflow in a defined way.

Bah, -bits triggers #[warn(unsigned_negation)]. But (0 - bits) % u32::BITS is fine. As I expected, the optimized codegen is identical - do you still care to have this updated?

If codegen is fine, I'm happy with leaving it like this for semantic value. multiple modulo stuff hurts my brain, for the most part.

cuviper · 2014-12-10T20:47:48Z

RE squashing and authorship, do what you need to do. It should keep author if you merely squash mine together, but if you squash something into your own (like the build fixes) then yes it will be lost.

Fixes rust-lang#19707. In terms of output, it currently uses the form `argument #1`, `argument #2`, etc. If anyone has any better suggestions I would be glad to consider them.

Tiny typo of "positive"

Without that flag, LLVM generates unaligned memory access instructions, which are not allowed on ARMv5. For example, the 'hello world' example from `cargo --new` failed with: ``` $ ./hello Hello, world! thread 'main' panicked at 'assertion failed: end <= len', src/libcollections/vec.rs:1113 note: Run with `RUST_BACKTRACE=1` for a backtrace. ``` I traced this error back to the following assembler code in `BufWriter::flush_buf`: ``` 6f44: e28d0018 add r0, sp, rust-lang#24 [...] 6f54: e280b005 add fp, r0, #5 [...] 7018: e5cd001c strb r0, [sp, rust-lang#28] 701c: e1a0082a lsr r0, sl, rust-lang#16 7020: 03a01001 moveq r1, #1 7024: e5cb0002 strb r0, [fp, #2] 7028: e1cba0b0 strh sl, [fp] ``` Note that `fp` points to `sp + 29`, so the three `str*`-instructions should fill up a 32bit - value at `sp + 28`, which is later used as the value `n` in `Ok(n) => written += n`. This doesn't work on ARMv5 as the `strh` can't write to the unaligned contents of `fp`, so the upper bits of `n` won't get cleared, leading to the assertion failure in Vec::drain. With `+strict-align`, the code works as expected.

ARMv5 needs +strict-align Without that flag, LLVM generates unaligned memory access instructions, which are not allowed on ARMv5. For example, the 'hello world' example from `cargo --new` failed with: ``` $ ./hello Hello, world! thread 'main' panicked at 'assertion failed: end <= len', src/libcollections/vec.rs:1113 note: Run with `RUST_BACKTRACE=1` for a backtrace. ``` I traced this error back to the following assembler code in `BufWriter::flush_buf`: ``` 6f44: e28d0018 add r0, sp, rust-lang#24 [...] 6f54: e280b005 add fp, r0, #5 [...] 7018: e5cd001c strb r0, [sp, rust-lang#28] 701c: e1a0082a lsr r0, sl, rust-lang#16 7020: 03a01001 moveq r1, #1 7024: e5cb0002 strb r0, [fp, #2] 7028: e1cba0b0 strh sl, [fp] ``` Note that `fp` points to `sp + 29`, so the three `str*`-instructions should fill up a 32bit - value at `sp + 28`, which is later used as the value `n` in `Ok(n) => written += n`. This doesn't work on ARMv5 as the `strh` can't write to the unaligned contents of `fp`, so the upper bits of `n` won't get cleared, leading to the assertion failure in Vec::drain. With `+strict-align`, the code works as expected.

Building for x86_64-unknown-linux-musl currently results in an executable lacking debug information for musl libc itself. If you request a backtrace in GDB while control flow is within musl – including sycalls made by musl – the result looks like: #0 0x0000000000434b46 in __cp_end () #1 0x0000000000432dbd in __syscall_cp_c () #2 0x0000000000000000 in ?? () i.e. not very helpful. Adding --enable-debug resolves this, and --enable-optimize re-enables optimisations which default to off given the previous flag.

Add --enable-debug flag to musl CI build script Building for x86_64-unknown-linux-musl currently results in an executable lacking debug information for musl libc itself. If you request a backtrace in GDB while control flow is within musl – including sycalls made by musl – the result looks like: ``` #0 0x0000000000434b46 in __cp_end () #1 0x0000000000432dbd in __syscall_cp_c () #2 0x0000000000000000 in ?? () ``` i.e. not very helpful. Adding --enable-debug resolves this, and --enable-optimize re-enables optimisations which default to off given the previous flag.

bitv: Avoid undefined behavior in mask_for_bits

7d354b4

It was relying on the fact that x86 shifts are effectively %BITS, so shifting a u32 by 32 preserves all bits. But this is undefined behavior in general. Add an explicit %BITS to be safe.

Gankra merged this pull request into Gankra:bitv Dec 10, 2014

Gankra reviewed Dec 10, 2014
View reviewed changes

Gankra added a commit that referenced this pull request Jul 14, 2015

Merge pull request #2 from ben0x539/patch-1

2d5c1bb

Tiny typo of "positive"

cuviper deleted the bitv branch March 4, 2016 08:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bitv: Avoid undefined behavior in mask_for_bits #2

bitv: Avoid undefined behavior in mask_for_bits #2

cuviper commented Dec 10, 2014

Gankra commented Dec 10, 2014

Gankra commented Dec 10, 2014

Gankra Dec 10, 2014

cuviper Dec 10, 2014

Gankra Dec 10, 2014

cuviper Dec 10, 2014

Gankra Dec 10, 2014

cuviper commented Dec 10, 2014

bitv: Avoid undefined behavior in mask_for_bits #2

bitv: Avoid undefined behavior in mask_for_bits #2

Conversation

cuviper commented Dec 10, 2014

Gankra commented Dec 10, 2014

Gankra commented Dec 10, 2014

Gankra Dec 10, 2014

Choose a reason for hiding this comment

cuviper Dec 10, 2014

Choose a reason for hiding this comment

Gankra Dec 10, 2014

Choose a reason for hiding this comment

cuviper Dec 10, 2014

Choose a reason for hiding this comment

Gankra Dec 10, 2014

Choose a reason for hiding this comment

cuviper commented Dec 10, 2014