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

Merge the runtime directories #1852

Merged
merged 1 commit into from Jun 28, 2018

Conversation

Projects
None yet
7 participants
@shindere
Copy link
Contributor

commented Jun 21, 2018

This PR merges the asmrun/ and byterun/ directories into the
runtime/ directory.

This has been achieved in four steps, each of them being implemented
in its own commit.

  1. A few adjustments in asmrun/Makfile and byterun/Makefile.

This includes giving better names to the variables listing files.

Also, this first commit makes sure the object files get a name which
shows to which library they belong. For instance, the object file
produced from foo.c for building libcamlrun.a is called
foo_libcamlrun.o.

  1. Give files a unique name.

Five files were present with the same basename under both asmrun/ and
`byterun/". This commit renames them so that their basenames are unique.

  1. Rename the byterun/ directory to runtime/.

This step leaves asmrun/ unchanged, except for the changes that
are strictly required by the renaming of byterun/ to runtime/.

  1. Move the content of the asmrun/ directory into runtime/.

This mainly involves moving all the files from asmrun/ to runtime/
and merging the two Makefiles. More than a mere merge, this step tries
to reorganise the resulting makefile to imiprove its readability
and clarity as much as possible.

As a sidenote: commit 3 leaves the repository in a state were asmrun/
coexists with runtime/. If, for some reason, it turns out that it
is preferable to avoid such an intermediate state, commits 3 and 4 can
always be squashed. They have been
implemented separately as an attempt to make the reviewing process simpler
and also because each commit seemed a big enough change in itself.

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2018

I like the change, but can you make the rationale explicit? What are the expected benefits?

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2018

@mshinwell

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2018

I would suggest that we agree on a common suffix for native and bytecode files in the runtime/ directory -- there seem to be a couple of different conventions at present.

As an aside, something that I trip up on frequently is that the header files are in a caml/ subdirectory. Could these just go directly in runtime/ too? (Or if not, maybe we could rename caml/ somewhere else -- possibly to include/, or even runtime_include/ at the top level.)

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2018

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2018

@alainfrisch: to complete what I wrote about the rationale, I think
the layout that this PR sets up is also closer than the previous one
to the way the developers think and talk about the code. I mean for
what I can hear generally talk about "the runtime system".

@dbuenzli

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2018

FWIW as a casual reader of the runtime system I'm usually heading to asmrun only to remember that what I'm looking for is in byterun so I welcome the change.

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2018

@xavierleroy

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2018

Re: include files, we need them in a caml/ subdirectory so that we can write #include <caml/mlvalues.h>. runtime/caml is a better (easier to find!) place than byterun/ocaml, but I agree with @mshinwell that include/caml would be an even better place. Actually I looked into this a while ago but didn't finish. Let's wait until the fate of this PR is decided before giving include/caml another try.

@xavierleroy
Copy link
Contributor

left a comment

Some not-very-consistent file names.

floats str array io extern intern hash sys parsing gc_ctrl md5 obj \
lexing $(UNIX_OR_WIN32) printexc callback weak compact finalise custom \
globroots backtrace_prim_native backtrace natdynlink debugger meta \
dynlink clambda_checks spacetime_native spacetime_snapshot afl bigarray)

This comment has been minimized.

Copy link
@xavierleroy

xavierleroy Jun 21, 2018

Contributor

Why fail_native but signals_asm ? It could make sense to have the same _native suffix for all files.

floats str array io extern intern hash sys parsing gc_ctrl md5 obj \
lexing $(UNIX_OR_WIN32) printexc callback weak compact finalise custom \
globroots backtrace_prim_native backtrace natdynlink debugger meta \
dynlink clambda_checks spacetime_native spacetime_snapshot afl bigarray)

This comment has been minimized.

Copy link
@xavierleroy

xavierleroy Jun 21, 2018

Contributor

Re backtrace_prim_native: why not just backtrace_native?

dynlink.c backtrace_prim.c backtrace.c spacetime.c afl.c \
bigarray.c
dynlink.c backtrace_prim_bytecode.c backtrace.c spacetime_bytecode.c \
afl.c bigarray.c

This comment has been minimized.

Copy link
@xavierleroy

xavierleroy Jun 21, 2018

Contributor

Likewise, backtrace_bytecode instead of backtrace_prim_bytecode?

interp misc stacks fix_code startup_aux startup_bytecode freelist major_gc \
minor_gc memory alloc roots_bytecode globroots fail_bytecode signals \
signals_byt printexc backtrace_prim_bytecode backtrace compare ints \
floats str array io extern intern hash sys meta parsing gc_ctrl md5 obj \

This comment has been minimized.

Copy link
@xavierleroy

xavierleroy Jun 21, 2018

Contributor

Likewise, why signals_byt but roots_bytecode?

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2018

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2018

@shindere shindere force-pushed the shindere:merge-runtime-directories branch 4 times, most recently from 5154b0a to f0d1501 Jun 22, 2018

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2018

PR rebased and updated to use shorter suffixes for filenames:
_byt instead of _bytecode and _nat instead of _native.

@mshinwell your initial comment about making all the filenames coherent
has also been taken into account.

@shindere shindere force-pushed the shindere:merge-runtime-directories branch 2 times, most recently from 34da0e8 to acf8d8f Jun 26, 2018

@damiendoligez
Copy link
Member

left a comment

Looks good except for a few comments that should be fixed.

@@ -70,7 +70,7 @@ CAMLprim value caml_backtrace_status(value vunit)
(!li->loc_valid && li->loc_is_raise)
caml_debuginfo_location guarantees that when li->loc_valid is
0, then li->loc_is_raise is always 1, so the latter test is
useless. We kept it to keep code identical to the byterun/
useless. We kept it to keep code identical to the runtime/

This comment has been minimized.

Copy link
@damiendoligez

damiendoligez Jun 27, 2018

Member

I don't understand this comment. Can we take the opportunity to fix it?

@@ -59,8 +60,8 @@ CAMLextern int caml_backtrace_active;
/* The [backtrace_slot] type represents values stored in the
* [caml_backtrace_buffer]. In bytecode, it is the same as a
* [code_t], in native code it as a [frame_descr *]. The difference
* doesn't matter for code outside [backtrace_prim.c], so it is just
* exposed as a [backtrace_slot].
* doesn't matter for code outside [backtrace{byt,nat}.c],

This comment has been minimized.

Copy link
@damiendoligez

damiendoligez Jun 27, 2018

Member

There's a missing underscore.

*
* [backtrace.c] has a unique implementation, and exposes a uniform
* higher level API above [backtrace_prim.c].
* higher level API above [backtrace_prim_{bytecode,native}.c].

This comment has been minimized.

Copy link
@damiendoligez

damiendoligez Jun 27, 2018

Member

Same here.
[edit: this refers to a comment about 10 lines above that wasn't recorded by GitHub for some reason]

@xavierleroy
Copy link
Contributor

left a comment

Thanks for your work on appropriate names for source files. I finished reading the PR and have a few more comments that I would like to see discussed before merging.

  • The naming of object files foo_libcamlrund.$(O)$, etc, is quite heavy. Befoer we made do with one letter suffixes .d.$(O). Maybe one letter is no longer enough but two letters would certainly be.

  • Alternatively, object files could be generated in subdirectories of runtime/. Then, it would make sense to use a full name such as libcamlrund, but as a directory name: libcamlrund/foo.$(O). For one thing, it would reduce clutter in the runtime/ directory. We did something similar with the compilerlibs/ directory at top-level, which contains compiled files only.

  • I still dislike the GNU make kung-fu on lines 331 to 365 of runtime/Makefile. Spelling out the 9 rules for .c to .$(O) would be shorter and easier to read.

@damiendoligez

This comment has been minimized.

Copy link
Member

commented Jun 27, 2018

@xavierleroy

I still dislike the GNU make kung-fu on lines 331 to 365 of runtime/Makefile. Spelling out the 9 rules for .c to .$(O) would be shorter and easier to read.

I rather like the declarative flavour of the first part (lines 331-347).

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Jun 27, 2018

@shindere shindere force-pushed the shindere:merge-runtime-directories branch from fa91e96 to 4e6cffc Jun 27, 2018

@xavierleroy

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2018

deciding to put the object files in a different directory makes the build rules slightly more complex

I convinced myself that the same rules apply to name_suffix.$(O) and to directory/name.$(O). Well, make clean needs another rm pattern.

and there is also the fact that these directories will have to be created somewhere and it's not yet clear to me what would be the best place.

Just like compilerlibs/: the directories are part of the source tree and don't need to be created during build. Git cannot record empty directories, so just put a .gitignore in there so that the directory is not empty.

build the whole compiler out of source tree

That is the extreme solution, and an overkill in my opinion. I don't mind some derived files lying around; I'm worried about having 9 derived files with super-long names for every .c source file.

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Jun 27, 2018

@shindere shindere force-pushed the shindere:merge-runtime-directories branch 2 times, most recently from bd6d063 to 933c361 Jun 28, 2018

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Jun 28, 2018

@xavierleroy: I just pushed a commit that shortens the names of the
generated object files. Those for the bytecode (resp. native)
runtime have a suffix starting with _b (resp. _n).
Nothing else is added to this suffix for the "normal" versoins
(those for libcamlrun and libasmrun. For the other libraries, the
suffixes used before the PR are used, so e.g. foo_b.o, foo_bd.o and
foo_npic.o.

@xavierleroy
Copy link
Contributor

left a comment

The shorter object file names are welcome, thanks. I reviewed the new commits since last time and have no further comments to make.

@shindere shindere force-pushed the shindere:merge-runtime-directories branch from 42c8f8d to d3e7359 Jun 28, 2018

@shindere shindere merged commit d3e7359 into ocaml:trunk Jun 28, 2018

0 of 4 checks passed

continuous-integration/appveyor/branch Waiting for AppVeyor build to complete
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Jun 28, 2018

Squashed and merged, thanks a lot for the reviews.

@shindere shindere deleted the shindere:merge-runtime-directories branch Jun 28, 2018

@xavierleroy

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2018

FYI: CI fails on OpenBSD 32, tests/output-complete-obj/'test.ml' with 1.1.1 (ocamlc.byte). See https://ci.inria.fr/ocaml/job/main/526/flambda=false,label=ocaml-openbsd-32/console

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Jun 28, 2018

@xavierleroy

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2018

I am not sure it's related to this GPR

You're very likely right. I see earlier failures, indeed. Sorry for the noise.

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Jun 28, 2018

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Jun 29, 2018

@xavierleroy The failure on OpenBSD you reported here is actually caused by
#GPR#1856, see comment at #1856 (comment)

@gasche

This comment has been minimized.

Copy link
Member

commented Mar 31, 2019

It seems that #8561 is a user-visible regression, and that may have been caused by this change. What would be the right fix?

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.