Skip to content

Fix miscompilation / liveness errors for string operations [superceded] #3940

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

Conversation

mshinwell
Copy link
Collaborator

@mshinwell mshinwell commented Apr 29, 2025

This fixes the random CI failures seen on arm64.

It turns out that the Cmm machtypes given to "pointer + offset" calculations used for all of the different varieties of string reads and writes are wrong. Some of them have been wrong for a very long time (30 years). They are currently given type Int, but in the case where the pointers may be into the OCaml heap (e.g. for string and bytes rather than Bigstring.t) they must have type Addr. Otherwise, CSE may apply, causing pointers into the middle of values to be live across a GC. For example in warnings.ml:

  and loop_letter_num tokens modifier i =
    if i >= String.length s then error () else
    match s.[i] with  (* first string access *)
    | '0' .. '9' ->
        let i, n1, n2 = get_range i in
        loop (Num(n1,n2,modifier)::tokens) i
    | 'A' .. 'Z' | 'a' .. 'z' ->
       loop (Letter(s.[i],Some modifier)::tokens) (i+1)  (* repeated string access after potential GC *)
    | _ -> error ()

This patch affects the system compiler as well as the flambda-backend compiler, hence the use of a patch file to fix the former. I will deal with getting this fixed upstream. The system compiler patch isn't currently as good as the flambda-backend patch since it doesn't special-case out-of-heap accesses.

There are a couple of other changes currently in here too, which can be discussed and maybe split:

  • fixes to stop the system compiler applying Comballoc across Ccheckbound, which can raise an exception. I need to discuss this with @stedolan though, maybe this is only required if the allocation is on the major heap? (This problem does not arise with the flambda-backend compiler since bounds checks are expanded before Flambda 2.)
  • fixes to silence the debug runtime with the system compiler, which I think was causing excessive CI logs in some cases.

This bug was very troublesome to track down and @xclerc must be thanked for his help with this, especially since we thought there was a second bug, which was actually caused by a mistake in my hack for tracking down the first bug! Some of the CI changes in this PR were written by @xclerc .

Once the dust has settled I will re-enable the arm64 builds as required.

@mshinwell mshinwell marked this pull request as draft April 29, 2025 11:35
@mshinwell mshinwell force-pushed the warnings-bug-coredump branch from cec55be to 12c9243 Compare May 2, 2025 09:42
@mshinwell mshinwell changed the title warnings.ml debugging Fix miscompilation / liveness errors for string operations May 2, 2025
@mshinwell mshinwell added bug Something isn't working to upstream PR should be sent to upstream OCaml labels May 2, 2025
@mshinwell mshinwell marked this pull request as ready for review May 2, 2025 09:53
@mshinwell mshinwell requested a review from xclerc May 2, 2025 09:53
@mshinwell mshinwell force-pushed the warnings-bug-coredump branch from 1b834d7 to 604df77 Compare May 2, 2025 10:01
@mshinwell
Copy link
Collaborator Author

Something is wrong with the CI here, re-opening.

@mshinwell mshinwell closed this May 2, 2025
@mshinwell mshinwell changed the title Fix miscompilation / liveness errors for string operations Fix miscompilation / liveness errors for string operations [superceded] May 2, 2025
@mshinwell
Copy link
Collaborator Author

See #3960

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working to upstream PR should be sent to upstream OCaml
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant