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

Miscompilation in 4.08 #8558

Closed
xclerc opened this issue Mar 29, 2019 · 17 comments

Comments

Projects
None yet
4 participants
@xclerc
Copy link
Contributor

commented Mar 29, 2019

The following code, when compiled with 4.08, raises
Assert_failure at run time (it does not with 4.07):

let () =
  let f () = () in
  let r = ref 0 in
  let g () = f (incr r) in
  g ();
  assert (!r = 1)

The initial lambda is:

(seq
  (let
    (*match*/86 =
       (let
         (f/80 = (function param/81 0a)
          r/82 = (makemutable 0 (int) 0)
          g/83 = (function param/84 (apply f/80 (+:=1 r/82))))
         (seq (apply g/83 0a)
           (if (== (field 0 r/82) 1) 0a
             (raise
               (makeblock 0 (global Assert_failure/28!) [0: "b.ml" 6 2]))))))
    0a)
  0a)

which becomes, after Simplif.simplify_local_functions:

(seq
  (let
    (*match*/86 =
       (let (r/82 = (makemutable 0 (int) 0))
         (seq
           (catch
             (catch (exit 4 0a) with (4 param/84) (exit 5 (+:=1 r/82)))
            with (5 param/81) 0a)
           (if (== (field 0 r/82) 1) 0a
             (raise
               (makeblock 0 (global Assert_failure/28!) [0: "b.ml" 6 2]))))))
    0a)
  0a)

then translated by Simplif.simplify_exits into:

(seq
  (let
    (*match*/86 =
       (let (r/82 = (makemutable 0 (int) 0))
         (seq (let (param/88 =a 0a param/87 =a (+:=1 r/82)) 0a)
           (if (== (field 0 r/82) 1) 0a
             (raise
               (makeblock 0 (global Assert_failure/28!) [0: "b.ml" 6 2]))))))
    0a)
  0a)

and finally by Simplif.simplify_lets into:

(seq
  (let
    (*match*/86 =
       (let (r/82 =v[int] 0)
         (seq 0a
           (if (== r/82 1) 0a
              (raise
                (makeblock 0 (global Assert_failure/28!) [0: "b.ml" 6 2]))))))
    0a)
  0a)

The problem, as I understand it, is that Simplif.simplify_exits introduces
let-bindings with kind Alias while the expression bound to param/87
contains a side effect. The let-binding then disappears because param/87
is never used. Indeed, a (tested) fix is to change Alias to Strict here.

It is not immediately clear to me what a good fix is (mainly because I do
not know whether expressions passed to static exits are expected to be pure).
Changing Alias to Strict unconditionally seems a bit harsh. We could
inspect the expression to check whether it is pure, or not apply one of
the rewriting above when it is not.

@xclerc

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2019

note: it only affects native code because
simplify_local_functions is not applied in
bytecode mode.

@alainfrisch alainfrisch added this to the 4.08 milestone Mar 29, 2019

@alainfrisch alainfrisch added the bug label Mar 29, 2019

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2019

note: it only affects native code because simplify_local_functions is not applied in bytecode mode.

(I believe it is used also in bytecode without -g.)

I did not expect static exits arguments were assumed to be pure. In particular, static exits were already used to compile tuple-bindings, so the bug might have been present in another form before. I will investigate a bit.

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2019

Compilation of tuple-bindings will only pass variables to static exits. We should probably do the same for static exits introduced when eliminating local functions.

@xclerc

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2019

(I believe it is used also in bytecode without -g.)

Indeed, one is affected if !Clflags.native_code || not !Clflags.debug.

@mshinwell

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2019

Can we change the type so that only variables are permitted in the relevant term? (This sort of change is generally good, in any case...)

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2019

As a matter of fact, it seems that all current uses of Lstaticraise pass only Lvar arguments (except for the new function elimination pass). One could keep just use Alias in that case (and other simple pure expressions), and Strict otherwise.

@xavierleroy

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2019

I believe a strict let of a variable is optimized like an alias let. So please keep it simple and always use strict lets.

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2019

Actually, the compilation of pattern matching with exception cases could also use Lstaticraise with non-pure arguments. But those would cross a try...with and they were never optimized in Simplif.

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2019

I believe a strict let of a variable is optimized like an alias let. So please keep it simple and always use strict lets.

Where does this happen? In Simplif.simplify_lets, there is special logic for Alias.

@xavierleroy

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2019

Where does this happen?

| Llet(_str, _k, v, Lvar w, l2) when optimize ->

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2019

Ah right. Shouldn't this be generalized to at least constants?

Currently:

let f () =
  let a = 42 in
  let b = 43 in
  a + b

generates:

L1:     const 42
        push
        const 43
        push
        acc 0
        push
        acc 2
        addint
        return 3

while:

let f () =
  42 + 43

gives

L1:     const 42
        offsetint 43
        return 1
@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2019

Also, the simplification for Llet(_,_,Lvar,_) is disabled in bytecode with -g, so I'm concerned about introducing performance regression in that case if we simply change from Alias to Strict in simplify_exits. Perhaps we don't care, but just using Alias in case of Lvar (and, say, Lconst) would eliminate the risk, I think.

@xavierleroy

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2019

Let's fix the current bug ASAP. Then you can have your extra optimization :-)

@xavierleroy

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2019

Most lambda simplifications are turned off in bytecode debug mode, for a good reason: so that all local variables that are in scope still exist at run-time and can be printed. Better safe and slow than fast and sorry.

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2019

I agree, but simplify_exits still applies in that mode and would previously introduce Alias binding, which would be eliminated even in that mode. The simple fix you suggest could thus produce code slower than before. Slow is ok, but is even slower still ok?

@xavierleroy

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2019

Slow is ok, but is even slower still ok?

Unless you have experimental data showing a major speed difference, simpler is best.

This is a serious code generation bug that was introduced in 4.08. Let us fix it ASAP rather than having rhetorical arguments about optimizations.

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2019

I've opened a PR with the fix. I argue that the three extra lines that mostly eliminate the risk of performance regression are worth the trouble, but I'll switch to the simpler version if you insist.

alainfrisch added a commit to alainfrisch/ocaml that referenced this issue Mar 29, 2019

alainfrisch added a commit to alainfrisch/ocaml that referenced this issue Mar 29, 2019

xavierleroy added a commit that referenced this issue Apr 1, 2019

Use Strict bindings when simplifying Lstaticraise (#8559)
Alias bindings were used before, but as #8558 shows this can be incorrect,
as the argument to `Lstaticraise` may be impure.

Adds a regression test.

Closes: #8558

smuenzel-js added a commit to smuenzel-js/ocaml that referenced this issue Apr 2, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.