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

mips64-linux: declare it an n32 ABI target #237815

Closed
wants to merge 1 commit into from

Conversation

trofi
Copy link
Contributor

@trofi trofi commented Jun 14, 2023

Without the change build on mips64-unknown-linux-gnu fails as:

    $ nix-build -A buildPackages.gcc12 --argstr crossSystem mips64-linux

    In file included from ...-glibc-mips64-unknown-linux-gnu-2.37-8-dev/include/bits/stat.h:25,
                     from ...-glibc-mips64-unknown-linux-gnu-2.37-8-dev/include/fcntl.h:78,
                     from ../../../../gcc-12.3.0/libsanitizer/sanitizer_common/sanitizer_linux.cpp:55:
    ...-glibc-mips64-unknown-linux-gnu-2.37-8-dev/include/bits/struct_stat.h:190:8: error: redefinition of 'struct stat64'
      190 | struct stat64
          |        ^~~~~~
Description of changes
Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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
  • 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.

configureFlags = callFile ../common/configure-flags.nix { }
# On mips64-unknown-linux-gnu libsanitizer defines collide with
# glibc's definitions and fail the build. It was fixed in gcc-13+.
++ lib.optionals targetPlatform.isMips [ "--disable-libsanitizer" ];
Copy link
Member

Choose a reason for hiding this comment

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

We could be more specific here (like in my PR), since it's only an issue when the ABI isn't otherwise specified. Not sure if it's worth it or not. Up to @amjoseph-nixpkgs as an actual MIPS user, I'd say.

Copy link
Contributor Author

@trofi trofi Jun 14, 2023

Choose a reason for hiding this comment

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

Yeah, narrowing it down to n32 ABI sounds reasonable. Would be nice if our isMips64n32 helper worked as is. Not sure if it's broken helper or I'm using it incorrectly:

$ nix repl . --argstr crossSystem mips64-linux

nix-repl> stdenv.targetPlatform.config
"mips64-unknown-linux-gnu"

nix-repl> stdenv.targetPlatform.isMips64n32 # (was stdenv.hostPlatform.isMips64n32 before)

false

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And pkgs/development/compilers/gcc/common/platform-flags.nix already contains the equivalent code:

(lib.optional targetPlatform.isMips64n32 "--disable-libsanitizer") # libsanitizer does not compile on mips64n32

That explains why pkgsCross.mips64-linux-gnuabin32 is able to compile. Maybe we should just fix the predicate here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed mips64-linux to return true for isMips64n32.

Copy link

Choose a reason for hiding this comment

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

6cec030 looked like the right solution; I'm testing that right now...

    Without the change build on mips64-unknown-linux-gnu fails as:

        $ nix-build -A buildPackages.gcc12 --argstr crossSystem mips64-linux

        In file included from ...-glibc-mips64-unknown-linux-gnu-2.37-8-dev/include/bits/stat.h:25,
                         from ...-glibc-mips64-unknown-linux-gnu-2.37-8-dev/include/fcntl.h:78,
                         from ../../../../gcc-12.3.0/libsanitizer/sanitizer_common/sanitizer_linux.cpp:55:
        ...-glibc-mips64-unknown-linux-gnu-2.37-8-dev/include/bits/struct_stat.h:190:8: error: redefinition of 'struct stat64'
          190 | struct stat64
              |        ^~~~~~
@trofi trofi requested a review from Ericson2314 as a code owner June 14, 2023 22:39
@trofi trofi changed the title gcc12: disable libsanitizer for mips64 mips64-linux: declare it an n32 ABI target Jun 14, 2023
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin and removed 10.rebuild-darwin: 1 10.rebuild-darwin: 1-10 10.rebuild-linux: 1 labels Jun 15, 2023
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.

Please don't do this!

The n32 ABI is great for routers or machines with less than 4gb of RAM, but -- like the other two popular ILP32 ABIs (aarch64_ilp32 and the x86 x32 ABI) probably shouldn't be the default for any architecture.

Also, in the MIPS case, right now there are two pain points:

  • boost-context does not support n32 yet
    • it needs to know the entire processor context state, including local variables on the stack, so it can save/restore a thread. This requires hand-written assembler for every ABI, and nobody's done this for n32 yet.
    • this means you can't run Nix itself on n32
  • Rust doesn't support n32 yet

I'm working on both of these myself, but it's a low-priority task. My routers actually run a mixed abin32/abi64 userspace, where everything that uses Rust or boost-context is built for abi64 and the rest is abin32.

Fortunately as part of my work on #235230 I discovered a way to trick both Nix and gnu-config into letting the user specify the ABI without needing two hyphens:

  • mips64el-linux (abi64)
  • mips64el-linuxabin32 (abin32)
$ config.sub mips64el-linuxabin32
mips64el-unknown-linux-gnuabin32

$ config.sub mips64el-linux
mips64el-unknown-linux-gnu

As soon as I finish #235230 (hopefully this week) I will submit a PR to Nix so it understands that the "double" *-linuxabin32 means "I want the n32 ABI".

@trofi trofi closed this Jun 15, 2023
@trofi trofi deleted the gcc-12-mips-no-sanitizer branch June 15, 2023 06:14
@trofi
Copy link
Contributor Author

trofi commented Jun 20, 2023

At the risk of stating the obvious mips64-linux (and mips64el-linux) is already an ...:

# in nixpkgs.git adjusted to use `gcc-13`:

$ nix build -f. re2c --argstr crossSystem mips64-linux re2c && file result/bin/re2c; nix build -f. re2c --argstr crossSystem mips64el-linux re2c && file result/bin/re2c
result/bin/re2c: ELF 32-bit MSB executable, MIPS, N32 MIPS-III version 1 (SYSV), dynamically linked, interpreter /nix/store/0m8xmj5xamldxf3rxvpf2cdx2j3idpv3-glibc-mips64-unknown-linux-gnu-2.37-8/lib/ld.so.1, for GNU/Linux 3.10.0, not stripped
result/bin/re2c: ELF 32-bit LSB executable, MIPS, N32 MIPS-III version 1 (SYSV), dynamically linked, interpreter /nix/store/nb5jwa0xy04rfz78arwg2mla7xhfv493-glibc-mips64el-unknown-linux-gnu-2.37-8/lib/ld.so.1, for GNU/Linux 3.10.0, not stripped

... n32 ABI in nixpkgs on gcc.

AFAIU the change did not alter this fact and only exposed it to the packages so they would enable the usual n32-specific workarounds. It would be nice if predicate did not lie at least for these mips64-unknown-linux-gnu / mips64el-unknown-linux-gnu cases.

@ghost
Copy link

ghost commented Jun 23, 2023

At the risk of stating the obvious mips64-linux (and mips64el-linux) is already an ...:

Wow that was not obvious to me; thank you for letting me know!

It would be nice if predicate did not lie at least for these mips64-unknown-linux-gnu / mips64el-unknown-linux-gnu cases.

I totally agree. Trying to figure out where things went wrong here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants