Unify the two executable header implementations#13988
Conversation
|
#if defined(__STDC_VERSION__) && __STDC_VERSION__ >= 202311L
[[noreturn]]
#else
_Noreturn
#endif |
|
Hah - I'd forgotten that C11 giveth and C23 taketh away! I've updated that commit to define a |
|
@MisterDA - are you able/willing to do a full review? |
MisterDA
left a comment
There was a problem hiding this comment.
LGTM, the review commit-by-commit helps.
A few minor comments on top of the inline ones:
- I'd prefer seeing
perrorandstrerror_rover ad-hoc error messages in the unix path; read_sizeis fishy, I'm not sure if the size is stored in big or little endian (I'm guessing LE) but I worry there could be a possible mismatch if the endianness of the host and what's stored in the binary differ. If they can never differ, perhaps amemcpyis just as clear (that is, ifmemcpyis even allowed).
| ENTRYPOINT = wmainCRTStartup | ||
| endif | ||
| tmpheader.exe: OC_LDFLAGS += \ | ||
| -nostdlib -nostartfiles -Wl,--gc-sections -Wl,-e,$(ENTRYPOINT) |
There was a problem hiding this comment.
For reference: -nostdlib and -nostartfiles. I understand from the documentation that -nostdlib implies -nostartfiles. There's also this note:
In most cases, you need
libgcc.aeven when you want to avoid other standard libraries. In other words, when you specify-nostdlibor-nodefaultlibsyou should usually specify-lgccas well. This ensures that you have no unresolved references to internal GCC library subroutines.
If the program runs without -lgcc I suppose there's no harm. It seems to be needed for C++ and initializing data.
For reference on --gc-sections:
Once you’ve created the objects and static libraries with these switches, the linker can perform the dead code elimination. You can do this by specifying the
-Wl,--gc-sectionsswitch to yourgcccommand [...]. This causes the linker to perform a garbage collection and remove code and data that are never referenced.
There was a problem hiding this comment.
Hmm, I'm pretty sure that I just added those while both without noticing that -nostdlib implies -nostartfiles - it seems desirable to be explicit, even if I probably did that by mistake?
There was a problem hiding this comment.
However, I'm very intentionally not passing -lgcc - the code (including in Relocatable) is moderately carefully crafted not to require -lgcc
There was a problem hiding this comment.
Thanks for the explanation. clang lists -nostartfiles and -nostdlib but doesn't document them. I agree with you that it's preferable to be explicit and keep the two flags.
There was a problem hiding this comment.
I've added a little more comment above the recipe (and the links will remain in the PR, obviously, thanks!)
|
Thanks, @MisterDA!
This changes the behaviour, which is beyond the scope of what's in this PR. The original function is: static void errwrite(const char * msg)
{
fputs(msg, stderr);
}
Again, can we stick to the PR? I don't believe I've materially changed this function? |
|
Thanks for the explanations. The PR looks good to me, all the changes make sense to me on their own and taken together. |
7d5b8de to
5e40770
Compare
Affects forced recompilation of runtime-launch-info
stdlib/headernt.c was adapted in OCaml 3.00 to reduce its size by avoiding the use of the CRT and using Windows API functions directly (this is a well-studied trick on Windows, principally as a puzzle for producing tiny binaries). This got "regressed" slightly in OCaml 4.06, in the complex introduction of wide character support for Windows, as the mingw-w64 incantation required was unclear, so the entry point was changed to wmain, and the size of the header increased. By switching from wcslen (a CRT function) to lstrlen (a Win32 API function), headernt.c again only requires kernel32.dll. Additional flags are added for both ld (mingw-w64) and link (MSVC) to squeeze every last byte out of tmpheader.exe. The MSVC version of the header is once again no longer passed through strip, as this was found to be corrupting the executable (and had never been reducing its size anyway).
Modern C compilers are sufficiently intelligent not to need the inlining hints! Inline the definition from caml/misc.h for CAMLnoret. Co-authored-by: Antonin Décimo <antonin@tarides.com>
Already updated to remove the actual test in s.h, since XPG1 (1985) required it and it is therefore part of the Single Unix Specification (1992), but the _WIN32 guard and the loading of s.h are unnecessary.
Defined in sys/stat.h and unistd.h respectively by the Single Unix Specification.
O_BINARY is added for maximum Cygwin compatibility, but it's not a Posix flag.
Have exec.h include <stdint.h> itself. The bytecode executable header now only depends on exec.h.
Modernisations applied in headernt.c but not applied to header.c - use uint32_t rather than unsigned long
Brings the behaviour of the two headers into line with each other - if RNTM is ocamlrun on Unix, ocamlrun will now be sought in PATH.
damiendoligez
left a comment
There was a problem hiding this comment.
I had a quick look, and I trust @MisterDA's review.
|
@dra27 I'm assuming you don't want the commits squashed? |
On Unix systems, bytecode executables by default are shebang "scripts" where the
#!points toocamlrun(or a trivialshscript which exec'socamlrun). However, even on Unix not all systems have supported shebang scripts, and there has always been an alternative executable stub, written in C, living instdlib/header.c. On Windows, where the shebang mechanism is never available, the executable is always required. However, the code is different and it lives a separate life in stdlib/headernt.c. Today, all the Unix systems which can run OCaml also support shebang scripts but, mainly for historical reasons, the Unix version of the executable header remains because the Cygwin port of OCaml (which is a Unix-like clone running on top of Windows) uses it.Relocatable OCaml changes the mechanism by which these normal bytecode executables find
ocamlrun. This primary goal of this PR is to unify the common parts ofheader.candheadernt.cbecause the common code between the two is much larger in Relocatable OCaml. Along the way, there are a few minor differences of behaviour / bugs which can be addressed.The final commit then merges
headernt.cintoheader.c. This last part is not strictly necessary, but I propose is a good idea. It creates the following structure:In this PR, the "Header" is quite large for both implementations, "Common" is fairly small and "Main" is a similar size for each. With Relocatable OCaml, and with some additional fixes I propose afterwards, "Common" becomes much larger and the Unix part "Header" becomes very small. I think it will be slightly easier to maintain just
stdlib/header.crather than adding a thirdstdlib/header_common.c.In terms of review, I suggest that it's worth reviewing the final form of
stdlib/header.cas it ends up (i.e. as if it were a new file, rather than a diff) and then viewing the commits as the pathway from the current state to that. The commits along the way:wmainas the entrypoint caused a considerable increase in size of the executable header, which was the purpose of the original patch. On my laptop, the Linux executable header (in Ubuntu 24.04) is ~14KiB and the Cygwin header is about 10KiB (just for comparison). With the invocations corrected, the x64 mingw-w64 header reduces by 12KiB to 5KiB (x86 by 9.5KiB to 4.5KiB) and the MSVC x64 header by 8.5KiB to 4KiB (x86 by 7KiB to 3.5KiB)<stdint.h>incaml/exec.h,caml/exec.hbecomes "standalone" (which is useful in further work not to leak lots of other things fromcaml/mlvalues.h)execcallRNTMsection is not normally given a relative PATH, by switching fromexecvtoexecvp, we have the same semantics for the call betweenheader.candheadernt.cFor information, the work I wish to stack on top of this:
caml_executable_name- I propose sharing all thePATH-searching code withruntime/unix.c, especially as it's duplicated inconsistently at the momentand the fix for that similarly involves changes in both
header.candheadernt.cotherwise.