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

Add support for building profiled dynamic way #9900

Merged
merged 1 commit into from
Jun 25, 2024

Conversation

mpickering
Copy link
Collaborator

@mpickering mpickering commented Apr 17, 2024

Add support for profiled dynamic way

New options for cabal.project and ./Setup interface:

* `profiling-shared`: Enable building profiling dynamic way
* Passing `--enable-profiling` and `--enable-executable-dynamic` builds
  profiled dynamic executables.

Support for using `profiling-shared` is guarded behind a constraint
which ensures you are using `Cabal >= 3.13`.

In the cabal file:

* `ghc-prof-shared-options`, for passing options when building in
  profiling dynamic way

Other miscellenious fixes and improvements

* Some refactoring around ways so that which
  ways we should build for a library, foreign library and executable is
  computed by the `buildWays` function (rather than ad-hoc in three
  different places).

* Improved logic for detecting whether a compiler supports compiling
  a specific way. See functions `profilingVanillaSupported`,
  `dynamicSupported`, `profilingDynamicSupported` etc
  These functions report accurate infomation after ghc-9.10.1.

* Fixed logic for determining whether to build shared libraries. (see
  #10050)
  Now, if you explicitly enable `--*-shared`, then that will always take
  effect. If it's not specified then `--enable-executable-dynamic` will
  turn on shared libraries IF `--enable-profiling` is not enabled.

* Remove assumption that dynamically linked compilers can build dynamic
  libraries (they might be cross compilers.

* Query the build compiler to determine which library way is necessary
  to be built for TH support to work.
  (rather than assume all compilers are dynamically linked)

* An extensive test which checks how options for `./Setup` and
  `cabal-install` are translated into build ways.

Fixes #4816, #10049, #10050

@mpickering
Copy link
Collaborator Author

I require the version to be bumped to 3.13 before this can be finished.

@ulysses4ever
Copy link
Collaborator

@mpickering
Copy link
Collaborator Author

The test fails on 8.6.5 because of this GHC bug (fixed in 2018): e400b9babdcf11669f963aeec20078fe7ccfca0d

@mpickering mpickering force-pushed the wip/prof_dyn branch 3 times, most recently from d47096e to 870fa68 Compare May 29, 2024 13:37
@mpickering mpickering changed the title Draft: Add support for building profiled dynamic way Add support for building profiled dynamic way May 29, 2024
Cabal/src/Distribution/Simple/Configure.hs Outdated Show resolved Hide resolved
Cabal/src/Distribution/Simple/Configure.hs Outdated Show resolved Hide resolved
Cabal/src/Distribution/Simple/Configure.hs Outdated Show resolved Hide resolved
Cabal/src/Distribution/Simple/Configure.hs Outdated Show resolved Hide resolved
Cabal/src/Distribution/Simple/GHC.hs Show resolved Hide resolved
Cabal/src/Distribution/Simple/Configure.hs Show resolved Hide resolved
@mpickering
Copy link
Collaborator Author

Can someone please review this patch? I would like to get it off my plate as I have been working on it for quite a while.

Copy link
Collaborator

@geekosaur geekosaur left a comment

Choose a reason for hiding this comment

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

No additions beyond what @sheaf said.

@mpickering mpickering force-pushed the wip/prof_dyn branch 4 times, most recently from a66258f to 353881c Compare June 18, 2024 11:15
@mpickering
Copy link
Collaborator Author

I made two recent improvements.

  • Better handling of -dynamic-too
  • Enforce that Cabal >3.13 constraint applies to all setups if profiling dynamic is used anywhere (this is conservative).

Copy link
Collaborator

@andreabedini andreabedini left a comment

Choose a reason for hiding this comment

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

The two new fields ghc-prof-shared-options and ghcjs-prof-shared-options need to be guarded behind the new cabalSpecVersion.

I feel the code to handle "way" keeps getting more and more complex. Is there any way we can reduce this complexity?

E.g. do we really need to know what GHC support in what situation? can we instead just do what the user wants and leave GHC to deal with it? Otherwise we end up replicating a bunch of logic that belongs to the compiler.

@mpickering
Copy link
Collaborator Author

I feel the code to handle "way" keeps getting more and more complex. Is there any way we can reduce this complexity?

E.g. do we really need to know what GHC support in what situation? can we instead just do what the user wants and leave GHC to deal with it? Otherwise we end up replicating a bunch of logic that belongs to the compiler.

Is there a particular bit of code which you find too complicated? I have just extended how cabal-install has already worked in relation to ways (and actually simplified a lot of the calculations about which ways are built and needed).

I have fixed the issue now about the cabal spec version, that is a good point.

@mpickering mpickering force-pushed the wip/prof_dyn branch 2 times, most recently from 68dbcc9 to 2c5ebe8 Compare June 19, 2024 08:56
@mpickering mpickering added merge me Tell Mergify Bot to merge and removed attention: needs-review labels Jun 19, 2024
@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Jun 21, 2024
@Mikolaj
Copy link
Member

Mikolaj commented Jun 24, 2024

@andreabedini: was your concern answered satisfactorily? If so, could you mark it in the system so that mergify knows to proceed? Thank you!

@andreabedini
Copy link
Collaborator

@Mikolaj My comment about the complexity was not essential to the request for changes. My only request was to guard the fields behind cabalSpecVersion, which has been acted upon. Thanks @mpickering.

New options for cabal.project and ./Setup interface:

* `profiling-shared`: Enable building profiling dynamic way
* Passing `--enable-profiling` and `--enable-executable-dynamic` builds
  profiled dynamic executables.

Support for using `profiling-shared` is guarded behind a constraint
which ensures you are using `Cabal >= 3.13`.

In the cabal file:

* `ghc-prof-shared-options`, for passing options when building in
  profiling dynamic way

Other miscellenious fixes and improvements

* Some refactoring around ways so that which
  ways we should build for a library, foreign library and executable is
  computed by the `buildWays` function (rather than ad-hoc in three
  different places).

* Improved logic for detecting whether a compiler supports compiling
  a specific way. See functions `profilingVanillaSupported`,
  `dynamicSupported`, `profilingDynamicSupported` etc
  These functions report accurate infomation after ghc-9.10.1.

* Fixed logic for determining whether to build shared libraries. (see
  haskell#10050)
  Now, if you explicitly enable `--*-shared`, then that will always take
  effect. If it's not specified then `--enable-executable-dynamic` will
  turn on shared libraries IF `--enable-profiling` is not enabled.

* Remove assumption that dynamically linked compilers can build dynamic
  libraries (they might be cross compilers.

* Query the build compiler to determine which library way is necessary
  to be built for TH support to work.
  (rather than assume all compilers are dynamically linked)

* An extensive test which checks how options for `./Setup` and
  `cabal-install` are translated into build ways.

Fixes haskell#4816, haskell#10049, haskell#10050
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days merge me Tell Mergify Bot to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants