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.

Sign up for GitHub

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

Jump to bottom

Fix short-circuit in debug mode #42

Closed
wants to merge 1 commit into from
Closed

Fix short-circuit in debug mode #42

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ impl From<Choice> for bool {
/// verification.
#[inline]
fn from(source: Choice) -> bool {
debug_assert!(source.0 == 0u8 || source.0 == 1u8);
debug_assert!((source.0 == 0u8) | (source.0 == 1u8));
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the modified version still does two equality checks, and the debug_assert! branches on the result, it seems like this still involves flow from secret variables (source.0) into a branch.

Copy link
Contributor Author

@brycx brycx Apr 18, 2019 •
edited

Choose a reason for hiding this comment

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

I'm not quite sure what you mean. The debug_assert! should not branch on the result of (source.0 == 0u8), because after applying the strict OR evaluation, (source.0 == 1u8) should always be evaluated regardless of the first. So, IIUC, this means that debug_assert! can no longer branch. If you mean the actual equality checks, in the sense that they branch on true/false, then using the equality checks have given the best results when testing. I have also tested:

debug_assert!(((source.0 ^ 0u8) | (source.0 ^ 1u8)) == 1u8);
debug_assert!(((source.0 | 0u8) | (source.0 | 1u8)) == 1u8);

These seemed to give the same results, sometimes worse, compared to using the equality checks.

Copy link
Member

Choose a reason for hiding this comment

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

FWIW, I don't see branching on secret data in the assembly output of the new (using bitwise ORs with equality checks):

pub struct Choice(u8);

pub fn old(source: Choice) {
    assert!(source.0 == 0u8 || source.0 == 1u8);
}

pub fn new(source: Choice) {
    assert!((source.0 == 0u8) | (source.0 == 1u8));
}

compiles to (stripping out stdlib fluff):

example::old:
        push    rax
        mov     al, dil
        cmp     al, 0
        mov     byte ptr [rsp + 6], al
        jne     .LBB48_2
        mov     byte ptr [rsp + 7], 1
        jmp     .LBB48_3
.LBB48_2:
        mov     al, byte ptr [rsp + 6]
        cmp     al, 1
        sete    cl
        and     cl, 1
        mov     byte ptr [rsp + 7], cl
.LBB48_3:
        mov     al, byte ptr [rsp + 7]
        xor     al, -1
        test    al, 1
        jne     .LBB48_4
        jmp     .LBB48_5
.LBB48_4:
        lea     rdi, [rip + .L__unnamed_5]
        lea     rdx, [rip + .L__unnamed_6]
        mov     rax, qword ptr [rip + std::panicking::begin_panic@GOTPCREL]
        mov     esi, 52
        call    rax
        ud2
.LBB48_5:
        pop     rax
        ret

example::new:
        push    rax
        mov     al, dil
        cmp     al, 0
        sete    cl
        cmp     al, 1
        sete    al
        or      cl, al
        xor     cl, -1
        test    cl, 1
        jne     .LBB49_1
        jmp     .LBB49_2
.LBB49_1:
        lea     rdi, [rip + .L__unnamed_7]
        lea     rdx, [rip + .L__unnamed_8]
        mov     rax, qword ptr [rip + std::panicking::begin_panic@GOTPCREL]
        mov     esi, 55
        call    rax
        ud2
.LBB49_2:
        pop     rax
        ret

(And conversely I see a branching on the output of the first equality check in the old function.)

source.0 != 0
}
}
Expand Down Expand Up @@ -144,7 +144,7 @@ impl Not for Choice {
not(any(target_arch = "asmjs", target_arch = "wasm32"))
))]
fn black_box(input: u8) -> u8 {
debug_assert!(input == 0u8 || input == 1u8);
debug_assert!((input == 0u8) | (input == 1u8));

// Pretend to access a register containing the input. We "volatile" here
// because some optimisers treat assembly templates without output operands
Expand All @@ -160,7 +160,7 @@ fn black_box(input: u8) -> u8 {
))]
#[inline(never)]
fn black_box(input: u8) -> u8 {
debug_assert!(input == 0u8 || input == 1u8);
debug_assert!((input == 0u8) | (input == 1u8));
// We don't have access to inline assembly or test::black_box or ...
//
// Bailing out, hopefully the compiler doesn't use the fact that `input` is 0 or 1.
Expand Down