Extended COFF format (/bigobj) support #65

db4 · 2018-08-03T11:24:26Z

Looks like it works now. Actually, ECOFF is not very different from COFF: it uses another file header (ANON_OBJECT_HEADER_BIGOBJ, see winnt.h in Windows SDK) and allows more than 2^16 sections in a single file (NumberOfSections header field. has tipe DWORD)

bschommer · 2021-09-23T10:00:34Z

What is the status on this PR? Having support for /bigobj would be a nice to have feature.

dra27 · 2021-10-08T14:20:40Z

I had started a review to try to reduce some of the magic offset numbers. The main thing blocking it for me is a test case: both to verify myself that it works and to be able to continuous verify that it works in CI. At the moment I have to read up on how to generate one of these, generate it and figure out how to test for it in CI... none of which I mind having to do some day, but if it iwere to appear by magic, I'd be able to finish reviewing faster 🙂

bschommer · 2021-10-08T14:31:00Z

I had started a review to try to reduce some of the magic offset numbers. The main thing blocking it for me is a test case: both to verify myself that it works and to be able to continuous verify that it works in CI. At the moment I have to read up on how to generate one of these, generate it and figure out how to test for it in CI... none of which I mind having to do some day, but if it iwere to appear by magic, I'd be able to finish reviewing faster slightly_smiling_face

Doesn't the pull request already include some kind of mini test by compiling plug2.c with the /bigobj switch. Otherwise, I encountered it when working with the llvm libs compiled with MSCV.

dra27 · 2021-10-08T20:32:17Z

Ah: somehow I'd seen the first bit, and not noticed that it was plumbed in in test/Makefile too! This might just be related to another issue we've just started seeing with the latest version of the Windows SDK, but let's not get hopes up too much. I clearly do have less excuse to delay reviewing, beyond needing to refactor to test both with and without the switch on msvc...!

db4 · 2021-10-09T07:28:59Z

I had started a review to try to reduce some of the magic offset numbers. The main thing blocking it for me is a test case: both to verify myself that it works and to be able to continuous verify that it works in CI. At the moment I have to read up on how to generate one of these, generate it and figure out how to test for it in CI... none of which I mind having to do some day, but if it iwere to appear by magic, I'd be able to finish reviewing faster 🙂

Hmm, the original code contains those magic offsets as well. How are going to get rid of them? Replace by symbolic names?

As for tests - yes I was too lazy to create them :( Do you have any idea how to correctly test /bigobj functionality (especially for CI)

dra27

There were some minor refactorings - mainly to have slightly fewer names for numbers (I'd mis-characterised what I'd got before).

There are a couple of questions about the changes to the header reading.

For testing - does the current test definitely exercise the code which writes the new ECOFF file and is it then read back by another part of the test? (i.e. do we effectively do a round-trip of the file somewhere in amongst!).

Assuming that is the case, I think all that needs to happen is to do one test on MSVC with /bigobj and another without, which should be an easy tweak.

Because of #94, flexdll 0.40 will be released soon - as long as I can convince myself that the changes here definitely don't affect the normal COFF case (that's the basis for the questions about the version number reading following the \000\000\255\255 header) then I think this can (finally) be included, thanks!

dra27 · 2020-04-21T20:20:51Z

coff.ml

-  let get strtbl ic pos =
-    let buf = read ic pos 18 in
-    let auxn = int8 buf 17 in
+  let get bigobj strtbl ic pos =


Suggested change

let get bigobj strtbl ic pos =

let get ~bigobj strtbl ic pos =

(boolean flags are very unclear - I realise there are others already in the code!)

coff.ml

dra27 · 2020-04-21T20:31:34Z

coff.ml

@@ -397,7 +402,12 @@ module Symbol = struct
            "value=0x%08lx, sect=%s, storage=%s, aux=%S\n"
            s.value sect storage (Bytes.to_string s.auxs)

-  let put strtbl oc s =
+  let put bigobj strtbl oc s =


Suggested change

let put bigobj strtbl oc s =

let put ~bigobj strtbl oc s =

(as for get)

This breaks
https://github.com/alainfrisch/flexdll/blob/f40ed2d57bbb70ac8223d30a6ffa737f2c0e430e/coff.ml#L885

What's wrong with using the label here?! ~bigobj:x.bigobj?

coff.ml

dra27 · 2021-10-09T14:10:10Z

coff.ml

+      emit_int16 oc 0; (* Sig1 *)
+      emit_int16 oc 0xffff; (* Sig2 *)
+      emit_int16 oc 2; (* Version *)


Why is this written on this side (for bigobj) but not on the other (for the legacy one)?

There may well be a reason, I was just scratching my head trying to work it out!

I address your other comments later (they're mostly cosmetic and I have nothing against them. I tried to change the original sources minimally while you're trying to make them more readable). Regarding this:
https://github.com/alainfrisch/flexdll/blob/f40ed2d57bbb70ac8223d30a6ffa737f2c0e430e/coff.ml#L856-L873
I don't quite understand your question. Why Sig1, Sig2, and Version are not emitted for the standard COFF? Just because its header does not include these fields.

This makes sense to me now w.r.t. the comment below - I hadn't registered properly that what we had before was an incorrect reading of an ECOFF header (assuming an import library)

dra27 · 2021-10-09T14:10:43Z

coff.ml

@@ -859,11 +946,17 @@ module Lib = struct
    let obj size name =
 (*      Printf.printf "-> %s (size %i)\n" name size;  *)
      let pos = pos_in ic in
-      if size > 18 && read_str ic pos 4 = "\000\000\255\255"
-      then imports := Import.read ic pos size :: !imports


Was this an error before - having ascertained that the magic header was there, shouldn't have read the version, instead doesn't it get lumped onto the first symbol?

Sort of. This check would treat ANON_OBJECT_HEADER_BIGOBJ as IMPORT_OBJECT_HEADER (i.e. an import library). I didn't try to feed bigobj files to the current version of flexlink but suppose it will fail somewhere instead of showing an informative error message.

Ah, I see now (sorry, I could also have scoured the COFF spec, too) - what we had before was an assumption that the \000\000\255\255 header implied strictly an object library whereas now we actually read the header properly. Gotcha!

dra27 · 2021-10-09T14:14:43Z

(please could you rebase it on to current master, too)

…esting for bigobj support

Co-authored-by: David Allsopp <david.allsopp@metastack.com>

dra27 · 2021-10-10T10:14:39Z

You'll also want something like dra27@31e835f, I'm afraid!

db4 · 2021-10-10T10:38:08Z

You'll also want something like dra27@31e835f, I'm afraid!

OK, I'll do that. Thanks!

dra27 · 2021-10-10T11:49:13Z

I have the start of a plan to overhaul the way flexlink works over the next year or so so that instead of pretending be a linker, it is just a compilation step generating the extra objects you must then link yourself (the OCaml build system is becoming more complicated because there are many places we must worry about whether we're calling a linker, a C compiler or flexlink, which kinda defeats the point of flexlink pretending to be a linker). Anyway, the main thing is that at that point we'll definitely take the opportunity to drop all the old versions of OCaml 🙂

dra27 · 2021-10-10T11:51:29Z

Otherwise, I encountered it when working with the llvm libs compiled with MSCV.

Is there a quick test which could be extracted from that? As long as there aren't too many OCaml dependencies, CI already has an OCaml compiler available, so if there's something which can relatively quickly be built, it's always good to have more tests! Obviously, the test doesn't have to run on the entire matrix...

db4 · 2021-10-10T12:58:31Z

Is there a quick test which could be extracted from that?

I'll add /bigobj-enabled demo_msvc build to CI. But Appveyor is ridiculously laggy, so I have to wait another hour for the previous commit CI job to complete...

dra27 · 2021-10-10T13:14:37Z

Fortunately, once the AppVeyor build of master has finished the caches will all be primed, so the next build should be quite a bit quicker! We've (at OCaml Labs) nearly got our Windows build cluster up and running... I'm hoping to replace the AppVeyor CI with a rather more parallel one on that!

db4 · 2021-10-11T10:29:46Z

@dra27 I think I've addressed all your comments (except labeling bigobj that complicates partial application). CI is all green :)

dra27 · 2021-10-11T10:58:24Z

Makefile

@@ -207,7 +207,7 @@ flexdll_initer_mingw64.o: flexdll_initer.c


 demo_msvc: flexlink.exe flexdll_msvc.obj flexdll_initer_msvc.obj
-	$(MSVC_PREFIX) $(MAKE) -C test clean demo CHAIN=msvc CC="$(MSVCC)" O=obj
+	$(MSVC_PREFIX) $(MAKE) -C test clean demo CHAIN=msvc CC="$(MSVCC)" BIGOBJ="/bigobj" O=obj


These need removing now, I think?

Ahh, I've just forgot what I did 3 years ago :( My last commit adding
https://github.com/alainfrisch/flexdll/blob/4da3dac05d715f25a27d7b56bc37d9dde09e37f6/appveyor_build.sh#L168-L170
is useless, /bigobj test was already there (added in 7ccf31b). Will drop the last commit instead.

I think it's the original commit you want to drop - I'd prefer it to be that the test msvc and msvc64 by default do not test /bigobj but that AppVeyor runs those tests twice, once with and once without (which I think is what's then happening here?)

I think it's the original commit you want to drop - I'd prefer it to be that the test msvc and msvc64 by default do not test /bigobj

Why? 3 years ago I decided to test COFF/ECOFF simultaneously: for MSVC plug1.c is compiled without /bigobj and plug2.c with /bigobj.

but that AppVeyor runs those tests twice, once with and once without (which I think is what's then happening here?)

Yes, the last commit effectively runs the same test twice, that's why I'm going to drop it.

dra27 · 2021-10-11T11:00:09Z

I think there's an old part in the test Makefile which needs removing and I'd missed the comment about the label. Thanks for all this, though - it does indeed look very nearly ready to merge!

dra27 · 2021-11-29T17:22:38Z

Thanks very much!

db4 changed the title ~~Ecoff support~~ Extended COFF format (/bigobj) support Aug 3, 2018

alainfrisch closed this Sep 6, 2018

alainfrisch reopened this Sep 6, 2018

dra27 reviewed Oct 9, 2021

View reviewed changes

db4 and others added 4 commits October 9, 2021 21:49

Extended COFF format (/bigobj) support

1df2889

Skip anonymous obj files (sig1=0, sig2=0xFFFF, version=1), add some t…

7ccf31b

…esting for bigobj support

Refactor bigobj support

e8dbb4c

Apply suggestions from code review

9437fa3

Co-authored-by: David Allsopp <david.allsopp@metastack.com>

db4 force-pushed the ecoff-support branch from 9215557 to 9437fa3 Compare October 9, 2021 20:59

OCaml 3.x compatibility

79db71f

db4 force-pushed the ecoff-support branch from 64873a1 to 79db71f Compare October 10, 2021 12:46

dra27 reviewed Oct 11, 2021

View reviewed changes

db4 force-pushed the ecoff-support branch from 4da3dac to 79db71f Compare October 15, 2021 06:13

dra27 merged commit 52e8c68 into ocaml:master Nov 29, 2021

db4 deleted the ecoff-support branch December 7, 2021 14:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extended COFF format (/bigobj) support #65

Extended COFF format (/bigobj) support #65

db4 commented Aug 3, 2018

bschommer commented Sep 23, 2021

dra27 commented Oct 8, 2021

bschommer commented Oct 8, 2021

dra27 commented Oct 8, 2021

db4 commented Oct 9, 2021

dra27 left a comment

dra27 Apr 21, 2020

dra27 Apr 21, 2020

db4 Oct 9, 2021

dra27 Oct 11, 2021

dra27 Oct 9, 2021

db4 Oct 9, 2021

dra27 Oct 9, 2021

dra27 Oct 9, 2021

db4 Oct 9, 2021 •

edited

Loading

dra27 Oct 9, 2021

dra27 commented Oct 9, 2021

dra27 commented Oct 10, 2021

db4 commented Oct 10, 2021

dra27 commented Oct 10, 2021

dra27 commented Oct 10, 2021

db4 commented Oct 10, 2021

dra27 commented Oct 10, 2021

db4 commented Oct 11, 2021

dra27 Oct 11, 2021

db4 Oct 11, 2021 •

edited

Loading

dra27 Oct 11, 2021

db4 Oct 11, 2021

dra27 commented Oct 11, 2021

dra27 commented Nov 29, 2021

	let get bigobj strtbl ic pos =
	let get ~bigobj strtbl ic pos =

Extended COFF format (/bigobj) support #65

Extended COFF format (/bigobj) support #65

Conversation

db4 commented Aug 3, 2018

bschommer commented Sep 23, 2021

dra27 commented Oct 8, 2021

bschommer commented Oct 8, 2021

dra27 commented Oct 8, 2021

db4 commented Oct 9, 2021

dra27 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

db4 Oct 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dra27 commented Oct 9, 2021

dra27 commented Oct 10, 2021

db4 commented Oct 10, 2021

dra27 commented Oct 10, 2021

dra27 commented Oct 10, 2021

db4 commented Oct 10, 2021

dra27 commented Oct 10, 2021

db4 commented Oct 11, 2021

Choose a reason for hiding this comment

db4 Oct 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dra27 commented Oct 11, 2021

dra27 commented Nov 29, 2021

db4 Oct 9, 2021 •

edited

Loading

db4 Oct 11, 2021 •

edited

Loading