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

Simplification pass assumes laziness, resulting in miscompilation #4826

Closed
L-as opened this issue Aug 25, 2022 · 12 comments
Closed

Simplification pass assumes laziness, resulting in miscompilation #4826

L-as opened this issue Aug 25, 2022 · 12 comments

Comments

@L-as
Copy link
Contributor

L-as commented Aug 25, 2022

Summary

While UPLC/PLC/PIR are strict, a simplification pass assumes the code is to be interpreted lazily, thus resulting in the following behaviour:
let c x _ = x in c True (error ()) will not err. If you however set it to const, it will err, because it's not simplified (for some arcane reason).

Concrete example in fork of Plutus: https://github.com/mlabs-haskell/plutus/commits/las/miscompilation

Do you have a proposed solution for this issue?

It's not clear to me whether it's GHC doing this or a Plutus library. In the former case, disable the optimisation pass, however this would lead to decreased efficiency, in the latter case, keep track of whether it's possible for the term to err.

System info

No response

@L-as L-as added the security Anything related to the security of applications built using Plutus label Aug 25, 2022
@L-as L-as changed the title Simplification pass assumes laziness Simplification pass assumes laziness, resulting in miscompilation Aug 25, 2022
@michaelpj
Copy link
Contributor

Sorry, can you be more specific? Is this a PIR program? A Haskell program?

Unfortunately we can't stop GHC from touching the program entirely, so if this is Haskell it may indeed be something that GHC is doing.

@L-as
Copy link
Contributor Author

L-as commented Aug 26, 2022

I've posted a link to a fork in the summary where I've added goldens for the test.
Another link: mlabs-haskell@eee1404#diff-1eec071a1e24882fcf29911ff286bec003f983927819ae99ad431fdf75867799
It's a Haskell program.

@michaelpj
Copy link
Contributor

That's helpful, thanks.

@michaelpj michaelpj added bug and removed security Anything related to the security of applications built using Plutus labels Aug 26, 2022
@effectfully effectfully self-assigned this Feb 17, 2023
@effectfully
Copy link
Contributor

This was brought into the sprint, but hasn't been worked on yet. Chances are, it'll take a bit more time for us to get to this issue, I will keep you updated.

@thealmarty
Copy link
Contributor

thealmarty commented May 1, 2023

PR #5280 shows that with the noinline pragma the evaluation results for the two examples are now the same. I.e., it IS GHC's inliner that causes the inconsistency.

For now, users will have to turn off the GHC inliner by hand with the noinline pragma for affected functions. For example:

trueOrError :: CompiledCode Bool
trueOrError = $$(compile [|| c True (P.error () :: Bool) ||])
  where
    {-# NOINLINE c #-}
    c x _ = x

The team will be discussing and exploring better solutions. We welcome your input as well.

@effectfully
Copy link
Contributor

More discussion in this ADR.

@zliu41 do you have an idea on what we should do with this issue?

@zliu41
Copy link
Member

zliu41 commented May 16, 2023

Strict is the only option. The other option (nonstrict) requires a way to prevent GHC from inlining things but that isn't currently possible.

@effectfully
Copy link
Contributor

@zliu41 should we keep this issue open or advertise using Strict and close the issue or something else? I'm just not sure how proceed with this specific GitHub issue.

@zliu41
Copy link
Member

zliu41 commented May 16, 2023

Let's keep it open until we merge #5292 and start telling people to use Strict.

@effectfully
Copy link
Contributor

Let's keep it open until we merge #5292 and start telling people to use Strict.

@zliu41 we aren't telling people to use Strict quite yet, right?

@zliu41
Copy link
Member

zliu41 commented Jul 19, 2023

@zliu41 we aren't telling people to use Strict quite yet, right?

Right. The internal ticket tracking this is PLT-5830.

@effectfully
Copy link
Contributor

The internal ticket tracking this is PLT-5830.

It's merged now (has been for a while) and hence we can close this issue with the following general advice: use Strict, otherwise GHC will misbehave.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants