gcc: only disable aligned_alloc for darwin build/host/target platforms by eliasnaur · Pull Request #226290 · NixOS/nixpkgs

eliasnaur · 2023-04-15T14:09:46Z

Before this change a Darwin gcc would output binaries that avoid
aligned_alloc, regardless of target platform. That's fine for Darwin
targets, but not for non-darwin targets such as pkgsCross.raspberryPi.

This change replaces the single configure flag with a flag for
each of build, host, target.

Idea by @trofi.

Things done

trofi · 2023-04-15T14:50:35Z

pkgs/development/compilers/gcc/common/pre-configure.nix

I think it's a bit more tricky: aligned_alloc is gcc's builtin. As a result I expect it to be used both in gcc's binary and target libraries built by gcc. That means you likely want ac_cv_func_aligned_alloc=no for --host=darwin (and any --target=) to keep the properties of gcc binary. And for --target=darwin to keep the properties of target libraries like libgomp.

In theory it would be nice if gcc's build system provided hooks to affect only host or only target with ac_cv_func_aligned_alloc=no. But it's a bit tricky to get right. I think the canonical way to do it is to pass it via STAGE{2,3}_CONFIGURE_FLAGS to gcc.

Getting back to your issue you might get a reasonable outcome with lib.optionalString (hostPlatform.isDarwin || targetPlatform.isDarwin).

Getting back to your issue you might get a reasonable outcome with lib.optionalString (hostPlatform.isDarwin || targetPlatform.isDarwin).

So

lib.optionalString (hostPlatform.isDarwin || targetPlatform.isDarwin) '' export ac_cv_func_aligned_alloc=no ''

?

Isn't your new condition a super-set of the old, because of the ||? My issue is that without this PR (or similar) cross-compiled binaries for Raspberry Pi don't reference aligned_alloc whereas cross-compiled binaries built on Linux do. It seems to me your suggestion strictly expands the number of configurations where aligned_alloc will be left out.

Did you perhaps mean lib.optionalString (hostPlatform.isDarwin && targetPlatform.isDarwin), with && instead of ||?

My suggestion was based on the following goals:

when building on darwin build (either for linux or for darwin target) there should be no references to `aligned_allos)

when building for target darwin (either on linux or darwin build) there should be no references to `aligned_allos)

Hence the ||.

But I want to have darwin building for linux result in the same binaries as when linux builds for linux. That is, I want the aligned_alloc reference in the binaries because that's what linux building for linux have.

Do you know of a way to achieve that, without breaking darwin?

Reading through gcc/doc/install.texi I found this bit (TIL!):

@subsubheading Overriding @command{configure} test results Sometimes, it might be necessary to override the result of some @command{configure} test, for example in order to ease porting to a new system or work around a bug in a test. The toplevel @command{configure} script provides three variables for this: @table @code @cindex @code{build_configargs} @item build_configargs The contents of this variable is passed to all build @command{configure} scripts. @cindex @code{host_configargs} @item host_configargs The contents of this variable is passed to all host @command{configure} scripts. @cindex @code{target_configargs} @item target_configargs The contents of this variable is passed to all target @command{configure} scripts. @end table

Thus most robust way to get that behavior might be the (untested):

lib.optionalString (buildPlatform.isDarwin) '' export build_configargs=ac_cv_func_aligned_alloc=no '' ++ lib.optionalString (hostPlatform.isDarwin) '' export host_configargs=ac_cv_func_aligned_alloc=no '' ++ lib.optionalString (targetPlatform.isDarwin) '' export target_configargs=ac_cv_func_aligned_alloc=no ''

Thank you very much! Separate configure flags seem to be the correct fix, and achieves my goal of build platform independent binaries. PTAL.

@trofi

Before this change a Darwin gcc would output binaries that avoid aligned_alloc, regardless of target platform. That's fine for Darwin targets, but not for non-darwin targets such as pkgsCross.raspberryPi. This change replaces the single configure flag with a flag for each of build, host, target. Idea by @trofi.

eliasnaur · 2023-04-20T19:42:43Z

Excuse my ignorance, but what is the next step in getting this merged? More approvals?

trofi · 2023-04-20T19:47:50Z

Getting review from @NixOS/darwin-maintainers would be nice. Otherwise I support merging it tostaging.

eliasnaur · 2023-04-27T16:36:44Z

Time for staging, or wait a bit more?

wegank · 2023-04-27T16:39:35Z

I guess this can only take place after 2023-05-15?

veprbl

Looks like a sensible change.

eliasnaur · 2023-04-27T21:47:52Z

I guess this can only take place after 2023-05-15?

Does the ban apply to darwin-only changes as well?

vcunat · 2023-05-01T08:02:18Z

Well, perhaps we could say that gcc isn't a critical package on darwin (as clang is the default stdenv.cc), but any breaking changes have cutoff today (except desktop environment stuff).

veprbl · 2023-05-01T23:51:09Z

@eliasnaur Since this only affects cross, do you think you could massage this into a zero rebuild change?

winterqt · 2023-05-01T23:53:00Z

If it only affects cross, it should be enough to just do the inverse of this:

nixpkgs/pkgs/development/compilers/gcc/12/default.nix

Lines 72 to 74 in 120dd6a

    
           # We only apply this patch when building a native toolchain for aarch64-darwin, as it breaks building 
        
           # a foreign one: https://github.com/iains/gcc-12-branch/issues/18 
        
           ++ optional (stdenv.isDarwin && stdenv.isAarch64 && buildPlatform == hostPlatform && hostPlatform == targetPlatform) (fetchpatch {

eliasnaur · 2023-05-02T18:16:30Z

If I make the PR zero-rebuild as you describe, won't it break cross-building a darwin gcc from linux? If so, I'd rather wait 2 weeks for this PR to get into staging. I have other mass-rebuild reproducibility PRs that need to wait for staging anyway.

Et7f3 · 2023-05-02T18:57:51Z

cross-building a darwin gcc from linux

macOS/Darwin is a special case, as not the whole OS is open-source. It’s only possible to cross compile between aarch64-darwin and x86_64-darwin. aarch64-darwin support was recently added, so cross compilation is barely tested.

https://nix.dev/tutorials/cross-compilation#determining-the-host-platform-config

So I would say it is fine

eliasnaur · 2023-05-02T19:41:55Z

cross-building a darwin gcc from linux

macOS/Darwin is a special case, as not the whole OS is open-source. It’s only possible to cross compile between aarch64-darwin and x86_64-darwin. aarch64-darwin support was recently added, so cross compilation is barely tested.

https://nix.dev/tutorials/cross-compilation#determining-the-host-platform-config

So I would say it is fine

It may not work today, but won't we like to be able to cross-compile to darwin in the future?

If I seem overly concerned about adding these special cases, it's because I've spent quite some time finding and fixing reproducibility ruining special cases already[0]. I'd rather not leave more for future contributors to untangle.

[0] Apart from this PR, there are #226048, #223861, #225328, of which two trigger mass rebuilding.

EDIT: almost forgot #229377.

uri-canva · 2023-05-02T22:12:15Z

I think it's worth keeping it as is and wait to merge.

eliasnaur · 2023-05-16T22:51:56Z

The staging unrestricting date has come and passed, so I assume this can be merged?

veprbl · 2023-05-16T23:42:35Z

The staging unrestricting date has come and passed, so I assume this can be merged?

According to #232237, you are correct.
Let's do it.

eliasnaur requested a review from matthewbauer as a code owner April 15, 2023 14:09

eliasnaur mentioned this pull request Apr 15, 2023

gcc: remove aligned_alloc hack on Darwin #226089

Closed

12 tasks

trofi reviewed Apr 15, 2023

View reviewed changes

eliasnaur force-pushed the targetPlatform-aligned-alloc branch from 74a0638 to b9aabd8 Compare April 16, 2023 13:01

trofi approved these changes Apr 16, 2023

View reviewed changes

eliasnaur changed the title ~~gcc: don't disable aligned_alloc on Darwin when cross-compiling~~ gcc: only disable aligned_alloc for darwin build/host/target platforms Apr 16, 2023

eliasnaur mentioned this pull request Apr 18, 2023

stdenv: delete the NIX_COREFOUNDATION_RPATH sledgehammer #226048

Closed

12 tasks

veprbl approved these changes Apr 27, 2023

View reviewed changes

mweinelt added the 12.approvals: 2 This PR was reviewed and approved by two persons. label May 6, 2023

veprbl merged commit 8d2846e into NixOS:staging May 16, 2023

Uh oh!

Conversation

eliasnaur commented Apr 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Things done

Uh oh!

trofi Apr 15, 2023

Choose a reason for hiding this comment

Uh oh!

eliasnaur Apr 15, 2023

Choose a reason for hiding this comment

Uh oh!

trofi Apr 15, 2023

Choose a reason for hiding this comment

Uh oh!

eliasnaur Apr 15, 2023

Choose a reason for hiding this comment

Uh oh!

trofi Apr 16, 2023

Choose a reason for hiding this comment

Uh oh!

eliasnaur Apr 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eliasnaur commented Apr 20, 2023

Uh oh!

trofi commented Apr 20, 2023

Uh oh!

eliasnaur commented Apr 27, 2023

Uh oh!

wegank commented Apr 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

veprbl left a comment

Choose a reason for hiding this comment

Uh oh!

eliasnaur commented Apr 27, 2023

Uh oh!

vcunat commented May 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

veprbl commented May 1, 2023

Uh oh!

winterqt commented May 1, 2023

Uh oh!

eliasnaur commented May 2, 2023

Uh oh!

Et7f3 commented May 2, 2023

Uh oh!

eliasnaur commented May 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

uri-canva commented May 2, 2023

Uh oh!

eliasnaur commented May 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

veprbl commented May 16, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

eliasnaur commented Apr 15, 2023 •

edited

Loading

eliasnaur Apr 16, 2023 •

edited

Loading

wegank commented Apr 27, 2023 •

edited

Loading

vcunat commented May 1, 2023 •

edited

Loading

eliasnaur commented May 2, 2023 •

edited

Loading

eliasnaur commented May 16, 2023 •

edited

Loading