Closed db4 closed 2 years ago
What is the status on this PR? Having support for /bigobj
would be a nice to have feature.
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 🙂
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.
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...!
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)
(please could you rebase it on to current master, too)
You'll also want something like https://github.com/dra27/flexdll/commit/31e835f853eaabb4194b6cc4e6ac0bf7ffa65701, I'm afraid!
You'll also want something like dra27@31e835f, I'm afraid!
OK, I'll do that. Thanks!
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 🙂
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...
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...
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!
@dra27 I think I've addressed all your comments (except labeling bigobj
that complicates partial application). CI is all green :)
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!
Thanks very much!
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 tipeDWORD
)