Deprecation of Bigarray.*.map_file and introduction of Unix.map_file, continued by xavierleroy · Pull Request #1077 · ocaml/ocaml

xavierleroy · 2017-03-03T15:13:58Z

This is a follow-up to GPR #997 with a different implementation that creates fewer dependencies.

We add to the runtime a byterun/bigarray.c file that contains the bigarray creation functions that used to be in otherlibs/bigarray/bigarray_stubs.c. In the latter file we keep all primitives needed to implement the Bigarray interface. The functions in the new byterun/bigarray.c make it possible to create bigarrays both from the bigarray library and from the unix library.

The header file bigarray.h moves to byterun/caml/bigarray.h accordingly.

The map_file implementations move to otherlibs/unix/mmap.c and otherlibs/win32unix/mmap.c. Some bigarray allocation code shared between the two implementations is put in otherlibs/unix/mmap_ba.c.

Through a couple of #ifdef, the map_file implementations can also be compiled from within otherlibs/bigarray with the same semantics they had in 4.04.

As a consequence, the bigarray library still contains a standalone, Unix-independent implementation of Bigarray.*.map_file; the only difference with 4.04 is that it is marked deprecated.

Current status: compiled and lightly tested under Unix. Win32 implementation neither compiled nor tested. I'm submitting it as a GPR so that it gets some CI.

gasche · 2017-03-03T16:20:20Z

From appveyor:

Running tests from 'tests/unboxed-primitive-args' ...
test_common.c(17) : fatal error C1083: Cannot open include file: 'bigarray.h': No such file or directory

xavierleroy · 2017-03-03T18:16:59Z

Thanks for the ping. Pushed a trivial fix for the failing test.

gasche · 2017-03-05T19:45:01Z

testsuite/tests/lib-bigarray-file/mapfile.ml

+      with
+      | Unix.Unix_error((Unix.EACCES | Unix.EPERM), _, _) -> true
+      | Unix.Unix_error(err, _, _) ->
+          Printf.eprintf "Unexpected error %s\n" (Unix.error_message err);


It would be nice to include a flush %! to the format to avoid delayed printing. (Same below)

gasche · 2017-03-05T19:47:02Z

It looks like the test still fails on Windows:

Running tests from 'tests/lib-bigarray-file' ...
 ... testing 'mapfile.ml': ocamlcUnexpected error Invalid argument
*** Bad result (map_file errors, test 1)
************* TEST FAILED ****************
make[7]: *** [/cygdrive/c/projects/ocaml/testsuite/makefiles/Makefile.several:114: run-file] Error 2
 => failed
make[2]: Entering directory '/cygdrive/c/projects/ocaml/testsuite'

(@xavierleroy: I'm writing this tautologic message because I'm not sure you have seen the last failure; I myself don't get notifications when the CI results are available, which would be convenient, so I have to resort to polling; given that I just did a polling cycle I thought you could be interested in a delayed notification. Feel free to grumble otherwise.)

ghost · 2017-03-06T07:18:46Z

@xavierleroy I think we need to reference at least one stub from byterun/bigarray.c in the stdlib, otherwise we won't be able to dynlink bigarray in native code. We had the same problem with the compiler libraries in the past: they use stubs from byterun/terminfo.c but since nothing in the stdlib references them, terminfo.o is never statically linked in, even with -linkall

xavierleroy · 2017-03-06T08:10:53Z

@gasche : like you, I poll occasionally. I'm on the Win32 test failure.

@diml : I ran into the issue you mention. Currently, it is avoided by forcing the registration of caml_ba_ops (the bigarray custom operations) in caml_init_custom_operations, with the extra benefit that there is no longer any need for initialization code in the Bigarray library.

xavierleroy · 2017-03-06T14:42:43Z

All right, it seems that CI is happy (the Travis failure is because of no Changes entry, despite the label to that effect), so I turned off the fasten seat belts sign ^H^H^H^H^H^H I removed the work-in-progress label to indicate that this PR is ready for code review. (Yay.) I'm sorry that it turned out a little more complicated than I hoped. On the other hand, some of the #ifdef ugliness will go away when Bigarray.*.map_file is removed for good.

gasche · 2017-03-06T15:13:58Z

(It would definitely make sense to add the number of the present GPR to the GPR#997 change entry, so that people can have the full picture.)

xavierleroy · 2017-03-07T08:34:40Z

Fair enough, in the end I updated the Changes file.

gasche · 2017-03-07T16:12:29Z

@diml, do you think it would be possible for you (or someone else) to review this proposed change in the following days? This is the only remaining blocker that I am aware of for Camomile, which means that after this is merged (or, if this takes too long, after #997 is reverted as a first step), I could ask @lefessan to update the opam-builder bot to a more recent version of the 4.05 branch.

ghost · 2017-03-07T16:51:33Z

@gasche sure, I was planning to look at this today or tomorrow

gasche · 2017-03-07T16:55:46Z

Thanks!

ghost · 2017-03-07T17:41:59Z

otherlibs/unix/mmap_ba.c

+  Assert((flags & CAML_BA_KIND_MASK) <= CAML_BA_CHAR);
+  for (i = 0; i < num_dims; i++) dimcopy[i] = dim[i];
+  asize = SIZEOF_BA_ARRAY + num_dims * sizeof(intnat);
+  res = caml_alloc_custom(&caml_ba_mapped_ops, asize, 0, 1);


What is the motivation for using 0, 1 rather than size, CAML_BA_MAX_MEMORY as before? Isn't that bad if shared=false for instance?

BTW, I never remember how these values are interpreted, so 0, 1 might be the right setting, but I would think it's safer to be conservative for the upcoming release

The old code used 0 and CAML_BA_MAX_MEMORY in this case (i.e. size is 0 for mmaped bigarrays), so the new code behaves like the old code. Whether it is the right thing to do is unclear.

Indeed, I missed that the size was 0 in the old code

ghost · 2017-03-07T17:50:21Z

otherlibs/win32unix/mmap.c

+   It is included for backward compatibility with the old
+   Bigarray.*.map_file implementation.  */
+
+static void caml_ba_sys_error(void)


This function seems to be unused

You're correct, wrong cut and paste from ../unix/mmap.c. Fixed.

shindere · 2017-03-07T19:20:27Z

@xavierleroy:

In byterun/bigarray.c:

The hearder looks wrong: 200 -> 2017, Rocquencourt -> Paris
(perhaps Manuel's name also?)
The file uses both Assert and CAMLassert, I am wondering in
which context each of them should be used

Incidentally, in byterun/caml/misc.h one reads:

#ifndef CAML_AVOID_CONFLICTS
#define Assert CAMLassert
#endif

And it seems it's the only place in the repo that mentions this
CAML_AVOID_CONFLICTS macro.

And finally, before merging, wouldn't it be worth sending this PR to precheck?

xavierleroy · 2017-03-08T10:33:54Z

@shindere : OK for Assert that should be CAMLassert, fixed in the latest commit.

Concerning the header, it is the same as for otherlibs/bigarray/bigarray_stubs.c, from which byterun/bigarray.c was excerpted. Yes, the bigarray_stubs.c dates back to 2000, was written with the help of Manuel Serrano, and I was located in Rocquencourt at this time. We don't have a policy of updating file headers, so why not keep the original header?

shindere · 2017-03-08T10:42:28Z

Thanks, @xavierleroy. I'm very fine with anything. Just wanted to make sure it was not a mistake. Re Assert: shall we get rid of CAML_AVOID_CONFLICTS and of Assert itself and consistently replace it by CAMLassert everywhere?

damiendoligez · 2017-03-09T13:08:02Z

Re Assert: shall we get rid of CAML_AVOID_CONFLICTS and of Assert itself and consistently replace it by CAMLassert everywhere?

That would be a good thing, but of course not in this PR.

shindere · 2017-03-09T14:27:45Z

Damien Doligez (2017/03/09 05:08 -0800):

>Re Assert: shall we get rid of CAML_AVOID_CONFLICTS and of Assert itself and consistently replace it by CAMLassert everywhere? That would be a good thing, but of course not in this PR.

Sure. Will do it in a different PR. Can Assert be removed completely, all should it rather be moved to compatibility.h? I would personnally remove it because as far as I know it is not documented so user-code is not supposed to use it.

damiendoligez · 2017-03-10T10:51:27Z

Yes, remove it completely.

ghost · 2017-03-10T13:13:46Z

The windows CI is failing with:

../win32unix/mmap.c(168) : error C2371: 'caml_ba_sys_error' : redefinition; different basic types

Otherwise the I read the changes and the patch looks correct to me.

@SInCE

This reverts commit 5ed7200. GPR#997 introduced a hard dependency of Bigarray on Unix, while there previously only was a type-level dependency. This break some programs, such as Camomile, that linked bigarray.cma but not unix.cma. A solution is being worked out in GPR ocaml#1077 to remove the dependency, but I would like to go forward with opam package testing on the 4.05 branch, and getting the details right for GPR#1077 requires some care, so I wouldn't feel comfortable rushing to merge it. I had to handle the following conflicts: otherlibs/unix/unix.mli ("@SInCE 4.05.0" was added in faab91a) testsuite/tests/lib-bigarray-file/mapfile.ml (changed by 5839c98; I kept and adapted the new version) testsuite/tests/lib-bigarray-file/mapfile.reference

@SInCE

This reverts commit 5ed7200. GPR#997 introduced a hard dependency of Bigarray on Unix, while there previously only was a type-level dependency. This break some programs, such as Camomile, that linked bigarray.cma but not unix.cma. A solution is being worked out in GPR ocaml#1077 to remove the dependency, but I would like to go forward with opam package testing on the 4.05 branch, and getting the details right for GPR#1077 requires some care, so I wouldn't feel comfortable rushing to merge it. I had to handle the following conflicts: otherlibs/unix/unix.mli ("@SInCE 4.05.0" was added in faab91a) testsuite/tests/lib-bigarray-file/mapfile.ml (changed by 5839c98; I kept and adapted the new version) testsuite/tests/lib-bigarray-file/mapfile.reference

gasche · 2017-03-12T17:18:13Z

CI systems force us to realize that we work to make the machine happy, and not the other way around.

… continued This is a follow-up to GPR #997 with a different implementation that creates fewer dependencies. We add to the runtime a `byterun/bigarray.c` file that contains the bigarray creation functions that used to be in `otherlibs/bigarray/bigarray_stubs.c`. In the latter file we keep all primitives needed to implement the Bigarray interface. The functions in the new `byterun/bigarray.c` make it possible to create bigarrays both from the bigarray library and from the unix library. The header file `bigarray.h` moves to `byterun/caml/bigarray.h` accordingly. The `map_file` implementations move to `otherlibs/unix/mmap.c` and `otherlibs/win32unix/mmap.c`. Some bigarray allocation code shared between the two implementations is put in `otherlibs/unix/mmap_ba.c`. Through a couple of `#ifdef`, the `map_file` implementations can also be compiled from within `otherlibs/bigarray` with the same semantics they had in 4.04. As a consequence, the bigarray library still contains a standalone, Unix-independent implementation of `Bigarray.*.map_file`; the only difference with 4.04 is that it is marked deprecated. Current status: compiled and lightly tested under Unix. Win32 implementation neither compiled nor tested.

caml_unix_map_file and related functions were defined both in the stub lib for Unix and in the stub lib for Bigarray. This caused strange behavior when both Unix and Bigarray were linked, as found by the extra tests. The extra tests (all two of them) check that Unix.map_file raises reasonable Unix_error exceptions in common error cases.

Also: cosmetic improvement to corresponding test.

Follow-up to e763c83

xavierleroy · 2017-03-13T18:39:04Z

One more conflict resolved. One more rebasing done. I'm getting tired. Can we please merge? pretty please?

gasche · 2017-03-13T21:28:21Z

The CI is "all green" and the merge target is trunk. I think it's an easy choice to merge -- let's do it right now. But the real question is whether you also want to merge this in 4.05?

shindere · 2017-03-14T07:37:15Z

This broke CI because the defined MAP_FILE clashes with a macro defined in
sys/mman.h.

Fix pushed to trunk: a01cfa2

xavierleroy · 2017-03-14T19:26:46Z

@shindere : thanks for fixing this problem that I should have seen coming!

@gasche : I don't know if I want to backport this to 4.05 (it will take a bit of work, now that we've done the revert on 4.05). I'd like some more discussion on caml-devel and a consensus.

shindere · 2017-03-14T19:45:20Z

Xavier Leroy (2017/03/14 12:26 -0700):

@shindere : thanks for fixing this problem that I should have seen coming!

My pleasure!

shindere · 2017-03-19T09:26:56Z

Other fix pushed to trunk: a6d7705
(missing inclusion of caml_copy_string in otherlibs/win32unix/mmap.c).

@SInCE

This reverts commit 5ed7200. GPR#997 introduced a hard dependency of Bigarray on Unix, while there previously only was a type-level dependency. This break some programs, such as Camomile, that linked bigarray.cma but not unix.cma. A solution is being worked out in GPR ocaml#1077 to remove the dependency, but I would like to go forward with opam package testing on the 4.05 branch, and getting the details right for GPR#1077 requires some care, so I wouldn't feel comfortable rushing to merge it. I had to handle the following conflicts: otherlibs/unix/unix.mli ("@SInCE 4.05.0" was added in faab91a) testsuite/tests/lib-bigarray-file/mapfile.ml (changed by 5839c98; I kept and adapted the new version) testsuite/tests/lib-bigarray-file/mapfile.reference

xavierleroy added no-change-entry-needed work-in-progress labels Mar 3, 2017

xavierleroy mentioned this pull request Mar 3, 2017

Deprecate Bigarray.*.map_file and add Unix.map_file #997

Merged

gasche reviewed Mar 5, 2017

View reviewed changes

xavierleroy removed the work-in-progress label Mar 6, 2017

xavierleroy removed the no-change-entry-needed label Mar 7, 2017

ghost reviewed Mar 7, 2017

View reviewed changes

gasche mentioned this pull request Mar 12, 2017

Revert "Deprecate Bigarray.*.map_file and add Unix.map_file (#997)" #1102

Merged

xavierleroy force-pushed the map_file_migration branch from 743bf00 to a239dc1 Compare March 12, 2017 18:04

xavierleroy added 14 commits March 13, 2017 19:15

Possibly wrong #include + update dependencies

731d25f

Define CAML_INTERNALS to see more of caml/bigarray.h

fc5ed42

Update tests/unboxed-primitive-args: bigarray.h is now in caml/

32906dc

win32unix/mmap.c: error reporting was all wrong, attempting a fix

2c2d5f7

More tweaks on the Unix.map_file error test

803256c

Unix.map_file: fix error reporting in the win32 implementation

fdc2805

Also: cosmetic improvement to corresponding test.

Update Changes for GPR#1077 (Unix.map_file)

04cf067

Wrong error reporting in the IN_OCAML_BIGARRAY legacy case

d0ca012

bigarray.c: Assert -> CAMLassert

309be60

Missing declaration of caml_ba_sys_error()

a5795f0

Follow-up to e763c83

Wrong .depend, shoot again

6af452d

Replace Assert with CAMLassert

8afbd72

xavierleroy force-pushed the map_file_migration branch from 179ad24 to 8afbd72 Compare March 13, 2017 18:36

gasche merged commit 34d21c4 into trunk Mar 13, 2017

xavierleroy deleted the map_file_migration branch March 14, 2017 13:44

This was referenced Jul 14, 2017

Merge 4.05 into trunk #1244

Merged

Introduce UnixLabels.map_file #1245

Merged

hannesm mentioned this pull request Nov 16, 2017

Support OCaml 4.06.0 mirage/ocaml-solo5#27

Merged

This was referenced Mar 14, 2019

reversing the Unix and Bigarray dependency #6139

Closed

Cannot close properly a file with memory mapped blocks (Bigarray.map_file) #6567

Closed

stedolan pushed a commit to stedolan/ocaml that referenced this pull request Mar 21, 2023

Bump magic numbers (ocaml#1077)

d1435ed

Conversation

xavierleroy commented Mar 3, 2017

Uh oh!

gasche commented Mar 3, 2017

Uh oh!

xavierleroy commented Mar 3, 2017

Uh oh!

gasche Mar 5, 2017

Choose a reason for hiding this comment

Uh oh!

gasche commented Mar 5, 2017

Uh oh!

ghost commented Mar 6, 2017

Uh oh!

xavierleroy commented Mar 6, 2017

Uh oh!

xavierleroy commented Mar 6, 2017

Uh oh!

gasche commented Mar 6, 2017

Uh oh!

xavierleroy commented Mar 7, 2017

Uh oh!

gasche commented Mar 7, 2017

Uh oh!

ghost commented Mar 7, 2017

Uh oh!

gasche commented Mar 7, 2017

Uh oh!

ghost Mar 7, 2017

Choose a reason for hiding this comment

Uh oh!

ghost Mar 10, 2017

Choose a reason for hiding this comment

Uh oh!

xavierleroy Mar 10, 2017

Choose a reason for hiding this comment

Uh oh!

ghost Mar 10, 2017

Choose a reason for hiding this comment

Uh oh!

ghost Mar 7, 2017

Choose a reason for hiding this comment

Uh oh!

xavierleroy Mar 8, 2017

Choose a reason for hiding this comment

Uh oh!

shindere commented Mar 7, 2017

Uh oh!

xavierleroy commented Mar 8, 2017

Uh oh!

shindere commented Mar 8, 2017 via email

Uh oh!

damiendoligez commented Mar 9, 2017

Uh oh!

shindere commented Mar 9, 2017 via email

Uh oh!

damiendoligez commented Mar 10, 2017

Uh oh!

ghost commented Mar 10, 2017

Uh oh!

gasche commented Mar 12, 2017

Uh oh!

xavierleroy commented Mar 13, 2017

Uh oh!

gasche commented Mar 13, 2017

Uh oh!

shindere commented Mar 14, 2017

Uh oh!

xavierleroy commented Mar 14, 2017

Uh oh!

shindere commented Mar 14, 2017 via email

Uh oh!

shindere commented Mar 19, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development