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

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

Closed
wants to merge 1 commit into from

Conversation

fgaz
Copy link
Member

@fgaz fgaz commented Nov 17, 2023

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
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.

### RISC-V


# https://gcc.gnu.org/git?p=gcc.git;a=commit;h=f797260adaf52bee0ec0e16190bbefbe1bfc3692
Copy link

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

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

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

Choose a reason for hiding this comment

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

Copy link
Member Author

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!

pkgs/development/compilers/gcc/patches/default.nix Outdated Show resolved Hide resolved
@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.

@fgaz
Copy link
Member Author

fgaz commented Nov 17, 2023

...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).

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
@NickCao
Copy link
Member

NickCao commented Dec 16, 2023

Workarounds removed in #274827

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants