Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cabal: Use -flink-rts when available #7764

Merged
merged 1 commit into from
Apr 22, 2022
Merged

Conversation

bgamari
Copy link
Contributor

@bgamari bgamari commented Oct 20, 2021

Previously Cabal did quite some headstands to link against libHSrts.
Note only this is complex but it couples very tightly to GHC's
implementation. Thankfully, as of GHC 9.0 GHC provides a -flink-rts
flag for precisely this purpose. Use it when available.

Fixes #7763.


Please include the following checklist in your PR:

Please also shortly describe how you tested your change. Bonus points for added tests!

@bgamari
Copy link
Contributor Author

bgamari commented Oct 21, 2021

This appears to fix the original ticket on Windows. However, my testing on Linux suggests that this feature is actually slightly broken on that platform. Specifically, Cabal must ensure that objects linked into a shared object are built with -fPIC (which may mean using the dyn way, since all transitive objects in the final link must be position independent). It's quite unclear how the original design intended this to be ensured.

@Mikolaj
Copy link
Member

Mikolaj commented Oct 21, 2021

Is it broken with GHC 8.10 as well? I assume it was broken for a long time?

@Mikolaj
Copy link
Member

Mikolaj commented Nov 29, 2021

@bgamari: you mentioned Windows and Linux. Could this affect M1 as well, as in #7837? If so, we can ask @vmchale to test once the PR is complete.

@bgamari
Copy link
Contributor Author

bgamari commented Feb 24, 2022

Is it broken with GHC 8.10 as well? I assume it was broken for a long time?

Indeed, as far as I can tell this is just a hole in the design. Specifically, one can only expect to build a foreign library on most platforms in the dynamic way. Perhaps Cabal should throw an error to catch this case?

@bgamari bgamari marked this pull request as ready for review February 25, 2022 14:20
@Mikolaj
Copy link
Member

Mikolaj commented Feb 25, 2022

My suspicion is that Cabal should either implicitly enable --enable-dynamic-libraries' when building foreign library`s on some platforms or refuse to build them if it's disabled.

Sounds reasonable. I think, as long as the error messages are clear, it's less surprising when stuff refuses to build and asks to enable --enable-dynamic-libraries than when it's enabled implicitly, the user doesn't notice and is utterly confused why the build products are not static as requested.

@bgamari
Copy link
Contributor Author

bgamari commented Feb 25, 2022

Unfortunately the issue is even more confused than I initially thought. It appears that, in an attempt to mitigate the issue described above, GHC.Distribution.Simple.GHC.gbuild explicitly enables -fPIC when building a foreign library. Meanwhile, it passes -static, a combination that makes very little sense and which causes GHC with this patch to link against the the vanilla RTS, which isn't PIC.

Ultimately GHC doesn't make things easier here as it defines -dynamic to serve a dual-purpose: it both tells GHC to link against Haskell code dynamically and it tells GHC to produce PIC code. the ghcOptDynLinkMode name given to this flag inCabal's GHC flag type seems to suggest that Cabal is under the impression that it only did the former, which is likely the reason for the confusion here.

@bgamari
Copy link
Contributor Author

bgamari commented Feb 25, 2022

It appears that the -static flag arises from Distribution.Simple.GHC.gbuildNeedDynamic. Specifically, that function returns False if the library type is standalone. This seems to be quite intentional but it's not clear to me why it would do this and, as far as I can tell, it's wrong.

@bgamari
Copy link
Contributor Author

bgamari commented Feb 25, 2022

It appears that the -static flag arises from Distribution.Simple.GHC.gbuildNeedDynamic. Specifically, that function returns False if the library type is standalone. This seems to be quite intentional but it's not clear to me why it would do this and, as far as I can tell, it's wrong.

Thinking about this a bit more, I suspect that this is due to the fact that the author wanted a "standalone" library to have no dynamic dependencies. That is, they expected GHC to link against static Haskell dependencies. However, it's unclear to me how this could have ever worked on platforms that require dynamic libraries to be PIC since, while Cabal does pass -fPIC when to the GHC invocation used to compile the objects of the foreign-library, its (static) dependencies will have been built as non-PIC.

@hsyl20
Copy link
Collaborator

hsyl20 commented Mar 3, 2022

Ultimately GHC doesn't make things easier here as it defines -dynamic to serve a dual-purpose: it both tells GHC to link against Haskell code dynamically and it tells GHC to produce PIC code.

Couldn't we use -fexternal-dynamic-refs instead?

@Mikolaj
Copy link
Member

Mikolaj commented Apr 19, 2022

@bgamari: any news?

@Mikolaj
Copy link
Member

Mikolaj commented Apr 19, 2022

@Bodigrim points our attention to https://gitlab.haskell.org/ghc/ghc/-/issues/20520#note_411402, citing:

Unfortunately, this is at its core a Cabal bug. I have tried to fix this bug in Cabal #7764 but have run into inconsistencies in the current implementation of foreign-library that I can't currently explain. Specifically, Cabal passes -static to GHC when building a type: standalone library. However, this is inconsistent with the users intent to build a dynamic library. I'm afraid the fix for this will need to be bumped to %9.2.3 at the very least.

@bgamari: do you have any vague suggestions how to fix that in Cabal?

@gbaz
Copy link
Collaborator

gbaz commented Apr 21, 2022

I think based on discussion that this is a strict improvement, but there remain cases that make no sense in general with the static/dynamic issue. So we should merge this, but also open a new ticket trying to specify what the semantics actually are.

@Bodigrim
Copy link
Collaborator

This does not seem to resolve the reproducer in https://gitlab.haskell.org/ghc/ghc/-/issues/20520, and I'm not qualified to jusge about other benefits.

@Mikolaj
Copy link
Member

Mikolaj commented Apr 22, 2022

This does not seem to resolve the reproducer in https://gitlab.haskell.org/ghc/ghc/-/issues/20520, and I'm not qualified to jusge about other benefits.

Oh dear, I thought it does, based on "This appears to fix the original ticket on Windows."

@bgamari: do we need to do anything more to fix the original issue? Remove the --static that cabal inserts? Add -fexternal-dynamic-refs @hsyl20 is suggesting?

Copy link
Collaborator

@Bodigrim Bodigrim left a comment

Choose a reason for hiding this comment

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

Sorry for confusion, I tested a wrong branch. This indeed fixes the reproducer.

@Mikolaj Mikolaj added merge me Tell Mergify Bot to merge squash+merge me Tell Mergify Bot to squash-merge and removed merge me Tell Mergify Bot to merge squash+merge me Tell Mergify Bot to squash-merge labels Apr 22, 2022
@Mikolaj
Copy link
Member

Mikolaj commented Apr 22, 2022

@Mergifyio rebase

Previously Cabal did quite some headstands to link against libHSrts.
Note only this is complex but it couples very tightly to GHC's
implementation. Thankfully, as of GHC 9.0 GHC provides a `-flink-rts`
flag for precisely this purpose. Use it when available.
@mergify
Copy link
Contributor

mergify bot commented Apr 22, 2022

rebase

✅ Branch has been successfully rebased

@mergify mergify bot merged commit 12f76dc into haskell:master Apr 22, 2022
jneira added a commit to jneira/cabal that referenced this pull request Apr 24, 2022
It seems haskell#7764 fixed the test, but the pr was merged without
having the ci green
jneira added a commit to jneira/cabal that referenced this pull request Apr 24, 2022
@jneira
Copy link
Member

jneira commented Apr 24, 2022

This should had included a changelog entry, added in #8111

mergify bot pushed a commit that referenced this pull request Apr 25, 2022
* Unmark foreign lib test as broken for win

It seems #7764 fixed the test, but the pr was merged without
having the ci green

* Add changelog for #7763, #7764 and #8111
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
attention: needs-review merge me Tell Mergify Bot to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cabal incorrectly constructs libHSrts library path
6 participants