Navigation Menu

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

Jump to bottom

gcc9: add patches required for target musl #101189

Closed
wants to merge 1 commit into from
Closed

gcc9: add patches required for target musl #101189

wants to merge 1 commit into from

Conversation

AmineChikhaoui
Copy link
Member

@AmineChikhaoui AmineChikhaoui commented Oct 20, 2020 •
edited

These patches are taken from:
https://github.com/richfelker/musl-cross-make/tree/master/patches/gcc-9.2.0
and based on a recommendation from the musl maintainers as these patches
are deemed critical for gcc9/musl to work correctly.

This started with an obscure bug I had while investigating the following
program which was getting unexpectedly aborted instead of catching the
exception:

#include <boost/thread.hpp>
#include <iostream>
#include <thread>

struct Foo {};

void foo() {
  try {
    std::cout << "entered thread" << std::endl;
    throw Foo();
  } catch (...) {
  }
}

void bar() {
  try {
    foo();
  } catch (...) {
  }
}

int main() {
  std::thread _bar(bar);
  boost::thread _foo(foo);
  _bar.join();
  _foo.join();
  std::cout << "should ignore exception" << std::endl;
  return 0;
}

Thanks to nsz and dalias in #musl freenode channel they traced down the
issue to be linked to a missing patch (0014-fix-gthr-weak-refs-for-libgcc.patch
in this case) which was needed. They also recommended adding the following
patches that I added as those are all critical.
I think these patches aren't really musl specific and could be applied
all the time, in fact 0014-fix-gthr-weak-refs-for-libgcc.patch is
already upstream in GCC 10 but I'm not sure if we should do that.

Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@AmineChikhaoui
Copy link
Member Author

also ping @domenkozar @dtzWill @shlevy and @rasendubi (based on https://github.com/NixOS/rfcs/blob/master/rfcs/0023-musl-libc.md#responsibility)

@omasanori
Copy link
Contributor

Result of nixpkgs-review pr 101189 1

2 packages marked as broken and skipped:
  • hydra-migration
  • nix-exec
2 packages blacklisted:
  • tests.nixos-functions.nixos-test
  • tests.nixos-functions.nixosTest-test
3 packages failed to build:
  • crystal2nix
  • fusionInventory
  • python37Packages.nixpkgs
55 packages built:
  • bundix
  • busybox-sandbox-shell
  • cabal2nix
  • cachix
  • common-updater-scripts
  • dep2nix
  • disnix
  • disnixos
  • go2nix
  • haskellPackages.cachix
  • haskellPackages.hercules-ci-agent
  • haskellPackages.hocker
  • haskellPackages.nix-paths
  • hercules-ci-agent
  • hydra-unstable
  • lispPackages.quicklisp-to-nix
  • lispPackages.quicklisp-to-nix-system-info
  • nix (nixStable)
  • nix-bundle
  • nix-direnv
  • nix-doc
  • nix-du
  • nix-index
  • nix-pin
  • nix-plugins
  • nix-prefetch
  • nix-prefetch-bzr
  • nix-prefetch-cvs
  • nix-prefetch-docker
  • nix-prefetch-git
  • nix-prefetch-hg
  • nix-prefetch-scripts
  • nix-prefetch-svn
  • nix-serve
  • nix-update
  • nix-update-source
  • nixFlakes (nixUnstable)
  • nixos-generators
  • nixos-shell
  • nixpkgs-review
  • nixui
  • pkgsMusl.stdenv
  • pkgsStatic.stdenv
  • python27Packages.nix-kernel
  • python37Packages.nassl
  • python37Packages.nix-kernel
  • python37Packages.pythonix
  • python37Packages.sslyze
  • python38Packages.nassl
  • python38Packages.nix-kernel
  • python38Packages.nixpkgs
  • python38Packages.pythonix
  • sslyze (python38Packages.sslyze)
  • vgo2nix
  • vulnix

@dtzWill
Copy link
Member

dtzWill commented Oct 30, 2020

LGTM from inspection, build-testing a bit presently.

We may wish to bump to 1.2.1 while we're rebuilding musl things... :).

@dtzWill
Copy link
Member

dtzWill commented Oct 30, 2020

Does gcc10 (or versions before 9) need any of these patches, is that known? :)

@AmineChikhaoui
Copy link
Member Author

Thanks for the review @dtzWill . I don't see gcc 10 in https://github.com/richfelker/musl-cross-make/tree/master/patches, so I would assume no. For that particular problem I was investigating the patch I needed was already upstream in gcc 10 for example.

Good point about bumping to 1.2.1, I might try that in the project I'm experimenting with.

@AmineChikhaoui
Copy link
Member Author

@dtzWill I bumped musl version in the last commit, tested that pkgsMusl.stdenv builds but didn't do other tests yet.

@dtzWill
Copy link
Member

dtzWill commented Nov 2, 2020 •
edited

1.2.1 update may be better tackled separately? It's not widely adopted in musl-based distros "yet" AFAICT... Alpine used it with patches, and just past few days bumped to git and calling for a 1.2.2 on the mailing list[1].

Which makes me think perhaps we should wait until that settles a bit? :)

[1] https://www.openwall.com/lists/musl/2020/10/30/13

@AmineChikhaoui
Copy link
Member Author

@dtzWill Right, that's certainly more safe. I'll revert the bump in that case.

@AmineChikhaoui
gcc9: add patches required for target musl
40c9af9 
These patches are taken from:
  https://github.com/richfelker/musl-cross-make/tree/master/patches/gcc-9.2.0
and based on a recommendation from the musl maintainers as these patches
are deemed critical for gcc9/musl to work correctly.

This started with an obscure bug I had while investigating the following
program which was getting unexpectedly aborted instead of catching the
exception:
```
\#include <boost/thread.hpp>
\#include <iostream>
\#include <thread>

struct Foo {};

void foo() {
  try {
    std::cout << "entered thread" << std::endl;
    throw Foo();
  } catch (...) {
  }
}

void bar() {
  try {
    foo();
  } catch (...) {
  }
}

int main() {
  std::thread _bar(bar);
  boost::thread _foo(foo);
  _bar.join();
  _foo.join();
  std::cout << "should ignore exception" << std::endl;
  return 0;
}
```

Thanks to nsz and dalias in #musl freenode channel they traced down the
issue to be linked to a missing patch (0014-fix-gthr-weak-refs-for-libgcc.patch
in this case) which was needed. They also recommended adding the following
patches that I added as those are all critical.
I think these patches aren't really musl specific and could be applied
all the time, in fact 0014-fix-gthr-weak-refs-for-libgcc.patch is
already upstream in GCC 10 but I'm not sure if we should do that.
@AmineChikhaoui
Copy link
Member Author

@dtzWill Anything remaining that I should take care of?

@AmineChikhaoui
Copy link
Member Author

@dtzWill do you know who should hit the merge button for this?

@AmineChikhaoui
Copy link
Member Author

There doesn't seem to be any interest for merging this for over a month now so closing the PR.
If there is a maintainer interested, feel free to apply the patch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dtzWill dtzWill dtzWill approved these changes

@matthewbauer matthewbauer Awaiting requested review from matthewbauer

@thoughtpolice thoughtpolice Awaiting requested review from thoughtpolice

Assignees
No one assigned
Labels
10.rebuild-darwin: 0 10.rebuild-linux: 11-100
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@AmineChikhaoui @omasanori @dtzWill