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

lib.systems: add various has* flags useful for embedded systems #352629

Open
wants to merge 1 commit into
base: staging
Choose a base branch
from

Conversation

kuruczgy
Copy link
Contributor

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • 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
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.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.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added 6.topic: lib The Nixpkgs function library 6.topic: llvm/clang Issues related to llvmPackages, clangStdenv and related labels Oct 31, 2024
lib/systems/default.nix Outdated Show resolved Hide resolved
Copy link
Member

@FliegendeWurst FliegendeWurst left a comment

Choose a reason for hiding this comment

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

Should target staging

@kuruczgy kuruczgy changed the base branch from master to staging October 31, 2024 20:21
kuruczgy added a commit to kuruczgy/lean-esp32 that referenced this pull request Nov 1, 2024
This requires two main changes:
- An update to the nixpkgs patch. It is now based on this draft PR:
  NixOS/nixpkgs#352629
- An update to the Lean runtime, as Lean is updated to v4.10.0. The
  changes come from Lean commit
  0a1a855ba80e51515570439f3d73d3d9414ac053.
@kuruczgy
Copy link
Contributor Author

Thank you both for the feedback.

Before continuing with this, I would like some feedback from someone on the high level concept first. (E.g. the concept is fine, and we don't need to rethink and use some other approach.)

If it's confirmed that this is desirable change, I can update the PR to a non-draft quality one. (Split the two linunwind cases, fix the typos, fix the formatting, rebase onto latest master.)

@FliegendeWurst
Copy link
Member

I think the concept is fine. To convince more people I would suggest showing a use for each new condition.

@kuruczgy kuruczgy force-pushed the wip/embedded-cross-system branch from 4729f93 to e76ac5d Compare December 7, 2024 17:03
@kuruczgy
Copy link
Contributor Author

kuruczgy commented Dec 7, 2024

Updated, the major new change is that I now have two separate hasSystemLibunwind and hasStackUnwinding flags. My assumption is that Darwin and FreeBSD want to use a libunwind that comes from "somewhere else" (I assume stdenv handles this for those platforms), though I still had to add some exceptions to match the previous behavior. If someone has knowledge of these platforms please weigh on whether these exceptions are actually needed or were just accidental inconsistencies of the previous logic.

If someone is interested in somewhat of a "real world" use-case for this code, here is my example which originally motivated these changes: https://github.com/kuruczgy/lean-esp32/blob/c912a815d079cd5a3d029771c247e692f5516b71/flake.nix

Notably, targeting embedded platforms using nixpkgs is still very rough, so I expect several more changes to be needed, for example support for picolibc or some saner defaults for the hardening flags (currently a lot of them can blow up code size).

@kuruczgy kuruczgy marked this pull request as ready for review December 7, 2024 17:04
Copy link
Member

@alyssais alyssais left a comment

Choose a reason for hiding this comment

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

A lot of these are policy decisions rather than facts about platforms — "You often don't want…", "is probably a rare edge-case" etc. I don't think that belongs in lib/systems — it would make more sense as a Nixpkgs config option maybe. A default for that based on the system being built for would probably be fine.

The ones that are actual platform facts, like hasSystemLibunwind are fine.

@kuruczgy
Copy link
Contributor Author

kuruczgy commented Dec 7, 2024

I don't think that belongs in lib/systems — it would make more sense as a Nixpkgs config option maybe.

You mean something like this?

import nixpkgs {
  localSystem = "x86_64-linux";
  crossSystem = "riscv32-none-elf";
  config = {
    hasExceptions = false;
    hasFilesystem = false;
    ...
  };
}

I don't quite follow, how would nixpkgs know which of the two systems (build or host) the config options apply to?

Also tbh. most of these options are determined by the libc, which is part of the platform. (E.g. you want hasFilesystem = true iff your libc has some implementation for fopen, etc.)

@alyssais
Copy link
Member

alyssais commented Dec 7, 2024

I don't quite follow, how would nixpkgs know which of the two systems (build or host) the config options apply to?

Hmm, yeah — maybe they should be properties of the platform that you can pass in when you instantiate Nixpkgs, like some of the other attributes here, and have a name that suggests it's a policy decision rather than a platform fact, like "enableLocalization" instead of "hasLocalization"?

Also tbh. most of these options are determined by the libc, which is part of the platform. (E.g. you want hasFilesystem = true iff your libc has some implementation for fopen, etc.)

That makes sense, although here you're not checking libc. Maybe that would be better?

@kuruczgy
Copy link
Contributor Author

kuruczgy commented Dec 7, 2024

Hmm, yeah — maybe they should be properties of the platform that you can pass in when you instantiate Nixpkgs, like some of the other attributes here, and have a name that suggests it's a policy decision rather than a platform fact, like "enableLocalization" instead of "hasLocalization"?

Yes, I would be fine renaming all of them (except for maybe the stack unwinding options) to enable*.

That makes sense, although here you're not checking libc. Maybe that would be better?

I don't think we have much infrastructure yet for configuring/querying facts about the libc. Even just using a different libc is non-trivial. (E.g. to use picolibc I have to set libcCross using an overlay, setting just the libc option in the platform is not enough if your libc is not in the list of predefined options.)

The ideal solution might be something like the linux configuration system we have so we can assert/query facts about the libc, but I feel like we are quite far away from that.

@alyssais
Copy link
Member

alyssais commented Dec 7, 2024

Even just using a different libc is non-trivial.

Using e.g. musl on Linux is pretty trivial today.

setting just the libc option in the platform is not enough if your libc is not in the list of predefined options

Sounds like it should be added to the list then?

Anyway, we certainly have the libc information in the platforms, so these checks could definitely use that.

@kuruczgy
Copy link
Contributor Author

kuruczgy commented Dec 8, 2024

Anyway, we certainly have the libc information in the platforms, so these checks could definitely use that.

My issue is that one libc can be configured in many different ways. Just because you know that you have "picolibc" or "newlib" you don't know exactly what it supports.

For example, in picolibc you can configure at compile time whether you want locale support: https://github.com/picolibc/picolibc/blob/e3a4f137d85c5e902ef0efb7917790518764596e/doc/build.md#internationalization-options

If you turn this off, you also have to set -DLIBCXX_ENABLE_LOCALIZATION=OFF for libcxx (hasLocalization = false in this PR), otherwise libcxx won't compile. (Though I also could not compile libcxx with localization when I did enable support in picolibc, so there is probably something else missing too.)

Note that in embedded contexts "aggressively disable support for anything you aren't using" is probably a more common behavior due to the much stricter code size limits. (Whereas on desktops you probably don't care about an extra few MB of code size and usually just enable everything someone might need.)

Another example is clocks. (hasMonotonicClock in this PR). libcxx expects clock_gettime to exists (it checks whether the _POSIX_TIMERS macro is defined). Otherwise, it errors out at compile time: https://github.com/llvm/llvm-project/blob/eeadd0128df848eb858ae718984a13fa2c923775/libcxx/src/chrono.cpp#L229

Now picolibc does not provide clock_gettime (and can't, because the implementation would be different on every platform) , but if you wanted to provide it (e.g. your MCU has an RTC that's easily accessible through some MMIO registers) I think the best way would be to patch picolibc to provide clock_gettime for your specific platform. (I think this would be the easiest way to have libcxx pick up your clock_gettime function and provide all the C++ chrono functionality on top.)

Also note that in theory it's not always necessary for all this to be decided at libc/libcxx compile time. For example, if you enable posix-io in picolibc, it will provide a file IO implementation (e.g. fopen), assuming open, close, read, write, lseek are defined symbols. Importantly, the resolution of these symbols is not done until final link time, so they don't have to be defined during libc/libcxx compilation, and you can instead just define them in your application code, and have the linker figure it all out. (Note that I again could not get this to work because libcxx expects some headers for filesystem support that picolibc does not provide, but at least in theory this is how this should work.)

This deferred symbol resolution method could maybe work for some cases (e.g. clock and filesystem) and then we could enable these unconditionally and expect the linker to just throw the code out if it's not being used. (But not for some others, e.g. locales probably incur extra code size on printf which the linker can't just remove.) Also note that this is still somewhat theoretical, as I noted above in practice there are still some kinks.

kuruczgy added a commit to kuruczgy/lean-esp32 that referenced this pull request Dec 9, 2024
This requires two main changes:
- An update to the nixpkgs patch. It is now based on this draft PR:
  NixOS/nixpkgs#352629
- An update to the Lean runtime, as Lean is updated to v4.10.0. The
  changes come from Lean commit
  0a1a855ba80e51515570439f3d73d3d9414ac053.
@alyssais
Copy link
Member

Right — if these are actual properties of the libc, it's fine to have them here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: lib The Nixpkgs function library 6.topic: llvm/clang Issues related to llvmPackages, clangStdenv and related 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 12. first-time contribution This PR is the author's first one; please be gentle!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants