Once we have some logic in the compiler codebase that understand magic numbers, we can use it in other places than (ocaml)objinfo. In particular, when the compiler itself reads a compiled object file, it currently has the same irritating behaviour of not recognizing that object file from other OCaml versions are a bit more valid than random text files with the wrong extension, and we could easily fix that. Depending on the feedback on this PR, I may try to do this it as well.

There is no Changes entry for this one. I'm not quite sure where I should put it, and if I understand correctly objinfo is not currently installed along with the compiler so it's arguably not an user-facing change that needs to be documented.

Look for read_cmi in typing/cmi_format.ml.

Oh, I hadn't seen that the cmi reading part had this same logic. But I think this could be generalized to other areas of the compiler (all those that read object files), for example driver/pparse.ml (I'm not familiar with the workflow of the people passing binary AST files, but I would assume that version mismatches do happen), asmcomp/compilenv, bytecomp/byte{link,librarian}, dynlink, and toplevel/topdirs (load directive).

I agree it's a good idea to generalize it, especially if you share the implementation.

It is installed:

$ which ocamlobjinfo
/home/whitequark/.opam/4.02.3/bin/ocamlobjinfo

So I started working a bit more on this PR, but:

• I'd rather wait for the Changelog to be branched off before adding a Changes entry (as this is not going to be in 4.03, I suppose)
• there is some non-trivial interaction between extending this work to pparse.ml and Make Pparse functions type-safe #351 , so I would rather do the extension after Make Pparse functions type-safe #351 has been merged.

I'll thus wait for the release branching.

I had forgotten about this PR; I would like to work on it more sometime after the release.

@gasche Would you have time to bring this up-to-date?

@mshinwell thanks for your hard triaging work today :-)

I rebased my patch against trunk. I should also look at Damien's remark on using in in typing/Cmi_format.read_cmi and other places; this is nice but it also increases the invasiveness of the patch a bit.

There seems to be a minor double-close_in bug in the current read_cmi: it is my impression that the Error exception path results in ic being closed twice.

So I tried to use the new functions in a few places in the compiler -- namely typing/Cmi_format.read_cmi and asmcomp/Compilenv.read_unit_info. I would be interested in feedback before trying to convert the other use sites -- ideally, I would like an approval to merge in principle.

A difficulty that made this quite more difficult than I expected is that all use-sites have already their own error-reporting logic, and that it is not trivial to integrate the Magic_number explicit approach without either changing the error-reporting logic or ending up with code that is roughly as long as what was before (because the concision gain is lost in re-routing exceptions of one kind into another). I had to do several iterations of convenience functions in the Magic_number module to end up with something worthwhile.

Changing the error-reporting logic results in better-quality error messages (which was the goal in the first place), but it requires changing the exception types for the affected modules. We have to think about this, because it means that it can break compiler-libs-using code. Luckily, the changes made will break programs at compile-time, not runtime.

For each of the two updated sites I thus did two iterations:

• the first one is to just use the functions of Magic_number, with no changes to the error reporting logic:
  read_cmi is updated in 3f36680, and read_unit_info in 7eeb8f2
• the second one changes the error-reporting logic to provide consistent information among all Magic_number users: read_cmi is updated in 2212e4c (besides regularity we do not gain much as it already supported older/newer recommendations), read_unit_info is updated in 47012bb.

@damiendoligez, would have a look and give your opinion on whether you think these changes are worthwhile? If we consider that the improved error messages are not worth changing the error type declarations, I can easily roll-back to the "first iteration" approach.

I just rebased the PR on top of trunk. I also redesigned the API of the Magic_number module, and (following @shindere's comments on feature bloat) restricted the scope of the PR to improving objinfo.ml only. I also rebased the changes to Compilenv and Cmi_format to use the new module, to test the new API, but I will keep them for a future PR.

In his review @Drup had made criticism of the API, which relied too much on exceptions. I used exceptions to make caller-site changes less invasive. The new API is written without any exception, purely using the result type. I think the new API is sensibly better, but adapting the callsites is now more work. (I buy @Drup's argument that we don't read magic numbers very often in the compiler codebase and can afford the extra verbosity in exchange for more user-friendly failure messages.) In general I went in the direction of making the modified code as good as I could, even if it meant restructuring the code rather than just replacing a couple function calls; the changes to objinfo.ml are also a refactoring.

@Drup, would you be willing to give another round of review to the PR?

(I managed to convince Travis to indeed check this PR.)

I rebased the PR to address @Drup's last batch of comments.

I went ahead and merged in trunk. Thanks again @damiendoligez and @Drup for the reviews.

This story arc is not yet complete; only ocamlobjinfo uses the new machinery as of now, all other parts of the compiler that read magic numbers could be improved by adopting it as well. I (and maybe others as well?) will send extra PRs over time.

The objinfo test is now broken on Wygwin32.

File question.cmxs
Error: size of section .data unknown
Unable to read info on file question.cmxs

And by the way, this also affects the 4.10 branch.

Do you think that it is related to this PR? This PR was merged two days ago, in trunk only (I think? no reason to cherry-pick to 4.10), so I don't think it could explain a broken test on 4.10.

(I think that today, being a general strike day in France, is an excellent day to not investigate an obscure Cygin32 failure (don't count on me!), but I'm going to humor you and ask the first question that comes to mind.) Do we know when the failures started to happen?

I confirm the hypothesis mentionned above.

objinfo_helper can not find the caml_header_plugin symbol for several
reasons. I have a dirty but working fix. Still discussing off-line to come
up with a cleaner one, will open a new PR ASAP. Sorry for the false positive
here.