Skip to content

gcc: fix missing atomic ops errors on risc-v#268067

Closed
fgaz wants to merge 1 commit intoNixOS:masterfrom
fgaz:gcc/riscv-atomic
Closed

gcc: fix missing atomic ops errors on risc-v#268067
fgaz wants to merge 1 commit intoNixOS:masterfrom
fgaz:gcc/riscv-atomic

Conversation

@fgaz
Copy link
Member

@fgaz fgaz commented Nov 17, 2023 •
edited
Loading

Description of changes

Fixes errors like:

ld: [...]: undefined reference to `__atomic_exchange_1'

Fixes #258614 (comment)

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
    • riscv64-linux
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
    • cmakeMinimal (was broken by this)
    • aws-sdk-cpp
    • libjxl
    • numactl
    • mariadb
    • zstd
    • llvm and clang 17
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.11 Release Notes (or backporting 23.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@fgaz fgaz requested a review from a user November 17, 2023 09:36
ghost
ghost reviewed Nov 17, 2023
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

I'm not excited about carrying a gcc patch simply in order to eliminate LDFLAGS from a few packages.

It looks like this patch didn't make it into gcc 13.2, so if there is a workaround (which there is) we should be using the workaround instead until this patch shows up in a release. Generally we ship merged-but-unreleased patches only when there is no alternative available.

pkgs/development/compilers/gcc/patches/default.nix Outdated
Copy link

@ghost ghost Nov 17, 2023

Choose a reason for hiding this comment

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

What release is this commit part of? git tag --contains doesn't report any.

Copy link
Member Author

@fgaz fgaz Nov 17, 2023

Choose a reason for hiding this comment

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

None yet as far as I can see

Copy link
Member Author

@fgaz fgaz Nov 17, 2023

Choose a reason for hiding this comment

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

Turns out gcc 13 includes the patch

Copy link

@ghost ghost Nov 19, 2023

Choose a reason for hiding this comment

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

Then why not just use

Copy link
Member Author

@fgaz fgaz Nov 19, 2023

Choose a reason for hiding this comment

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

Yes that's perfect!

I thought that upgrade was still far off, good to know that's not the case!

@fgaz
Copy link
Member Author

fgaz commented Nov 17, 2023

I'm not excited about carrying a gcc patch simply in order to eliminate LDFLAGS from a few packages.

I suspect that there are way more broken packages that we don't know about, since nobody builds the whole nixpkgs on risc-v.

For now I just discovered cmake and llvm 17, so I can certainly apply the workaround to those too.

Up to you.

@ofborg ofborg bot added the 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. label Nov 17, 2023
@ofborg ofborg bot added 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. labels Nov 17, 2023
@fgaz
Copy link
Member Author

fgaz commented Nov 17, 2023 •
edited
Loading

...and of course it's a mass rebuild because optionalString false returns an empty string, which is different from an empty LDFLAGS. If we proceed with this I guess I'll have to split the workaround removal (or move everything to staging).

@fgaz
gcc: fix missing atomic ops errors on risc-v
32db619 
Fixes errors like:

    ld: [...]: undefined reference to `__atomic_exchange_1'

* Backport patch from gcc 13 that uses libatomic for missing ops:
  https://gcc.gnu.org/git?p=gcc.git;a=commit;h=f797260adaf52bee0ec0e16190bbefbe1bfc3692
  A few conflicts were resolved, so the patch is included as a file.
* Remove workarounds from
  * aws-sdk-cpp
  * libjxl
  * numactl
  * mariadb
  * zstd
@ghost
Copy link

ghost commented Nov 19, 2023

...and of course it's a mass rebuild because optionalString false returns an empty string, which is different from an empty LDFLAGS.

One of the many reasons to move towards env = lib.optionalAttrs { LDFLAGS = ... }...

@fgaz
Copy link
Member Author

fgaz commented Nov 19, 2023

Closing due to #268067 (comment).

I'll apply the workaround to cmake in another pr so risc-v is not blocked for another 1-2 staging cycles, then once #145377 is in I'll remove all workarounds.

@fgaz fgaz closed this Nov 19, 2023
This was referenced Nov 20, 2023
Merged
Merged
@NickCao
Copy link
Member

NickCao commented Dec 16, 2023

Workarounds removed in #274827

@fgaz fgaz added the 6.topic: riscv64 RISC-V is an open standard instruction set architecture label Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dasJ dasJ Awaiting requested review from dasJ

@ajs124 ajs124 Awaiting requested review from ajs124

@thoughtpolice thoughtpolice Awaiting requested review from thoughtpolice

@nh2 nh2 Awaiting requested review from nh2

@edolstra edolstra Awaiting requested review from edolstra

1 more reviewer
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

6.topic: riscv64 RISC-V is an open standard instruction set architecture 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@fgaz @NickCao