Skip to content

Brittle LLVM integer value range propagation #80620

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

Closed
leonardo-m opened this issue Jan 2, 2021 · 3 comments
Closed

Brittle LLVM integer value range propagation #80620

leonardo-m opened this issue Jan 2, 2021 · 3 comments
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-bug Category: This is a bug. C-optimization Category: An issue highlighting optimization opportunities or PRs implementing such T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@leonardo-m
Copy link

I think LLVM should avoid panicking code in both function:

pub fn foo1(x: i32, m: i32) -> Option<i32> {
    if m <= 0 || x < 0 || x >= m { return None; } //***
    let a = 100;
    if x == 1 {
        Some((a + m) % m)
    } else {
        None
    }
}

pub fn foo2(x: i32, m: i32) -> Option<i32> {
    if m <= 0 { return None; } //***
    if x < 0 || x >= m { return None; } //***
    let a = 100;
    if x == 1 {
        Some((a + m) % m)
    } else {
        None
    }
}

rustc 1.51.0-nightly 17eec14 2021-01-01 gives:

example::foo1:
    push    rax
    cmp     edi, 1
    jne     .LBB0_10
    cmp     edi, esi
    jge     .LBB0_10
    test    edi, edi
    js      .LBB0_10
    test    esi, esi
    jle     .LBB0_10
    test    esi, esi
    je      .LBB0_8
    lea     eax, [rsi + 100]
    cmp     esi, -1
    jne     .LBB0_7
    cmp     eax, -2147483648
    je      .LBB0_9
.LBB0_7:
    cdq
    idiv    esi
    mov     eax, 1
    pop     rcx
    ret
.LBB0_10:
    xor     eax, eax
    pop     rcx
    ret
.LBB0_8:
    lea     rdi, [rip + str.0]
    lea     rdx, [rip + .L__unnamed_1]
    mov     esi, 57
    call    qword ptr [rip + core::panicking::panic@GOTPCREL]
    ud2
.LBB0_9:
    lea     rdi, [rip + str.1]
    lea     rdx, [rip + .L__unnamed_1]
    mov     esi, 48
    call    qword ptr [rip + core::panicking::panic@GOTPCREL]
    ud2


example::foo2:
    test    esi, esi
    jle     .LBB1_3
    cmp     edi, 1
    jne     .LBB1_3
    cmp     esi, 1
    je      .LBB1_3
    lea     eax, [rsi + 100]
    cdq
    idiv    esi
    mov     eax, 1
    ret
.LBB1_3:
    xor     eax, eax
    ret
@leonardo-m leonardo-m added the C-bug Category: This is a bug. label Jan 2, 2021
@jrmuizel
Copy link
Contributor

jrmuizel commented Mar 2, 2021

This is fixed by #81451 even without using the constraint-elimination pass.

@leonardo-m
Copy link
Author

The code at the top was slightly simplified, this is the real code (a modified extended Euclidean algorithm), and I think it shows the problem isn't solved yet:

#![feature(destructuring_assignment)]

pub fn foo1(mut x: i32, m: i32) -> Option<i32> {
    if m <= 0 { return None; }
    if x < 0 || x >= m { return None; }

    let mut y = x;
    x = m;
    let mut a = 0;
    let mut b = 1;
    while y != 0 {
        (x, y, a, b) = (y, x % y, b, a - x / y * b);
    }
    if x == 1 {
        Some((a + m) % m)
    } else {
        None
    }
}

pub fn foo2(mut x: i32, m: i32) -> Option<i32> {
    if m <= 0 || x < 0 || x >= m { return None; }

    let mut y = x;
    x = m;
    let mut a = 0;
    let mut b = 1;
    while y != 0 {
        (x, y, a, b) = (y, x % y, b, a - x / y * b);
    }
    if x == 1 {
        Some((a + m) % m)
    } else {
        None
    }
}

Gives:

foo1:
        test    esi, esi
        jle     .LBB0_9
        test    edi, edi
        js      .LBB0_9
        cmp     edi, esi
        jge     .LBB0_9
        test    edi, edi
        je      .LBB0_4
        xor     ecx, ecx
        mov     r9d, 1
        mov     r8d, esi
.LBB0_8:
        mov     eax, r8d
        cdq
        idiv    edi
        mov     r8d, eax
        mov     eax, r9d
        imul    r8d, r9d
        sub     ecx, r8d
        mov     r10d, edi
        mov     r8d, edi
        mov     r9d, ecx
        mov     edi, edx
        mov     ecx, eax
        test    edx, edx
        jne     .LBB0_8
        cmp     r10d, 1
        jne     .LBB0_9
.LBB0_6:
        add     eax, esi
        cdq
        idiv    esi
        mov     eax, 1
        ret
.LBB0_4:
        xor     eax, eax
        mov     r10d, esi
        cmp     r10d, 1
        je      .LBB0_6
.LBB0_9:
        xor     eax, eax
        ret

foo2:
        sub     rsp, 8
        cmp     edi, esi
        jge     .LBB1_6
        test    edi, edi
        js      .LBB1_6
        test    esi, esi
        jle     .LBB1_6
        test    edi, edi
        je      .LBB1_4
        xor     ecx, ecx
        mov     r9d, 1
        mov     r8d, esi
.LBB1_8:
        mov     eax, r8d
        cdq
        idiv    edi
        mov     r8d, eax
        mov     eax, r9d
        imul    r8d, r9d
        sub     ecx, r8d
        mov     r10d, edi
        mov     r8d, edi
        mov     r9d, ecx
        mov     edi, edx
        mov     ecx, eax
        test    edx, edx
        jne     .LBB1_8
        cmp     r10d, 1
        jne     .LBB1_6
.LBB1_9:
        test    esi, esi
        je      .LBB1_13
        add     eax, esi
        cmp     esi, -1
        jne     .LBB1_12
        cmp     eax, -2147483648
        je      .LBB1_14
.LBB1_12:
        cdq
        idiv    esi
        mov     eax, 1
        pop     rcx
        ret
.LBB1_4:
        xor     eax, eax
        mov     r10d, esi
        cmp     r10d, 1
        je      .LBB1_9
.LBB1_6:
        xor     eax, eax
        pop     rcx
        ret
.LBB1_13:
        lea     rdi, [rip + str.0]
        lea     rdx, [rip + .L__unnamed_1]
        mov     esi, 57
        call    qword ptr [rip + core::panicking::panic@GOTPCREL]
        ud2
.LBB1_14:
        lea     rdi, [rip + str.1]
        lea     rdx, [rip + .L__unnamed_1]
        mov     esi, 48
        call    qword ptr [rip + core::panicking::panic@GOTPCREL]
        ud2

.L__unnamed_2:
        .ascii  "./example.rs"

str.0:
        .ascii  "attempt to calculate the remainder with a divisor of zero"

str.1:
        .ascii  "attempt to calculate the remainder with overflow"

.L__unnamed_1:
        .quad   .L__unnamed_2
        .asciz  "\f\000\000\000\000\000\000\000 \000\000\000\016\000\000"

@jieyouxu jieyouxu added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. C-optimization Category: An issue highlighting optimization opportunities or PRs implementing such and removed needs-triage-legacy labels Feb 18, 2024
@klensy
Copy link
Contributor

klensy commented Apr 16, 2024

No panic today for rustc 1.79.0-nightly (0d8b334 2024-04-14) https://godbolt.org/z/1nrMdTahW

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-bug Category: This is a bug. C-optimization Category: An issue highlighting optimization opportunities or PRs implementing such T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants