treewide: migrate to -fno-common by Gaelan · Pull Request #110571 · NixOS/nixpkgs

Gaelan · 2021-01-23T08:46:38Z

Motivation for this change

GCC 10 defaults to -fno-common, a change that broke some packages. To ease the transition when migrating to GCC 10, we changed the default back in our build of GCC. This commit undoes that, bringing us closer to upstream and reducing our technical debt. (Clang, which made the same change, also cites "performance and code-size benefits"; it's unclear if this is the case for GCC.)

In an effort to reduce the potential breakage from this PR, I've made no effort to patch or upgrade packages broken by this. Instead, for any package that fails to build, I fix it by setting NIX_CFLAGS_COMPILE = "-fcommon", i.e. returning to the status quo. This brings the vast majority of packages onto the new default, and has no risk of introducing new bugs due to incorrect patches. As the rest of the world migrates towards GCC 10, most of these will probably get fixed by upstream, and we can remove the flag; for the rest, we can decide whether to write/find a patch or keep the flag indefinitely.

This PR is not ready to merge at this point. Before doing so, I'd like to try building at least these things:

The configuration.nix of the laptop I'm testing this on
Some (or all) of the configurations in release.nix
Packages listed as broken (or previously broken) in the Gentoo and Fedora issues for migrating to GCC 10.

It might also be a good idea to set up a hydra job.

Things done

Gaelan · 2021-01-23T10:41:40Z

Huh, it's weird that this causes an stdenv rebuild on darwin. It should only affect GCC.

SuperSandro2000 · 2021-01-23T12:41:20Z

https://gcc.gnu.org/onlinedocs/gcc/Code-Gen-Options.html

-fcommon
In C code, this option controls the placement of global variables defined without an initializer, known as tentative definitions in the C standard. Tentative definitions are distinct from declarations of a variable with the extern keyword, which do not allocate storage.

The default is -fno-common, which specifies that the compiler places uninitialized global variables in the BSS section of the object file. This inhibits the merging of tentative definitions by the linker so you get a multiple-definition error if the same variable is accidentally defined in more than one compilation unit.

The -fcommon places uninitialized global variables in a common block. This allows the linker to resolve all tentative definitions of the same variable in different compilation units to the same object, or to a non-tentative definition. This behavior is inconsistent with C++, and on many targets implies a speed and code size penalty on global variable references. It is mainly useful to enable legacy code to link without errors.

r-burns

I wonder if this would warrant a hydra job. Since this primarily affects linux, it could be a good task for spare x86_64-linux builders.

r-burns · 2021-01-24T10:22:42Z

pkgs/tools/archivers/cpio/default.nix

The darwin stdenv contains cpio, but we can avoid the rebuild:

Suggested change

NIX_CFLAGS_COMPILE = "-fcommon";

NIX_CFLAGS_COMPILE = if stdenv.cc.isGNU then "-fcommon" else null;

Also it looks like gentoo has a fix for this: https://bugs.gentoo.org/705900

It goes to staging anyway so we don't really need to worry about stdenv rebuilds.

Aha, that solves the Darwin mystery.

My inclination is to use CFLAGS instead of patches initially, because it keeps the PR simple and seems less likely to break things; we can always switch to patches on a per-package basis later. But I can probably be convinced otherwise.

I ran into this for #126411 too. I opted for patches in the cases I came across because I figured they'd break sooner and trigger people to reevaluate them. For cpio in particular I've also sent an email to upstream (no response so far). So I'm in favor of having this apply for Darwin and clang too : )

Gaelan · 2021-01-25T09:49:12Z

iso_minimal now builds. I can upload to cachix if it helps anyone.

Gaelan · 2021-01-25T10:00:42Z

Bah, there's a merge conflict, but if I try to merge in aa8868c (the output of git merge-base origin/master origin/staging), the PR pulls in a bunch of unrelated commits for some reason.

ruuda · 2021-01-25T13:59:59Z

There’s a conflict in squashfsTools because lz4 support recently became unconditional, and it touched related lines.

thefloweringash · 2021-02-05T07:12:07Z

Clang 11 is making the same change (changelog, D75056) so these changes shouldn't be restricted to GCC. I ran into this when working on aarch64-darwin which has a hydra job. It could be useful to pick through the set of new failures in the change from clang 10 to clang 11: https://hydra.nixos.org/eval/1646384?compare=1646102#tabs-now-fail.

Gaelan · 2021-02-15T00:02:31Z

Sorry this PR is languishing (and for all the force pushes) - I can't manage to fix the merge conflict without GitHub pulling in thousands of unrelated commits. I thought git merge xyz, where xyz is a commit appearing in both master and staging, would do the trick, but it doesn't seem to work.

trofi · 2022-06-18T19:36:45Z

All failing packages noticed on hydra are now fixed in master or staging. I'd like to do one extra run once staging trickles down into master to check if we have any extra darwin fallout: #177171

After that we'll be ready for merge.

trofi · 2022-06-20T20:56:42Z

Failures extracted from this iteration:

gmtp: gmtp: apply -fcommon workaround for real #178536
uim: uim: pull upstream fix for -fno-common toolchains #178538
vscode-extensions.ms-python.python (lttng-ust-2.10.5): vscode-extensions.ms-python.python: pull upstream fix for -fno-common… #178370
ocaml-ng.ocamlPackages_4_05.lablgtk: ocaml-ng.ocamlPackages_4_05.lablgtk: add -fcommon workaround #178373
a52dec (darwin): ended up being not -fcommon-specific
- a52dec aarch64-darwin fix: a52dec: fix build on aarch64-darwin #180194
bs-platform: bs-platform: workaround -fno-common build failure on aarch64 #179253
sbcl (darwin): sbcl: pull darwin fix pending upstream inclusion for -fno-common tool… #179934
texlive.bin.xdvi (darwin): texlive.bin.xdvi: pull upstream darwin fix for -fno-common toolchains #179943

vcunat · 2022-06-25T06:32:05Z

Jobset: as x86_64-darwin is now catching up about a week of outage on Hydra, I canceled it here and put in aarch64-darwin instead (was idling right now).

GCC 10 sets -fno-common by default. This broke some packages, so when moving to GCC 10 we initially disabled this behavior. This commit reverts that, bringing us closer to the standard and upstream. Co-authored-by: Sergei Trofimovich <[email protected]>

trofi · 2022-07-06T07:43:23Z

I hereby declare all (~240) the knows failures to be fixed. Should be ready for staging.

nixos-discourse · 2022-07-06T16:49:44Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/help-disabling-fno-common-hack/19031/3

Artturin · 2022-07-06T20:36:52Z

@ofborg build nixosTests.gnome

vcunat · 2022-07-07T08:08:56Z

Thanks. This must've taken really lots of work. I'm convinced it's in good enough shape for staging. Minor remaining issues can always get fixed during staging-next.

trofi · 2022-07-09T07:21:17Z

Recent package additions that fail now:

decoder: decoder: pull patch pending upstream inclusion for -fno-common toolch… #180820

Artturin · 2022-07-09T12:06:52Z

Thank you for your efforts trofi

ofborg bot added 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. 10.rebuild-linux-stdenv This PR causes stdenv to rebuild on Linux and must target a staging branch. labels Jan 23, 2021

ofborg bot requested review from Phreedom, edolstra, ftrvxmtrx and matthewbauer January 23, 2021 10:26

r-burns reviewed Jan 24, 2021

View reviewed changes

ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Jan 25, 2021

ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Jan 25, 2021

Gaelan force-pushed the no-common branch from dcff9cb to 03ba355 Compare January 25, 2021 09:58

ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Jan 25, 2021

Gaelan force-pushed the no-common branch from a10af6a to 03ba355 Compare January 25, 2021 10:04

ofborg bot requested review from KoviRobi, lovek323, ruuda and samueldr January 25, 2021 10:14

bobrik mentioned this pull request Feb 4, 2021

Native support for Apple Silicon #105026

Merged

21 tasks

Gaelan force-pushed the no-common branch from 3474e59 to bf923cc Compare February 14, 2021 23:58

Gaelan force-pushed the no-common branch from bf923cc to d6fa252 Compare February 15, 2021 00:04

kenranunderscore mentioned this pull request Jun 11, 2022

sil-q: init at 1.5.0 #177331

Merged

13 tasks

azuwis mentioned this pull request Jun 16, 2022

sketchybar: 2.5.2 -> 2.7.1 #177829

Merged

13 tasks

trofi force-pushed the no-common branch from eae7867 to 7f81ace Compare June 18, 2022 07:09

trofi force-pushed the no-common branch from 7f81ace to 2cb5dbb Compare June 19, 2022 07:12

trofi force-pushed the no-common branch 2 times, most recently from 3fdf85c to e9ee2e0 Compare July 2, 2022 17:16

trofi force-pushed the no-common branch from e9ee2e0 to 5b47eb1 Compare July 6, 2022 07:39

trofi changed the title ~~WIP: treewide: migrate to -fno-common~~ treewide: migrate to -fno-common Jul 6, 2022

trofi marked this pull request as ready for review July 6, 2022 07:40

trofi requested a review from matthewbauer as a code owner July 6, 2022 07:40

vcunat merged commit 43f9c19 into NixOS:staging Jul 7, 2022

trofi mentioned this pull request Jul 14, 2022

lsof: 4.94.0 -> 4.95.0 #179945

Merged

13 tasks

vbgl added a commit to vbgl/nixpkgs that referenced this pull request Aug 3, 2022

ocamlPackages_4_06.num: fix after NixOS#110571

2cae89e

vbgl added a commit that referenced this pull request Aug 9, 2022

ocamlPackages_4_06.num: fix after #110571

e983107

trofi mentioned this pull request Mar 3, 2023

tbb: Split into tbb_2020_3 and tbb_2021_8 #217585

Merged

13 tasks

	NIX_CFLAGS_COMPILE = "-fcommon";
	NIX_CFLAGS_COMPILE = if stdenv.cc.isGNU then "-fcommon" else null;

Uh oh!

Comments

Conversation

Gaelan commented Jan 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation for this change

Things done

Uh oh!

Gaelan commented Jan 23, 2021

Uh oh!

SuperSandro2000 commented Jan 23, 2021

Uh oh!

r-burns left a comment

Choose a reason for hiding this comment

Uh oh!

r-burns Jan 24, 2021

Choose a reason for hiding this comment

Uh oh!

r-burns Jan 24, 2021

Choose a reason for hiding this comment

Uh oh!

SuperSandro2000 Jan 24, 2021

Choose a reason for hiding this comment

Uh oh!

Gaelan Jan 24, 2021 • 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!

toonn Oct 19, 2021

Choose a reason for hiding this comment

Uh oh!

Gaelan commented Jan 25, 2021

Uh oh!

Gaelan commented Jan 25, 2021

Uh oh!

ruuda commented Jan 25, 2021

Uh oh!

thefloweringash commented Feb 5, 2021

Uh oh!

Gaelan commented Feb 15, 2021

Uh oh!

trofi commented Jun 18, 2022

Uh oh!

trofi commented Jun 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vcunat commented Jun 25, 2022

Uh oh!

trofi commented Jul 6, 2022

Uh oh!

nixos-discourse commented Jul 6, 2022

Uh oh!

Artturin commented Jul 6, 2022

Uh oh!

vcunat commented Jul 7, 2022

Uh oh!

trofi commented Jul 9, 2022 • edited by Artturin Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Artturin commented Jul 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

Gaelan commented Jan 23, 2021 •

edited

Loading

Gaelan Jan 24, 2021 •

edited

Loading

trofi commented Jun 20, 2022 •

edited

Loading

trofi commented Jul 9, 2022 •

edited by Artturin

Loading