Skip to content

Enable msvc64 asmcomp tests#974

Closed
dra27 wants to merge 3 commits intoocaml:trunkfrom
dra27:enable-msvc64-asmcomp-test
Closed

Enable msvc64 asmcomp tests#974
dra27 wants to merge 3 commits intoocaml:trunkfrom
dra27:enable-msvc64-asmcomp-test

Conversation

@dra27
Copy link
Copy Markdown
Member

@dra27 dra27 commented Dec 15, 2016

So for some reason I decided to port tests/asmcomp to msvc64 this evening. It was interesting, because I've never written any assembler before (and indeed, arguably, I still haven't...).

The first commit corrects a grim hack I added in 4.03.0 which allowed the msvc32 version of these tests to work - i386nt.asm had become stale and I should have fixed that, rather than resorting to hacking the assembler output.

This GPR includes a fix which is already part of #955 (and the fix in #955 is neater, so it should be merged first and the middle commit here removed).

There is a problem which seems to arise because of symbols not being decorated in the amd64 backend - ml64 is very fussy about names and I had to rename some symbols in cmm files. Is that OK - or is this is a sign of a (presumably) hidden problem with the msvc64 backend?

Another thing - why do we not run the programs compiled from the .cmm files? I ask because all of these seem to segfault as soon as they enter the OCaml portion (i.e. some can display usage instructions, but that comes from main.c). The mingw64 examples segfault too, so I'm guessing that something about amd64.S is wrong and therefore (presumably) my translation in amd64nt.asm. Presumably it is informative to run these programs too?

/cc @xavierleroy, @mshinwell

Previously the assembler output had symbols removed by grep - now these
are added to the "dummy" symbols in i386nt.asm.
@dra27
Copy link
Copy Markdown
Member Author

dra27 commented Dec 15, 2016

There was one other question - why is integr.cmm not tested (apart from that the fact that it doesn't link)?

@xavierleroy
Copy link
Copy Markdown
Contributor

Not to rain on your parade, but:

The tests in testsuite/asmcomp have a rather specific purpose, namely to help with porting to a new target architecture. (That's because those tests exercise only the ocamlopt back-end, and don't even need the runtime system.) Once the whole OCaml system is running, I think those tests are highly redundant with what is tested elsewhere by calling ocamlopt normally.

I don't think any future target architecture will use the ml64 assembler...

@dra27
Copy link
Copy Markdown
Member Author

dra27 commented Dec 17, 2016

Is that true for the flambda tests too? In which case, are they more sensibly put elsewhere to stay away from continuous integration? (up one level in testsuite/asmcomp - cf. testsuite/interactive)

@dra27
Copy link
Copy Markdown
Member Author

dra27 commented Dec 17, 2016

Could the symbol naming thing ever be an issue, or is that only arising with those hand-crafted (?) cmm files?

@dra27
Copy link
Copy Markdown
Member Author

dra27 commented Dec 19, 2016

OK, having spoken with Mark it sounds as though we have two, or perhaps even three sets of this tests in one directory where perhaps there should be two directories.

For me, it's a bookkeeping nuisance that msvc64 skips all the tests as it makes #975 unnecessarily harder to merge. Can I suggest this course of action:

  1. Keep the fix to i386nt.asm, just because it's cleaner than what I did before!
  2. Drop the new amd64nt.asm and associated cmm changes
  3. Move all the cmm stuff to tests/asmcomp/porting and disable it for all architectures (like manual-intf-c is at the moment). A developer writing a new backend can then explicitly enable them (or, perhaps better, I can make it that when called from make all or make parallel they don't run but when called via make one DIR=tests/asmcomp/porting or cd tests/asmcomp/porting ; make then they do run) and I'll put a README.md in the directory explaining that?
  4. Move the remaining ML tests (inc. the flambda ones) to tests/asmcomp/ocamlopt and enable the MLCASES for msvc64, because those do test ocamlopt in full and probably should have been being run all along?

@shindere
Copy link
Copy Markdown
Contributor

shindere commented Dec 22, 2016 via email

@dra27
Copy link
Copy Markdown
Member Author

dra27 commented Dec 22, 2016

@shindere - I want to unblock 975 asap (though I like your proposed change there a lot and will implement that first)... I'm happy to make the changes detailed, as long as there's consensus on the plan!

@shindere
Copy link
Copy Markdown
Contributor

shindere commented Dec 22, 2016 via email

@dra27
Copy link
Copy Markdown
Member Author

dra27 commented Dec 23, 2016

ocamltest doesn't affect the reporting at the end of the testsuite run, though, does it (at least initially?), which is what 975 works on? My hope with the changes there is to have it in place during the development cycle for 4.05 - there were quite a few last-minute testsuite breakages fixed in 4.04.0 (it's been nice with the 4.03 and 4.04 releases to know that the test suite really is supposed to be a 100% pass on all platforms!). There doesn't seem to be any objection (so far) to the principle of what I suggest here - I'll put the diff in (probably after Christmas now) and we can discuss it further then.

Good news for merging ocamltest soon!

@shindere
Copy link
Copy Markdown
Contributor

shindere commented Jan 9, 2017 via email

@xavierleroy
Copy link
Copy Markdown
Contributor

Catching up on this discussion: I agree tests/asmcomp is a mess right now and should be split in manual / automatic tests as @dra27 suggests.

@dra27
Copy link
Copy Markdown
Member Author

dra27 commented Feb 16, 2017

@damiendoligez - is it ok to merge testsuite alterations in 4.05 as a matter of course (obviously after review for trunk), or would you prefer those to be explicitly approved too?

@damiendoligez
Copy link
Copy Markdown
Member

Testsuite changes are OK for 4.05, but now that the branch is done, we want to merge ocamltest into trunk very soon, and I'm not sure how much it will impact your work here.

@dra27
Copy link
Copy Markdown
Member Author

dra27 commented Feb 16, 2017

We'll see - it's at least part of the reason I hadn't moved on this and a couple of others!

@damiendoligez damiendoligez added this to the 4.07-or-later milestone Sep 29, 2017
@damiendoligez damiendoligez removed this from the consider-for-4.07 milestone Jun 4, 2018
@xavierleroy
Copy link
Copy Markdown
Contributor

2.5 years later, any news on this PR? My take is that the "asmcomp" tests (handwritten in C--) are useful only when porting OCaml to a new architecture. Hence, running them on x86 with MASM is not particularly useful -- they are run on x86 anyway. If we agree on this reasoning, could we please close this PR?

@dra27 dra27 mentioned this pull request Nov 30, 2019
@dra27 dra27 closed this Nov 25, 2020
@dra27 dra27 deleted the enable-msvc64-asmcomp-test branch July 6, 2021 14:48
EmileTrotignon pushed a commit to EmileTrotignon/ocaml that referenced this pull request Jan 12, 2024
* line editing on the If statements tutorial
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants