Skip to content

[WIP] Compilers without support for flat float arrays#1589

Closed
xclerc wants to merge 1761 commits intoocaml:trunkfrom
xclerc:flat-float-arrays
Closed

[WIP] Compilers without support for flat float arrays#1589
xclerc wants to merge 1761 commits intoocaml:trunkfrom
xclerc:flat-float-arrays

Conversation

@xclerc
Copy link
Copy Markdown
Contributor

@xclerc xclerc commented Jan 30, 2018

(This comment is very long, in an attempt to cover/discuss all cases.)

There seems to be various problems regarding the interaction between
distributions built with and without support for flat float arrays (FFA below).
These problems have a common root that is the production / expectation
regarding blocks with tag 254:

  • a program/compiler with support for FFA will produce values with tag
    254 for the float array, floatarray and all-float record types;
  • a program/compiler without support for FFA will expect values with tag
    254 only for floatarray and all-float record types, and will expect values
    with tag 0 for the float array type.

A program expecting a block with tag 0 but actually getting a block with tag
254 is more than likely to segfault at some point.

The current PR addresses various occurrences of this issue.

Mixing modules

It is possible to produce a program that segfaults by compiling a module
with the following signature with a compiler with support for FFA:

val f : ... -> float array

and then use it from a module compiled without support for FFA:

let () = Array.iter print_float (Aux.f ...)

This can be fixed by adding a new flag to cmi files (akin to the flag for
unsafe strings) in order to avoid mixing modules. See commit 92d483ab.

(For this to work properly, it needs the bytecode compiler in the boot/
directory to be compiled without support for flat float array. This is checked
by the travis script. See commits 8444618e and b03f3d84.)

Bytecode executables

A bytecode executable produced by a compiler with support for FFA can
segfault if executed with an ocamlrun VM compiled without support for FFA
if it contains a MAKEFLOATBLOCK instruction generated for a value of type
float array. For instance:

let a = [| 0.; 1.; 2. |]
let () = Array.iter print_float a

(Note that if this snippet is added to the code of the compiler, e.g. in
driver/main.ml, a segfault will occur while trying to build a distribution
configured with -no-flat-float-array.)

This is currently documented on the manual page for ocamlrun. See commit
ec17a4d1.

Another option would be to bump the magic number for bytecode executables,
and store the information about FFA support, refusing to execute a file if a
segfault might occur.

Actual fixes are also possible, depending on whether it is deemed important
to provide backward compatibility. For instance, some key array-related
primitives might keep their original semantics (i.e. include a dynamic check
for the kind of array) only for bytecode compilation. It is trivial for get/set/length
primitives but is slightly more complex for blit/gather (because these ones
currently assume that the various arrays involved in a given operation have the
same layout).

Marshaling in user code

If a program compiled with support for FFA marshals a value of type float array
then unmarshaled by a program compiled without support for FFA, array uses
can segfault.

This is documented in the ocamldoc comment of the Marshal module.
See commit 9e5e2015.

Marshaling in compiler code, dynamic linking

While this is not currently the case, a compiler might marshal float array
values to cmK files. Then unmarshaling such values might cause segfaults.

This can be avoided by adding a boolean indicating whether the file might
contain flat arrays to all cmK files. The boolean is also used to ensure
that a program compiled without support for FFA does not try to dynamically
load files compiled with support for FFA. See commit a817ad8c.

Miscellaneous

  • Added a Sys.flat_float_array constant to give access to the information
    about FFA support (note: could be determined through user-land code e.g.
    Obj.(tag @@ repr [| 0. |] = double_array_tag)) - see commit 2b6d4cb9;
  • Updated sources in tools/ - see commit 89680d8d.

TODO

This PR is still a work in progress; pending:

  • (possibly) rewrite some changes above as they are are partially redundant;
  • update chapter 20 ("Interfacing C with OCaml") of the manual;
  • make sure that the various CI instances build variants without FFA support;
  • create opam switches without FFA support to encourage testing.

@xclerc
Copy link
Copy Markdown
Contributor Author

xclerc commented Feb 8, 2018

The changes above are not sufficient, as shown by the Travis failure
of the flambda variant. Indeed, an flambda compiler built using a
boostraping compiler without FFA support but run with an ocamlrun
with FFA support will fail, because the flambda compiler builds value
of type float array.

One solution is to avoid the construction of value with type float array,
(see commit c668190e) but it is brittle at best, because such values are
intermediary results (e.g. Array.map (fun x -> Some x) (Array.of_list fs)
with fs a list of float value in file asmcomp/export_info.ml).

Other solutions include:

  • having the primitive retain their "old" semantics when in bytecode mode;
  • restore the "old" semantics of array primitives and introduce new
    primitives, to be used ` in 4.07+.

@damiendoligez damiendoligez self-assigned this Mar 28, 2018
@damiendoligez
Copy link
Copy Markdown
Member

I have a few remarks:

  • It's nice to detect and prevent the mixing of files from differently-configured compilers.
  • Cross-compatibility is not a goal, performance is. If you reintroduce run-time checks in the primitives, that defeats the whole purpose of the configure option. (re: your "other solutions")
  • The only place where I'm interested in cross-compatibility is making the bootstrap easier or faster.
  • I'm worried about the dissymetry of your code: you refuse FFA-marshalled values in no-FFA programs, but you accept the reverse. This is probably OK as long as you stay in OCaml, but any (user-defined) C primitive that deals with float array will crash or even return buggy results. I'll be more at ease if you just forbid mixing the two configurations altogether.

@xclerc
Copy link
Copy Markdown
Contributor Author

xclerc commented Mar 29, 2018

I'm worried about the dissymetry of your code: you refuse FFA-marshalled values in no-FFA programs, but you accept the reverse. This is probably OK as long as you stay in OCaml, but any (user-defined) C primitive that deals with float array will crash or even return buggy results. I'll be more at ease if you just forbid mixing the two configurations altogether.

I understand your point, but there is currently only one bootstrap directory
that is used whatever the user pass to the configure script. Do you suggest
to have two bootstrap directories?

@xclerc
Copy link
Copy Markdown
Contributor Author

xclerc commented Mar 29, 2018

any (user-defined) C primitive that deals with float array will crash or even return buggy results

I would expect the primitive to behave according to the value of FLAT_FLOAT_ARRAY
in ${PREFIX}/lib/ocaml/Makefile.config.

@damiendoligez
Copy link
Copy Markdown
Member

Do you suggest to have two bootstrap directories?

Ouch, no. I'll keep your not-completely-safe solution rather than duplicate the bootstrap binaries.

I would expect the primitive to behave according to the value of FLAT_FLOAT_ARRAY
in ${PREFIX}/lib/ocaml/Makefile.config.

Yes, so your FFA-configured program accepts a no-FFA marshalled value and crashes. I guess we'll have to live with that.

@xclerc
Copy link
Copy Markdown
Contributor Author

xclerc commented Jul 24, 2018

Would it make sense to use the marshaling header (or magic number)
to detect incompatibilities to could lead to a segfault?

gasche and others added 22 commits September 13, 2018 16:46
(The bug is already present in 4.06 and 4.07.)
We could factorize the binder judgments in a related way, but it would
require a different set of higher-order operator, and I thought that
the lower number of occurrences did not justify the extra complexity.
This work was conducted by Alban Reynaud during an internship at INRIA
Saclay, with the help and under the supervision of Gabriel Scherer in
the Parsifal team.

A summary is included below. For more details, see

  https://github.com/Cemoixerestre/Internship-2018-ocaml-recursive-value

or the comments in the code itself, which has evolved a bit from
Alban's internship report above.

Summary:
--------

Rec_check is based on the idea of assigning "access modes" to variables

    m ::= Dereference (* full access and inspection of the value *)
        | Unguarded (* the value result is directly returned *)
        | Guard (* the value is stored under a data constructor *)
        | Delayed (* the value is not needed at definition time *)
        | Unused (* the value is not used at all *)

Before this patchset, the implementation (contributed by Jeremy Yallop)
was formulated as a type-checker manipulating "uses", which are mappings
from free variables to modes

    u ::= (x : m)*

and contexts Γ mapping variables to uses

    Γ ::= (x : u)*

The check relied on a judgment of the form

    Γ ⊢ e : u

where, in the typing algorithm, `Γ` and `e` are the inputs, and `u` is
the output — this could be written `-Γ ⊢ e : +u`.

After this patchset, the implementation uses a simpler notion of context
mapping variables to mods

    Γ ::= (x : m)*

and the judgment has the simpler structure

     Γ ⊢ e : m

but now `m` is an input to the judgment, indicating in which usage
context `e` is checked, and `Γ` is the output: `+Γ ⊢ e : -m`.

Other fixes:
------------

This patchset also fixes a soundness bug around `{ foo with ... }`
which is already present in 4.06 and 4.07.

Another soundness bug found during this work was submitted and fixed
independently as GPR ocaml#1915.

This work also led to the refactoring of GPR ocaml#1922, and the refactoring
of the testsuite in GPR ocaml#1937.
We provide an ocamldoc-style module comment to explain the general
lines of the recursive check. This complements existing comments about
the term- and binding-judgments that are more implementation-oriented.

It was natural to reorder the code so that the "static or dynamic
size" code is apart from the "usage of recursive variables" code.

Finally, I also added copyright headers to reflect the now shared
authorship of the code. (Alban has signed a CLA.)
New implementations of the static check for recursive definitions
Make marshalled Custom_tag objects store their length.
Add support for -args and -args0 to ocamlmklib
Octachron and others added 25 commits November 6, 2018 22:18
documentation: precedence table for the standard library.
Warn on literal patterns found anywhere in a constructor's arguments.
This shadows the standard Stdlib module.
…mpiled without support for flat float arrays.
…y cannot be used to execute bytecode files compiled with such support.
…am with support for flat float arrays to a program without such support.
@damiendoligez
Copy link
Copy Markdown
Member

Would it make sense to use the marshaling header (or magic number)
to detect incompatibilities to could lead to a segfault?

Yes, it would!

@gasche
Copy link
Copy Markdown
Member

gasche commented Apr 28, 2021

What is the status of this PR? Is there still interest in working on this? (There was an issue with the rebase apparaently, which should be re-done to keep a clean patchset.)

@gasche gasche closed this Mar 10, 2023
@damiendoligez
Copy link
Copy Markdown
Member

@xclerc feel free to reopen if/when you want to resume work on this.

OlivierNicole pushed a commit to OlivierNicole/ocaml that referenced this pull request Feb 28, 2025
* Start of implicit-source-positions

Expand [%src_pos] to hard-coded record (ocaml#1565)

* Update codeowners

* Revert "Update codeowners"

This reverts commit 313cb2d1415a34971ea6e3cbd02d008e255ca888.

* Start

* backup

* Hacky checkpoint with hard-coded record

* Rename test

* Shadowing works as intended

* Simpler test case

* Fix label layouts

* Change name to lexing_position, fix record layout, add to shadowing test

* Add to test

* Formatting

* Tests, fix predef lexing_position layout, add CR for untypeast shadowing

* Fix test

* Fix untracked file

* Revert "Fix untracked file"

This reverts commit 2a18a6aef66322e4e897aef29ace14eda25b7785.

* reformat

* Bootstrap, with predef lexing_position

---------

Co-authored-by: Richard Eisenberg <reisenberg@janestreet.com>

Separate `arg_label`s for Parse and Typed trees (ocaml#1589)

* src_pos in fn decls and defns

* Revert is_optional modifications for now

* src_pos shouldn't work in arbitrary places

* Tidy code

* Whitespace

* Add test

* Comment on documentation

* Whitespace

* more whitespace

* Amend test to use Position

* Correctly print [%src_pos] rather than lexing_position and fix corresponding test

* Store hard-coded string in variable

* Separate parsed and typed labels

* Delete fn_decl_and_defn.ml

* Delete invalid_usages.ml

* Minimize diffs in PR

* Minimize diffs in PR

* Update named_function.ml for less diffs

* Clean up

* Put arg_label in Outcome tree

* Revert "Put arg_label in Outcome tree"

This reverts commit cd5ace530c602975bac5f259e7d078a52dfabdaa.

* is_optional for Parsetree arg_labels

* Restore Asttypes.arg_label, re-export arg_label from Types

* Document distinction between arg_labels

* Fix whitespace

* Add todo comment

Add `arg_label` to `Otyp_arrow` (ocaml#1644)

* Put arg_label in Outcome tree

* Add comment for Outcometree.arg_label

`[%src_pos]` in fn decls and defns (ocaml#1579)

* src_pos in fn decls and defns

* Revert is_optional modifications for now

* src_pos shouldn't work in arbitrary places

* Tidy code

* Whitespace

* Add test

* Comment on documentation

* Whitespace

* more whitespace

* Amend test to use Position

* Correctly print [%src_pos] rather than lexing_position and fix corresponding test

* Store hard-coded string in variable

* Hacky outcometree printing without creating nontrivial node

* Update documentation for Position labels

* Update tests

* Fix erroneously printing lexing_position for Position arguments

* Clean up code

* Reconstruct constraint for Position arguments in Untypeast

* Update tests

* Move comments around

* Slightly prettify

* Use Location.none in Untypeast reconstructed extensions

* Add Ttyp_src_pos to Typedtree

* Add CR

* Comments

* Clean up code

* Update comment in printtyp.ml

* Add documentation for Ttyp_src_pos, remove extraneous comments

* *synonyms.ml

* src_pos in fn decls and defns

* Revert is_optional modifications for now

* src_pos shouldn't work in arbitrary places

* Tidy code

* Whitespace

* Add test

* Comment on documentation

* Whitespace

* Correctly print [%src_pos] rather than lexing_position and fix corresponding test

* Store hard-coded string in variable

* Hacky outcometree printing without creating nontrivial node

* Update documentation for Position labels

* Update tests

* Fix erroneously printing lexing_position for Position arguments

* Clean up code

* Reconstruct constraint for Position arguments in Untypeast

* Update tests

* Move comments around

* Slightly prettify

* Use Location.none in Untypeast reconstructed extensions

* Add Ttyp_src_pos to Typedtree

* Add CR

* Comments

* Clean up code

* Add documentation for Ttyp_src_pos, remove extraneous comments

* *synonyms.ml

* Merge cleanup

* Add label to Octy_arrow

* Rename function

* transl_label checks for Position

* Pass None to transl_label for classes

* Add comment

* Delete comment

* Consider Position when approximating type

* Add tests for application, recursion

* Error messages mention Position

* Prettify

* Comments

* Rename function

* Add comment

* Remove extraneous calls to label translation

* Test type-based disambiguation

* Add comment for fn app labels

* Add commuting tests

* Remove duplicated src_pos match logic

* Reduce instances of src_pos string

* src_pos in fn decls and defns

* src_pos shouldn't work in arbitrary places

* Whitespace

* Correctly print [%src_pos] rather than lexing_position and fix corresponding test

* Store hard-coded string in variable

* Hacky outcometree printing without creating nontrivial node

* Clean up code

* Slightly prettify

* Add CR

* Clean up code

* Update comment in printtyp.ml

* Merge cleanup

* Add label to Octy_arrow

* Rename function

* transl_label checks for Position

* Pass None to transl_label for classes

* Add comment

* Delete comment

* Consider Position when approximating type

* Add tests for application, recursion

* Error messages mention Position

* Prettify

* Comments

* Rename function

* Add comment

* Remove extraneous calls to label translation

* Test type-based disambiguation

* Add comment for fn app labels

* Add commuting tests

* Remove duplicated src_pos match logic

* Reduce instances of src_pos string

* Make things is_optional again

* Add commuting tests

* Add transl_label_from_pat

* Whitespace

* Parenthesize src_pos in error message

* Fix test

Create `Lexing.position` values when `[%src_pos]` in expressions (ocaml#1661)

* Everything

* checkpoint

Construct lambda for src_pos directly

* Revert now unneeded changes to predef

* Clean up comments, whitespace

Implicitly supply source position arguments (ocaml#1671)

* Everything

* Apply position arguments when expected type is nothing, refactor creating src_pos exprs

* Add warning instead of modifying existing one

* Fix test

* Move test

* Resolved comments

Clearer classic mode label equivalence checks (ocaml#1692)

* Everything

* Apply position arguments when expected type is nothing, refactor creating src_pos exprs

* Add warning instead of modifying existing one

* Fix test

* Move test

* Resolved comments

* Classic mode equivalence check

* Refactor

* Implicit source position merge conflicts (ocaml#2275)

* Implicit Source Positions Conflict resolution

This feature solely fixes merge conflicts that the implicit
source positions project had after not having been rebased since the
summer.

Sadly, I mistakenly committed a new test! I meant to do it on a
different branch, but I accidentally committed some more changes
after that making it trickier to split... Please let me know if I should
only keep this feature do "merge conflict resolution". Thanks!

Testing
-------
- I ran `make test`, and it passes.
- `make install` works, I migrated bonsai to start using this and it works!
- Added test on let operators.
  - Sadly it doesn't quite work, but I think it would be cool if it did
    work as it would allow codebases that use let+ and let* to get
    source code locations! I am unsure about its sound-ness though and
    left CR src_pos'es/question CR's.

Please let me know if there is other additional testing/ci I should perform. Thanks!

I also performed additional cleanup in this feature from self-questions
I had during the merge. Please let me know if there are any changes I
should perform. Thanks!

* More cleanup after self-review

* Made CI on Github pass

- CI seems to run `make ci` instead of just `make test`. I've fixed
  more `make ci` changes, although not all, pushing to let ci run
  in case the current "compiler-libs.common" being missing is due
  to a misconfiguration on my environment/if it also fails on the
  github ci being missing is due to a misconfiguration on my
  environment/if it also fails on the github ci.

- Performed a bootstrap

- Fixed chamelon

- Additionally turned my questions on let operator support into CR
  src_pos:

* Removed empty file added accidentally

* Updated let_operator support comment

* Widened question on chamelon compatibility

* Implicit source positions object system support (ocaml#2307)

* Implicit Source Positions Conflict resolution

This feature solely fixes merge conflicts that the implicit
source positions project had after not having been rebased since the
summer.

Sadly, I mistakenly committed a new test! I meant to do it on a
different branch, but I accidentally committed some more changes
after that making it trickier to split... Please let me know if I should
only keep this feature do "merge conflict resolution". Thanks!

Testing
-------
- I ran `make test`, and it passes.
- `make install` works, I migrated bonsai to start using this and it works!
- Added test on let operators.
  - Sadly it doesn't quite work, but I think it would be cool if it did
    work as it would allow codebases that use let+ and let* to get
    source code locations! I am unsure about its sound-ness though and
    left CR src_pos'es/question CR's.

Please let me know if there is other additional testing/ci I should perform. Thanks!

I also performed additional cleanup in this feature from self-questions
I had during the merge. Please let me know if there are any changes I
should perform. Thanks!

* Moves changes from original class-type support branch into a rebased branch

* Removed lingering merge conflict markers

* More tests + manually moved a commit from the original class type branch

- For some reason I originally missed a commit that typed the argument
  on classes from the original branch. This feature also grabs the
  tests. Some of my questions revolving `Principal` are no longer
  needed as they seem to have disappeared! I suspect that `make ci`
  now passing in the parent feature is partly/transitively responsible
  somehow for `Principal` now no longer showing up.

* Fixed incorrect merging of invalid_usages.ml

* Removes weird whitespace observed after self-review

* More tests! Found out that application on an inheritance call is unhandled!

* Explicit passing positional argument in a pcl_apply works, erasure still does not.

* Added Pcl_apply support

* More cleanup + removed a question cr from the parent feature.

* More tests. Found another bug! Class type arrows seeem to be left untranslated...

* Fixed type annotation bug + added more tests

* Removed duplicated test

* Added more tests + fixed weird whitespace

* Added question on the two class system environments (val_env vs. met_env)

* Deduplicated more code after self-review

* minor whitespace change

* Removed resolved question CR and addressed new CRs

* Implicit source positions directory locations (ocaml#2346)

* Implicit Source Positions Conflict resolution

This feature solely fixes merge conflicts that the implicit
source positions project had after not having been rebased since the
summer.

Sadly, I mistakenly committed a new test! I meant to do it on a
different branch, but I accidentally committed some more changes
after that making it trickier to split... Please let me know if I should
only keep this feature do "merge conflict resolution". Thanks!

Testing
-------
- I ran `make test`, and it passes.
- `make install` works, I migrated bonsai to start using this and it works!
- Added test on let operators.
  - Sadly it doesn't quite work, but I think it would be cool if it did
    work as it would allow codebases that use let+ and let* to get
    source code locations! I am unsure about its sound-ness though and
    left CR src_pos'es/question CR's.

Please let me know if there is other additional testing/ci I should perform. Thanks!

I also performed additional cleanup in this feature from self-questions
I had during the merge. Please let me know if there are any changes I
should perform. Thanks!

* More cleanup after self-review

* Made CI on Github pass

- CI seems to run `make ci` instead of just `make test`. I've fixed
  more `make ci` changes, although not all, pushing to let ci run
  in case the current "compiler-libs.common" being missing is due
  to a misconfiguration on my environment/if it also fails on the
  github ci being missing is due to a misconfiguration on my
  environment/if it also fails on the github ci.

- Performed a bootstrap

- Fixed chamelon

- Additionally turned my questions on let operator support into CR
  src_pos:

* Removed empty file added accidentally

* Updated let_operator support comment

* Widened question on chamelon compatibility

* Moves changes from original class-type support branch into a rebased branch

* Created branch with working changes for directory positions.

* Added a test sanity checking being able to pass in the flag.

* Changed test to properly test that the right basename is supplied

* Removed merge conflict markers upon self review.

* Removed lingering merge artifacts upon self review

* Renamed flag from -dir to -directory

* Fix typo

* Removed directory flag from ocamldoc options

* [Implicit Source Positions] - Better Error Messages (ocaml#2364)

* Rename src_pos -> call_pos

Also left some self notes regarding the remaining CR src_pos

* Improved error messages for %call_pos

* Addressed a CR src_pos and removed an already addressed CR src_pos

* Cleanup after self-review

* Updated missing rename after self-review

* Removed addressed c r questions

* Fixed merge conflicts. `make ci` passes locally

* bootstrap

---------

Co-authored-by: jose r <45022810+Enoumy@users.noreply.github.com>
Co-authored-by: enoumy <enoumy@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.