Clarify and fix construction of MKEXE in configure

[dra27]

This is a long overdue overhaul of the way $(MKEXE) is constructed. $(MKEXE) either builds an executable using the C compiler (Unix, and Cygwin in non-shared mode) or it's a call to a linker wrapper (flexlink). There's also the separate $(MKEXE_USING_COMPILER) which is needed for the bytecode startup header and for building the non-shared runtime when bootstrapping flexlink.

At present, there's a lot of subtlety embedded in both configure.ac and Makefile.config.in (for example, the fact that the Microsoft linker requires linker flags after objects, not before, is encoded in Makefile.config.in and not mentioned) and a few latent bugs (for example, the handling of $(LDFLAGS) is missing on the flexlink paths). There's also a lot of "weirdness" about argument prefixing - for example, the Unicode startup instruction (-municode) should be passed directly to gcc in $(MKEXE_USING_COMPILER) but prefixed -link when passed to flexlink. However, in the msvc ports, the equivalent /ENTRY:wmainCRTStartup must be passed prefixed -link to flexlink and also prefixed -link//link to cl. There is also the question of $(CFLAGS), which may be used when $(MKEXE) is a C compiler, but cannot be passed to flexlink, which doesn't call the C compiler.

Ultimately, I expect to remove this confusion by changing flexlink to cease being a wrapper around the linker and instead a compilation step to generate the required startup objects. However, this work is un-designed and un-scheduled, so we the mess could do with dealing with. The present code also severely complicates some other simplifications in the build system for the generation of utils/config.ml and ocamltest/ocamltest_config.ml.

My solution here is to change the way $mkexe is constructed in configure.ac. configure now considers $mkexe_cmd - the command to be executed (which might include parameters). The variables which are to be passed to it are considered at the end of the process, just before config.status is generated (this is what AC_CONFIG_COMMANDS_PRE does). This allows all of the computations in the main body of configure to consider the flags only, and not how they will be passed - for example, $mkexedebugflag is now -g at all times (or cleared in MSVC, where it can't be used) and then only at the end do we worry about whether it needs to be prefixed -link.

I've renamed MKEXE_USING_COMPILER to be more explicit about how it really works - it is now MKEXE_VIA_CC. I've attempted with comments before the AC_CONFIG_COMMANDS_PRE call to explain the variables which are constructed. A nice side-effect of this, if we also move the testsuite/manual commands (as suggested in #10411 (comment)) is that Makefile.config no longer contains if statements - it's actually a configuration file.

Along the way, $(OC_LDFLAGS) and $(LDFLAGS) are now correct handled by flexlink.

This has been through precheck, but it should obviously go through again once reviewed.

[shindere]

Many thanks for this PR. It's hard to review but I'm pretty sure it was
way harder to write. Also, it clarifies a partof the build system which has
always been obscure and fragile and does so in a principled way.

So, really, thank you, David.

At the moment I have just one name-related question, about
mkexe_extra_flags. How aobout calling it something like
mkexe_flexlink_extra_flags, or simply mkexe_flexlink_flags? It would
make it clear from the name that it's flexlink specific and the comment
saying this in configure.ac could then be removed. What do you think?

[shindere]

Regarding configure.ac:

  # If $mkexe_ldflags_prefix is given, apply it $oc_ldflags

Suggestion: apply it to ?

[shindere]

Other question about configure.ac: does your treatment of LDFLAGS also
work when LDFLAGS contains more than two flags or is specified only when
calling make? I would have expect a foreach construct to add the prefix in
front of any word contained in LDFLAGS, which would even make the if
construct unnecdessary, I believe.

[shindere]

I may have misread you, sorry. I see instances of addprefix in
configure.ac. Just wanted to make sure the right prefix will be added
where it's due, even if LDFLAGS is passed when calling make rather than when
calling configure, which I think is allowed but the GNU build system
standards.

[shindere]

Other configure.ac question, about how mkexe_extra_flags is computed.

Would it perhaps be possible to compute it in a more principled way? For
isntance I assume that the argument to -stack depends on the architecture?
Wouldn't such a change make the ocde a boit less redundant / more readable?

[shindere]

In the definitions of mksharedlib in configure.ac, would it be possible
to use \$(CC) rather than jsut $CC, so that the compiler can be
overriden at make time?

I do realise it is not this PR which introduced the "problem", but thought
that while these definitions were being modified, why not also fix this?

[shindere]

My last question for this round of review is: is it really necessary that

-maindll occurs concretely in utils/config.mlp ? Couldn't things be done
so that the concrete option does not need to appear there?

Will happily approve the PR when all the remarks have been discussed!

[dra27]

Many thanks for looking at this, @shindere! Some replies just before I start crafting additional commits:

• mkexe_extra_flags isn't flexlink-specific - it's flags which must be passed to both the C compiler and flexlink. I don't mind, though - the idea is that FLEXLINK_FLAGS could include another variable which doesn't go into MKEXE_VIA_CC, but it doesn't at the moment. That said, I think your idea of splitting mkexe_extra_flags into a -stack argument and a -merge-manifest argument is interesting and much more principled - I'll have a look at that instead!
• The typo is definitely to be corrected! In fact, that whole sentence should be reviewed. Will push something.
• I have fixed the definitions of mksharedlib - the ones which are $CC are in single quotes, so the $doesn't need escaping. The only definition of</code>mksharedlib<code>in double-quotes is the one for</code><em>-apple-darwin</em><code>because the string is long so I had to escape the newline (and therefore the$` as well)
• in utils/config.mlp, the machinery there is allowing a different flexlink binary to be substituted in using the OCAML_FLEXLINK environment variable (this machinery has been there since the bootstrap was added in 4.03). The -maindll occurs in the same way as -exe does, therefore: it controls which of flexlink's three modes it's going to do. I think it's OK that it knows the parameters that flexlink takes (in the same way as it's building the -chain argument). It could be done by feeding three different sets of flexlink_flags from configure (the flags for MKEXE, MKDLL and MKSHAREDLIB), but I think building those in OCaml is slightly better than doing it in m4sh! The major reason possibly to leave it as it is is that I do still intend to remove OCAML_FLEXLINK completely (following on from 4.13's overhaul of the flexlink bootstrap), so in principle all that stuff in utils/config.mlp will be completely deleted.

[dra27]

Where LDFLAGS is concerned, I think I've made a mistake reading the FlexDLL sources - I thought those arguments get inserted verbatim, but they are quoted.

[dra27]

The main thing to note is that $(LDFLAGS) gets added in the "normal" way for the Unix and non-flexlink Cygwin builds. The mangling here is only for flexlink.

I think that what we should be doing at this stage:

• LDFLAGS needs to be added in two contexts - both in MKEXE_VIA_CC (where it gets passed to the C compiler, and where MSVC requires /link to be pre-pended) and in MKEXE etc. where it gets passed to flexlink and needs -link pre-pended.
• I think the best trade-off (which isn't what I've done here) is to say that LDFLAGS at present cannot contain quoted arguments (e.g. LDFLAGS="-L \"C:\Program Files\My Library\") and treat it in the same way as I have OC_LDFLAGS with $(addprefix so LDFLAGS="-lsomething -msomething" is transformed to -link -lsomething -link -msomething when passed to flexlink and the effect is to pass those two flags verbatim to the linker.
• I can't remember how much I've discussed this before, but I would like to move in the future towards flexlink not being called at all - so having flexdll as a library which constructs the objects and moving the linking step back into OCaml. It's made total sense to have a 15 year experiment with wrapping the linker, but I think it basically brings its own set of interesting problems. I think our build system will be much simpler to go back to OCaml always linking binaries using a C compiler with the extra objects and processing needed for flexdll as explicit steps in the pipeline. The relevance here is that this change in the future would then simplify the processing of LDFLAGS because we'd only ever build executables in one style. So in other words we have an academic restriction on what can be in LDFLAGS now (for Windows only) but in the future we expect that restriction to disappear. Crucially, we never expect the user to be putting flexlink-specific flags in LDFLAGS - i.e. the user is always specifying LDFLAGS for the real C compiler/linker.

[shindere]

David Allsopp (2021/07/21 03:18 -0700):

Many thanks for looking at this, @shindere!

Pleasure.

Some replies just before I start crafting additional commits: - `mkexe_extra_flags` isn't flexlink-specific - it's flags which must be passed to both the C compiler _and_ flexlink. I don't mind, though - the idea is that `FLEXLINK_FLAGS` _could_ include another variable which doesn't go into `MKEXE_VIA_CC`, but it doesn't at the moment. That said, I think your idea of splitting `mkexe_extra_flags` into a `-stack` argument and a `-merge-manifest` argument is interesting and much more principled - I'll have a look at that instead!

Perhaps I was mislead by a comment in `configure.ac` which was suggesting they were. Sorry. It would be good to make sure the comment explaining the variable is accurate.

- I have fixed the definitions of `mksharedlib` - the ones which are `$CC` are in single quotes, so the $` doesn't need escaping. The only definition of `mksharedlib` in double-quotes is the one for `*-apple-darwin*` because the string is long so I had to escape the newline (and therefore the `\$` as well)

Thanks. I'm wondering: shouldn't we make them alll uniform? If double-quotes are needed for darwin, why not use the same style everywhere just for consistency, kind of?

- in `utils/config.mlp`, the machinery there is allowing a different flexlink binary to be substituted in using the `OCAML_FLEXLINK` environment variable (this machinery has been there since the bootstrap was added in 4.03). The `-maindll` occurs in the same way as `-exe` does, therefore: it controls which of flexlink's three modes it's going to do. I think it's OK that it knows the parameters that flexlink takes (in the same way as it's building the `-chain` argument). It could be done by feeding three different sets of flexlink_flags from configure (the flags for `MKEXE`, `MKDLL` and `MKSHAREDLIB`), but I think building those in OCaml is slightly better than doing it in m4sh! The major reason possibly to leave it as it is is that I do still intend to remove `OCAML_FLEXLINK` completely (following on from 4.13's overhaul of the flexlink bootstrap), so in principle all that stuff in `utils/config.mlp` will be completely deleted.

Sounds cool! Willyou please ping when you wnat me to review again?

[dra27]

Rebased, with 3 additional commits addressing the comments made previously. Also running through precheck#687

[dra27]

This changes the order, relative to that originally given in #10835 which needs to be checked, but I don't think that it was ever correct for hard-coded flags to appear _after $(LDFLAGS)

[shindere]

David Allsopp (2022/05/10 08:06 -0700):

@dra27 commented on this pull request. > [[*,i[3456]86-*]], # Disable DT_TEXTREL warnings on Linux and BSD i386 # See #9800 - [mksharedlib="$CC -shared \$(LDFLAGS) -Wl,-z,notext"], This changes the order, relative to that originally given in #10835 which needs to be checked, but I don't think that it was ever correct for hard-coded flags to appear _after `$(LDFLAGS)`

I agree it was not!

[shindere]

I wonder whether it wouldn't be worth definind FLEXLINK_CMD in
configure.ac, although it's a constant.

If that was done, couldn't we then have configure expand the definition of
mkexe_cmd?

Also, although I guess the situation has not been introduced by the present
PR, there are many repetitions regarding the -stack flags with vaariations
I guess depending on whether one is on a 21 or 64 bits architecture. Could
this be factorized somewhat?

[dra27]

I wonder whether it wouldn't be worth definind FLEXLINK_CMD in configure.ac, although it's a constant.

If that was done, couldn't we then have configure expand the definition of mkexe_cmd?

I think this might be hard to do simultaneously with this PR - your PR to generate utils/config.ml from configure and then my WIP to eliminate BOOT_FLEXLINK_CMD... but the end result would be to do this, yes! At the moment, MKEXE in the build needs to include $(FLEXLINK_CMD) so that the bootstrapped flexlink mode can override it, but only in certain places (for example, it must not be overridden in utils/Makefile).

Also, although I guess the situation has not been introduced by the present PR, there are many repetitions regarding the -stack flags with vaariations I guess depending on whether one is on a 21 or 64 bits architecture. Could this be factorized somewhat?

I think this could be, yes - I'll have a look in a bit!