[WIP] Compilers without support for flat float arrays#1589
[WIP] Compilers without support for flat float arrays#1589xclerc wants to merge 1761 commits intoocaml:trunkfrom
Conversation
|
The changes above are not sufficient, as shown by the Travis failure One solution is to avoid the construction of value with type Other solutions include:
|
|
I have a few remarks:
|
I understand your point, but there is currently only one bootstrap directory |
I would expect the primitive to behave according to the value of |
Ouch, no. I'll keep your not-completely-safe solution rather than duplicate the bootstrap binaries.
Yes, so your FFA-configured program accepts a no-FFA marshalled value and crashes. I guess we'll have to live with that. |
|
Would it make sense to use the marshaling header (or magic number) |
(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.
Suggested-by: Jeremy Yallop
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
documentation: precedence table for the standard library.
Warn on literal patterns found anywhere in a constructor's arguments.
This shadows the standard Stdlib module.
s/string_of_int/Int.to_string/g
…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.
Yes, it would! |
|
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.) |
|
@xclerc feel free to reopen if/when you want to resume work on this. |
* 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>
(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:
254 for the
float array,floatarrayand all-float record types;254 only for
floatarrayand all-float record types, and will expect valueswith tag 0 for the
float arraytype.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:
and then use it from a module compiled without support for FFA:
This can be fixed by adding a new flag to
cmifiles (akin to the flag forunsafe 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
8444618eandb03f3d84.)Bytecode executables
A bytecode executable produced by a compiler with support for FFA can
segfault if executed with an
ocamlrunVM compiled without support for FFAif it contains a
MAKEFLOATBLOCKinstruction generated for a value of typefloat array. For instance:(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 distributionconfigured with
-no-flat-float-array.)This is currently documented on the manual page for
ocamlrun. See commitec17a4d1.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 arraythen unmarshaled by a program compiled without support for FFA, array uses
can segfault.
This is documented in the ocamldoc comment of the
Marshalmodule.See commit
9e5e2015.Marshaling in compiler code, dynamic linking
While this is not currently the case, a compiler might marshal
float arrayvalues 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 ensurethat a program compiled without support for FFA does not try to dynamically
load files compiled with support for FFA. See commit
a817ad8c.Miscellaneous
Sys.flat_float_arrayconstant to give access to the informationabout FFA support (note: could be determined through user-land code e.g.
Obj.(tag @@ repr [| 0. |] = double_array_tag)) - see commit2b6d4cb9;tools/- see commit89680d8d.TODO
This PR is still a work in progress; pending: