Skip to content

Resurrect Cygwin64#9927

Merged
dra27 merged 1 commit intoocaml:trunkfrom
dra27:resurrect-cygwin64
Sep 21, 2020
Merged

Resurrect Cygwin64#9927
dra27 merged 1 commit intoocaml:trunkfrom
dra27:resurrect-cygwin64

Conversation

@dra27
Copy link
Copy Markdown
Member

@dra27 dra27 commented Sep 17, 2020 •
edited
Loading

Cygwin64 has been building with shared-library/natdynlink support for a couple of years. This PR restores support for this.

The problem is that Cygwin64 requires DLLs to load in a memory range which is more than +/-2GiB from the executable, which prevents FlexDLL from performing the required relocations for dynamic loading. Previous work tried to fix this in flexlink by using branch islands, but this is quite heavy and doesn't work for data symbols. It's also incredibly hairy to do for ARM64, which is where this alternate approach has sprung from.

The method for this fix (related to #1633), is a partial return to using __declspec macros by using __declspec(dllimport) for all symbols for Cygwin64. At present, the change is limited to Cygwin64 - there are presently nasty hacks in flexlink with base addresses in the mingw64 and msvc64 ports which can be removed, but I don't propose changing those ports this close to a version freeze.

The change is made in 3 commits which are best viewed separately - indeed the first two are best viewed using a better visualiser than GitHub (patdiff is excellent for this). The first commit removes CAMLexport which is no longer required (and hasn't been for some time). The second eliminates the use CAMLextern in the tree (but leaves CAMLextern defined in the public facing headers). Replacing CAMLextern is the new CAMLAPI macro intended for use in the tree only.

Why? The problem with CAMLextern is that it's just looked like an alias for extern and has been haphazardly used as such. There are two kinds of export necessary now - making a symbol accessible to the rest of the runtime (i.e. a normal C extern declaration) or making the symbol accessible to dynamically loaded code (an actual export). The many micro PRs previously merged have gone through and rationalised the use of the old CAMLextern to be the latter, so this switch just marks those CAMLextern declarations as actually part of the public API. The idea now is that extern should be written extern and CAMLAPI should be added to things which actually need to be visible to stub libraries.

Other PRs have also attempted to limit the use of C primitives from C code. A side-effect of this is that CAMLprim at the moment does not need to include CAMLAPI. That might get revisited, especially when #1633 re-rises as enabling the use of -fvisibility.

@dra27
Copy link
Copy Markdown
Member Author

dra27 commented Sep 17, 2020

I'm at present having arguments with Jenkins, but should eventually have a run of this in precheck...

@dra27
Copy link
Copy Markdown
Member Author

dra27 commented Sep 17, 2020

Huzzah - precheck shows a full testsuite pass for a --enable-shared Cygwin64 build.

@xavierleroy
Copy link
Copy Markdown
Contributor

xavierleroy commented Sep 18, 2020

I agree with the use of __declspec(dllimport) for run-time functions and variables that can be accessed by stub DLLs. Other than that, this PR feels too complicated and intrusive. First, CAMLexport encodes a bit of information that doesn't harm and might very well be useful later, so don't remove. Second, I see no need to introduce yet another macro CAMLAPI when the existing CAMLextern does the job, with the appropriate definition.

What I would like to see at this point is the 10-line PR that redefines CAMLextern to add a __declspec(dllimport) when appropriate to unblock Cygwin64.

@dra27 dra27 force-pushed the resurrect-cygwin64-review-base branch from 9d00e56 to 702d8b7 Compare September 18, 2020 13:24
@dra27 dra27 force-pushed the resurrect-cygwin64 branch from c65f31a to 57d4242 Compare September 18, 2020 13:24
@dra27
Copy link
Copy Markdown
Member Author

dra27 commented Sep 18, 2020

Revised version pushed on top of #9924 and #9925. The other two commits come from -fvisbility work which hasn't been pushed but the point was trying to maintain the state of the headers for the future.

  • I disagree that CAMLexport encodes anything, given the inconsistency with which it was applied. It's had a purpose originally of course, but now it's a macro which implies it does something without actually doing it. (in particular CAMLexport in the C file and no CAMLextern declaration in a header or even worse no header declaration at all is actually now subtly broken).
  • I've put the __declspec(dllimport) back on CAMLextern - grep'ing opam sources suggests it's actually used less "in the wild" than I'd feared. Again, my concern here is that the difference between CAMLextern and extern is subtle, the difference between CAMLAPI extern and extern is not.

@xavierleroy
Copy link
Copy Markdown
Contributor

xavierleroy commented Sep 18, 2020

Thanks for the simplified PR, it's much easier for me to read, and it looks very good to me.

Re CAMLAPI vs CAMLextern, this can be argued, but maybe later?

@dra27
Copy link
Copy Markdown
Member Author

dra27 commented Sep 18, 2020

Re CAMLAPI vs CAMLextern, this can be argued, but maybe later?

Indeed!

@dra27 dra27 force-pushed the resurrect-cygwin64-review-base branch from 702d8b7 to 2bc77ee Compare September 21, 2020 09:34
@dra27 dra27 force-pushed the resurrect-cygwin64 branch from 57d4242 to 0ffc560 Compare September 21, 2020 09:35
@dra27
Copy link
Copy Markdown
Member Author

dra27 commented Sep 21, 2020

Rebased - this is just dependent on #9925

@dra27 dra27 force-pushed the resurrect-cygwin64 branch from 0ffc560 to 001c2d1 Compare September 21, 2020 12:36
@dra27 dra27 changed the base branch from resurrect-cygwin64-review-base to trunk September 21, 2020 12:36
@dra27 dra27 marked this pull request as ready for review September 21, 2020 12:36
@dra27 dra27 mentioned this pull request Sep 21, 2020
Merged
4 tasks
xavierleroy
xavierleroy approved these changes Sep 21, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@xavierleroy xavierleroy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Thanks for the heroic effort! Please merge when you're ready.

@dra27 dra27 merged commit 58adc59 into ocaml:trunk Sep 21, 2020
@dra27 dra27 deleted the resurrect-cygwin64 branch September 21, 2020 16:15
@dra27
Copy link
Copy Markdown
Member Author

dra27 commented Sep 21, 2020

Done - thanks for your copious reviewing assistance!

@dra27 dra27 mentioned this pull request Apr 16, 2021
Merged
@xavierleroy xavierleroy mentioned this pull request Apr 27, 2021
Closed
fdopen added a commit to fdopen/ocaml-integers that referenced this pull request Mar 18, 2022
@fdopen
restore dll support for cygwin64 / newer binutils versions (mingw64)
593b657 
`CAMLextern` must now be used for functions used by foreign stub code.
For details: ocaml/ocaml#9927

Since this makes it more complicated to use, I took this as an
opportunity to shorten the header and remove all functions from it
that are only used internally by integers/ctypes.
@fdopen fdopen mentioned this pull request Mar 18, 2022
Merged
@dra27 dra27 mentioned this pull request Nov 28, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@xavierleroy xavierleroy xavierleroy approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@dra27 @xavierleroy