Skip to content

refactor(protocol/lazy-pages): improve #[cfg(...)] attributes, add support Android x86_64#5599

Open
StackOverflowExcept1on wants to merge 3 commits into
masterfrom
av/lazy-pages-cfg
Open

refactor(protocol/lazy-pages): improve #[cfg(...)] attributes, add support Android x86_64#5599
StackOverflowExcept1on wants to merge 3 commits into
masterfrom
av/lazy-pages-cfg

Conversation

@StackOverflowExcept1on

Copy link
Copy Markdown
Member

No description provided.

@StackOverflowExcept1on StackOverflowExcept1on added the type: refactor Internal improvements without changing behavior label Jun 22, 2026
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Summary of Changes

Hello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request refines the conditional compilation logic for the lazy-pages protocol, specifically targeting improved support for Android environments. By consolidating platform checks, the changes ensure better compatibility across Linux and Android targets for both x86_64 and aarch64 architectures.

Highlights

  • Improved Conditional Compilation: Refactored #[cfg(...)] attributes to use any(target_os = "linux", target_os = "android"), simplifying the logic for shared architecture support.
  • Android x86_64 Support: Extended support for the Android x86_64 architecture within the lazy-pages protocol implementation.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request simplifies the platform-specific configuration in protocol/lazy-pages/src/sys/unix.rs by grouping linux and android target operating systems together for both x86_64 and aarch64 architectures. The Android-specific ucontext_t struct padding and pointer casting are now handled inline using conditional compilation attributes (#[cfg(target_os = 'android')]) within the unified aarch64 block, reducing code duplication. There are no review comments, and I have no additional feedback to provide.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

@StackOverflowExcept1on StackOverflowExcept1on changed the title fix(protocol/lazy-pages): improve #[cfg(...)] attributes, add support Android x86_64 refactor(protocol/lazy-pages): improve #[cfg(...)] attributes, add support Android x86_64 Jun 23, 2026
@StackOverflowExcept1on

Copy link
Copy Markdown
Member Author

@claude opus review

Also, please note that android_ucontext_t and cast to this type are temporary workaround. Issue with uc_mcontext offset has been fixed in rust-lang/libc#5189, but fix has not yet been merged into libc-0.2 branch.

@claude

claude Bot commented Jun 23, 2026

Copy link
Copy Markdown

Claude finished @StackOverflowExcept1on's task in 1m 27s —— View job


Code Review — refactor(protocol/lazy-pages): improve #[cfg(...)] attributes, support Android x86_64

  • Read the PR diff and surrounding code
  • Analyze correctness of the #[cfg(...)] consolidation
  • Verify the Android ucontext_t workaround
  • Post review

Nice cleanup. Folding android into the existing linux arch arms via any(target_os = "linux", target_os = "android") and inlining the Android-only padding fix with #[cfg(target_os = "android")] removes the duplicated aarch64 arm with no behavioral change for Linux/macOS. The diff is well-scoped (+7/-12) and reads cleanly.

Correctness ✅

  • aarch64 arm: On Linux, ucontext stays *mut nix::libc::ucontext_t and is used directly; on Android it is shadowed with let ucontext = ucontext as *mut android_ucontext_t. Both the struct definition and the rebind are gated on #[cfg(target_os = "android")], so there are no unused-item / dead-code warnings on Linux. android_ucontext_t::uc_mcontext is typed nix::libc::mcontext_t, which matches unix_aarch64::get_esr(&...mcontext_t) — types line up. 👍
  • x86_64 arm: Reuses the Linux path (uc_mcontext.gregs[REG_ERR]) unchanged. REG_ERR/gregs are shared x86_64 register definitions, so this is consistent.
  • The trailing compile_error! fallback still guards unsupported targets.

Things worth confirming

medium — x86_64 Android does not get the padding workaround; please confirm the layout actually matches.

File: protocol/lazy-pages/src/sys/unix.rs lines 25-32

The aarch64 arm needs the android_ucontext_t shim because bionic places __padding before uc_mcontext (the rust-lang/libc#5159 offset bug). The x86_64 arm relies on nix::libc::ucontext_t matching bionic's x86_64 layout exactly, with no shim. That's plausible (the padding hack is arch-specific in bionic), but since the whole point of this PR is enabling Android x86_64, it's worth double-checking that reading uc_mcontext.gregs[REG_ERR] resolves to the correct offset on Android x86_64 — i.e. that the same #5159-class offset skew doesn't silently apply there. If it's already been exercised on a device/emulator, great; if not, a quick offset_of! sanity check would de-risk it.

low — link the upstream fix so the workaround can be reclaimed later.

File: protocol/lazy-pages/src/sys/unix.rs lines 35-49

Per your note, android_ucontext_t is a temporary workaround pending rust-lang/libc#5189 landing in the libc-0.2 branch. The doc comment links the issue (#5159) but not the fix (#5189). Adding the fix PR (e.g. // TODO: remove once rust-lang/libc#5189 lands in libc-0.2) makes it greppable so the shim and the cast get removed once nix's libc bumps. Fix this →

Nit (optional)

pub on android_ucontext_t and its fields is redundant inside a function body (item is function-local), but it's harmless and was carried over from the prior code, so no need to change.

Overall: looks good to merge once the Android x86_64 layout is confirmed. The two items above are verification/maintainability notes, not blockers.
· branch av/lazy-pages-cfg

@StackOverflowExcept1on

StackOverflowExcept1on commented Jun 23, 2026

Copy link
Copy Markdown
Member Author

@claude opus review

medium - x86_64 Android does not get the padding workaround

#include <ucontext.h>

static_assert(REG_ERR == 19);

static_assert(offsetof(ucontext_t, uc_flags) == 0);
static_assert(offsetof(ucontext_t, uc_link) == 8);
static_assert(offsetof(ucontext_t, uc_stack) == 16);
static_assert(offsetof(ucontext_t, uc_mcontext) == 40);
static_assert(offsetof(ucontext_t, uc_sigmask) == 296);
static_assert(offsetof(ucontext_t, uc_sigmask64) == 296);
static_assert(offsetof(ucontext_t, __fpregs_mem) == 304);

static_assert(sizeof(ucontext_t) == 816);
+ '[' -z /home/.../Android/Sdk/ndk/27.2.12479018 ']'
+ CMAKE_BUILD_TYPE=Release
+ PATH_TO_NDK=/home/.../Android/Sdk/ndk/27.2.12479018
+ mkdir -p build
+ cd build
+ cmake -DCMAKE_TOOLCHAIN_FILE=/home/.../Android/Sdk/ndk/27.2.12479018/build/cmake/android.toolchain.cmake -DANDROID_ABI=x86_64 -DANDROID_PLATFORM=35 -DANDROID_STL=c++_shared -DANDROID_USE_LEGACY_TOOLCHAIN_FILE=OFF -DCMAKE_BUILD_TYPE=Release ..
-- Android: Targeting API '35' with architecture 'x86_64', ABI 'x86_64', and processor 'x86_64'
-- Android: Selected unified Clang toolchain
-- The C compiler identification is Clang 18.0.3
-- The CXX compiler identification is Clang 18.0.3
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /home/.../Android/Sdk/ndk/27.2.12479018/toolchains/llvm/prebuilt/linux-x86_64/bin/clang - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /home/.../Android/Sdk/ndk/27.2.12479018/toolchains/llvm/prebuilt/linux-x86_64/bin/clang++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Configuring done (0.3s)
-- Generating done (0.0s)
-- Build files have been written to: /mnt/tmpfs/library-android/build
+ cmake --build .
[ 50%] Building CXX object CMakeFiles/library.dir/library.cpp.o
[100%] Linking CXX shared library liblibrary.so
[100%] Built target library
fn main() {
    const _: () = {
        assert!(libc::REG_ERR == 19);

        assert!(core::mem::offset_of!(libc::ucontext_t, uc_flags) == 0);
        assert!(core::mem::offset_of!(libc::ucontext_t, uc_link) == 8);
        assert!(core::mem::offset_of!(libc::ucontext_t, uc_stack) == 16);
        assert!(core::mem::offset_of!(libc::ucontext_t, uc_mcontext) == 40);
        assert!(core::mem::offset_of!(libc::ucontext_t, uc_sigmask64) == 296);

        assert!(core::mem::size_of::<libc::ucontext_t>() == 816);
    };
}
$ rustc --version --verbose
rustc 1.96.0 (ac68faa20 2026-05-25) (built from a source tarball)
binary: rustc
commit-hash: ac68faa20c58cbccd01ee7208bf3b6e93a7d7f96
commit-date: 2026-05-25
host: x86_64-linux-android
release: 1.96.0
LLVM version: 21.1.8

$ cargo build --release
   Compiling libc v0.2.186
   Compiling app v0.1.0 (/data/data/com.termux/files/home/app)
    Finished `release` profile [optimized] target(s) in 1.51s

low - link the upstream fix so the workaround can be reclaimed later.
Nit (optional)

fixed

@claude

claude Bot commented Jun 23, 2026

Copy link
Copy Markdown

Claude finished @StackOverflowExcept1on's task in 35s —— View job


Code Review (follow-up) — Android x86_64 verification

  • Read the updated diff and the new offset evidence
  • Confirm the x86_64 Android layout question (medium)
  • Confirm the #5189 TODO link (low) and pub nit are addressed

Thanks for the thorough evidence — all three items are resolved. ✅

medium — x86_64 Android padding: resolved

Your bionic static_assert dump and the Rust offset_of!/size_of const _ block agree on every field, so the concern doesn't apply to x86_64:

field C offsetof Rust offset_of!
uc_flags 0 0
uc_link 8 8
uc_stack 16 16
uc_mcontext 40 40
uc_sigmask64 296 296
sizeof 816 816

The key one is uc_mcontext == 40, and nix::libc::ucontext_t reports the same. The #5159-class skew is aarch64-only in bionic (on x86_64 the __padding sits after uc_mcontext as __fpregs_mem, not before it), so the shim is correctly scoped to the aarch64 arm. The fact that the const _ asserts compile on x86_64-linux-android is itself the proof — offset_of! on a layout mismatch would fail the build. No shim needed. 👍

low — link upstream fix: fixed

unix.rs:35 now carries // TODO: remove once https://github.com/rust-lang/libc/pull/5189 lands in libc-0.2, and keeps the #5159 issue + bionic source links. Greppable for reclaiming the shim once nix's libc bumps.

nit — redundant pub: fixed

android_ucontext_t and its fields are now private (function-local), matching their scope.


LGTM. The cfg consolidation is clean, the Android shim is correctly arch-scoped, and the workaround is documented for removal. No further concerns from me.
· branch av/lazy-pages-cfg

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

Labels

type: refactor Internal improvements without changing behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant