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

Double return bug #720

Closed
mamonet opened this issue Jun 14, 2024 · 12 comments
Closed

Double return bug #720

mamonet opened this issue Jun 14, 2024 · 12 comments
Labels
bug Something isn't working engine Issue in the engine f* F* backend workaround This bug has a workaround

Comments

@mamonet
Copy link
Member

mamonet commented Jun 14, 2024

There are cases where extracted F* files in libcrux-ml-kem have redundant third return value where just double are supposed to be at the receiving side. This breaks lax-check of affected F* files.
Here is an example of this bug https://github.com/cryspen/libcrux/blob/dev/libcrux-ml-kem/proofs/fstar/extraction/Libcrux_ml_kem.Ntt.fst#L34

It's fixed manually here https://github.com/cryspen/libcrux/blob/dev_ml_kem_lax/libcrux-ml-kem/proofs/fstar/extraction/Libcrux_ml_kem.Ntt.fst#L34

Workaround: add () at the end of your function, see the comment below.

@W95Psp W95Psp self-assigned this Jun 21, 2024
@W95Psp W95Psp added engine Issue in the engine f* F* backend labels Jun 21, 2024
@W95Psp
Copy link
Collaborator

W95Psp commented Jun 21, 2024

Thanks for the bug report Mamone!

I'm looking at it now, here is a minimized reproducer:

fn f(n: &mut usize) {
    for _ in 0..10 {
        *n += 1;
    }
}

The problem shows up only when n is a &mut.

@W95Psp
Copy link
Collaborator

W95Psp commented Jun 24, 2024

Thanks for the issue!
I spent a few hours on that, I don't have a fix yet.

I think this issue shows up mainly in functions that ends with a loop.
A workaround is to add a () at the end of the function, e.g.:

fn f(n: &mut usize) {
    for _ in 0..10 {
        *n += 1;
    }
}

becomes:

fn f(n: &mut usize) {
    for _ in 0..10 {
        *n += 1;
    };
   ()
}

@franziskuskiefer
Copy link
Member

@W95Psp this is overdue for a while now. What's the status and plan to get it fixed?

@W95Psp
Copy link
Collaborator

W95Psp commented Aug 19, 2024

I removed the due date and set the priority to low. We have a workaround, and loads of other things to do, so let's pick that whenever we have time.

@karthikbhargavan
Copy link
Contributor

I found a few other instances of this. I think this has to have a higher priority since the bug and its fix are quite obscure.

@franziskuskiefer franziskuskiefer added bug Something isn't working and removed workaround This bug has a workaround labels Oct 27, 2024
@W95Psp W95Psp added the workaround This bug has a workaround label Nov 12, 2024
@W95Psp
Copy link
Collaborator

W95Psp commented Nov 27, 2024

My reproducer:

fn f(n: &mut usize) {
    for _ in 0..10 {
        *n += 1;
    }
}

works now, let's find another reproducer or close otherwise

@franziskuskiefer
Copy link
Member

Looks like this is working for the cases in libcrux as well 🥳
Let's link the PR that fixed it. Are there tests? If not we should add some and close this issue with that.

@W95Psp
Copy link
Collaborator

W95Psp commented Nov 28, 2024

Well, we did no any work towards this 😅
Somehow something fixed it. I think that's one of the new phases added by @maximebuyse that interact with this.
Shall we just close or shall we bisect to find out what fixed it?

@karthikbhargavan
Copy link
Contributor

karthikbhargavan commented Nov 28, 2024 via email

@franziskuskiefer
Copy link
Member

I'm still seeing this in the ML-DSA code in libcrux.
Here's an example https://hax-playground.cryspen.com/#fstar+tc/de59826b83/gist=b2613b7c8515f901ecc1a2bd77339e17

@franziskuskiefer
Copy link
Member

Somehow something fixed it.

This is still generally broken. For example

fn looping(array: &mut [u8; 5]) {
    for i in 0..array.len() {
        array[i] = i as u8;
    }
}

@maximebuyse
Copy link
Contributor

Fixed by #1223

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working engine Issue in the engine f* F* backend workaround This bug has a workaround
Projects
None yet
Development

No branches or pull requests

5 participants