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

gcc9: add patches required for target musl #101189

Closed

Conversation

@AmineChikhaoui
Copy link
Member

@AmineChikhaoui AmineChikhaoui commented Oct 20, 2020

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

@AmineChikhaoui AmineChikhaoui commented Oct 21, 2020

@omasanori
Copy link
Contributor

@omasanori omasanori commented Oct 29, 2020

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 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 dtzWill commented Oct 30, 2020

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

@AmineChikhaoui
Copy link
Member Author

@AmineChikhaoui AmineChikhaoui commented Oct 30, 2020

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 AmineChikhaoui force-pushed the gcc-target-musl-patches branch from 0f522ed to f6d6c6b Oct 30, 2020
@AmineChikhaoui
Copy link
Member Author

@AmineChikhaoui AmineChikhaoui commented Oct 30, 2020

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

@ofborg ofborg bot requested review from thoughtpolice and dtzWill Oct 30, 2020
@dtzWill
Copy link
Member

@dtzWill dtzWill commented Nov 2, 2020

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

@AmineChikhaoui AmineChikhaoui commented Nov 3, 2020

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

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 AmineChikhaoui force-pushed the gcc-target-musl-patches branch from f6d6c6b to 40c9af9 Nov 3, 2020
@AmineChikhaoui
Copy link
Member Author

@AmineChikhaoui AmineChikhaoui commented Nov 16, 2020

@dtzWill Anything remaining that I should take care of?

@AmineChikhaoui
Copy link
Member Author

@AmineChikhaoui AmineChikhaoui commented Nov 26, 2020

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

@AmineChikhaoui
Copy link
Member Author

@AmineChikhaoui AmineChikhaoui commented Dec 6, 2020

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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants