Make it easier to bump and duplicate magic numbers

[shindere]

This stems from #11996 and addresses this comment by @nojb.

The magic numbers (actually strings) used to identify binary files produced by the compiler are the concatenation of three pieces: an 8-bytes prefix common to all of them (Caml1999), a 1-byte file type (executable, CMO, CMA etc.) and a 3-bytes version identifier.

The current definitions of these magic numbers carry some redundancy: the prefix and version fragments appear in each magic number definition. This makes bumping them non-trivial, since a change in the version string will need to be applied to each definition, of which only a sub-string needs to be modified.

Also, a few magic numbers are needed at several places in the codebase: the magic number for executables, for instance, is used in both the runtime (C world) and in the compiler (OCaml world) and its definition is thus duplicated. The definition of magic numbers for CMO, CMA and CMXS formats are required by both the compiler and the dynamic loader (dynlink) and here, the cost for avoiding the duplication is that dynlink depends on the Config module of compilerlibs, hence the link with #11996.

The present PR is a proposal to address all these limitations by doing three things:

1. Define the basic blocks from which magic numbers are built in the configure system (namely in build-aux/ocaml_version.m4, close to the version numbers), so that all the pieces can be disseminated wherever they are useful in the codebase during the configure stage.
2. Make all the fragments of magic numbers available individually in each file that needs them and reconstruct them from these bits either at configure or at compile time. This reduces the need to bump magic numbers individually, as the version is stored in fewer places from which all the magic numbers are derived.
3. Introduce the bump-magic-numbers tool to both document and automate the bumping process.

The bump-magic-numbers tool is used in Inria's CI bootstrap job. This means that the tool, which is intended to be used when preparing releases, will be tested regularly.

Moreover, this means that the current PR makes the bootstrap CI job both more strict and more faithful to what happens when preparing a release since it makes sure the compiler bootstrap works even when all the magic numbers are changed (as happens for releases), whereas on current trunk the bootstrap job only changes the magic number for executables.

[dustanddreams]

Consider keeping the files list sorted.

Suggested change

	$(addprefix runtime/caml/, m.h s.h version.h exec.h)
	$(addprefix runtime/caml/, exec.h m.h s.h version.h)

[shindere]

Hello, thanks a lot for the review! :) Miod Vallat (2023/10/12 00:41 -0700):

Consider keeping the files list sorted. ```suggestion $(addprefix runtime/caml/, exec.h m.h s.h version.h) ```

Okay why not :) Done.

[shindere]

Florian Angeletti (2023/10/13 02:15 -0700):

I am not sure if we want to move the computation of those constants to runtime time.

Is it the performances that worry you, or something else? Until now I can't think of another implementation but suggestions are warmly welcome!

[MisterDA]

Would it make sense to prefix these with OCAML_? I tend to get confused about which variables are relatively standard, say CC or CPP, and which are specific to the project.

[Octachron]

I concur, that sounds like a good idea, at the very least for the MAGIC_* variables.

[MisterDA]

Suggested change

	sed -i -e s/'define MAGIC_VERSION "..."'/"define MAGIC_VERSION \"$new_num\""/ \
	sed -i.tmp -e s/'define MAGIC_VERSION "..."'/"define MAGIC_VERSION \"$new_num\""/ \

For the rare case where you'd want to use this script from a BSD or macOS system. You need to specify a (possibly empty) extension for in-place editing with BSD sed. I also suggest changing the sed invocations below. This is the pattern that was previously used.

[Octachron]

After test, this is still incompatible with macOS sed? Using an explicit temporary file seems simpler .

[MisterDA]

Suggested change

	dd7927e156b7cb2f9
	Beware, though, that PR #12652, which is more recent than the commit
	mentionned above, changes the way magic numbers are defined.
	https://github.com/ocaml/ocaml/commit/dd7927e156b7cb2f9cb73d2d54a15a9c81921392[dd7927e]
	Beware, though, that https://github.com/ocaml/ocaml/pull/12652/[PR #12652],
	which is more recent than the commit mentioned above, changes the way
	magic numbers are defined.

I suggest adding links. There's also a typo s/mentionned/mentioned.

[shindere]

Thanks a lot for the review! Antonin Décimo (2023/10/16 05:07 -0700):

@MisterDA commented on this pull request. > +AC_SUBST([MAGIC_PREFIX], [MAGIC__PREFIX]) +AC_DEFINE([MAGIC_PREFIX], ["][MAGIC__PREFIX]["]) +AC_SUBST([MAGIC_VERSION], [MAGIC__VERSION]) +AC_DEFINE([MAGIC_VERSION], ["][MAGIC__VERSION]["]) +AC_SUBST([EXEC_FORMAT], [EXEC__FORMAT]) +AC_DEFINE([EXEC_FORMAT], ["][EXEC__FORMAT]["]) +AC_SUBST([MAGIC_LENGTH], [MAGIC__LENGTH]) +AC_SUBST([CMX_FORMAT]) +AC_SUBST([CMXA_FORMAT]) Would it make sense to prefix these with `OCAML_`? I tend to get confused about which variables are relatively standard, say `CC` or `CPP`, and which are specific to the project.

Well we could, but as long as it is for local variables, I never felt a pressing need for doing such a namespacing. By local I mean that the variables in question areuned internally only, in files that do not get installed, which is the case here I thnk. (We are only at the interface between our configure and build system.)

> +# is intended to be committed) and a few other *generated* files which are +# ignored by git but which need to be updated here so that the script +# can be used as part of the bootstrap procedure. +# So the update of build-aux/ocaml_version.m4 (as well as the one this +# implies to configure) are not strictly speaking needed for the +# on-going bootstrap, but rather for future builds. The update of +# generated files, in contrast, *is* required by the on-going bootstrap. + +# Fail on error +set -e + +new_num=$1 + +# Bump magic numbers in runtime/caml/exec.h + +sed -i -e s/'define MAGIC_VERSION "..."'/"define MAGIC_VERSION \"$new_num\""/ \ ```suggestion sed -i.tmp -e s/'define MAGIC_VERSION "..."'/"define MAGIC_VERSION \"$new_num\""/ \ ``` For the rare case where you'd want to use this script from a BSD or macOS system. You need to specify a (possibly empty) extension for in-place editing with BSD sed. I also suggest changing the sed invocations below. This is the pattern that was previously used.

I have done this change, thanks!

> dd7927e + +Beware, though, that PR #12652, which is more recent than the commit +mentionned above, changes the way magic numbers are defined. ```suggestion https://github.com/ocaml/ocaml/commit/dd7927e156b7cb2f9cb73d2d54a15a9c81921392[dd7927e] Beware, though, that #12652/[PR #12652], which is more recent than the commit mentioned above, changes the way magic numbers are defined. ``` I suggest adding links. There's also a typo `s/mentionned/mentioned`.

All done, thanks

[Octachron]

@shindere, after thinking further, I am fine with computing the magic prefix for the various binary file formats at runtime.

[shindere]

Florian Angeletti (2023/10/23 06:20 -0700):

@shindere, after thinking further, I am fine with computing the magic prefix for the various binary file formats at runtime.

Thanks! I am actually preparing an implementation that will satisfy your initial requirement. Let me finish it andshow it to you because I think there are chances you'll like it.

[shindere]

I have just pushed an updated version. It computes all the strings in C and OCaml sources at configure time. Following a clever suggestion by @dra27, the string representing magic numbers are uniquely quoted with `{magic|` nand `|magic}`, which makes the automaiton of their bumping easy. The `bump-magic-numbers` script has also been fixed to nolonger use sed's in-place editing mode, which should hopefully make the script more portable.

[Octachron]

I think it is better to remove the link to this commit, since the commit will point to files that are no longer versioned.

[Octachron]

I would propose to merge the previous paragraph with this sentence, while removing the Beware warning:

Since https://github.com/ocaml/ocaml/pull/12652/[PR #12652], to bump the magic numbers to version xyz simply run:

[Octachron]

The current state looks nice to me: it improves phase separation (more configuration-time variables are computed at configuration time) and the documentation of the bootstrap process.
I will let you decide if you want to improve the documentation and variable naming for the ambiguous one before merging.

[shindere]

Thanks! I just pushed a fix to the documentation (as a separate commit at the moment, to make sure it's okay, but which intends to be squashed if it is). Which variables do you feel would deserve a less ambiguous name?

[Octachron]

I would say MAGIC_PREFIX, MAGIC_LENGTH, MAGIC_VERSION and maybe EXEC_MAGIC_LENGTH whose meaning without context is a bit unclear.

Maybe the most natural way to fix that point is to not shorten MAGIC_NUMBER to MAGIC (in other words keep MAGIC_NUMBER_PREFIX) since MAGIC could refer to some other "magic" constants rather than a file format magic number. Moreover keeping MAGIC_NUMBER would be consistent with the variable names for the file format CMX_MAGIC_NUMBER, ... .

[shindere]

Florian Angeletti (2023/10/24 02:37 -0700):

I would say `MAGIC_PREFIX`, `MAGIC_LENGTH`, `MAGIC_VERSION` and maybe `EXEC_MAGIC_LENGTH` whose meaning without context is a bit unclear.

Given that `EXEC_MAGIC_LENGTH` is what is currently used on trunk, do we really want to change this at the risk of breaking code whereas we could avoid breaking it just by not doing that change? For the moment I tookthe conservative route and renamed only the other variables which were not there until now. I also removed the definition of `magic_length` in `Config` and its use in Misc because that made the code in Misc look a bit strange. In the future it could be good to move the definition of `kind_length` and `version_length` to the confiugration system, too, for coherency. But that can wait and, as discussed with @dra27 (who suggested this removal) it seems several approaches are possible so there is still romm for expermients to find out the most relevantone.