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.

Sign up for GitHub

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

lix: only use LTO with GCC #353576

Merged
merged 1 commit into from
Dec 7, 2024
Merged

lix: only use LTO with GCC #353576

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkgs/tools/package-management/lix/common.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ stdenv.mkDerivation {
[
# Enable LTO, since it improves eval performance a fair amount
# LTO is disabled on static due to strange linking errors
(lib.mesonBool "b_lto" (!stdenv.hostPlatform.isStatic))
(lib.mesonBool "b_lto" (!stdenv.hostPlatform.isStatic && stdenv.cc.isGNU))
Copy link
Contributor

@JohnRTitor JohnRTitor Nov 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Can we disable it for Darwin instead?

Clang LTO on Linux should be fine.

And add a comment there why its disabled on Darwin.

Copy link
Member

Choose a reason for hiding this comment

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

Have you tested it? I would suspect it is more likely to be LLVM exposing UB than a Darwin issue. And the Nix derivation uses isGNU here.

Copy link
Contributor

@JohnRTitor JohnRTitor Nov 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Tried building lix from master with clangStdenv and it built fine. (x86_64-linux)

Copy link
Member

Choose a reason for hiding this comment

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

Interesting. What about with libc++?

Copy link
Contributor

Choose a reason for hiding this comment

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

This does work if that's what you are asking:

❯  nix build --impure --expr '(builtins.getFlake "github:nixos/nixpkgs/master").legacyPackages."${builtins.currentSystem}".libcxx.override {stdenv = (builtins.getFlake "github:nixos/nixpkgs/master").legacyPackages."${builtins.currentSystem}".clangStdenv;}' --print-out-paths 
/nix/store/nqpv4xbh5zqnvfn80702gahiklxxjcr3-libcxx-18.1.8

PS: if you have a shorthand for these overrides in cli, let me know!

Copy link
Member

Choose a reason for hiding this comment

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

what in the hell. maybe we have a missing include but I'm guessing those sys constants are just broken. (though libcxxStdenv is SURELY not using an alternative libc right? just libc++?)

it may also be broken toolchain on the nixpkgs side for all i know, like this release blocker for lix 2.92: #177129

Copy link
Member

Choose a reason for hiding this comment

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

This does seem to only be a missing include. Sent a patch upstream in https://gerrit.lix.systems/c/lix/+/2286

Using this override still fails when linking with things like the aws-sdk though (probably because they're using glibc?), and being a bit more forceful with libcxx usage like so

let
  pkgs = import <nixpkgs> {
    config = {
      replaceStdenv = { pkgs }: pkgs.libcxxStdenv;
    };
    overlays = [ ];
  };
in

pkgs.lix

results in the always fun error: infinite recursion encountered, so I don't think we can determine whether this is a libcxx or Darwin issue just yet

I'm inclined to say we should change this to stdenv.cc.libcxx != null for now, as that would cover Darwin and the possibly broken (and currently un-evaluatable) libcxx Linux build -- which can always be revisited later

Also let me know if there's anything I can help with to get this merged. Would love to see a fix after all this time :)

Copy link
Member

@lf- lf- Dec 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

i believe there is a libcxxStdenvPackages somewhere that is a nixpkgs staged from libcxxStdenv but yeah you can't really mix libc++ and libstdc++ in one binary at least if you're linking c++ symbols between them.

if this pr is acceptable i can just like, merge it. i have commit rights. i think it's more important that lix works than that it has LTO in all configurations possible (including weird ones) given that iirc LTO doesn't make that huge a difference anyway.

Copy link
Member

@winterqt winterqt Dec 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

i think it's more important that lix works than that it has LTO in all configurations possible

Agreed.

I'm inclined to say we should change this to stdenv.cc.libcxx != null for now, as that would cover Darwin and the possibly broken (and currently un-evaluatable) libcxx Linux build -- which can always be revisited later

Also agreed.

Copy link
Member

Choose a reason for hiding this comment

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

if this pr is acceptable

I'd say it 100% is. My suggestions were by no means a blocker, just a way to hopefully get things going

i believe there is a libcxxStdenvPackages somewhere that is a nixpkgs staged from libcxxStdenv

TIL that is pkgsLLVM...I thought that used clangStdenv. Time to build :)
image

(lib.mesonEnable "gc" true)
(lib.mesonBool "enable-tests" true)
(lib.mesonBool "enable-docs" enableDocumentation)
Expand Down
Loading